Enforce absolute URLs in Edge Functions runtime #33410

javivelasco · 2022-01-17T20:43:15Z

We currently have inconsistencies when working with URLs in the Edge Functions runtime, this PR addresses them introducing a warning for inconsistent usage that will break in the future. Here is the reasoning.

The Browser

When we are in a browser environment there is a fixed location stored at globalThis.location. Then, if one tries to build a request with a relative URL it will work using that location global hostname as base to construct its URL. For example:

// https://nextjs.org
new Request('/test').url; // https://nextjs.org/test
Response.redirect('/test').headers.get('Location'); // https://nextjs.org/test

However, if we attempt to run the same code from about:blank it would not work because the global to use as a base String(globalThis.location) is not a valid URL. Therefore a call to Response.redirect('/test') or new Response('/test') would fail.

Edge Functions Runtime

In Next.js Edge Functions runtime the situation is slightly different from a browser. Say that we have a root middleware (pages/_middleware) that gets invoked for every page. In the middleware file we expose the handler function and also define a global variable that we mutate on every request:

// pages/_middleware

let count = 0;

export function middleware(req: NextRequest) {
  console.log(req.url);
  count += 1;
}

Currently we cache the module scope in the runtime so subsequent invocations would hold the same globals and the module would not be evaluated again. This would make the counter to increment for each request that the middleware handles. It is for this reason that we can't have a global location that changes across different invocations. Each invocation of the same function uses the same global which also holds primitives like URL or Request so changing an hypothetical globalThis.location per request would affect concurrent requests being handled.

Then, it is not possible to use relative URLs in the same way the browser does because we don't have a global to rely on to use its host to compose a URL from a relative path.

Why it works today

We are not validating what is provided to, for example, NextResponse.rewrite() nor NextResponse.redirect(). We simply create a Response instance that adds the corresponding header for the rewrite or the redirect. Then it is the consumer the one that composes the final destination based on the request. Theoretically you can pass any value and it would fail on redirect but won't validate the input.

Of course this is inconsistent because it doesn't make sense that NextResponse.rewrite('/test') works but fetch(new NextRequest('/test')) does not. Also we should validate what is provided. Finally, we want to be consistent with the way a browser behaves so new Request('/test') should not work if there is no global location which we lack.

What this PR does

We will have to deprecate the usage of relative URLs in the previously mentioned scenarios. In preparation for it, this PR adds a validation function in those places where it will break in the future, printing a warning with a link that points to a Next.js page with an explanation of the issue and ways to fix it.

Although middleware changes are not covered by semver, we will roll this for some time to make people aware that this change is coming. Then after a reasonable period of time we can remove the warning and make the code fail when using relative URLs in the previously exposed scenarios.

matamatanot · 2022-01-17T22:44:26Z

we are still not covering breaking changes with semver so I suppose it is fine.

Does this mean that this breaking change will be introduced in 12.0.9 or 12.1.0?

javivelasco · 2022-01-19T12:52:25Z

@matamatanot I corrected the PR to include a warning for now. We will land the change after some time but it will probably happen in a minor bump since we are warning the breaking changes are not covered by semver yet for Middleware.

ijjk · 2022-01-19T13:24:19Z

Stats from current PR

