-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Issue #11014 - Cleanup of relative redirect handling Jetty-12 #11019
Conversation
+ Handle request relative redirects + Moved to Response + Changed default to allow relative
+ Handle request relative redirects + Moved to Response + Changed default to allow relative
+ Handle request relative redirects + Moved to Response + Changed default to allow relative
+ Handle request relative redirects + Moved to Response + Changed default to allow relative
+ Handle request relative redirects + Moved to Response + Changed default to allow relative
+ Handle request relative redirects + Moved to Response + Changed default to allow relative
@@ -76,7 +76,7 @@ public class HttpConfiguration implements Dumpable | |||
private CookieCompliance _requestCookieCompliance = CookieCompliance.RFC6265; | |||
private CookieCompliance _responseCookieCompliance = CookieCompliance.RFC6265; | |||
private boolean _notifyRemoteAsyncErrors = true; | |||
private boolean _relativeRedirectAllowed; | |||
private boolean _relativeRedirectAllowed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(for other reviewers) This changes the default to always use relative redirects (path only).
Prior versions of Jetty would try to always use absolute URIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind the default behavior change, but maybe it should be justified with a comment?
@Deprecated | ||
static String toRedirectURI(Request request, String location) | ||
{ | ||
if (!URIUtil.hasScheme(location) && !request.getConnectionMetaData().getHttpConfiguration().isRelativeRedirectAllowed()) | ||
{ | ||
StringBuilder url = new StringBuilder(128); | ||
HttpURI uri = request.getHttpURI(); | ||
URIUtil.appendSchemeHostPort(url, uri.getScheme(), Request.getServerName(request), Request.getServerPort(request)); | ||
|
||
if (location.startsWith("/")) | ||
{ | ||
// absolute in context | ||
location = URIUtil.normalizePathQuery(location); | ||
} | ||
else | ||
{ | ||
// relative to request | ||
String path = uri.getPath(); | ||
String parent = (path.endsWith("/")) ? path : URIUtil.parentPath(path); | ||
location = URIUtil.normalizePathQuery(URIUtil.addEncodedPaths(parent, location)); | ||
if (location != null && !location.startsWith("/")) | ||
url.append('/'); | ||
} | ||
|
||
if (location == null) | ||
throw new IllegalStateException("redirect path cannot be above root"); | ||
url.append(location); | ||
|
||
location = url.toString(); | ||
} | ||
// TODO do we need to do request relative without scheme? | ||
|
||
return location; | ||
return Response.toRedirectURI(request, location); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have left the original code, and not moved it to Response
.
The response is never used in the implementation, and enforces that in order to build a redirect URI, you need a Request
(not a Response
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbordet, Sending a redirect is a response function and it naturally lives next to the sendRedirect methods. Plus we often pass the request and the response into these static methods e.g. Response.sendRedirect(Request, Response, Callback, location)
jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java
Show resolved
Hide resolved
@@ -76,7 +76,7 @@ public class HttpConfiguration implements Dumpable | |||
private CookieCompliance _requestCookieCompliance = CookieCompliance.RFC6265; | |||
private CookieCompliance _responseCookieCompliance = CookieCompliance.RFC6265; | |||
private boolean _notifyRemoteAsyncErrors = true; | |||
private boolean _relativeRedirectAllowed; | |||
private boolean _relativeRedirectAllowed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind the default behavior change, but maybe it should be justified with a comment?
Fixes #11014