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

Redirect with a 308 rather than 301 in a specific case #1342

Merged
merged 1 commit into from Nov 25, 2018

Conversation

pgjones
Copy link
Member

@pgjones pgjones commented Aug 9, 2018

With strict slashes enabled and the route defined with a trailing
slash e.g. /a/ a request to /a currently results in a 301 response
to redirect to /a/. This is good for GET and HEAD requests, but
problematic for other verbs as clients will typically treat a 301
response as meaning redirect as a GET verb as noted in RFC 2616
section-10.3.2 (RFC 2616 stated this was erroneous, and then RFC 7231
allowed this behaviour). RFC 7538 later introduced a 308 code to work
as 301 was originally intended.

In principle I think the 301 could be changed to 308 wholesale, but
that would likely create some confusing changes to existing code. So I
hope this is a good compromise.

Note: I need to add a changelog entry. However first I'd like to see if this makes sense to others...

@pgjones
Copy link
Member Author

pgjones commented Aug 9, 2018

I don't think the build failure is related to this change.

@davidism
Copy link
Member

Makes sense, this has come up before. I'm fine with changing this to 308 by default everywhere. What confusing changes would that cause?

@pgjones
Copy link
Member Author

pgjones commented Aug 11, 2018

I couldn't find a previous discussion, could you link?

I've seen a lot of test cases that assert there is a redirect with a 301 status code, it isn't really confusing (so I'll reword) but I suspect this would catch people out. That said I think 308 everywhere makes more sense. I'll update.

@davidism
Copy link
Member

According to MDN, 308 is supported in all browsers except IE on Windows <= 8. Windows 8 is EOL. If anyone still needs to support very old browsers, they can do RequestRedirect.code = 301.

@davidism davidism added this to the 0.15 milestone Nov 22, 2018
With strict slashes enabled and the route defined with a trailing
slash e.g. `/a/` a request to `/a` currently results in a 301 response
to redirect to `/a/`. This is good for GET and HEAD requests, but
problematic for other verbs as clients will typically treat a 301
response as meaning redirect as a GET verb as noted in RFC 2616
section-10.3.2 (RFC 2616 stated this was erroneous, and then RFC 7231
allowed this behaviour). RFC 7538 later introduced a 308 code to work
as 301 was originally intended. As 308 is now widely supported, 301
can be changed to 308.
@davidism
Copy link
Member

davidism commented Nov 23, 2018

Realized that the test client doesn't handle 308, and doesn't pass the body on for 307 or 308. Started to work on that and realized I can't find an exact definition of what data must be copied to the new request. At minimum it's the body and content type so the next request will be able to see form data.

@davidism
Copy link
Member

Did a simple experiment with the following. Using the network dev tool, it looks like Firefox sends the body and all headers (except for location) to the redirected URL.

@Request.application
def app(request):
    if request.path == "/":
        return redirect("/other", code=307)
    return Response("hello")
data = new FormData()
data.append("name": "foo")
fetch("/", {method: "POST", body: data, headers: {"x-other": "bar"}})

@davidism
Copy link
Member

davidism commented Nov 23, 2018

Apparently Firefox sends the headers for 302 as well (even the content type, just not the body), so the test client needs a bigger change. Chrome strips the content type though.

@davidism
Copy link
Member

OK, got way too deep with the test client rewrite, I'm just going to make that its own PR. Force pushed to resolve conflicts here.

@davidism davidism merged commit 2a8c816 into pallets:master Nov 25, 2018
@davidism
Copy link
Member

#1402

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants