Added a UriCompliance.Violation.USER_INFO to deprecate user info in HttpURI

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java

@@ -109,7 +109,12 @@ public enum Violation implements ComplianceViolation
          * Allow path characters not allowed in the path portion of the URI and HTTP specs.
          * <p>This would allow characters that fall outside of the {@code unreserved / pct-encoded / sub-delims / ":" / "@"} ABNF</p>
          */
-        ILLEGAL_PATH_CHARACTERS("https://datatracker.ietf.org/doc/html/rfc3986#section-3.3", "Illegal Path Character");
+        ILLEGAL_PATH_CHARACTERS("https://datatracker.ietf.org/doc/html/rfc3986#section-3.3", "Illegal Path Character"),
+
+        /**
+         * Allow user info in the authority portion of the URI and HTTP specs.
+         */
+        USER_INFO("https://datatracker.ietf.org/doc/html/rfc9110#name-deprecation-of-userinfo-in-", "Deprecated User Info");
 
         private final String _url;
         private final String _description;
@@ -175,7 +180,8 @@ public String getDescription()
             Violation.AMBIGUOUS_PATH_SEPARATOR,
             Violation.AMBIGUOUS_PATH_ENCODING,
             Violation.AMBIGUOUS_EMPTY_SEGMENT,
-            Violation.UTF16_ENCODINGS));
+            Violation.UTF16_ENCODINGS,
+            Violation.USER_INFO));
 
     /**
      * Compliance mode that allows all URI Violations, including allowing ambiguous paths in non-canonical form, and illegal characters

jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java

@@ -34,6 +34,7 @@
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.Matchers.sameInstance;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -59,6 +60,7 @@ public void testBuilder()
 
         assertThat(uri.getScheme(), is("http"));
         assertThat(uri.getUser(), is("user:password"));
+        assertTrue(uri.hasViolation(Violation.USER_INFO));
         assertThat(uri.getHost(), is("host"));
         assertThat(uri.getPort(), is(8888));
         assertThat(uri.getPath(), is("/ignored/../p%61th;ignored/info;param"));
@@ -81,6 +83,7 @@ public void testBuilder()
 
         assertThat(uri.getScheme(), is("https"));
         assertThat(uri.getUser(), nullValue());
+        assertFalse(uri.hasViolation(Violation.USER_INFO));
         assertThat(uri.getHost(), is("[::1]"));
         assertThat(uri.getPort(), is(8080));
         assertThat(uri.getPath(), is("/some%20encoded/evening;id=12345"));
@@ -98,6 +101,7 @@ public void testExample()
 
         assertThat(uri.getScheme(), is("http"));
         assertThat(uri.getUser(), is("user:password"));
+        assertTrue(uri.hasViolation(Violation.USER_INFO));
         assertThat(uri.getHost(), is("host"));
         assertThat(uri.getPort(), is(8888));
         assertThat(uri.getPath(), is("/ignored/../p%61th;ignored/info;param"));
@@ -155,11 +159,8 @@ public void testParse()
         assertThat(uri.getHost(), is("foo"));
         assertThat(uri.getPath(), is("/bar"));
 
-        // We do allow nulls if not encoded.  This can be used for testing 2nd line of defence.
-        builder.uri("http://fo\000/bar");
-        uri = builder.asImmutable();
-        assertThat(uri.getHost(), is("fo\000"));
-        assertThat(uri.getPath(), is("/bar"));
+        // We do not allow nulls if not encoded.
+        assertThrows(IllegalArgumentException.class, () -> builder.uri("http://fo\000/bar").asImmutable());
     }
 
     @Test
@@ -327,6 +328,7 @@ public void testBasicAuthCredentials()
         assertEquals("http://user:password@example.com:8888/blah", uri.toString());
         assertEquals(uri.getAuthority(), "example.com:8888");
         assertEquals(uri.getUser(), "user:password");
+        assertTrue(uri.hasViolation(Violation.USER_INFO));
     }
 
     @Test
@@ -1198,4 +1200,36 @@ public void testFromBad(String scheme, String server, int port, String expectedS
         HttpURI httpURI = HttpURI.from(scheme, server, port, null);
         assertThat(httpURI.asString(), is(expectedStr));
     }
+
+    public static Stream<String> badAuthorities()
+    {
+        return Stream.of(
+            "http://#host/path",
+            "https:// host/path",
+            "https://h st/path",
+            "https://h\000st/path",
+            "https://h%GGst/path",
+            "https://host%/path",
+            "https://host%0/path",
+            "https://host%u001f/path",
+            "https://host%:8080/path",
+            "https://host%0:8080/path",
+            "https://user%@host/path",
+            "https://user%0@host/path",
+            "https://host:notport/path",
+            "https://user@host:notport/path",
+            "https://user:password@host:notport/path",
+            "https://user @host.com/",
+            "https://user#@host.com/",
+            "https://[notIpv6]/",
+            "https://bad[0::1::2::3::4]/"
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource("badAuthorities")
+    public void testBadAuthority(String uri)
+    {
+        assertThrows(IllegalArgumentException.class, () -> HttpURI.from(uri));
+    }
 }

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

@@ -79,6 +79,7 @@ public class HttpConfiguration implements Dumpable
     private long _minResponseDataRate;
     private HttpCompliance _httpCompliance = HttpCompliance.RFC7230;
     private UriCompliance _uriCompliance = UriCompliance.DEFAULT;
+    private UriCompliance _redirectUriCompliance = null; // TODO default to UriCompliance.DEFAULT in 12.1 ?;
     private CookieCompliance _requestCookieCompliance = CookieCompliance.RFC6265;
     private CookieCompliance _responseCookieCompliance = CookieCompliance.RFC6265;
     private MultiPartCompliance _multiPartCompliance = MultiPartCompliance.RFC7578;
@@ -159,6 +160,7 @@ public HttpConfiguration(HttpConfiguration config)
         _notifyRemoteAsyncErrors = config._notifyRemoteAsyncErrors;
         _relativeRedirectAllowed = config._relativeRedirectAllowed;
         _uriCompliance = config._uriCompliance;
+        _redirectUriCompliance = config._redirectUriCompliance;
         _serverAuthority = config._serverAuthority;
         _localAddress = config._localAddress;
     }
@@ -598,11 +600,24 @@ public UriCompliance getUriCompliance()
         return _uriCompliance;
     }
 
+    public UriCompliance getRedirectUriCompliance()
+    {
+        return _redirectUriCompliance;
+    }
+
     public void setUriCompliance(UriCompliance uriCompliance)
     {
         _uriCompliance = uriCompliance;
     }
 
+    /**
+     * @param uriCompliance The {@link UriCompliance} to apply in {@link Response#toRedirectURI(Request, String)} or {@code null}.
+     */
+    public void setRedirectUriCompliance(UriCompliance uriCompliance)
+    {
+        _redirectUriCompliance = uriCompliance;
+    }
+
     /**
      * @return The CookieCompliance used for parsing request {@code Cookie} headers.
      * @see #getResponseCookieCompliance()

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

@@ -33,6 +33,7 @@
 import org.eclipse.jetty.http.HttpURI;
 import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.http.Trailers;
+import org.eclipse.jetty.http.UriCompliance;
 import org.eclipse.jetty.io.ByteBufferPool;
 import org.eclipse.jetty.io.Content;
 import org.eclipse.jetty.io.QuietException;
@@ -301,25 +302,32 @@ static void sendRedirect(Request request, Response response, Callback callback,
             return;
         }
 
-        if (consumeAvailable)
+        try
         {
-            while (true)
+            if (consumeAvailable)
             {
-                Content.Chunk chunk = response.getRequest().read();
-                if (chunk == null)
+                while (true)
                 {
-                    response.getHeaders().put(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE);
-                    break;
+                    Content.Chunk chunk = response.getRequest().read();
+                    if (chunk == null)
+                    {
+                        response.getHeaders().put(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE);
+                        break;
+                    }
+                    chunk.release();
+                    if (chunk.isLast())
+                        break;
                 }
-                chunk.release();
-                if (chunk.isLast())
-                    break;
             }
-        }
 
-        response.getHeaders().put(HttpHeader.LOCATION, toRedirectURI(request, location));
-        response.setStatus(code);
-        response.write(true, null, callback);
+            response.getHeaders().put(HttpHeader.LOCATION, toRedirectURI(request, location));
+            response.setStatus(code);
+            response.write(true, null, callback);
+        }
+        catch (Throwable failure)
+        {
+            callback.failed(failure);
+        }
     }
 
     /**
@@ -333,6 +341,8 @@ static void sendRedirect(Request request, Response response, Callback callback,
      */
     static String toRedirectURI(Request request, String location)
     {
+        HttpConfiguration httpConfiguration = request.getConnectionMetaData().getHttpConfiguration();
+
         // is the URI absolute already?
         if (!URIUtil.hasScheme(location))
         {
@@ -353,12 +363,19 @@ static String toRedirectURI(Request request, String location)
                 throw new IllegalStateException("redirect path cannot be above root");
 
             // if relative redirects are not allowed?
-            if (!request.getConnectionMetaData().getHttpConfiguration().isRelativeRedirectAllowed())
-            {
+            if (!httpConfiguration.isRelativeRedirectAllowed())
                 // make the location an absolute URI
                 location = URIUtil.newURI(uri.getScheme(), Request.getServerName(request), Request.getServerPort(request), location, null);
-            }
         }
+
+        UriCompliance redirectCompliance = httpConfiguration.getRedirectUriCompliance();
+        if (redirectCompliance != null)
+        {
+            String violations = UriCompliance.checkUriCompliance(redirectCompliance, HttpURI.from(location), null);
+            if (StringUtil.isNotBlank(violations))
+                throw new IllegalArgumentException(violations);
+        }
+
         return location;
     }
 

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

