Don't throw errors when we give "asPathToSearchParams()" an invalid path with multiple leading slashes #70144

takueof · 2024-09-16T07:18:45Z

For Contributors

Improving Documentation

Run pnpm prettier-fix to fix formatting issues before opening the PR.
Read the Docs Contribution Guide to ensure your contribution follows the docs guidelines: https://nextjs.org/docs/community/contribution-guide

Adding or Updating Examples

The "examples guidelines" are followed from our contributing doc https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
Make sure the linting passes by running pnpm build && pnpm lint. See https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md

Fixing a bug

~~Related issues linked using fixes #number~~
Tests added. See: https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
Errors have a helpful link attached, see https://github.com/vercel/next.js/blob/canary/contributing.md

For Maintainers

Minimal description (aim for explaining to someone not on the team to understand the PR)
When linking to a Slack thread, you might want to share details of the conclusion
Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
Add review comments if necessary to explain to the reviewer the logic behind a change

What?

If you access a root page that has two or more extra slashes at the end, such as https://foo.com//, errors throw in the URL constructor and Next.js will stop rendering.

TypeError: Failed to construct 'URL': Invalid URL

UA (Web Browser) location bar screenshot

Why?

Because URL constructor in asPathToSearchParams() utility function can't parse URL root-relative paths that have two or more extra slashes, throw an error.

The root cause is that the path string returned by getURL() can have two or more slashes at the beginning.

How?

Add processing to the asPathToSearchParams() utility function to replace two or more leading slashes with one slash.

Note

This bug has been around for over 7 years before this PR was created date
This bug does not occur in HTTP web browser environments, only in HTTPS web browser environments
I created a PR because this bug can't be fixed without addressing it on the Next.js side

Closes NEXT-
Fixes #

ijjk · 2024-09-16T07:19:08Z

Allow CI Workflow Run

approve CI run for commit: 4a0d267

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

huozhi · 2024-09-16T10:44:37Z

If you have more trailing slashes, even standard util will treat them as part of pathname, sounds correct it was thrown as invalid URL?

new URL('https://fooo.com//').pathname
>> '//'

takueof · 2024-09-16T12:22:17Z

@huozhi
Thank you for your reply.

Yes, but it doesn't happen in your example...

It occurs with the following pattern (this is the code pattern used within /packages/next/src/shared/lib/router/utils/as-path-to-search-params.ts):

new URL(asPath, 'http://n').searchParams

The following error should be thrown:

Uncaught TypeError: Failed to construct 'URL': Invalid URL

Screenshot:

new URL() throw by Google Chrome in JavaScript console

V8 (Blink), SpiderMonkey (Gecko) and JavaScriptCore (WebKit) all have the same behavior.

takueof · 2024-09-16T12:42:36Z

Initially, I thought that changing

new URL(asPath, 'https://n.com').searchParams

to

new URL(`https://n.com${asPath}`).searchParams

would complete the fix.

However, when I investigated deeper, I started to think that the fundamental solution might not be solved unless I changed getURL().

The reason is that the return value of getURL() is different for http:// and https://.

If we execute now getURL() on http://n.com//, / will be returned
If we execute now getURL() on https://n.com//, // will be returned

By fixing the core part, we can avoid problems that may occur in other code that uses the result of getURL().

huozhi · 2024-09-17T09:02:56Z

The test seem cannot demostrate the issue you describe, if I manually run in https mode and just visit the page, it looks working correctly 🤔

takueof · 2024-09-17T14:27:59Z

Oh, after splitting the test file into http and https, the e2e test is now working.

If you git pull the latest commit and run the following command, it does what I want:

pnpm build && pnpm test-start test/e2e/invalid-url-slash

I'm very sorry.

huozhi · 2024-09-21T10:59:04Z

Tests are passed regardless we have the changes

takueof · 2024-10-14T00:17:32Z

Hello, maintainers!
Thank you for considering my PR.

It's been about 4 weeks since I last commented.
If you don't mind, would it be possible for you to tell me about your current review status...?

Sorry for the inconvenience 🙏

ijjk · 2024-10-14T20:25:56Z

Failing test suites

Commit: 6a78b62

pnpm test-dev test/development/acceptance-app/undefined-default-export.test.ts

Undefined default export > should error when page component export is not valid

Expand output

● Undefined default export › should error when page component export is not valid

Expected Redbox but found none

  61 |     )
  62 |
> 63 |     await session.assertHasRedbox()
     |     ^
  64 |     expect(await session.getRedboxDescription()).toInclude(
  65 |       'The default export is not a React Component in "/server-with-errors/page-export/page"'
  66 |     )

  at Object.<anonymous> (development/acceptance-app/undefined-default-export.test.ts:63:5)

