Issue #5029 Relative Redirection #5038
Conversation
Provide option to allow relative redirection
Fixed checkstyle
| @@ -74,6 +74,7 @@ | |||
| <Set name="requestCookieCompliance"><Call class="org.eclipse.jetty.http.CookieCompliance" name="valueOf"><Arg><Property name="jetty.httpConfig.requestCookieCompliance" deprecated="jetty.httpConfig.cookieCompliance" default="RFC6265"/></Arg></Call></Set> | |||
| <Set name="responseCookieCompliance"><Call class="org.eclipse.jetty.http.CookieCompliance" name="valueOf"><Arg><Property name="jetty.httpConfig.responseCookieCompliance" default="RFC6265"/></Arg></Call></Set> | |||
| <Set name="multiPartFormDataCompliance"><Call class="org.eclipse.jetty.server.MultiPartFormDataCompliance" name="valueOf"><Arg><Property name="jetty.httpConfig.multiPartFormDataCompliance" default="RFC7578"/></Arg></Call></Set> | |||
| <Set name="relativeRedirectionAllowed"><Property name="jetty.httpConfig.relativeRedirectionAllowed" default="false"/></Set> | |||
There was a problem hiding this comment.
I don't like the name.
It's not whether relative redirects are allowed or not.
It's about whether we generate relative redirects or not.
generateRelativeRedirects?
useRelativeRedirects?
There was a problem hiding this comment.
In the existing releases, If the application uses ...
response.sendRedirect("/alt/path"); // becomes an absolute-uri
response.sendRedirect("www.machine.com/alt/path"); // becomes an absolute-uri by adding scheme
response.sendRedirect("https://www.machine.com/alt/path"); // stays an absolute-uriWith this change, the default is false, which preserves the behavior.
If they set it to true, then the following happens ...
response.sendRedirect("/alt/path"); // no change to whats added, it is sent as `Location: /alt/path`
response.sendRedirect("www.machine.com/alt/path"); // no change to whats added, it is sent as `Location: www.machine.com/alt/path`
response.sendRedirect("https://www.machine.com/alt/path"); // no change to whats added, it is sent as `Location: https://www.machine.com/alt/path`Perhaps the name should indicate that Jetty, by default, ensures that the Location header is an absolute-uri, but if we change this configuration, it takes the Location as-is.
ensureAbsoluteUriLocation with a default of true?
There was a problem hiding this comment.
The behaviour is definitely an 'Allow', as it just decides if a passed relative location can be allowed to remain relative.
It could be inverted to an ensure (or enforce?) but then isEnsureAbsoluteUriLocation is both a mouthful and doesn't scan.
After chatting to @sbordet online, I have renamed to relativeRedirectAllowed... but I could be pushed to allowRelativeRedirect if we can stomach isAllowRelativeRedirect ?
|
@gregw also, are we sure that |
|
HTTP -> HTTPS redirection works because it passes an absolute URL: https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java#L649-L651 |
Provide option to allow relative redirection.