Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP 307 redirects break when normalizing URLs #9350

Closed
lxhom opened this issue Mar 7, 2023 · 2 comments · Fixed by #9351
Closed

HTTP 307 redirects break when normalizing URLs #9350

lxhom opened this issue Mar 7, 2023 · 2 comments · Fixed by #9351

Comments

@lxhom
Copy link

lxhom commented Mar 7, 2023

Describe the bug

HTTP 307 description if you're not familiar with it

HTTP 307 Temporary Redirect redirect status response code indicates that the resource requested has been temporarily moved to the URL given by the Location headers.

The method and the body of the original request are reused to perform the redirected request. In the cases where you want the method used to be changed to GET, use 303 See Other instead. This is useful when you want to give an answer to a PUT method that is not the uploaded resources, but a confirmation message (like "You successfully uploaded XYZ").

(from MDN: HTTP 307 Temporary Redirect)

My use case is a bit complicated, but I need to redirect a POST request like this:

POST server1.com/path -> HTTP 307, Location: server2.com/path
POST server2.com/path -> HTTP 200

This works with SvelteKit, except when it normalizes paths:

POST server1.com/path/ -> HTTP 307, Location: server2.com/path/
POST server2.com/path/ -> HTTP 301, Location: server2.com/path, X-SvelteKit-Normalize: 1
GET  server2.com/path  -> HTTP 405

This is due to this condition:

if (normalized !== url.pathname && !state.prerendering?.fallback) {
return new Response(undefined, {
status: 301,
headers: {
'x-sveltekit-normalize': '1',
location:
// ensure paths starting with '//' are not treated as protocol-relative
(normalized.startsWith('//') ? url.origin + normalized : normalized) +
(url.search === '?' ? '' : url.search)
}
});
}

This should check if the original request is a 307, and use 307 too if that is the case.

Reproduction

Without normalization:

❯ curl -Lvd "a" localhost:3000/from
> POST /from HTTP/1.1
> Host: localhost:3000
> 
< HTTP/1.1 307 Temporary Redirect
< location: http://localhost:3000/to
< 
* Issue another request to this URL: 'http://localhost:3000/to'
> POST /to HTTP/1.1
> Host: localhost:3000
> 
< HTTP/1.1 200 OK
< 
Posted!

With normalization:

❯ curl -Lvd "a" localhost:3000/from/
> POST /from/ HTTP/1.1
> Host: localhost:3000
> 
< HTTP/1.1 301 Moved Permanently
< location: /from
< x-sveltekit-normalize: 1
< 
* Issue another request to this URL: 'http://localhost:3000/from'
* Switch from POST to GET
> GET /from HTTP/1.1
> Host: localhost:3000
> 
< HTTP/1.1 405 Method Not Allowed
< 
GET method not allowed    

For example code see lxhom/sk-307-demo@f8d580e (the commit only includes the changed files from a base SK project)

Logs

No response

System Info

System:
    OS: Linux 6.1 Arch Linux (btw)
    CPU: (4) x64 AMD A8-7410 APU with AMD Radeon R5 Graphics
    Memory: 818.83 MB / 6.74 GB
    Container: Yes
    Shell: 5.8 - /usr/local/bin/zsh
  Binaries:
    Node: 19.4.0 - ~/.nvm/versions/node/v19.4.0/bin/node
    Yarn: 1.22.19 - /usr/bin/yarn
    npm: 9.2.0 - ~/.nvm/versions/node/v19.4.0/bin/npm
  Browsers:
    Firefox: 109.0
    Chrome: 110

Severity

serious, but I can work around it

Additional Information

I'd submit this as a PR (because this seems really easy), but I'm rather new to SK and I don't know if this would break other stuff.

@Rich-Harris
Copy link
Member

Honestly it sounds like we should just be using 307s in the first place — I don't recall why we originally chose 301 but it's very possible that there's no real reasoning behind it. The current behaviour is straightforwardly buggy.

Or maybe 308s, since we want to transfer link juice. A permanent redirect carries the risk that the app developer will change trailingSlash from always to never or vice versa, which combined with any cached redirects will cause an infinite redirect loop, but a) browsers clear the redirect cache when that happens so it self-heals immediately, and b) we're already using 301s which are equally permanent, and no-one has complained yet.

Does that sound reasonable?

Rich-Harris added a commit that referenced this issue Mar 7, 2023
* return 308s instead of 301s when normalising paths - fixes #9350

* remove only
@lxhom
Copy link
Author

lxhom commented Mar 7, 2023

Looks perfect, that fixes it! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants