Issue #11014 - Cleanup of relative redirect handling Jetty-12 (#11019)

* Cleanup of relative redirect handling #11014
+ Handle request relative redirects
+ Moved to Response
+ Changed default to allow relative
* Updates to javadoc

jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectPatternRule.java

@@ -17,7 +17,6 @@
 
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpStatus;
-import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Response;
 import org.eclipse.jetty.util.Callback;
 import org.eclipse.jetty.util.annotation.Name;
@@ -88,7 +87,7 @@ protected boolean handle(Response response, Callback callback)
             {
                 String location = getLocation();
                 response.setStatus(getStatusCode());
-                response.getHeaders().put(HttpHeader.LOCATION, Request.toRedirectURI(this, location));
+                response.getHeaders().put(HttpHeader.LOCATION, Response.toRedirectURI(this, location));
                 callback.succeeded();
                 return true;
             }

jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectRegexRule.java

@@ -18,7 +18,6 @@
 
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpStatus;
-import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Response;
 import org.eclipse.jetty.util.Callback;
 import org.eclipse.jetty.util.annotation.Name;
@@ -86,7 +85,7 @@ protected boolean handle(Response response, Callback callback)
             {
                 String target = matcher.replaceAll(getLocation());
                 response.setStatus(_statusCode);
-                response.getHeaders().put(HttpHeader.LOCATION, Request.toRedirectURI(this, target));
+                response.getHeaders().put(HttpHeader.LOCATION, Response.toRedirectURI(this, target));
                 callback.succeeded();
                 return true;
             }

jetty-core/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/RedirectRegexRuleTest.java

@@ -71,7 +71,7 @@ public void testLocationWithPathReplacement() throws Exception
 
         HttpTester.Response response = HttpTester.parseResponse(_connector.getResponse(request));
         assertEquals(HttpStatus.FOUND_302, response.getStatus());
-        assertEquals("http://localhost/docs/top.html", response.get(HttpHeader.LOCATION));
+        assertEquals("/docs/top.html", response.get(HttpHeader.LOCATION));
     }
 
     @Test

jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java

@@ -277,7 +277,9 @@ public AuthenticationState validateRequest(Request request, Response response, C
                 // Redirect to original request
                 Session session = request.getSession(false);
                 HttpURI savedURI = (HttpURI)session.getAttribute(__J_URI);
-                String originalURI = savedURI != null ? savedURI.asString() : Request.getContextPath(request);
+                String originalURI = savedURI != null
+                    ? savedURI.getPathQuery()
+                    : Request.getContextPath(request);
                 if (originalURI == null)
                     originalURI = "/";
                 UserAuthenticationSent formAuth = new UserAuthenticationSent(getAuthenticationType(), user);

jetty-core/jetty-security/src/test/java/org/eclipse/jetty/security/FormAuthenticatorTest.java

@@ -129,17 +129,17 @@ public void testLoginRedirect() throws Exception
 
         response = _connector.getResponse("GET /ctx/any/user HTTP/1.0\r\nHost:host:8888\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/login"));
+        assertThat(response, containsString("Location: /ctx/login"));
         assertThat(response, containsString("Set-Cookie: JSESSIONID="));
 
         response = _connector.getResponse("GET /ctx/known/user HTTP/1.0\r\nHost:host:8888\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/login"));
+        assertThat(response, containsString("Location: /ctx/login"));
         assertThat(response, containsString("Set-Cookie: JSESSIONID="));
 
         response = _connector.getResponse("GET /ctx/admin/user HTTP/1.0\r\nHost:host:8888\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/login"));
+        assertThat(response, containsString("Location: /ctx/login"));
         assertThat(response, containsString("Set-Cookie: JSESSIONID="));
     }
 
@@ -167,7 +167,7 @@ public void testUseExistingSession() throws Exception
 
         response = _connector.getResponse("GET /ctx/any/user HTTP/1.0\r\nHost:host:8888\r\nCookie: JSESSIONID=" + sessionId + "\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/login"));
+        assertThat(response, containsString("Location: /ctx/login"));
         assertThat(response, not(containsString("Set-Cookie: JSESSIONID=")));
     }
 
@@ -182,13 +182,13 @@ public void testError() throws Exception
         String sessionId = "unknown";
         response = _connector.getResponse("GET /ctx/any/user HTTP/1.0\r\nHost:host:8888\r\nCookie: JSESSIONID=" + sessionId + "\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/login"));
+        assertThat(response, containsString("Location: /ctx/login"));
         assertThat(response, containsString("Set-Cookie: JSESSIONID="));
         sessionId = sessionId(response);
 
         response = _connector.getResponse("GET /ctx/j_security_check?j_username=user&j_password=wrong HTTP/1.0\r\nHost:host:8888\r\nCookie: JSESSIONID=" + sessionId + "\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/error"));
+        assertThat(response, containsString("Location: /ctx/error"));
         assertThat(response, not(containsString("Set-Cookie: JSESSIONID=")));
 
         response = _connector.getResponse("""
@@ -201,7 +201,7 @@ public void testError() throws Exception
             j_username=user&j_password=wrong
             """);
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/error"));
+        assertThat(response, containsString("Location: /ctx/error"));
         assertThat(response, not(containsString("Set-Cookie: JSESSIONID=")));
     }
 
