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

Revise redir status code shortcuts #5858

Open
francislavoie opened this issue Oct 5, 2023 · 7 comments
Open

Revise redir status code shortcuts #5858

francislavoie opened this issue Oct 5, 2023 · 7 comments
Labels
discussion 💬 The right solution needs to be found documentation 📚 Improvements or additions to documentation

Comments

@francislavoie
Copy link
Member

See https://caddyserver.com/docs/caddyfile/directives/redir

Currently, we only have shortcuts for temporary -> 302 Found and permanent -> 301 Moved Permanently.

This is incomplete, because commonly used is also 307 Temporary Redirect and 308 Permanent Redirect. Shortcuts for those would probably be desirable.

We should also document what happens to the method in each of the cases (or what clients should be doing, if they're conformant), because that's a differentiating factor.

@francislavoie francislavoie added discussion 💬 The right solution needs to be found documentation 📚 Improvements or additions to documentation labels Oct 5, 2023
dheerajsingh0 added a commit to dheerajsingh0/caddy that referenced this issue Oct 7, 2023
@dheerajsingh0
Copy link

Hi @francislavoie , I have created the PR for the above issue. Can you please look into the PR and provide feedback?

PR Link - #5868

dheerajsingh0 added a commit to dheerajsingh0/caddy that referenced this issue Oct 10, 2023
@mholt
Copy link
Member

mholt commented Oct 11, 2023

What's wrong with 302 + 301 though? I don't see 307 or 308 used nearly as often (though I agree they are semantically sensible, albeit newer and less traditional). Do 301+302 not work?

@francislavoie
Copy link
Member Author

@mholt we use 308 for the HTTP->HTTPS redirects, for example. The difference is semantically important relating to how the HTTP method behaves for form submissions and such.

@mholt
Copy link
Member

mholt commented Oct 11, 2023

I still don't really understand the differences between them 😄 And if I did once, I have since forgotten. (EDIT: Sorry; I know you linked it but we were in the hospital having a baby when I saw this issue.)

What is the use case for opening this issue and requesting this? I know you probably see a lot in our forums, I am just trying to understand the motivation.

@mholt
Copy link
Member

mholt commented Oct 21, 2023

(Apologies for my lack of headspace earlier. I edited my post to clarify that on that day we were in the hospital having a baby, with complications that kept us up for a few days.)

Ok, now I get it. 307+308 means the client must preserve the method when conducting the redirect. The HTTP spec for 301+302 actually already requires the method and body to remain unchanged when redirecting. 307+308 were invented "for historical reasons."

Francis explained to me that 301+302 are useful when you want to convey to the client a new location for a resource, like an uploaded file or something. Then future clients can use a GET going forward, even if POST was used to upload.

It's not clear to me which status code should be "the" temporary/permanent redirects if we were to choose one each. Are we seeing cases where people are having errors with our current choice of "temporary" and "permanent"?

@francislavoie
Copy link
Member Author

francislavoie commented Oct 21, 2023

One case I find our current setup misleading is when users want to set up manual HTTP->HTTPS redirects. You'd think you want permanent for that, but actually we use 308 for the auto-https HTTP->HTTPS redirects even though we don't have a shortcut name for that, and we don't use 308 by default when no status is specified (to a certain amount of surprise/confusion). We also don't document anywhere that we use 308 for those.

I'm trying to find some links to the forums where some of this confusion happened, but it was a while back and searching the forums is a bit of a struggle. Here's one though https://caddy.community/t/writing-dry-https-upgrade-config-with-strip-www-feature/15261/5

It's too late to change the default to 308 now as it would be a breaking change with potentially weird/surprising side effects for some people (but maybe it would be fine, I dunno, feels risky though). But for Caddy v3 or whatever, might be something to do.

@mholt
Copy link
Member

mholt commented Oct 21, 2023

I'm actually kind of inclined to change the 301+302s to 307+308 -- I wouldn't consider it a breaking change since the spec'ed behavior for 301+302 is the same as 307+308, i.e. the client must preserve the method and body. If the number is the problem, but the defined behavior is the same, we might as well just change the number we return.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 💬 The right solution needs to be found documentation 📚 Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants