Cleanup of relative redirect handling #11014

+ Handle request relative redirects
+ Moved to Response
+ Changed default to allow relative

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-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;
 
@@ -277,7 +279,7 @@ static void sendRedirect(Request request, Response response, Callback callback,
      * @param code the redirect HTTP status code
      * @param location the redirect location
      * @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,53 @@ 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 location URL to redirect to (can be a relative path)
+     * @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());

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

@@ -166,6 +166,8 @@ public boolean handle(Request request, Response response, Callback callback)
     @Test
     public void testRedirectGET() throws Exception
     {
+        server.getConnectors()[0].getConnectionFactory(HttpConnectionFactory.class)
+            .getHttpConfiguration().setRelativeRedirectAllowed(false);
         server.setHandler(new Handler.Abstract()
         {
             @Override
@@ -233,9 +235,67 @@ public boolean handle(Request request, Response response, Callback callback)
         assertThat(response.get(HttpHeader.LOCATION), is("/somewhere/else"));
     }
 
+    @Test
+    public void testRequestRelativeRedirectGET() throws Exception
+    {
+        server.getConnectors()[0].getConnectionFactory(HttpConnectionFactory.class)
+            .getHttpConfiguration().setRelativeRedirectAllowed(true);
+        server.setHandler(new Handler.Abstract()
+        {
+            @Override
+            public boolean handle(Request request, Response response, Callback callback)
+            {
+                Response.sendRedirect(request, response, callback, "somewhere/else");
+                return true;
+            }
+        });
+        server.start();
+
+        String request = """
+                GET /path HTTP/1.0\r
+                Host: hostname\r
+                \r
+                """;
+        HttpTester.Response response = HttpTester.parseResponse(connector.getResponse(request));
+        assertEquals(HttpStatus.MOVED_TEMPORARILY_302, response.getStatus());
+        assertThat(response.get(HttpHeader.LOCATION), is("/somewhere/else"));
+
+        request = """
+                GET /path/ HTTP/1.1\r
+                Host: hostname\r
+                Connection: close\r
+                \r
+                """;
+        response = HttpTester.parseResponse(connector.getResponse(request));
+        assertEquals(HttpStatus.MOVED_TEMPORARILY_302, response.getStatus());
+        assertThat(response.get(HttpHeader.LOCATION), is("/path/somewhere/else"));
+
+        request = """
+                GET /path/index.html HTTP/1.1\r
+                Host: hostname\r
+                Connection: close\r
+                \r
+                """;
+        response = HttpTester.parseResponse(connector.getResponse(request));
+        assertEquals(HttpStatus.MOVED_TEMPORARILY_302, response.getStatus());
+        assertThat(response.get(HttpHeader.LOCATION), is("/path/somewhere/else"));
+
+        request = """
+                GET /path/to/ HTTP/1.1\r
+                Host: hostname\r
+                Connection: close\r
+                \r
+                """;
+        response = HttpTester.parseResponse(connector.getResponse(request));
+        assertEquals(HttpStatus.MOVED_TEMPORARILY_302, response.getStatus());
+        assertThat(response.get(HttpHeader.LOCATION), is("/path/to/somewhere/else"));
+    }
+
     @Test
     public void testRedirectPOST() throws Exception
     {
+        server.getConnectors()[0].getConnectionFactory(HttpConnectionFactory.class)
+            .getHttpConfiguration().setRelativeRedirectAllowed(false);
         server.setHandler(new Handler.Abstract()
         {
             @Override

jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerTest.java

@@ -3636,7 +3636,7 @@ public void testWelcomeRedirectAlt() throws Exception
             """);
         response = HttpTester.parseResponse(rawResponse);
         assertThat(response.toString(), response.getStatus(), is(HttpStatus.MOVED_TEMPORARILY_302));
-        assertThat(response, headerValue("Location", "http://local/context/dir/index.html"));
+        assertThat(response, headerValue("Location", "/context/dir/index.html"));
 
         Files.writeString(inde, "<h1>Hello Inde</h1>", UTF_8);
         rawResponse = _local.getResponse("""
@@ -3647,7 +3647,7 @@ public void testWelcomeRedirectAlt() throws Exception
             """);
         response = HttpTester.parseResponse(rawResponse);
         assertThat(response.toString(), response.getStatus(), is(HttpStatus.MOVED_TEMPORARILY_302));
-        assertThat(response, headerValue("Location", "http://local/context/dir/"));
+        assertThat(response, headerValue("Location", "/context/dir/"));
 
         rawResponse = _local.getResponse("""
             GET /context/dir/ HTTP/1.1\r
@@ -3657,7 +3657,7 @@ public void testWelcomeRedirectAlt() throws Exception
             """);
         response = HttpTester.parseResponse(rawResponse);
         assertThat(response.toString(), response.getStatus(), is(HttpStatus.MOVED_TEMPORARILY_302));
-        assertThat(response, headerValue("Location", "http://local/context/dir/index.html"));
+        assertThat(response, headerValue("Location", "/context/dir/index.html"));
 
         if (deleteFile(index))
         {
@@ -3669,7 +3669,7 @@ public void testWelcomeRedirectAlt() throws Exception
                 """);
             response = HttpTester.parseResponse(rawResponse);
             assertThat(response.toString(), response.getStatus(), is(HttpStatus.MOVED_TEMPORARILY_302));
-            assertThat(response, headerValue("Location", "http://local/context/dir/index.htm"));
+            assertThat(response, headerValue("Location", "/context/dir/index.htm"));
 
             if (deleteFile(inde))
             {
@@ -3721,7 +3721,7 @@ public void testWelcomeRedirectDirWithQuestion() throws Exception
             """);
         response = HttpTester.parseResponse(rawResponse);
         assertThat(response.toString(), response.getStatus(), is(HttpStatus.MOVED_TEMPORARILY_302));
-        assertThat(response, containsHeaderValue("Location", "http://local/context/dir%3F/"));
+        assertThat(response, containsHeaderValue("Location", "/context/dir%3F/"));
 
         rawResponse = _local.getResponse("""
             GET /context/dir%3F/ HTTP/1.1\r
@@ -3731,7 +3731,7 @@ public void testWelcomeRedirectDirWithQuestion() throws Exception
             """);
         response = HttpTester.parseResponse(rawResponse);
         assertThat(response.toString(), response.getStatus(), is(HttpStatus.MOVED_TEMPORARILY_302));
-        assertThat(response, containsHeaderValue("Location", "http://local/context/dir%3F/index.html"));
+        assertThat(response, containsHeaderValue("Location", "/context/dir%3F/index.html"));
     }
 
     /**
@@ -3761,7 +3761,7 @@ public void testWelcomeRedirectDirWithSemicolon() throws Exception
             """);
         response = HttpTester.parseResponse(rawResponse);
         assertThat(response.toString(), response.getStatus(), is(HttpStatus.MOVED_TEMPORARILY_302));
-        assertThat(response, containsHeaderValue("Location", "http://local/context/dir%3B/"));
+        assertThat(response, containsHeaderValue("Location", "/context/dir%3B/"));
 
         rawResponse = _local.getResponse("""
             GET /context/dir%3B/ HTTP/1.1\r
@@ -3771,7 +3771,7 @@ public void testWelcomeRedirectDirWithSemicolon() throws Exception
             """);
         response = HttpTester.parseResponse(rawResponse);
         assertThat(response.toString(), response.getStatus(), is(HttpStatus.MOVED_TEMPORARILY_302));
-        assertThat(response, containsHeaderValue("Location", "http://local/context/dir%3B/index.html"));
+        assertThat(response, containsHeaderValue("Location", "/context/dir%3B/index.html"));
     }
 
     @Test