Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix:(next/image) handle remotePatterns with a dot in the pathname #60488

Merged
merged 11 commits into from
Feb 8, 2024

Conversation

vordgi
Copy link
Contributor

@vordgi vordgi commented Jan 10, 2024

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.

@ijjk
Copy link
Member

ijjk commented Jan 10, 2024

Allow CI Workflow Run

  • approve CI run for commit: 246a09a

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

1 similar comment
@ijjk
Copy link
Member

ijjk commented Jan 10, 2024

Allow CI Workflow Run

  • approve CI run for commit: 246a09a

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

@ijjk
Copy link
Member

ijjk commented Jan 10, 2024

Allow CI Workflow Run

  • approve CI run for commit: f6f4064

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

@vordgi
Copy link
Contributor Author

vordgi commented Jan 10, 2024

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?

@vordgi vordgi changed the title fix all remote patterns Fix remotePatterns when all paths and/or domains are allowed Jan 10, 2024
@vordgi
Copy link
Contributor Author

vordgi commented Jan 11, 2024

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 (.)

@ijjk
Copy link
Member

ijjk commented Feb 1, 2024

Tests Passed

@ijjk
Copy link
Member

ijjk commented Feb 1, 2024

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: 9e222c1

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! 🎉

Please add two new tests - one for hostname and one for pathname since both of those are changed.

The relevant test file is here: https://github.com/vercel/next.js/blob/543ca115716536b753549d950274da21ea979e64/test/unit/image-optimizer/match-remote-pattern.test.ts

Also the manifest file that we output from next build likely needs to be updated.

images.remotePatterns = (config?.images?.remotePatterns || []).map((p) => ({
// Should be the same as matchRemotePattern()
protocol: p.protocol,
hostname: makeRe(p.hostname).source,
port: p.port,
pathname: makeRe(p.pathname ?? '**').source,
}))

Finally, its worth updating your branch since micromatch was recently changed to picomatch (maybe that already fixed it?)

@vordgi
Copy link
Contributor Author

vordgi commented Feb 1, 2024

@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

@vordgi
Copy link
Contributor Author

vordgi commented Feb 1, 2024

@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

@vordgi vordgi requested a review from a team as a code owner February 1, 2024 16:16
@vordgi vordgi requested review from manovotny and molebox and removed request for a team February 1, 2024 16:16
@vordgi
Copy link
Contributor Author

vordgi commented Feb 1, 2024

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

@vordgi
Copy link
Contributor Author

vordgi commented Feb 1, 2024

LOL, I felt it!!!

avatars.*.example.com

image

@vordgi
Copy link
Contributor Author

vordgi commented Feb 1, 2024

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?

@vordgi
Copy link
Contributor Author

vordgi commented Feb 2, 2024

Turned off this test and left the todos associated with picomatch

@@ -117,6 +117,8 @@ describe('matchRemotePattern', () => {
expect(m(p, new URL('https://avatars.example.com'))).toBe(false)
expect(m(p, new URL('https://avatars.sfo1.example.com'))).toBe(true)
expect(m(p, new URL('https://avatars.iad1.example.com'))).toBe(true)
/** @todo fix the check so the test works */
// expect(m(p, new URL('https://avatars.iad1.iad2.example.com'))).toBe(false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should work. A single asterisk should only match a single segment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. There is .toBe(false) and it failed 😬

Copy link
Contributor Author

@vordgi vordgi Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m({ hostname: 'avatars.*.example.com' }, new URL('https://avatars.iad1.iad2.example.com')) // true (expected - false)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you're saying this is already broken, even without the changes from your PR. That is surprising 😳

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The manifest file that we output from next build needs to be updated to match this same logic:

images.remotePatterns = (config?.images?.remotePatterns || []).map((p) => ({
// Should be the same as matchRemotePattern()
protocol: p.protocol,
hostname: makeRe(p.hostname).source,
port: p.port,
pathname: makeRe(p.pathname ?? '**').source,
}))

vordgi and others added 2 commits February 6, 2024 08:13
@vordgi vordgi requested a review from a team as a code owner February 6, 2024 04:23
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the PR! 🎉

I reverted the "todo" comments since we want to keep the commit history as simple as possible and only include the lines of code that need to change.

For the single asterisk bug, you can open a new issue to track it (and open a corresponding PR to fix it).

Thanks!

@styfle styfle changed the title Fix remotePatterns when all paths and/or domains are allowed fix:(next/image) handle remotePatterns with a dot in the pathname Feb 8, 2024
@styfle styfle added the CI approved Approve running CI for fork label Feb 8, 2024
@styfle styfle merged commit e8a8221 into vercel:canary Feb 8, 2024
70 checks passed
@vordgi vordgi deleted the fix-all-remote-patterns branch February 9, 2024 05:00
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI approved Approve running CI for fork locked type: next
Projects
None yet
3 participants