next.js icon indicating copy to clipboard operation
next.js copied to clipboard
Published 2 months ago • verified publisher icon vercel

Fix remotePatterns when all paths and/or domains are allowed

Open alexdln opened this issue 2 years ago • 12 comments

Fixing a bug

What?

Fix remotePatterns when all paths and/or domains are allowed.

Why?

micromatch creates a very strange regex for all paths - /^(?:(?!\.)(?:(?:(?!(?:^|[\\/])\.).)*?)[\\/]?)$/. That is, paths cannot start with a dot or contain a slash followed by a dot.

Interestingly, here are some valid paths:

  • /a/a.a/6a00d8341c4fbe53ef02c8d3a82122200d-600wi
  • ////a/a.a/6a00d8341c4fbe53ef02c8d3a82122200d-600wi
  • ///:?%;№%/a/a.a/6a00d8341c4fbe53ef02c8d3a82122200d-600wi./
  • /:./6a00d8341c4fbe53ef02c8d3a82122200d-600wi./

And here are some invalid ones:

  • /.a/6a00d8341c4fbe53ef02c8d3a82122200d-600wi
  • /a/.a/6a00d8341c4fbe53ef02c8d3a82122200d-600wi
  • ./a/6a00d8341c4fbe53ef02c8d3a82122200d-600wi

I don't think this check makes any sense.

How?

If the user allows all (**) - it means any path or domain will be considered valid.

Fixes #60483, #58139, #46903

alexdln avatar Jan 10 '24 19:01 alexdln

Allow CI Workflow Run

  • [x] approve CI run for commit: 246a09a8f0c6878a19c82f442ad6d282392c32bc

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

ijjk avatar Jan 10 '24 19:01 ijjk

Allow CI Workflow Run

  • [x] approve CI run for commit: 246a09a8f0c6878a19c82f442ad6d282392c32bc

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

ijjk avatar Jan 10 '24 19:01 ijjk

Allow CI Workflow Run

  • [x] approve CI run for commit: f6f4064a5907e52fed4c7595b4b4031c3f90774e

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

ijjk avatar Jan 10 '24 19:01 ijjk

I see a test for remotePatterns - test\integration\image-optimizer\test\index.test.ts, but it only checks for schema validation errors, not patterns. In this case, should tests for patterns be added and should they be written in this file?

alexdln avatar Jan 10 '24 19:01 alexdln

Maybe remove micromatch and write custom regexp?

Something like: for pathnames:

new RegExp(`^${pathnameRule.replace(/([\[\]\\^$.|?+()])/g, '\\$1').replace(/\*\*/g, '.+').replace(/\*/g, '[^\\]+')}$`)

for domains:

new RegExp(`^${domainRule.replace(/([\[\]\\^$.|?+()])/g, '\\$1').replace(/\*\*/g, '.+').replace(/\*/g, '[^.]+')}$`)

For domains with validation something like:

.replace(/\*\*/g, '(\.?[\d\w][\d\w-]*)*').replace(/\*/g, '[\d\w][\d\w-]*')}$`

domainRule.replace(/([\[\]\\^$.|?+()])/g, '\\$1') - part for the normalization regular expression, so that the point is checked as a point (\.), and not as any character (.)

alexdln avatar Jan 11 '24 04:01 alexdln

Tests Passed

ijjk avatar Feb 01 '24 14:02 ijjk

Stats from current PR

Default Build (Increase detected ⚠️)
General
vercel/next.js canary vordgi/next.js fix-all-remote-patterns Change
buildDuration 11.7s 11.6s N/A
buildDurationCached 6.4s 4.9s N/A
nodeModulesSize 196 MB 196 MB N/A
nextStartRea..uration (ms) 433ms 436ms N/A
Client Bundles (main, webpack) Overall increase ⚠️
vercel/next.js canary vordgi/next.js fix-all-remote-patterns Change
3f784ff6-HASH.js gzip 53.4 kB 53.4 kB N/A
423.HASH.js gzip 185 B 181 B N/A
68-HASH.js gzip 29.6 kB 29.8 kB ⚠️ +197 B
framework-HASH.js gzip 45.2 kB 45.2 kB ✓
main-app-HASH.js gzip 238 B 239 B N/A
main-HASH.js gzip 31.8 kB 31.8 kB N/A
webpack-HASH.js gzip 1.7 kB 1.7 kB ✓
Overall change 76.5 kB 76.7 kB ⚠️ +197 B
Legacy Client Bundles (polyfills)
vercel/next.js canary vordgi/next.js fix-all-remote-patterns Change
polyfills-HASH.js gzip 31 kB 31 kB ✓
Overall change 31 kB 31 kB ✓
Client Pages
vercel/next.js canary vordgi/next.js fix-all-remote-patterns Change
_app-HASH.js gzip 194 B 195 B N/A
_error-HASH.js gzip 182 B 181 B N/A
amp-HASH.js gzip 502 B 501 B N/A
css-HASH.js gzip 320 B 322 B N/A
dynamic-HASH.js gzip 2.5 kB 2.5 kB N/A
edge-ssr-HASH.js gzip 255 B 256 B N/A
head-HASH.js gzip 350 B 349 B N/A
hooks-HASH.js gzip 368 B 369 B N/A
image-HASH.js gzip 4.19 kB 4.18 kB N/A
index-HASH.js gzip 257 B 256 B N/A
link-HASH.js gzip 2.67 kB 2.61 kB N/A
routerDirect..HASH.js gzip 310 B 311 B N/A
script-HASH.js gzip 384 B 383 B N/A
withRouter-HASH.js gzip 306 B 308 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B ✓
Overall change 106 B 106 B ✓
Client Build Manifests
vercel/next.js canary vordgi/next.js fix-all-remote-patterns Change
_buildManifest.js gzip 483 B 484 B N/A
Overall change 0 B 0 B ✓
Rendered Page Sizes
vercel/next.js canary vordgi/next.js fix-all-remote-patterns Change
index.html gzip 528 B 527 B N/A
link.html gzip 542 B 538 B N/A
withRouter.html gzip 523 B 522 B N/A
Overall change 0 B 0 B ✓
Edge SSR bundle Size
vercel/next.js canary vordgi/next.js fix-all-remote-patterns Change
edge-ssr.js gzip 94 kB 94.1 kB N/A
page.js gzip 149 kB 149 kB N/A
Overall change 0 B 0 B ✓
Middleware size
vercel/next.js canary vordgi/next.js fix-all-remote-patterns Change
middleware-b..fest.js gzip 620 B 623 B N/A
middleware-r..fest.js gzip 151 B 149 B N/A
middleware.js gzip 47.4 kB 37.7 kB N/A
edge-runtime..pack.js gzip 1.94 kB 1.92 kB N/A
Overall change 0 B 0 B ✓
Next Runtimes
vercel/next.js canary vordgi/next.js fix-all-remote-patterns Change
app-page-exp...dev.js gzip 166 kB 166 kB N/A
app-page-exp..prod.js gzip 95.1 kB 95 kB N/A
app-page-tur..prod.js gzip 96.9 kB 96.8 kB N/A
app-page-tur..prod.js gzip 91.5 kB 91.3 kB N/A
app-page.run...dev.js gzip 135 kB 135 kB N/A
app-page.run..prod.js gzip 90 kB 89.9 kB N/A
app-route-ex...dev.js gzip 22 kB 22 kB ✓
app-route-ex..prod.js gzip 14.8 kB 14.8 kB ✓
app-route-tu..prod.js gzip 14.8 kB 14.8 kB ✓
app-route-tu..prod.js gzip 14.6 kB 14.6 kB ✓
app-route.ru...dev.js gzip 21.7 kB 21.7 kB ✓
app-route.ru..prod.js gzip 14.6 kB 14.6 kB ✓
pages-api-tu..prod.js gzip 9.43 kB 9.43 kB ✓
pages-api.ru...dev.js gzip 9.7 kB 9.7 kB ✓
pages-api.ru..prod.js gzip 9.43 kB 9.43 kB ✓
pages-turbo...prod.js gzip 22 kB 22 kB ✓
pages.runtim...dev.js gzip 22.7 kB 22.7 kB ✓
pages.runtim..prod.js gzip 22 kB 22 kB ✓
server.runti..prod.js gzip 49.7 kB 49.7 kB N/A
Overall change 198 kB 198 kB ✓
Diff details
Diff for page.js

Diff too large to display

Diff for edge-runtime-webpack.js 
@@ -46,12 +46,6 @@
   /******/ __webpack_require__.m = __webpack_modules__;
   /******/
   /************************************************************************/
-  /******/ /* webpack/runtime/amd options */
-  /******/ (() => {
-    /******/ __webpack_require__.amdO = {};
-    /******/
-  })();
-  /******/
   /******/ /* webpack/runtime/chunk loaded */
   /******/ (() => {
     /******/ var deferred = [];
Diff for middleware.js

Diff too large to display

Diff for edge-ssr.js

Diff too large to display

Diff for link-HASH.js 
@@ -1,7 +1,7 @@
 (self["webpackChunk_N_E"] = self["webpackChunk_N_E"] || []).push([
   [644],
   {
-    /***/ 1794: /***/ function (
+    /***/ 8959: /***/ function (
       __unused_webpack_module,
       __unused_webpack_exports,
       __webpack_require__
@@ -9,7 +9,7 @@
       (window.__NEXT_P = window.__NEXT_P || []).push([
         "/link",
         function () {
-          return __webpack_require__(8156);
+          return __webpack_require__(6318);
         },
       ]);
       if (false) {
@@ -18,7 +18,7 @@
       /***/
     },
 
-    /***/ 9227: /***/ function (module, exports) {
+    /***/ 8811: /***/ function (module, exports) {
       "use strict";
 
       Object.defineProperty(exports, "__esModule", {
@@ -36,9 +36,6 @@
         PrefetchKind: function () {
           return PrefetchKind;
         },
-        PrefetchCacheEntryStatus: function () {
-          return PrefetchCacheEntryStatus;
-        },
         ACTION_REFRESH: function () {
           return ACTION_REFRESH;
         },
@@ -77,13 +74,6 @@
         PrefetchKind["FULL"] = "full";
         PrefetchKind["TEMPORARY"] = "temporary";
       })(PrefetchKind || (PrefetchKind = {}));
-      var PrefetchCacheEntryStatus;
-      (function (PrefetchCacheEntryStatus) {
-        PrefetchCacheEntryStatus["fresh"] = "fresh";
-        PrefetchCacheEntryStatus["reusable"] = "reusable";
-        PrefetchCacheEntryStatus["expired"] = "expired";
-        PrefetchCacheEntryStatus["stale"] = "stale";
-      })(PrefetchCacheEntryStatus || (PrefetchCacheEntryStatus = {}));
       function isThenable(value) {
         // TODO: We don't gain anything from this abstraction. It's unsound, and only
         // makes sense in the specific places where we use it. So it's better to keep
@@ -110,7 +100,7 @@
       /***/
     },
 
-    /***/ 1008: /***/ function (module, exports, __webpack_require__) {
+    /***/ 1200: /***/ function (module, exports, __webpack_require__) {
       "use strict";
 
       Object.defineProperty(exports, "__esModule", {
@@ -122,7 +112,7 @@
           return getDomainLocale;
         },
       });
-      const _normalizetrailingslash = __webpack_require__(348);
+      const _normalizetrailingslash = __webpack_require__(7774);
       const basePath =
         /* unused pure expression or super */ null && (false || "");
       function getDomainLocale(path, locale, locales, domainLocales) {
@@ -146,7 +136,7 @@
       /***/
     },
 
-    /***/ 7843: /***/ function (module, exports, __webpack_require__) {
+    /***/ 3611: /***/ function (module, exports, __webpack_require__) {
       "use strict";
       /* __next_internal_client_entry_do_not_use__  cjs */
       Object.defineProperty(exports, "__esModule", {
@@ -163,17 +153,17 @@
       const _react = /*#__PURE__*/ _interop_require_default._(
         __webpack_require__(959)
       );
-      const _resolvehref = __webpack_require__(4582);
-      const _islocalurl = __webpack_require__(9964);
-      const _formaturl = __webpack_require__(7393);
-      const _utils = __webpack_require__(161);
-      const _addlocale = __webpack_require__(3889);
-      const _routercontextsharedruntime = __webpack_require__(6120);
-      const _approutercontextsharedruntime = __webpack_require__(6635);
-      const _useintersection = __webpack_require__(3627);
-      const _getdomainlocale = __webpack_require__(1008);
-      const _addbasepath = __webpack_require__(6314);
-      const _routerreducertypes = __webpack_require__(9227);
+      const _resolvehref = __webpack_require__(5332);
+      const _islocalurl = __webpack_require__(214);
+      const _formaturl = __webpack_require__(587);
+      const _utils = __webpack_require__(9388);
+      const _addlocale = __webpack_require__(5071);
+      const _routercontextsharedruntime = __webpack_require__(7554);
+      const _approutercontextsharedruntime = __webpack_require__(8625);
+      const _useintersection = __webpack_require__(1488);
+      const _getdomainlocale = __webpack_require__(1200);
+      const _addbasepath = __webpack_require__(3810);
+      const _routerreducertypes = __webpack_require__(8811);
       const prefetched = new Set();
       function prefetch(router, href, as, options, appOptions, isAppRouter) {
         if (false) {
@@ -494,44 +484,39 @@
                 isAppRouter
               );
             },
-            onTouchStart: false
-              ? 0
-              : function onTouchStart(e) {
-                  if (
-                    !legacyBehavior &&
-                    typeof onTouchStartProp === "function"
-                  ) {
-                    onTouchStartProp(e);
-                  }
-                  if (
-                    legacyBehavior &&
-                    child.props &&
-                    typeof child.props.onTouchStart === "function"
-                  ) {
-                    child.props.onTouchStart(e);
-                  }
-                  if (!router) {
-                    return;
-                  }
-                  if (!prefetchEnabled && isAppRouter) {
-                    return;
-                  }
-                  prefetch(
-                    router,
-                    href,
-                    as,
-                    {
-                      locale,
-                      priority: true,
-                      // @see {https://github.com/vercel/next.js/discussions/40268?sort=top#discussioncomment-3572642}
-                      bypassPrefetchedCheck: true,
-                    },
-                    {
-                      kind: appPrefetchKind,
-                    },
-                    isAppRouter
-                  );
+            onTouchStart(e) {
+              if (!legacyBehavior && typeof onTouchStartProp === "function") {
+                onTouchStartProp(e);
+              }
+              if (
+                legacyBehavior &&
+                child.props &&
+                typeof child.props.onTouchStart === "function"
+              ) {
+                child.props.onTouchStart(e);
+              }
+              if (!router) {
+                return;
+              }
+              if (!prefetchEnabled && isAppRouter) {
+                return;
+              }
+              prefetch(
+                router,
+                href,
+                as,
+                {
+                  locale,
+                  priority: true,
+                  // @see {https://github.com/vercel/next.js/discussions/40268?sort=top#discussioncomment-3572642}
+                  bypassPrefetchedCheck: true,
                 },
+                {
+                  kind: appPrefetchKind,
+                },
+                isAppRouter
+              );
+            },
           };
           // If child is an <a> tag and doesn't have a href attribute, or if the 'passHref' property is
           // defined, we specify the current 'href', so that repetition is not needed by the user.
@@ -594,7 +579,7 @@
       /***/
     },
 
-    /***/ 3627: /***/ function (module, exports, __webpack_require__) {
+    /***/ 1488: /***/ function (module, exports, __webpack_require__) {
       "use strict";
 
       Object.defineProperty(exports, "__esModule", {
@@ -607,7 +592,7 @@
         },
       });
       const _react = __webpack_require__(959);
-      const _requestidlecallback = __webpack_require__(1896);
+      const _requestidlecallback = __webpack_require__(1235);
       const hasIntersectionObserver =
         typeof IntersectionObserver === "function";
       const observers = new Map();
@@ -720,7 +705,7 @@
       /***/
     },
 
-    /***/ 8156: /***/ function (
+    /***/ 6318: /***/ function (
       __unused_webpack_module,
       __webpack_exports__,
       __webpack_require__
@@ -736,7 +721,7 @@
       /* harmony import */ var react_jsx_runtime__WEBPACK_IMPORTED_MODULE_0__ =
         __webpack_require__(1527);
       /* harmony import */ var next_link__WEBPACK_IMPORTED_MODULE_1__ =
-        __webpack_require__(3639);
+        __webpack_require__(2075);
       /* harmony import */ var next_link__WEBPACK_IMPORTED_MODULE_1___default =
         /*#__PURE__*/ __webpack_require__.n(
           next_link__WEBPACK_IMPORTED_MODULE_1__
@@ -767,12 +752,12 @@
       /***/
     },
 
-    /***/ 3639: /***/ function (
+    /***/ 2075: /***/ function (
       module,
       __unused_webpack_exports,
       __webpack_require__
     ) {
-      module.exports = __webpack_require__(7843);
+      module.exports = __webpack_require__(3611);
 
       /***/
     },
@@ -783,7 +768,7 @@
       return __webpack_require__((__webpack_require__.s = moduleId));
     };
     /******/ __webpack_require__.O(0, [888, 774, 179], function () {
-      return __webpack_exec__(1794);
+      return __webpack_exec__(8959);
     });
     /******/ var __webpack_exports__ = __webpack_require__.O();
     /******/ _N_E = __webpack_exports__;
Diff for 3f784ff6-HASH.js

Diff too large to display

Diff for 68-HASH.js

Diff too large to display

Diff for main-HASH.js

Diff too large to display

Diff for app-page-exp..ntime.dev.js

Diff too large to display

Diff for app-page-exp..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page.runtime.dev.js

Diff too large to display

Diff for app-page.runtime.prod.js

Diff too large to display

Diff for server.runtime.prod.js

Diff too large to display

Commit: 9e222c16daf3e0cd94d3230a2dd3ac59fac63150

ijjk avatar Feb 01 '24 14:02 ijjk

@styfle unfortunately, the bug is still there. However, now I understand that the solution above will only solve a specific bug of the tasks above, but there will still be situations like pathname: "/base/**”, where there will still be a path starting with a dot.

picomatch allows to pass the { dot: true } setting, it helps, I think it's better to use it then.

~~Also, while testing now, I noticed that this error goes away, but the upstream image response error arrives for https://bioage.typepad.com/.a/6a00d8341c4fbe53ef02c8d3a82122200d-600wi 403~~

~~Looking at where it comes from…~~ UPD: I didn't pay attention to where the link in the example leads, internal protection, everything is okay with this

alexdln avatar Feb 01 '24 15:02 alexdln

@styfle I checked where else there might be issues (seems to be working in other places) and saw comments on micromatch (which is no longer used). I think it's worth to check how picomatch works and either update it or correct the comments (micromatch -> picomatch).

image

alexdln avatar Feb 01 '24 15:02 alexdln

As a result, I only enabled this option for paths.

I added tests for segments starting with a dot and also saw an unclosed case, when under /*/ there may be several segments (/base/*/file.png for /base/segment1/segment2/file.png), also added checks

alexdln avatar Feb 01 '24 16:02 alexdln

LOL, I felt it!!!

avatars.*.example.com

image

alexdln avatar Feb 01 '24 17:02 alexdln

https://www.npmjs.com/package/picomatch#basic-globbing

Matches any character zero or more times, including path separators. Note that ** will only match path separators (/, and \ with the windows option) when they are the only characters in a path segment. Thus, foo**/bar is equivalent to foo*/bar, and foo/a**b/bar is equivalent to foo/a*b/bar, and more than two consecutive stars in a glob path segment are regarded as a single star. Thus, foo/***/bar is equivalent to foo/*/bar

So picomatch doesn't work the way it's described in the next.js documentation:

https://nextjs.org/docs/app/api-reference/components/image#remotepatterns

Wildcard patterns can be used for both pathname and hostname and have the following syntax:

* match a single path segment or subdomain ** match any number of path segments at the end or subdomains at the beginning

@styfle I think it's better to comment out the tests for this and leave a todo for fix, as it doesn't directly relate to the current tasks. What do you think?

alexdln avatar Feb 01 '24 17:02 alexdln

Turned off this test and left the todos associated with picomatch

alexdln avatar Feb 02 '24 04:02 alexdln