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

[takueof]

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.

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]

Allow CI Workflow Run

• approve CI run for commit: bba9faa

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

[huozhi]

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]

@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:

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

[takueof]

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]

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]

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]

Tests are passed regardless we have the changes

[takueof]

@huozhi
Thanks to reply.

I also confirmed that the e2e test did not fail using the old code.

Then I remembered that the case I encountered was a Pages Router, not an App Router.

When I rewrote the e2e test using Pages Router, changed utils.ts to its pre-patch state, and checked again, the e2e test failed.

After applying the patch to utils.ts, the e2e test now passes.

Thank you for making me realize that.

I have pushed the latest code, so please test it yourself 🙏

[takueof]

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]

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]

@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]

@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