Read more about building and testing Next.js in contributing.md.

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

404 handling > custom _error > production mode > next export > should handle double slashes correctly
404 handling > custom _error > production mode > next export > should handle double slashes correctly with query
404 handling > custom _error > production mode > next export > should handle double slashes correctly with hash
404 handling > custom _error > production mode > next export > should handle backslashes correctly
404 handling > custom _error > production mode > next export > should handle mixed backslashes/forward slashes correctly
404 handling > pages/404 > production mode > production mode > next export > should handle double slashes correctly
404 handling > pages/404 > production mode > production mode > next export > should handle double slashes correctly with query
404 handling > pages/404 > production mode > production mode > next export > should handle double slashes correctly with hash
404 handling > pages/404 > production mode > production mode > next export > should handle backslashes correctly
404 handling > pages/404 > production mode > production mode > next export > should handle mixed backslashes/forward slashes correctly

Expand output

● 404 handling › custom _error › production mode › next export › should handle double slashes correctly

expect(received).toBe(expected) // Object.is equality

Expected: "//google.com"
Received: "/google.com"

  108 |     const browser = await webdriver(appPort, '//google.com')
  109 |     await didNotReload(browser)
> 110 |     expect(await browser.eval('window.location.pathname')).toBe(
      |                                                            ^
  111 |       isExport ? '//google.com' : '/google.com'
  112 |     )
  113 |     expect(await browser.eval('window.location.search')).toBe('')

  at Object.toBe (integration/repeated-slashes/test/index.test.js:110:60)

● 404 handling › custom _error › production mode › next export › should handle double slashes correctly with query

expect(received).toBe(expected) // Object.is equality

Expected: "//google.com"
Received: "/google.com"

  137 |     const browser = await webdriver(appPort, '//google.com?h=1')
  138 |     await didNotReload(browser)
> 139 |     expect(await browser.eval('window.location.pathname')).toBe(
      |                                                            ^
  140 |       isExport ? '//google.com' : '/google.com'
  141 |     )
  142 |     expect(await browser.eval('window.location.search')).toBe('?h=1')

  at Object.toBe (integration/repeated-slashes/test/index.test.js:139:60)

● 404 handling › custom _error › production mode › next export › should handle double slashes correctly with hash

expect(received).toBe(expected) // Object.is equality

Expected: "//google.com"
Received: "/google.com"

  161 |     const browser = await webdriver(appPort, '//google.com#hello')
  162 |     await didNotReload(browser)
> 163 |     expect(await browser.eval('window.location.pathname')).toBe(
      |                                                            ^
  164 |       isExport ? '//google.com' : '/google.com'
  165 |     )
  166 |     expect(await browser.eval('window.location.hash')).toBe('#hello')

  at Object.toBe (integration/repeated-slashes/test/index.test.js:163:60)

● 404 handling › custom _error › production mode › next export › should handle backslashes correctly

expect(received).toBe(expected) // Object.is equality

Expected: "//google.com"
Received: "/google.com"

  250 |     const browser = await webdriver(appPort, '/\\google.com')
  251 |     await didNotReload(browser)
> 252 |     expect(await browser.eval('window.location.pathname')).toBe(
      |                                                            ^
  253 |       isExport ? '//google.com' : '/google.com'
  254 |     )
  255 |     expect(await browser.eval('window.location.hash')).toBe('')

  at Object.toBe (integration/repeated-slashes/test/index.test.js:252:60)

● 404 handling › custom _error › production mode › next export › should handle mixed backslashes/forward slashes correctly

expect(received).toBe(expected) // Object.is equality

Expected: "///google.com"
Received: "/google.com"

  275 |     const browser = await webdriver(appPort, '/\\/google.com#hello')
  276 |     await didNotReload(browser)
> 277 |     expect(await browser.eval('window.location.pathname')).toBe(
      |                                                            ^
  278 |       isExport ? '///google.com' : '/google.com'
  279 |     )
  280 |     expect(await browser.eval('window.location.hash')).toBe('#hello')

  at Object.toBe (integration/repeated-slashes/test/index.test.js:277:60)

● 404 handling › pages/404 › production mode › production mode › next export › should handle double slashes correctly

expect(received).toBe(expected) // Object.is equality

Expected: "//google.com"
Received: "/google.com"

  108 |     const browser = await webdriver(appPort, '//google.com')
  109 |     await didNotReload(browser)
> 110 |     expect(await browser.eval('window.location.pathname')).toBe(
      |                                                            ^
  111 |       isExport ? '//google.com' : '/google.com'
  112 |     )
  113 |     expect(await browser.eval('window.location.search')).toBe('')

  at Object.toBe (integration/repeated-slashes/test/index.test.js:110:60)

● 404 handling › pages/404 › production mode › production mode › next export › should handle double slashes correctly with query

expect(received).toBe(expected) // Object.is equality

Expected: "//google.com"
Received: "/google.com"

  137 |     const browser = await webdriver(appPort, '//google.com?h=1')
  138 |     await didNotReload(browser)
> 139 |     expect(await browser.eval('window.location.pathname')).toBe(
      |                                                            ^
  140 |       isExport ? '//google.com' : '/google.com'
  141 |     )
  142 |     expect(await browser.eval('window.location.search')).toBe('?h=1')

  at Object.toBe (integration/repeated-slashes/test/index.test.js:139:60)

● 404 handling › pages/404 › production mode › production mode › next export › should handle double slashes correctly with hash

expect(received).toBe(expected) // Object.is equality

Expected: "//google.com"
Received: "/google.com"

  161 |     const browser = await webdriver(appPort, '//google.com#hello')
  162 |     await didNotReload(browser)
> 163 |     expect(await browser.eval('window.location.pathname')).toBe(
      |                                                            ^
  164 |       isExport ? '//google.com' : '/google.com'
  165 |     )
  166 |     expect(await browser.eval('window.location.hash')).toBe('#hello')

  at Object.toBe (integration/repeated-slashes/test/index.test.js:163:60)

● 404 handling › pages/404 › production mode › production mode › next export › should handle backslashes correctly

expect(received).toBe(expected) // Object.is equality

Expected: "//google.com"
Received: "/google.com"

  250 |     const browser = await webdriver(appPort, '/\\google.com')
  251 |     await didNotReload(browser)
> 252 |     expect(await browser.eval('window.location.pathname')).toBe(
      |                                                            ^
  253 |       isExport ? '//google.com' : '/google.com'
  254 |     )
  255 |     expect(await browser.eval('window.location.hash')).toBe('')

  at Object.toBe (integration/repeated-slashes/test/index.test.js:252:60)

● 404 handling › pages/404 › production mode › production mode › next export › should handle mixed backslashes/forward slashes correctly

expect(received).toBe(expected) // Object.is equality

Expected: "///google.com"
Received: "/google.com"

  275 |     const browser = await webdriver(appPort, '/\\/google.com#hello')
  276 |     await didNotReload(browser)
> 277 |     expect(await browser.eval('window.location.pathname')).toBe(
      |                                                            ^
  278 |       isExport ? '///google.com' : '/google.com'
  279 |     )
  280 |     expect(await browser.eval('window.location.hash')).toBe('#hello')

  at Object.toBe (integration/repeated-slashes/test/index.test.js:277:60)

Read more about building and testing Next.js in contributing.md.

takueof · 2024-10-15T00:00:23Z

@ijjk
Thank you for your comment.

I didn't expect this PR at this point to have such an impact on other features (testing).
I will create a patch PR in my local environment while doing full testing again.

I apologize for taking up your valuable time.

takueof · 2024-10-17T01:26:22Z

@ijjk (Cc: @huozhi)
Hello.
Thank you for letting me know about the test failure the other day.

After that, I reviewed the patch and changed the patch so that the existing test set would not fail and the new test set would pass.

I apologize for the inconvenience, but I would appreciate it if you could review the PR again 🙏

The following screenshots are the results of the retest:

`pnpm test-dev test/development/acceptance-app/undefined-default-export.test.ts`

`pnpm test test/integration/repeated-slashes/test/index.test.js`

takueof · 2024-11-09T23:53:54Z

Hello.
I tried running the same tests as CI in my local environment, but the following command seems to give unintended results.

pnpm test-start test

I looked at the testing section in the documentation, but there was no command written to run exhaustive tests equivalent to CI.

I apologize for the inconvenience, but would it be possible for you to give me a list of commands (I believe there are probably multiple) to run the same tests in the local environment as those running in CI...?

takueof · 2024-12-09T14:17:15Z

Hello, can anyone answer the above question...?

takueof · 2024-12-18T20:13:27Z

@ijjk @huozhi

Please Help me.

Is it possible to read a comment mentioned above and receive a reply...?

…root-index-page-have-multiple-slash

ijjk added tests type: next labels Sep 16, 2024

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch from a559e15 to f807db6 Compare September 16, 2024 07:19

takueof marked this pull request as ready for review September 16, 2024 07:44

This comment was marked as resolved.

Sign in to view

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch from f807db6 to b211556 Compare September 16, 2024 08:23

huozhi added CI approved Approve running CI for fork and removed CI approved Approve running CI for fork labels Sep 16, 2024

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch 6 times, most recently from 80de3fd to 23fc909 Compare September 17, 2024 03:19

This comment was marked as resolved.