Default Build (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
buildDuration	18.7s	19s	⚠️ +288ms
buildDurationCached	4.2s	4.1s	-96ms
nodeModulesSize	354 MB	354 MB	⚠️ +1.19 kB

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
/ failed reqs	0	0	✓
/ total time (seconds)	4.123	4.034	-0.09
/ avg req/sec	606.4	619.77	+13.37
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	2.134	2.185	⚠️ +0.05
/error-in-render avg req/sec	1171.27	1144.22	⚠️ -27.05

Client Bundles (main, webpack, commons)

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	27.2 kB	27.2 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71 kB	71 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
_app-HASH.js gzip	1.37 kB	1.37 kB	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	326 B	326 B	✓
dynamic-HASH.js gzip	2.37 kB	2.37 kB	✓
head-HASH.js gzip	350 B	350 B	✓
hooks-HASH.js gzip	919 B	919 B	✓
image-HASH.js gzip	4.7 kB	4.7 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.13 kB	2.13 kB	✓
routerDirect..HASH.js gzip	321 B	321 B	✓
script-HASH.js gzip	383 B	383 B	✓
withRouter-HASH.js gzip	318 B	318 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	14.1 kB	14.1 kB	✓

Client Build Manifests

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
index.html gzip	530 B	530 B	✓
link.html gzip	544 B	544 B	✓
withRouter.html gzip	525 B	525 B	✓
Overall change	1.6 kB	1.6 kB	✓

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
buildDuration	22.4s	22.4s	-69ms
buildDurationCached	4.2s	4.2s	-26ms
nodeModulesSize	354 MB	354 MB	⚠️ +1.19 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
/ failed reqs	0	0	✓
/ total time (seconds)	4.086	4.051	-0.04
/ avg req/sec	611.81	617.14	+5.33
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	2.184	2.178	-0.01
/error-in-render avg req/sec	1144.79	1147.81	+3.02

Client Bundles (main, webpack, commons)

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	27.3 kB	27.3 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.3 kB	71.3 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	305 B	305 B	✓
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.36 kB	2.36 kB	✓
head-HASH.js gzip	342 B	342 B	✓
hooks-HASH.js gzip	911 B	911 B	✓
image-HASH.js gzip	4.73 kB	4.73 kB	✓
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	2.19 kB	2.19 kB	✓
routerDirect..HASH.js gzip	314 B	314 B	✓
script-HASH.js gzip	375 B	375 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	14.1 kB	14.1 kB	✓

Client Build Manifests

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes

	vercel/next.js canary	javivelasco/next.js absolute-urls	Change
index.html gzip	533 B	533 B	✓
link.html gzip	546 B	546 B	✓
withRouter.html gzip	527 B	527 B	✓
Overall change	1.61 kB	1.61 kB	✓

Commit: 1b86aca

matamatanot · 2022-01-19T23:27:48Z

We will land the change after some time but it will probably happen in a minor bump since we are warning the breaking changes are not covered by semver yet for Middleware.

Thanks! Middleware is clearly marked as Beta, so I think it should be fine.

icyJoseph · 2022-01-27T09:44:02Z

We had a test where we expected the result from NextResponse.redirect('https://example.com') to be https://example.com but after these changes, the response will be https://example.com/ instead, with a trailing slash.

In our case in particular we read this from the headers location, but I guess any url that is now going through validateUrl, will be impacted, which I think is the purpose of this change. (validate is doing more than just validate, but I reckon that's acceptable?)

Leaving this here, just in case someone bumps into a similar issue.

TheThirdRace · 2022-01-29T14:44:20Z

@icyJoseph I think you should open an issue for that situation. validate should clearly not modify the URL...

Follow up from #33410. Adds version history for Middleware so it's more clear it launched in beta, and that we have a place to track changes.

We currently have inconsistencies when working with URLs in the Edge Functions runtime, this PR addresses them introducing a warning for inconsistent usage that will break in the future. Here is the reasoning. ### The Browser When we are in a browser environment there is a fixed location stored at `globalThis.location`. Then, if one tries to build a request with a relative URL it will work using that location global hostname as _base_ to construct its URL. For example: ```typescript // https://nextjs.org new Request('/test').url; // https://nextjs.org/test Response.redirect('/test').headers.get('Location'); // https://nextjs.org/test ``` However, if we attempt to run the same code from `about:blank` it would not work because the global to use as a base `String(globalThis.location)` is not a valid URL. Therefore a call to `Response.redirect('/test')` or `new Response('/test')` would fail. ### Edge Functions Runtime In Next.js Edge Functions runtime the situation is slightly different from a browser. Say that we have a root middleware (`pages/_middleware`) that gets invoked for every page. In the middleware file we expose the handler function and also define a global variable that we mutate on every request: ```typescript // pages/_middleware let count = 0; export function middleware(req: NextRequest) { console.log(req.url); count += 1; } ``` Currently we cache the module scope in the runtime so subsequent invocations would hold the same globals and the module would not be evaluated again. This would make the counter to increment for each request that the middleware handles. It is for this reason that we **can't have a global location** that changes across different invocations. Each invocation of the same function uses the same global which also holds primitives like `URL` or `Request` so changing an hypothetical `globalThis.location` per request would affect concurrent requests being handled. Then, it is not possible to use relative URLs in the same way the browser does because we don't have a global to rely on to use its host to compose a URL from a relative path. ### Why it works today We are **not** validating what is provided to, for example, `NextResponse.rewrite()` nor `NextResponse.redirect()`. We simply create a `Response` instance that adds the corresponding header for the rewrite or the redirect. Then it is **the consumer** the one that composes the final destination based on the request. Theoretically you can pass any value and it would fail on redirect but won't validate the input. Of course this is inconsistent because it doesn't make sense that `NextResponse.rewrite('/test')` works but `fetch(new NextRequest('/test'))` does not. Also we should validate what is provided. Finally, we want to be consistent with the way a browser behaves so `new Request('/test')` _should_ not work if there is no global location which we lack. ### What this PR does We will have to deprecate the usage of relative URLs in the previously mentioned scenarios. In preparation for it, this PR adds a validation function in those places where it will break in the future, printing a warning with a link that points to a Next.js page with an explanation of the issue and ways to fix it. Although middleware changes are not covered by semver, we will roll this for some time to make people aware that this change is coming. Then after a reasonable period of time we can remove the warning and make the code fail when using relative URLs in the previously exposed scenarios.

javivelasco requested review from huozhi, ijjk, shuding and timneutkens as code owners January 17, 2022 20:43

ijjk added the type: next label Jan 17, 2022

This comment has been minimized.

Sign in to view

javivelasco force-pushed the absolute-urls branch from 8cbd298 to 1d7549f Compare January 19, 2022 10:42

This comment has been minimized.

Sign in to view

ijjk added the area: documentation label Jan 19, 2022

javivelasco added 9 commits January 19, 2022 13:57

Add URL behaviour tests

6d29b59

Make the runtime work only with absolute URLs

6735085

Fix remaining tests

b80aa39

Make tests pass for all node versions

6dea239

Update URL tests to show a warning

e7066d9

Add clone method for NextURL

45d50e7

Add a util to warn on relative URLs

3ef0050

Roll back to warn instead of failing

743f324

Fix lint warning

1b86aca

javivelasco force-pushed the absolute-urls branch from bb25df1 to 1b86aca Compare January 19, 2022 12:58

huozhi approved these changes Jan 19, 2022

View reviewed changes

kodiakhq bot merged commit e5dee17 into vercel:canary Jan 19, 2022

javivelasco deleted the absolute-urls branch January 19, 2022 15:22

javivelasco mentioned this pull request Jan 19, 2022

Edge function / middleware error: "TypeError: Unable to parse URL" when deployed to Vercel #31208

Closed

icyJoseph mentioned this pull request Jan 31, 2022

Hostname rewrites, triggers relative URL warnings vercel/examples#115

Closed

leerob mentioned this pull request Feb 13, 2022

Update Middleware docs to add version history. #34302

Merged

kodiakhq bot pushed a commit that referenced this pull request Feb 13, 2022

Update Middleware docs to add version history. (#34302)

ef31084

Follow up from #33410. Adds version history for Middleware so it's more clear it launched in beta, and that we have a place to track changes.

Schniz mentioned this pull request Feb 17, 2022

Remove deprecation for relative URL usage in middlewares #34461

Merged

balazsorban44 mentioned this pull request Feb 19, 2022

Edge Functions cause 500 Error with [email protected] #34566

Closed

1 task

github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce absolute URLs in Edge Functions runtime #33410

Enforce absolute URLs in Edge Functions runtime #33410

javivelasco commented Jan 17, 2022 •

edited

Loading

This comment has been minimized.

matamatanot commented Jan 17, 2022

This comment has been minimized.

javivelasco commented Jan 19, 2022

ijjk commented Jan 19, 2022

matamatanot commented Jan 19, 2022

icyJoseph commented Jan 27, 2022

TheThirdRace commented Jan 29, 2022

Enforce absolute URLs in Edge Functions runtime #33410

Enforce absolute URLs in Edge Functions runtime #33410

Conversation

javivelasco commented Jan 17, 2022 • edited Loading

The Browser

Edge Functions Runtime

Why it works today

What this PR does

This comment has been minimized.

matamatanot commented Jan 17, 2022

This comment has been minimized.

javivelasco commented Jan 19, 2022

ijjk commented Jan 19, 2022

Stats from current PR

matamatanot commented Jan 19, 2022

icyJoseph commented Jan 27, 2022

TheThirdRace commented Jan 29, 2022

javivelasco commented Jan 17, 2022 •

edited

Loading