Merge pull request #4648 from eclipse/jetty-9.4.x-4645-forwardedPortException

Issue #4645 - better error message for empty X-Forwarded-Port value

jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java

@@ -24,6 +24,7 @@ import java.lang.invoke.MethodType;
 import java.net.InetSocketAddress;
 import javax.servlet.ServletRequest;
 
+import org.eclipse.jetty.http.BadMessageException;
 import org.eclipse.jetty.http.HostPortHttpField;
 import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpFields;
@@ -376,18 +377,18 @@ public class ForwardedRequestCustomizer implements Customizer
 
         // Do a single pass through the header fields as it is a more efficient single iteration.
         Forwarded forwarded = new Forwarded(request, config);
-        try
+        for (HttpField field : httpFields)
         {
-            for (HttpField field : httpFields)
+            try
             {
                 MethodHandle handle = _handles.get(field.getName());
                 if (handle != null)
                     handle.invoke(forwarded, field);
             }
-        }
+            catch (Throwable t)
-        catch (Throwable e)
+            {
-        {
+                onError(field, t);
-            throw new RuntimeException(e);
+            }
         }
 
         if (forwarded._proto != null)
@@ -420,6 +421,11 @@ public class ForwardedRequestCustomizer implements Customizer
         }
     }
 
+    protected void onError(HttpField field, Throwable t)
+    {
+        throw new BadMessageException("Bad header value for " + field.getName(), t);
+    }
+
     protected String getLeftMost(String headerValue)
     {
         if (headerValue == null)
@@ -623,35 +629,37 @@ public class ForwardedRequestCustomizer implements Customizer
         @SuppressWarnings("unused")
         public void handleFor(HttpField field)
         {
+            String authority = getLeftMost(field.getValue());
             if (!getForwardedPortAsAuthority() && !StringUtil.isEmpty(getForwardedPortHeader()))
             {
                 if (_for == null)
-                    _for = new PossiblyPartialHostPort(getLeftMost(field.getValue()));
+                    _for = new PossiblyPartialHostPort(authority);
                 else if (_for instanceof PortSetHostPort)
-                    _for = new HostPort(HostPort.normalizeHost(getLeftMost(field.getValue())), _for.getPort());
+                    _for = new HostPort(HostPort.normalizeHost(authority), _for.getPort());
             }
             else if (_for == null)
             {
-                _for = new HostPort(getLeftMost(field.getValue()));
+                _for = new HostPort(authority);
             }
         }
 
         @SuppressWarnings("unused")
         public void handlePort(HttpField field)
         {
+            int port = HostPort.parsePort(getLeftMost(field.getValue()));
             if (!getForwardedPortAsAuthority())
             {
                 if (_for == null)
-                    _for = new PortSetHostPort(_request.getRemoteHost(), Integer.parseInt(getLeftMost(field.getValue())));
+                    _for = new PortSetHostPort(_request.getRemoteHost(), port);
                 else if (_for instanceof PossiblyPartialHostPort && _for.getPort() <= 0)
-                    _for = new HostPort(HostPort.normalizeHost(_for.getHost()), Integer.parseInt(getLeftMost(field.getValue())));
+                    _for = new HostPort(HostPort.normalizeHost(_for.getHost()), port);
             }
             else
             {
                 if (_host == null)
-                    _host = new PortSetHostPort(_request.getServerName(), Integer.parseInt(getLeftMost(field.getValue())));
+                    _host = new PortSetHostPort(_request.getServerName(), port);
                 else if (_host instanceof PossiblyPartialHostPort && _host.getPort() <= 0)
-                    _host = new HostPort(HostPort.normalizeHost(_host.getHost()), Integer.parseInt(getLeftMost(field.getValue())));
+                    _host = new HostPort(HostPort.normalizeHost(_host.getHost()), port);
             }
         }
 

jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java

@@ -32,6 +32,7 @@ import org.eclipse.jetty.http.HttpTester;
 import org.eclipse.jetty.server.handler.AbstractHandler;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
@@ -619,6 +620,25 @@ public class ForwardedRequestCustomizerTest
         expectations.accept(actual);
     }
 
+    @Test
+    public void testBadInput() throws Exception
+    {
+        Request request = new Request("Bad port value")
+            .headers(
+                "GET / HTTP/1.1",
+                "Host: myhost",
+                "X-Forwarded-Port: "
+            );
+
+        request.configure(customizer);
+
+        String rawRequest = request.getRawRequest((header) -> header);
+        System.out.println(rawRequest);
+
+        HttpTester.Response response = HttpTester.parseResponse(connector.getResponse(rawRequest));
+        assertThat("status", response.getStatus(), is(400));
+    }
+
     private static class Request
     {
         String description;

jetty-util/src/main/java/org/eclipse/jetty/util/HostPort.java

@@ -32,7 +32,7 @@ public class HostPort
     private final String _host;
     private final int _port;
 
-    public HostPort(String host, int port) throws IllegalArgumentException
+    public HostPort(String host, int port)
     {
         _host = host;
         _port = port;
@@ -61,7 +61,7 @@ public class HostPort
                 {
                     if (authority.charAt(close + 1) != ':')
                         throw new IllegalArgumentException("Bad IPv6 port");
-                    _port = StringUtil.toInt(authority, close + 2);
+                    _port = parsePort(authority.substring(close + 2));
                 }
                 else
                     _port = 0;
@@ -80,7 +80,7 @@ public class HostPort
                     else
                     {
                         _host = authority.substring(0, c);
-                        _port = StringUtil.toInt(authority, c + 1);
+                        _port = parsePort(authority.substring(c + 1));
                     }
                 }
                 else
@@ -103,10 +103,6 @@ public class HostPort
                 }
             };
         }
-        if (_host == null)
-            throw new IllegalArgumentException("Bad host");
-        if (_port < 0)
-            throw new IllegalArgumentException("Bad port");
     }
 
     /**
@@ -163,4 +159,23 @@ public class HostPort
         // normalize with [ ]
         return "[" + host + "]";
     }
+
+    /**
+     * Parse a string representing a port validating it is a valid port value.
+     *
+     * @param rawPort the port string.
+     * @return the integer value for the port.
+     * @throws IllegalArgumentException
+     */
+    public static int parsePort(String rawPort) throws IllegalArgumentException
+    {
+        if (StringUtil.isEmpty(rawPort))
+            throw new IllegalArgumentException("Bad port");
+
+        int port = Integer.parseInt(rawPort);
+        if (port <= 0 || port > 65535)
+            throw new IllegalArgumentException("Bad port");
+
+        return port;
+    }
 }

jetty-util/src/test/java/org/eclipse/jetty/util/HostPortTest.java

@@ -43,6 +43,7 @@ public class HostPortTest
             Arguments.of("[0::0::0::1]", "[0::0::0::1]", null),
             Arguments.of("[0::0::0::1]:80", "[0::0::0::1]", "80"),
             Arguments.of("0:1:2:3:4:5:6", "[0:1:2:3:4:5:6]", null),
+            Arguments.of("127.0.0.1:65535", "127.0.0.1", "65535"),
             // Localhost tests
             Arguments.of("localhost:80", "localhost", "80"),
             Arguments.of("127.0.0.1:80", "127.0.0.1", "80"),
@@ -79,7 +80,9 @@ public class HostPortTest
             "[0::0::0::0::1]:xxx",
             "host:-80",
             "127.0.0.1:-80",
-            "[0::0::0::0::1]:-80")
+            "[0::0::0::0::1]:-80",
+            "127.0.0.1:65536"
+            )
             .map(Arguments::of);
     }