@@ -216,13 +216,13 @@ public void testLoginQuery() throws Exception
         String sessionId = "unknown";
         response = _connector.getResponse("GET /ctx/any/user HTTP/1.0\r\nHost:host:8888\r\nCookie: JSESSIONID=" + sessionId + "\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/login"));
+        assertThat(response, containsString("Location: /ctx/login"));
         assertThat(response, containsString("Set-Cookie: JSESSIONID="));
         sessionId = sessionId(response);
 
         response = _connector.getResponse("GET /ctx/j_security_check?j_username=user&j_password=password HTTP/1.0\r\nHost:host:8888\r\nCookie: JSESSIONID=" + sessionId + "\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/any/user"));
+        assertThat(response, containsString("Location: /ctx/any/user"));
         assertThat(response, containsString("Set-Cookie: JSESSIONID="));
         String unsafeSessionId = sessionId;
         sessionId = sessionId(response);
@@ -240,7 +240,7 @@ public void testLoginQuery() throws Exception
 
         response = _connector.getResponse("GET /ctx/any/user HTTP/1.0\r\nHost:host:8888\r\nCookie: JSESSIONID=" + unsafeSessionId + "\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/login"));
+        assertThat(response, containsString("Location: /ctx/login"));
         assertThat(response, containsString("Set-Cookie: JSESSIONID="));
     }
 
@@ -255,7 +255,7 @@ public void testLoginForm() throws Exception
         String sessionId = "unknown";
         response = _connector.getResponse("GET /ctx/any/user HTTP/1.0\r\nHost:host:8888\r\nCookie: JSESSIONID=" + sessionId + "\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/login"));
+        assertThat(response, containsString("Location: /ctx/login"));
         assertThat(response, containsString("Set-Cookie: JSESSIONID="));
         sessionId = sessionId(response);
 
@@ -269,7 +269,7 @@ public void testLoginForm() throws Exception
             j_username=user&j_password=password
             """.formatted(sessionId));
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/any/user"));
+        assertThat(response, containsString("Location: /ctx/any/user"));
         assertThat(response, containsString("Set-Cookie: JSESSIONID="));
         String unsafeSessionId = sessionId;
         sessionId = sessionId(response);
@@ -287,7 +287,7 @@ public void testLoginForm() throws Exception
 
         response = _connector.getResponse("GET /ctx/any/user HTTP/1.0\r\nHost:host:8888\r\nCookie: JSESSIONID=" + unsafeSessionId + "\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/login"));
+        assertThat(response, containsString("Location: /ctx/login"));
         assertThat(response, containsString("Set-Cookie: JSESSIONID="));
     }
 
@@ -311,13 +311,13 @@ public void testRedirectToPost() throws Exception
             name1=value1&name2=value2\r
             """.formatted(sessionId));
         assertThat(response, containsString("HTTP/1.1 303 See Other"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/login"));
+        assertThat(response, containsString("Location: /ctx/login"));
         assertThat(response, containsString("Set-Cookie: JSESSIONID="));
         sessionId = sessionId(response);
 
         response = _connector.getResponse("GET /ctx/j_security_check?j_username=user&j_password=password HTTP/1.0\r\nHost:host:8888\r\nCookie: JSESSIONID=" + sessionId + "\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 302 Found"));
-        assertThat(response, containsString("Location: http://host:8888/ctx/any/user?action=form"));
+        assertThat(response, containsString("Location: /ctx/any/user?action=form"));
         assertThat(response, containsString("Set-Cookie: JSESSIONID="));
         String unsafeSessionId = sessionId;
         sessionId = sessionId(response);

jetty-core/jetty-server/src/main/config/modules/server.mod

@@ -63,7 +63,7 @@ etc/jetty.xml
 # jetty.httpConfig.maxErrorDispatches=10
 
 ## Relative Redirect Locations allowed
-# jetty.httpConfig.relativeRedirectAllowed=false
+# jetty.httpConfig.relativeRedirectAllowed=true
 
 ## Whether to use direct ByteBuffers for reading or writing
 # jetty.httpConfig.useInputDirectByteBuffers=true

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java

@@ -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;
     private HostPort _serverAuthority;
     private SocketAddress _localAddress;
     private int _maxUnconsumedRequestContentReads = 16;

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java

@@ -604,45 +604,18 @@ static List<HttpCookie> getCookies(Request request)
     }
 
     /**
-     * Common point to generate a proper "Location" header for redirects.
+     * Generate a proper "Location" header for redirects.
      *
      * @param request the request the redirect should be based on (needed when relative locations are provided, so that
      * server name, scheme, port can be built out properly)
      * @param location the location URL to redirect to (can be a relative path)
      * @return the full redirect "Location" URL (including scheme, host, port, path, etc...)
+     * @deprecated use {@link Response#toRedirectURI(Request, String)}
      */