@@ -22,6 +22,7 @@
 import java.util.concurrent.TimeUnit;
 import java.util.stream.Stream;
 
+import org.eclipse.jetty.http.HttpCompliance;
 import org.eclipse.jetty.http.HttpCookie;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpStatus;
@@ -100,6 +101,76 @@ public void testEncodedPath() throws Exception
         assertEquals(HttpStatus.BAD_REQUEST_400, response.getStatus());
     }
 
+    public static Stream<Arguments> getUriTests()
+    {
+        return Stream.of(
+            Arguments.of(UriCompliance.DEFAULT, "/", 200, "local"),
+            Arguments.of(UriCompliance.DEFAULT, "https://local/", 200, "local"),
+            Arguments.of(UriCompliance.DEFAULT, "https://other/", 400, "Authority!=Host"),
+            Arguments.of(UriCompliance.UNSAFE, "https://other/", 200, "other"),
+            Arguments.of(UriCompliance.DEFAULT, "https://user@local/", 400, "Deprecated User Info"),
+            Arguments.of(UriCompliance.LEGACY, "https://user@local/", 200, "local"),
+            Arguments.of(UriCompliance.LEGACY, "https://user@local:port/", 400, "Bad Request"),
+            Arguments.of(UriCompliance.LEGACY, "https://user@local:8080/", 400, "Authority!=Host"),
+            Arguments.of(UriCompliance.UNSAFE, "https://user@local:8080/", 200, "local:8080"),
+            Arguments.of(UriCompliance.DEFAULT, "https://user:password@local/", 400, "Deprecated User Info"),
+            Arguments.of(UriCompliance.LEGACY, "https://user:password@local/", 200, "local"),
+            Arguments.of(UriCompliance.DEFAULT, "https://user@other/", 400, "Deprecated User Info"),
+            Arguments.of(UriCompliance.LEGACY, "https://user@other/", 400, "Authority!=Host"),
+            Arguments.of(UriCompliance.DEFAULT, "https://user:password@other/", 400, "Deprecated User Info"),
+            Arguments.of(UriCompliance.LEGACY, "https://user:password@other/", 400, "Authority!=Host"),
+            Arguments.of(UriCompliance.UNSAFE, "https://user:password@other/", 200, "other"),
+            Arguments.of(UriCompliance.DEFAULT, "/%2F/", 400, "Ambiguous URI path separator"),
+            Arguments.of(UriCompliance.UNSAFE, "/%2F/", 200, "local")
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource("getUriTests")
+    public void testGETUris(UriCompliance compliance, String uri, int status, String content) throws Exception
+    {
+        server.stop();
+        for (Connector connector: server.getConnectors())
+        {
+            HttpConnectionFactory httpConnectionFactory = connector.getConnectionFactory(HttpConnectionFactory.class);
+            if (httpConnectionFactory != null)
+            {
+                HttpConfiguration httpConfiguration = httpConnectionFactory.getHttpConfiguration();
+                httpConfiguration.setUriCompliance(compliance);
+                if (compliance == UriCompliance.UNSAFE)
+                    httpConfiguration.setHttpCompliance(HttpCompliance.RFC2616_LEGACY);
+            }
+        }
+
+        server.setHandler(new Handler.Abstract()
+        {
+            @Override
+            public boolean handle(Request request, Response response, Callback callback)
+            {
+                String msg = String.format("authority=\"%s\"", request.getHttpURI().getAuthority());
+                response.getHeaders().put(HttpHeader.CONTENT_TYPE, "text/plain;charset=utf-8");
+                Content.Sink.write(response, true, msg, callback);
+                return true;
+            }
+        });
+        server.start();
+        String request = """
+                GET %s HTTP/1.1\r
+                Host: local\r
+                Connection: close\r
+                \r
+                """.formatted(uri);
+        HttpTester.Response response = HttpTester.parseResponse(connector.getResponse(request));
+        assertThat(response.getStatus(), is(status));
+        if (content != null)
+        {
+            if (status == 200)
+                assertThat(response.getContent(), is("authority=\"%s\"".formatted(content)));
+            else
+                assertThat(response.getContent(), containsString(content));
+        }
+    }
+
     @Test
     public void testAmbiguousPathSep() throws Exception
     {