bug: allow scheme-relative url in links #31119

SethFalco · 2021-11-07T22:27:26Z

Bug

Related issues linked using fixes #number
Errors have helpful link attached, see contributing.md

Documentation / Examples

Make sure the linting passes by running yarn lint

Relates Issues

Fixes Scheme-relative URLs have slashes merged #30947

This change considers // at the start of the URL to be a valid match for urlProtoMatch and separates it from the rest of the URL. This way, Next.js will stop changing scheme-relative to normal relative URLs.

<a href="//example.com">Scheme-relative URL</a>
— https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a

timneutkens

Can you add an integration test for this?

SethFalco · 2021-11-09T08:20:27Z

I've tried to add an integration test, but the process is confusing me. ^-^'

So when you say integration tests, you meant you want one in the production, development, or e2e folder, right?
Since it says in the test README:

integration: … We should not add any more tests here.
— https://github.com/vercel/next.js/blob/canary/test/readme.md

So I've been looking in the e2e directory to try and add one. But I can't run any of the existing e2e tests. ^-^'

$ yarn testheadless --testPathPattern "e2e/prerender-crawler.test.ts"
…
error - Failed to load SWC binary, see more info here: https://nextjs.org/docs/messages/failed-loading-swc
  console.error
    Failed to create next instance Error: next build failed with code 1
        at ChildProcess.<anonymous> (/home/seth/Documents/repos/dependency/next.js/test/lib/next-modes/next-start.ts:71:34)
        at ChildProcess.emit (node:events:390:28)
        at Process.ChildProcess._handle.onexit (node:internal/child_process:290:12)

      93 |     [filename: string]: string | FileRef
      94 |   }
    > 95 |   dependencies?: {
         |                 ^
      96 |     [name: string]: string
      97 |   }
      98 |   nextConfig?: NextConfig

      at Object.createNext (lib/e2e-utils.ts:95:17)
          at runMicrotasks (<anonymous>)
      at e2e/prerender-crawler.test.ts:7:16

Any hints to what I'm doing wrong here?

SethFalco · 2021-11-15T18:40:33Z

For now I've added a test, but have not executed it locally.
If it looks alright to you, it could be useful if I could see it run in GitHub Actions at least. ^-^'

ijjk

Hi, I'm not sure we want to allow protocol relative links like this as this can cause issues and doesn't make sense in the context of next/link since it's meant for client-transitions. We also have tests checking that this triggers errors here

next.js/test/integration/repeated-slashes/test/index.test.js

Line 286 in b204640

