fix(backend): Sanitize slashes in URL paths - #3982

🦋 Changeset detected

Latest commit: 6418a29

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Please note that I'm unsure whether that's the right way to fix the issue. At least, it's been working great for my Astro site. It may be better to handle the duplicate slashes at the middleware layer.

@mlafeldt Hello, and thanks for your contribution!
Can we have more info on this issue?
How did you end up with this double-slashed URL (https://myproject.pages.dev//v1.0.0/foo/bar.gz)? Is this a CF Pages default?

@mlafeldt Hello, and thanks for your contribution!
Can we have more info on this issue?
How did you end up with this double-slashed URL (https://myproject.pages.dev//v1.0.0/foo/bar.gz)? Is this a CF Pages default?

The double slash is part of the curl request I'm sending to Astro (see the HEAD example above). Anyone could be sending such a request to CF. IIRC, I first used multiple slashes by accident when I noticed the crashing middleware.

I haven't checked CF's implementation of new URL(path, base) (the code that throws the reported exception), but I can already tell you the behavior is inconsistent between JS runtimes.

Deno:

❯ deno
Deno 1.45.5
exit using ctrl+d, ctrl+c, or close()
REPL is running with all permissions allowed.
To specify permissions, run `deno repl` with allow flags.
> new URL('https://example.com/////foo/bar')
URL {
  href: "https://example.com/////foo/bar",
  origin: "https://example.com",
  protocol: "https:",
  username: "",
  password: "",
  host: "example.com",
  hostname: "example.com",
  port: "",
  pathname: "/////foo/bar",
  hash: "",
  search: ""
}
> new URL('/////foo/bar', 'https://example.com')
Uncaught TypeError: Invalid URL: '/////foo/bar' with base 'https://example.com'
    at getSerialization (ext:deno_url/00_url.js:98:11)
    at new URL (ext:deno_url/00_url.js:405:27)
    at <anonymous>:1:22

Node

❯ node
Welcome to Node.js v22.6.0.
Type ".help" for more information.
> new URL('https://example.com/////foo/bar')
URL {
  href: 'https://example.com/////foo/bar',
  origin: 'https://example.com',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'example.com',
  hostname: 'example.com',
  port: '',
  pathname: '/////foo/bar',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}
> new URL('/////foo/bar', 'https://example.com')
URL {
  href: 'https://foo/bar',
  origin: 'https://foo',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'foo',
  hostname: 'foo',
  port: '',
  pathname: '/bar',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''

Trying to replicate this issue on my end. I have an app deployed to CF pages as well - https://astro-clerk-template.pages.dev

I tried to tail it and sent a request via curl:

It's not throwing any errors on my end. Might be missing something.

Deployed a fresh Astro app without @clerk/astro as well and can confirm double slash works - https://astro-test-slashes.pages.dev//api/ping.json

Astro also made an attempt to handle duplicate slashes but reverted it

@wobsoriano I created a quick example that reproduces the problem: https://github.com/mlafeldt/fumbling-fusion

This works:

❯ curl -i https://fumbling-fusion.pages.dev/v1.0.0/xxx
HTTP/2 200 
date: Mon, 19 Aug 2024 20:21:23 GMT
content-type: text/plain;charset=UTF-8
content-length: 14
x-clerk-auth-message: 
x-clerk-auth-reason: dev-browser-missing
x-clerk-auth-reason: dev-browser-missing
x-clerk-auth-status: signed-out
x-clerk-auth-status: signed-out
x-clerk-auth-token: 
report-to: {"endpoints":[{"url":"https:\/\/a.nel.cloudflare.com\/report\/v4?s=9eKiN2HH08jMEjvEkZW7sr1F3immx8y9i%2Fgh5N%2BKCPx3PLKC2Gzz9unBXlmCgmSQRQPoyMjbvRR5koqdRb%2Fz3M0JtFWt669jkoYKoq%2Bd3cOAb%2BY8d3k3sc2yK7ozM4VUxQzXEWxrNV2xpEyk"}],"group":"cf-nel","max_age":604800}
nel: {"success_fraction":0,"report_to":"cf-nel","max_age":604800}
server: cloudflare
cf-ray: 8b5cd984b87962c7-HAM
alt-svc: h3=":443"; ma=86400

{"path":"xxx"}

This does not:

❯ curl -i https://fumbling-fusion.pages.dev//v1.0.0/xxx
HTTP/2 500 
date: Mon, 19 Aug 2024 20:21:19 GMT
content-length: 0
report-to: {"endpoints":[{"url":"https:\/\/a.nel.cloudflare.com\/report\/v4?s=23%2FEL%2BTGCohSPRlt0%2BP6YSDsL7vjiPsp6HGG4jYKDv3CxuLaICRrQZXr339MA9eDNgQuoLRJ5RyBjyhunRs5jMPeFH6liZscCVhHwfYUVA8YB7wSg8jxLThhledfngOAZWkvPAkZlfnizFON"}],"group":"cf-nel","max_age":604800}
nel: {"success_fraction":0,"report_to":"cf-nel","max_age":604800}
server: cloudflare
cf-ray: 8b5cd96d1a9262bf-HAM
alt-svc: h3=":443"; ma=86400

And the reported backtrace again:

GET https://fumbling-fusion.pages.dev//v1.0.0/xxx - Ok @ 8/19/2024, 10:21:19 PM
  (error) 20:21:19 [ERROR] TypeError: Invalid URL string.
    at new ClerkUrl (_astro-internal_middleware.mjs:2382:16)
    at createClerkUrl (_astro-internal_middleware.mjs:2388:10)
    at ClerkRequest.deriveUrlFromHeaders (_astro-internal_middleware.mjs:2419:12)
    at new ClerkRequest (_astro-internal_middleware.mjs:2394:26)
    at createClerkRequest (_astro-internal_middleware.mjs:2433:54)
    at astroMiddleware (_astro-internal_middleware.mjs:3656:26)
    at applyHandle (chunks/index_yfcIUlhQ.mjs:353:22)
    at chunks/index_yfcIUlhQ.mjs:372:18
    at server (_astro-internal_middleware.mjs:3884:31)
    at applyHandle (chunks/index_yfcIUlhQ.mjs:353:22)

Happy to provide more info.

Node

❯ node
Welcome to Node.js v22.6.0.
Type ".help" for more information.
> new URL('https://example.com/////foo/bar')
URL {
  href: 'https://example.com/////foo/bar',
  origin: 'https://example.com',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'example.com',
  hostname: 'example.com',
  port: '',
  pathname: '/////foo/bar',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}
> new URL('/////foo/bar', 'https://example.com')
URL {
  href: 'https://foo/bar',
  origin: 'https://foo',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'foo',
  hostname: 'foo',
  port: '',
  pathname: '/bar',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''

Two small comments on this example:

• It looks like it's working with /////foo/bar but if you use the //v1.0.0/xxx it still throws. It has to do with the dots.
• Even though it looks like it's working, if you look at the URL it creates the href is https://foo/bar which is wrong.

Relevant: nodejs/node#30776

@wobsoriano @anagstef I think I found a good solution to address the issue without messing with the URL at all. Please take a look at my latest commit: ac30531

@mlafeldt I'm curious, does this issue affect your project? Are you using URLs with double slashes in your app?

@mlafeldt I'm curious, does this issue affect your project? Are you using URLs with double slashes in your app?

Nope, routing on double slashes is not a good practice IMO, but I really prefer my app to not crash in case someone sends such a request (which I can't control). 😄 Hence the PR.

In general, the handling of multiple slashes in URL paths doesn't seem to be standardized. For example, github.com//////foo works fine, while x.com//foo returns a plain 400 error. This is why I like the current fix that leaves the URL as-is, allowing frameworks like Astro to do their routing as they please without Clerk interfering.

So there's a repro of the crash, a minimal non-invasive fix, and a test for that fix. What else do you need to move this forward? Happy to assist. 🙏

Hi @mlafeldt, sorry for the late reply. I was trying to replicate the issue with my astro + clerk app deployed to CF pages by duplicating slashes (https://astro-clerk-template.pages.dev////api////user.json) but it seems to automatically normalize it based on URL normalization settings. Ok this results to 404

The fix is minimal and non-invasive as you said, so I just approved the workflow to run the e2e tests and we can proceed after that.

Edit: Okay did some more tests. Without the Clerk integration and doing multiple slashes leads to a 404 on CF. Vercel normalizes it.

1. Vercel = https://test-astro-cf.vercel.app///api///user.json -> https://test-astro-cf.vercel.app/api/user.json
2. CF pages = https://test-astro-cf-8h1.pages.dev///api///user.json -> 404

@mlafeldt updated the changeset message, hope you don't mind ✌🏼

@wobsoriano Thanks!

Re Cloudflare: With a custom domain, I had to enable "Normalize URLs to origin" for multiple slashes to be collapsed.

However, when using the pages.dev domain, you can't configure normalization AFAIK.

As for the number of slashes, it's actually a bit more complicated. 😃

But I still think the fix makes sense in any case.

@wobsoriano Awesome! Looking forward to the next release. 🚀