Sign in to view

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch 3 times, most recently from a2879fd to 7a8c3ee Compare September 17, 2024 14:23

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch 5 times, most recently from 1ea30c7 to ed53e03 Compare September 21, 2024 02:52

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch 3 times, most recently from 26b30aa to 6691621 Compare October 3, 2024 22:47

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch from 6691621 to 6a78b62 Compare October 14, 2024 00:10

huozhi added the CI approved Approve running CI for fork label Oct 14, 2024

huozhi removed the CI approved Approve running CI for fork label Oct 14, 2024

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch from 6a78b62 to 14f9732 Compare October 15, 2024 10:55

takueof changed the title ~~Fix invalid pathname slashes in "getURL()" utility~~ Don't throw errors when we give "asPathToSearchParams()" an invalid path with multiple leading slashes. Oct 16, 2024

takueof changed the title ~~Don't throw errors when we give "asPathToSearchParams()" an invalid path with multiple leading slashes.~~ Don't throw errors when we give "asPathToSearchParams()" an invalid path with multiple leading slashes Oct 16, 2024

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch 2 times, most recently from 965ef85 to 1877dba Compare October 16, 2024 21:33

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch 2 times, most recently from 2b25d12 to 1632a03 Compare October 25, 2024 21:19

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch from 1632a03 to 70b12cc Compare November 7, 2024 23:52

huozhi added CI approved Approve running CI for fork and removed CI approved Approve running CI for fork labels Nov 8, 2024

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch from 70b12cc to bba9faa Compare November 8, 2024 04:23

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch from bba9faa to 08c8a7e Compare December 17, 2024 02:48

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch from 08c8a7e to 7211b4a Compare January 4, 2025 12:34

bugfix: Remove invalid pathname slashes in asPathToSearchParams()

1681620

takueof force-pushed the fix/wrong-value-set-to-asPath-when-root-index-page-have-multiple-slash branch from 7211b4a to 1681620 Compare February 18, 2025 14:02

takueof added 2 commits April 1, 2025 08:49

Merge branch 'vercel:canary' into fix/wrong-value-set-to-asPath-when-…

3d8d488

…root-index-page-have-multiple-slash

Merge branch 'vercel:canary' into fix/wrong-value-set-to-asPath-when-…

4a0d267

…root-index-page-have-multiple-slash

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't throw errors when we give "asPathToSearchParams()" an invalid path with multiple leading slashes #70144

Don't throw errors when we give "asPathToSearchParams()" an invalid path with multiple leading slashes #70144

takueof commented Sep 16, 2024 •

edited

Loading

ijjk commented Sep 16, 2024 •

edited

Loading

This comment was marked as resolved.

huozhi commented Sep 16, 2024

takueof commented Sep 16, 2024

takueof commented Sep 16, 2024

huozhi commented Sep 17, 2024

This comment was marked as resolved.

takueof commented Sep 17, 2024

huozhi commented Sep 21, 2024

takueof commented Oct 14, 2024

ijjk commented Oct 14, 2024 •

edited

Loading

takueof commented Oct 15, 2024

takueof commented Oct 17, 2024

takueof commented Nov 9, 2024

takueof commented Dec 9, 2024

takueof commented Dec 18, 2024

Don't throw errors when we give "asPathToSearchParams()" an invalid path with multiple leading slashes #70144

Are you sure you want to change the base?

Don't throw errors when we give "asPathToSearchParams()" an invalid path with multiple leading slashes #70144

Conversation

takueof commented Sep 16, 2024 • edited Loading

For Contributors

Improving Documentation

Adding or Updating Examples

Fixing a bug

For Maintainers

What?

Why?

How?

Note

ijjk commented Sep 16, 2024 • edited Loading

This comment was marked as resolved.

huozhi commented Sep 16, 2024

takueof commented Sep 16, 2024

takueof commented Sep 16, 2024

huozhi commented Sep 17, 2024

This comment was marked as resolved.

takueof commented Sep 17, 2024

huozhi commented Sep 21, 2024

takueof commented Oct 14, 2024

ijjk commented Oct 14, 2024 • edited Loading

Failing test suites

takueof commented Oct 15, 2024

takueof commented Oct 17, 2024

pnpm test-dev test/development/acceptance-app/undefined-default-export.test.ts

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

takueof commented Nov 9, 2024

takueof commented Dec 9, 2024

takueof commented Dec 18, 2024

takueof commented Sep 16, 2024 •

edited

Loading

ijjk commented Sep 16, 2024 •

edited

Loading

ijjk commented Oct 14, 2024 •

edited

Loading

`pnpm test-dev test/development/acceptance-app/undefined-default-export.test.ts`

`pnpm test test/integration/repeated-slashes/test/index.test.js`