it('should handle slashes in next/link correctly', async () => {

SethFalco · 2021-11-15T21:03:40Z

In our case, the usage is usually for relative links as you described, but there are just some cases where we want to take the user to a similar page on another domain. As the behavior was unexpected, I just thought it made more sense to fix it here.

As scheme-relative URLs are indeed widely recognized/supported, and your implementation already supports absolute URLs, I think it'd be odd not to support them.

I won't push if the consensus is against my proposal, but in my opinion it simply should accept (and shouldn't modify) valid URLs.

ijjk · 2022-04-08T18:21:41Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
buildDuration	15.1s	15.1s	⚠️ +9ms
buildDurationCached	6.2s	6.1s	-81ms
nodeModulesSize	478 MB	478 MB	⚠️ +24 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.066	3.039	-0.03
/ avg req/sec	815.45	822.52	+7.07
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.182	1.176	-0.01
/error-in-render avg req/sec	2115.56	2125.56	+10

Client Bundles (main, webpack) Overall increase ⚠️

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
925.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	28 kB	28 kB	⚠️ +11 B
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.7 kB	71.7 kB	⚠️ +11 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
_app-HASH.js gzip	1.36 kB	1.36 kB	✓
_error-HASH.js gzip	192 B	192 B	✓
amp-HASH.js gzip	309 B	309 B	✓
css-HASH.js gzip	327 B	327 B	✓
dynamic-HASH.js gzip	3.05 kB	3.05 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	920 B	920 B	✓
image-HASH.js gzip	5.64 kB	5.64 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.32 kB	2.32 kB	✓
routerDirect..HASH.js gzip	320 B	320 B	✓
script-HASH.js gzip	387 B	387 B	✓
withRouter-HASH.js gzip	319 B	319 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	15.9 kB	15.9 kB	✓

Client Build Manifests

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes Overall increase ⚠️

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
index.html gzip	531 B	532 B	⚠️ +1 B
link.html gzip	544 B	546 B	⚠️ +2 B
withRouter.html gzip	524 B	526 B	⚠️ +2 B
Overall change	1.6 kB	1.6 kB	⚠️ +5 B

Diffs

Diff for main-HASH.js

@@ -4448,10 +4448,10 @@
       }
       function isLocalURL(url) {
         // prevent a hydration mismatch on href for url with anchor refs
-        if (url.startsWith("/") || url.startsWith("#") || url.startsWith("?"))
+        if (/^\/(?!\/)/.test(url) || url.startsWith("#") || url.startsWith("?"))
           return true;
         try {
-          // absolute urls can be local if they are on the same origin
+          // absolute and scheme-relative urls can be local if they are on the same origin
           var locationOrigin = (0, _utils).getLocationOrigin();
           var resolved = new URL(url, locationOrigin);
           return (
@@ -4534,11 +4534,11 @@
             : (0, _formatUrl).formatWithValidation(href);
         // repeated slashes and backslashes in the URL are considered
         // invalid and will never match a Next.js page/file
-        var urlProtoMatch = urlAsString.match(/^[a-zA-Z]{1,}:\/\//);
-        var urlAsStringNoProto = urlProtoMatch
-          ? urlAsString.slice(urlProtoMatch[0].length)
+        var urlSchemeMatch = urlAsString.match(/^(?:[a-zA-Z]+:)?\/\//);
+        var urlAsStringNoScheme = urlSchemeMatch
+          ? urlAsString.slice(urlSchemeMatch[0].length)
           : urlAsString;
-        var urlParts = urlAsStringNoProto.split("?");
+        var urlParts = urlAsStringNoScheme.split("?");
         if ((urlParts[0] || "").match(/(\/\/|\\)/)) {
           console.error(
             "Invalid href passed to next/router: ".concat(
@@ -4547,9 +4547,10 @@
             )
           );
           var normalizedUrl = (0, _utils).normalizeRepeatedSlashes(
-            urlAsStringNoProto
+            urlAsStringNoScheme
           );
-          urlAsString = (urlProtoMatch ? urlProtoMatch[0] : "") + normalizedUrl;
+          urlAsString =
+            (urlSchemeMatch ? urlSchemeMatch[0] : "") + normalizedUrl;
         }
         // Return because it cannot be routed by the Next.js router
         if (!isLocalURL(urlAsString)) {

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-26c4d970f7a13427.js"
+      src="/_next/static/chunks/main-57ecebdd38e8e5a5.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-26c4d970f7a13427.js"
+      src="/_next/static/chunks/main-57ecebdd38e8e5a5.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-26c4d970f7a13427.js"
+      src="/_next/static/chunks/main-57ecebdd38e8e5a5.js"
       defer=""
     ></script>
     <script

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
buildDuration	18.1s	18s	-135ms
buildDurationCached	6.1s	6s	-87ms
nodeModulesSize	478 MB	478 MB	⚠️ +24 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.014	3.025	⚠️ +0.01
/ avg req/sec	829.45	826.31	⚠️ -3.14
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.195	1.188	-0.01
/error-in-render avg req/sec	2092.9	2104.63	+11.73

Client Bundles (main, webpack) Overall increase ⚠️

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
925.HASH.js gzip	178 B	178 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	28.3 kB	28.3 kB	⚠️ +9 B
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	72.2 kB	72.2 kB	⚠️ +9 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	179 B	179 B	✓
amp-HASH.js gzip	313 B	313 B	✓
css-HASH.js gzip	325 B	325 B	✓
dynamic-HASH.js gzip	3.03 kB	3.03 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	921 B	921 B	✓
image-HASH.js gzip	5.7 kB	5.7 kB	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	2.38 kB	2.38 kB	✓
routerDirect..HASH.js gzip	322 B	322 B	✓
script-HASH.js gzip	388 B	388 B	✓
withRouter-HASH.js gzip	317 B	317 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	15.9 kB	15.9 kB	✓

Client Build Manifests

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes Overall increase ⚠️

	vercel/next.js canary	SethFalco/next.js 30947-schema-relative-urls	Change
index.html gzip	531 B	532 B	⚠️ +1 B
link.html gzip	544 B	545 B	⚠️ +1 B
withRouter.html gzip	527 B	527 B	✓
Overall change	1.6 kB	1.6 kB	⚠️ +2 B

Diffs

Diff for main-HASH.js

@@ -4448,10 +4448,10 @@
       }
       function isLocalURL(url) {
         // prevent a hydration mismatch on href for url with anchor refs
-        if (url.startsWith("/") || url.startsWith("#") || url.startsWith("?"))
+        if (/^\/(?!\/)/.test(url) || url.startsWith("#") || url.startsWith("?"))
           return true;
         try {
-          // absolute urls can be local if they are on the same origin
+          // absolute and scheme-relative urls can be local if they are on the same origin
           var locationOrigin = (0, _utils).getLocationOrigin();
           var resolved = new URL(url, locationOrigin);
           return (
@@ -4534,11 +4534,11 @@
             : (0, _formatUrl).formatWithValidation(href);
         // repeated slashes and backslashes in the URL are considered
         // invalid and will never match a Next.js page/file
-        var urlProtoMatch = urlAsString.match(/^[a-zA-Z]{1,}:\/\//);
-        var urlAsStringNoProto = urlProtoMatch
-          ? urlAsString.slice(urlProtoMatch[0].length)
+        var urlSchemeMatch = urlAsString.match(/^(?:[a-zA-Z]+:)?\/\//);
+        var urlAsStringNoScheme = urlSchemeMatch
+          ? urlAsString.slice(urlSchemeMatch[0].length)
           : urlAsString;
-        var urlParts = urlAsStringNoProto.split("?");
+        var urlParts = urlAsStringNoScheme.split("?");
         if ((urlParts[0] || "").match(/(\/\/|\\)/)) {
           console.error(
             "Invalid href passed to next/router: ".concat(
@@ -4547,9 +4547,10 @@
             )
           );
           var normalizedUrl = (0, _utils).normalizeRepeatedSlashes(
-            urlAsStringNoProto
+            urlAsStringNoScheme
           );
-          urlAsString = (urlProtoMatch ? urlProtoMatch[0] : "") + normalizedUrl;
+          urlAsString =
+            (urlSchemeMatch ? urlSchemeMatch[0] : "") + normalizedUrl;
         }
         // Return because it cannot be routed by the Next.js router
         if (!isLocalURL(urlAsString)) {

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-26c4d970f7a13427.js"
+      src="/_next/static/chunks/main-57ecebdd38e8e5a5.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-26c4d970f7a13427.js"
+      src="/_next/static/chunks/main-57ecebdd38e8e5a5.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-26c4d970f7a13427.js"
+      src="/_next/static/chunks/main-57ecebdd38e8e5a5.js"
       defer=""
     ></script>
     <script

Commit: e12168f

balazsorban44 · 2022-04-11T09:41:45Z

Please stop force pushing (unless pushing actual changes, but a regular commit might be better anyway, to see the history), as it creates some noise on the PR (I counted 20+ so far) and generally has no effect on the speed we review PRs.

If you are trying to run the test suites, you can do so locally: https://github.com/vercel/next.js/blob/canary/contributing.md#testing 👍

SethFalco · 2022-04-11T10:40:50Z

Please stop force pushing (unless pushing actual changes, but a regular commit might be better anyway, to see the history), as it creates some noise on the PR (I counted 20+ so far) and generally has no effect on the speed we review PRs.

Ahh sorry, I was just checking back here occasionally and whenever the This branch is out-of-date with the base branch was displayed on GitHub, I clicked Rebase branch.

Figured it'd be handy to:

Know CI still passes with latest changes.
So whoever reviews it doesn't have to. ^-^'

I realized it was coming off a bit spammy with how long the comment section has gotten though, sorry about that!

ijjk

Hi, as mentioned previously I don't think we want to support this as this could open the door to accidental open-redirects and is modifying the behavior of our tests ensuring open-redirects aren't occurring.

A wrapper component around next/link could be added in your application to manually add the actual protocol to the href making this safer and allowing the schema to be relative to the current context.

SethFalco · 2022-05-22T06:45:55Z

@ijjk Fair enough, just a question!

Would you be happier with this if I didn't modify existing behavior and tests? For example, if I added a prop to next/link like allowSchemeRelative? Defaults to false and nothing changes for any existing code or tests, pass true and it'll do what we see in this PR?

If allowSchemeRelative is too specific, then a general property like linkTypes with the allowable types of URLs, such as Absolute, SchemeRelative,DomainRelative, PageRelative, Query, or Fragment, which defaults to the current behavior. (All of them except SchemeRelative I believe?) ← I think this is a better idea, and may prove useful for restricting links further, including absolute URLs which can also be open redirects.

However, the 2nd suggestion still impact tests as we can still shouldn't replace the prefixed // on scheme relative URLs then.

Could also help clean up a little for readability i.e.?

if (/^\/(?!\/)/.test(url) || url.startsWith('#') || url.startsWith('?'))

const ignoreHydration = [LinkType.DomainRelative, LinkType.Fragment, LinkType.Query];

if (ignoreHydration.includes(linkType))
  return true;

If not, that's no problem. I'll just handle it in my application(s)! Just thought it was worth asking.

A wrapper component around next/link could be added in your application to manually add the actual protocol to the href making this safer and allowing the schema to be relative to the current context.

I might be being naive here, but I'm not 100% sure that's possible because of Incremental Static Regeneration, I can hard-code it which is what I have now, but not use the scheme from the incoming request.

SethFalco · 2022-05-22T10:17:41Z

I opted to update the commit with the 2nd proposal though, have any thoughts on it?
I think the only thing left would be deciding what should happen when a non-allowed URL type is used, and adding tests.

The only functional change out-of-the-box is that scheme relative URLs will have a warning/error for not being allowed, rather than for repeated slashes. (Which is a huge improvement imo, if scheme relative URLs wouldn't be allowed, I'd at least expect them to be rejected rather than converted to domain relative URLs.)

SethFalco@c7d70d8

This way, developers can further limit URLs that are passed in. For example, if they only expect domain relative URLs, they can exclude both scheme and absolute URLs then:

import Link, { LinkType } from 'next/link'

export default function Example() {
  return (
    <Link href="/" passHref linkTypes={[LinkType.DomainRelative]}>
      <a>
        <!-- … -->
      </a>
    </Link>
  );
}

Meanwhile, developers could also extend it to include the non-defaults like scheme relative URLs like for my use-case.

ijjk · 2022-05-23T15:54:51Z

I think providing separate linkTypes like that increases the room for accidentally navigating incorrectly and potential open-redirects and the safest bet is to be explicit with providing the protocol or keep the href relative. Feel free to open an RFC on the ideas discussion thread to get feedback though!

SethFalco · 2022-05-23T16:33:15Z

Understood, I've created the following discussion:
#37128

SethFalco requested review from huozhi, ijjk, shuding and timneutkens as code owners November 7, 2021 22:27

ijjk added the type: next label Nov 7, 2021

SethFalco changed the title ~~bug: allow schema-relative url in links~~ bug: allow scheme-relative url in links Nov 7, 2021

timneutkens requested changes Nov 8, 2021

View reviewed changes

SethFalco force-pushed the 30947-schema-relative-urls branch 3 times, most recently from 9e15f17 to 187a406 Compare November 15, 2021 18:38

ijjk mentioned this pull request Nov 15, 2021

Ensure swc dep is copied for isolated tests #31462

Merged

10 tasks

ijjk requested changes Nov 15, 2021

View reviewed changes