+    @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);
     }
 
     /**

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java

@@ -29,6 +29,7 @@
 import org.eclipse.jetty.http.HttpHeaderValue;
 import org.eclipse.jetty.http.HttpMethod;
 import org.eclipse.jetty.http.HttpStatus;
+import org.eclipse.jetty.http.HttpURI;
 import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.http.Trailers;
 import org.eclipse.jetty.io.ByteBufferPool;
@@ -38,6 +39,7 @@
 import org.eclipse.jetty.server.internal.HttpChannelState;
 import org.eclipse.jetty.util.Callback;
 import org.eclipse.jetty.util.StringUtil;
+import org.eclipse.jetty.util.URIUtil;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -238,7 +240,7 @@ static <T> T as(Response response, Class<T> type)
      * @param request the HTTP request
      * @param response the HTTP response
      * @param callback the callback to complete
-     * @param location the redirect location
+     * @param location the redirect location as an absolute URI or encoded relative URI path.
      * @see #sendRedirect(Request, Response, Callback, int, String, boolean)
      */
     static void sendRedirect(Request request, Response response, Callback callback, String location)
@@ -256,7 +258,7 @@ static void sendRedirect(Request request, Response response, Callback callback,
      * @param request the HTTP request
      * @param response the HTTP response
      * @param callback the callback to complete
-     * @param location the redirect location
+     * @param location the redirect location as an absolute URI or encoded relative URI path.
      * @param consumeAvailable whether to consumer the available request content
      * @see #sendRedirect(Request, Response, Callback, int, String, boolean)
      */
@@ -275,9 +277,9 @@ static void sendRedirect(Request request, Response response, Callback callback,
      * @param response the HTTP response
      * @param callback the callback to complete
      * @param code the redirect HTTP status code
-     * @param location the redirect location
+     * @param location the redirect location as an absolute URI or encoded relative URI path.
      * @param consumeAvailable whether to consumer the available request content
-     * @see Request#toRedirectURI(Request, String)
+     * @see #toRedirectURI(Request, String)
      * @throws IllegalArgumentException if the status code is not a redirect, or the location is {@code null}
      * @throws IllegalStateException if the response is already {@link #isCommitted() committed}
      */
@@ -317,11 +319,54 @@ static void sendRedirect(Request request, Response response, Callback callback,
             }
         }
 
-        response.getHeaders().put(HttpHeader.LOCATION, Request.toRedirectURI(request, location));
+        response.getHeaders().put(HttpHeader.LOCATION, toRedirectURI(request, location));
         response.setStatus(code);
         response.write(true, null, callback);
     }
 
+    /**
+     * Common point to generate a proper "Location" header for redirects.
+     *
+     * @param request the request the redirect should be based on (needed when relative locations are provided, so that
+     * server name, scheme, port can be built out properly)
+     * @param location the redirect location as an absolute URI or encoded relative URI path. If a relative path starts
+     *                 with '/', then it is relative to the root, otherwise it is relative to the request.
+     * @return the full redirect "Location" URL (including scheme, host, port, path, etc...)
+     */
+    static String toRedirectURI(Request request, String location)
+    {
+        // is the URI absolute already?
+        if (!URIUtil.hasScheme(location))
+        {
+            // The location is relative
+            HttpURI uri = request.getHttpURI();
+
+            // Is it relative to the request?
+            if (!location.startsWith("/"))
+            {
+                String path = uri.getPath();
+                String parent = (path.endsWith("/")) ? path : URIUtil.parentPath(path);
+                location = URIUtil.addEncodedPaths(parent, location);
+            }
+
+            // Normalize out any dot dot segments
+            location = URIUtil.normalizePathQuery(location);
+            if (location == null)
+                throw new IllegalStateException("redirect path cannot be above root");
+
+            // if relative redirects are not allowed?
+            if (!request.getConnectionMetaData().getHttpConfiguration().isRelativeRedirectAllowed())
+            {
+                // make the location an absolute URI
+                StringBuilder url = new StringBuilder(128);
+                URIUtil.appendSchemeHostPort(url, uri.getScheme(), Request.getServerName(request), Request.getServerPort(request));
+                url.append(location);
+                location = url.toString();
+            }
+        }
+        return location;
+    }
+
     /**
      * <p>Adds an HTTP {@link HttpHeader#SET_COOKIE} header to the response.</p>
      *

jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HostHeaderCustomizerTest.java

@@ -34,6 +34,7 @@ public void testFixedHostPort() throws Exception
     {
         Server server = new Server();
         HttpConfiguration httpConfig = new HttpConfiguration();
+        httpConfig.setRelativeRedirectAllowed(false);
         final String serverName = "test_server_name";
         final int serverPort = 23232;
         final String redirectPath = "/redirect";
@@ -98,6 +99,7 @@ public void testHostPort() throws Exception
     {
         Server server = new Server();
         HttpConfiguration httpConfig = new HttpConfiguration();
+        httpConfig.setRelativeRedirectAllowed(false);
         final String serverName = "127.0.0.1";
         final String redirectPath = "/redirect";
         httpConfig.addCustomizer(new HostHeaderCustomizer());