Issue #7269 - Refinement of Host behavior based on Specs

+ Updates to HttpURI in regards to handling of no-port to conform to java.net.URI behaviors of no-port as well Signed-off-by: Joakim Erdfelt <[email protected]>
jetty · Dec 14, 2021 · 4a733ba · 4a733ba
1 parent c3c2f65
commit 4a733ba
Show file tree

Hide file tree

Showing 6 changed files with 117 additions and 43 deletions.
diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
@@ -138,6 +138,7 @@ String getMessage()
     }
 
     private static final Trie<Boolean> __ambiguousSegments = new ArrayTrie<>();
+    public static final int NO_PORT = -1; // value used in java.net.URI for no-port
 
     static
     {
@@ -158,7 +159,7 @@ String getMessage()
     private String _scheme;
     private String _user;
     private String _host;
-    private int _port;
+    private int _port = NO_PORT;
     private String _path;
     private String _param;
     private String _query;
@@ -184,9 +185,9 @@ String getMessage()
     public static HttpURI createHttpURI(String scheme, String host, int port, String path, String param, String query, String fragment)
     {
         if (port == 80 && HttpScheme.HTTP.is(scheme))
-            port = 0;
+            port = NO_PORT;
         if (port == 443 && HttpScheme.HTTPS.is(scheme))
-            port = 0;
+            port = NO_PORT;
         return new HttpURI(scheme, host, port, path, param, query, fragment);
     }
 
@@ -243,7 +244,7 @@ public HttpURI(HttpURI schemeHostPort, HttpURI uri)
 
     public HttpURI(String uri)
     {
-        _port = -1;
+        _port = NO_PORT;
         parse(State.START, uri, 0, uri.length());
     }
 
@@ -279,7 +280,7 @@ public void clear()
         _scheme = null;
         _user = null;
         _host = null;
-        _port = -1;
+        _port = NO_PORT;
         _path = null;
         _param = null;
         _query = null;
@@ -1083,7 +1084,7 @@ public String getPathQuery()
 
     public String getAuthority()
     {
-        if (_port > 0)
+        if (_port > NO_PORT)
             return _host + ":" + _port;
         return _host;
     }

diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/MetaData.java b/jetty-http/src/main/java/org/eclipse/jetty/http/MetaData.java
@@ -180,7 +180,7 @@ public Request(String method, HttpScheme scheme, HostPortHttpField hostPort, Str
         {
             this(method, new HttpURI(scheme == null ? null : scheme.asString(),
                 hostPort == null ? null : hostPort.getHost(),
-                hostPort == null ? -1 : hostPort.getPort(),
+                hostPort == null ? HttpURI.NO_PORT : hostPort.getPort(),
                 uri), version, fields, contentLength);
         }
 

diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java
@@ -2000,7 +2000,7 @@ public void testHost()
         HttpParser parser = new HttpParser(handler);
         parser.parseNext(buffer);
         assertEquals("host", _host);
-        assertEquals(0, _port);
+        assertEquals(HttpURI.NO_PORT, _port);
     }
 
     @Test
@@ -2016,7 +2016,7 @@ public void testUriHost11()
         parser.parseNext(buffer);
         assertEquals("No Host", _bad);
         assertEquals("http://host/", _uriOrStatus);
-        assertEquals(0, _port);
+        assertEquals(HttpURI.NO_PORT, _port);
     }
 
     @Test
@@ -2031,7 +2031,7 @@ public void testUriHost10()
         parser.parseNext(buffer);
         assertNull(_bad);
         assertEquals("http://host/", _uriOrStatus);
-        assertEquals(0, _port);
+        assertEquals(HttpURI.NO_PORT, _port);
     }
 
     @Test
@@ -2061,7 +2061,7 @@ public void testIPHost()
         HttpParser parser = new HttpParser(handler);
         parser.parseNext(buffer);
         assertEquals("192.168.0.1", _host);
-        assertEquals(0, _port);
+        assertEquals(HttpURI.NO_PORT, _port);
     }
 
     @Test
@@ -2078,7 +2078,7 @@ public void testIPv6Host()
         HttpParser parser = new HttpParser(handler);
         parser.parseNext(buffer);
         assertEquals("[::1]", _host);
-        assertEquals(0, _port);
+        assertEquals(HttpURI.NO_PORT, _port);
     }
 
     @Test
@@ -2924,7 +2924,7 @@ public void init()
     }
 
     private String _host;
-    private int _port;
+    private int _port = HttpURI.NO_PORT;
     private String _bad;
     private String _content;
     private String _methodOrVersion;

diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
@@ -1418,7 +1418,7 @@ private String findServerName()
         HttpField host = metadata == null ? null : metadata.getFields().getField(HttpHeader.HOST);
         if (host != null)
         {
-            if (!(host instanceof HostPortHttpField) && host.getValue() != null && !host.getValue().isEmpty())
+            if (!(host instanceof HostPortHttpField) && StringUtil.isNotBlank(host.getValue()))
                 host = new HostPortHttpField(host.getValue());
             if (host instanceof HostPortHttpField)
             {
@@ -1456,7 +1456,7 @@ public int getServerPort()
         int port = (uri == null || uri.getHost() == null) ? findServerPort() : uri.getPort();
 
         // If no port specified, return the default port for the scheme
-        if (port <= 0)
+        if (port == HttpURI.NO_PORT)
         {
             if (getScheme().equalsIgnoreCase(URIUtil.HTTPS))
                 return 443;
@@ -1472,7 +1472,7 @@ private int findServerPort()
         MetaData.Request metadata = _metaData;
         // Return host from header field
         HttpField host = metadata == null ? null : metadata.getFields().getField(HttpHeader.HOST);
-        if (host != null)
+        if ((host != null) && StringUtil.isNotBlank(host.getValue()))
         {
             // TODO is this needed now?
             HostPortHttpField authority = (host instanceof HostPortHttpField)
@@ -1486,7 +1486,7 @@ private int findServerPort()
         if (_channel != null)
             return getLocalPort();
 
-        return -1;
+        return HttpURI.NO_PORT;
     }
 
     @Override

diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/HostPort.java b/jetty-util/src/main/java/org/eclipse/jetty/util/HostPort.java
@@ -25,6 +25,7 @@
  */
 public class HostPort
 {
+    public static final int NO_PORT = -1;  // value used in java.net.URI for no-port
     private final String _host;
     private final int _port;
 
@@ -40,10 +41,9 @@ public HostPort(String authority) throws IllegalArgumentException
             throw new IllegalArgumentException("No Authority");
         try
         {
-            if (authority.isEmpty())
+            if (StringUtil.isBlank(authority))
             {
-                _host = authority;
-                _port = 0;
+                throw new IllegalArgumentException("Empty authority");
             }
             else if (authority.charAt(0) == '[')
             {
@@ -52,6 +52,8 @@ else if (authority.charAt(0) == '[')
                 if (close < 0)
                     throw new IllegalArgumentException("Bad IPv6 host");
                 _host = authority.substring(0, close + 1);
+                if (!isLooseIpv6Address(_host))
+                    throw new IllegalArgumentException("Invalid Host");
 
                 if (authority.length() > close + 1)
                 {
@@ -61,7 +63,7 @@ else if (authority.charAt(0) == '[')
                 }
                 else
                 {
-                    _port = 0;
+                    _port = NO_PORT;
                 }
             }
             else
@@ -70,22 +72,29 @@ else if (authority.charAt(0) == '[')
                 int c = authority.lastIndexOf(':');
                 if (c >= 0)
                 {
-                    // ipv6address
+                    // possible ipv6address
                     if (c != authority.indexOf(':'))
                     {
+                        if (!isLooseIpv6Address(authority))
+                            throw new IllegalArgumentException("Invalid Host");
+
                         _host = "[" + authority + "]";
-                        _port = 0;
+                        _port = NO_PORT;
                     }
                     else
                     {
                         _host = authority.substring(0, c);
                         _port = parsePort(authority.substring(c + 1));
+                        if (StringUtil.isBlank(_host))
+                            throw new IllegalArgumentException("No Host");
+                        // TODO: validate _host is valid (see issue #7269)
                     }
                 }
                 else
                 {
                     _host = authority;
-                    _port = 0;
+                    _port = NO_PORT;
+                    // TODO: validate _host is valid (see issue #7269)
                 }
             }
         }
@@ -99,6 +108,53 @@ else if (authority.charAt(0) == '[')
         }
     }
 
+    /**
+     * <p>
+     * Perform a loose validation of the characters in a IPv6 address.
+     * This is not strict, and does not validate all of the aspects of IPv6,
+     * is only designed to catch really bad cases such as <code>X:Y:Z</code>.
+     * </p>
+     *
+     * For normal IPv6, see ...
+     * Per https://datatracker.ietf.org/doc/html/rfc7230#section-5.4
+     * and https://datatracker.ietf.org/doc/html/rfc7230#section-2.7.1
+     * and https://datatracker.ietf.org/doc/html/rfc3986#section-3.2.2
+     *
+     * Updates for IPv4 literals in IPv6 see
+     * https://tools.ietf.org/html/rfc2732#section-2
+     *
+     * Updates for IPv6 with zone identifiers see
+     * https://datatracker.ietf.org/doc/html/rfc6874
+     *
+     * @param authority the raw string authority
+     * @return true if it loosely follows the IPv6 address rules
+     */
+    private boolean isLooseIpv6Address(String authority)
+    {
+        if (StringUtil.isBlank(authority))
+            return false;
+        int start = 0;
+        int end = authority.length() - 1;
+        if (authority.charAt(start) == '[')
+            start = 1;
+        if (authority.charAt(end) == ']')
+            end = end - 1;
+        for (int i = start; i < end; i++)
+        {
+            char c = authority.charAt(i);
+            if (c == '%')
+                return true; // don't bother validating the rest
+            if (!((c >= '0' && c <= '9') ||
+                (c >= 'a' && c <= 'f') ||
+                (c >= 'A' && c <= 'F') ||
+                (c == '.') || (c == ':')))
+            {
+                return false;
+            }
+        }
+        return true;
+    }
+
     /**
      * Get the host.
      *

diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/HostPortTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/HostPortTest.java
@@ -34,16 +34,18 @@ public class HostPortTest
     private static Stream<Arguments> validAuthorityProvider()
     {
         return Stream.of(
-            Arguments.of("", "", null),
-            Arguments.of(":80", "", "80"),
             Arguments.of("host", "host", null),
+            Arguments.of("127.0.0.1:", "127.0.0.1", null),
+            Arguments.of("[0::0::0::0::1]:", "[0::0::0::0::1]", null),
             Arguments.of("host:80", "host", "80"),
             Arguments.of("10.10.10.1", "10.10.10.1", null),
             Arguments.of("10.10.10.1:80", "10.10.10.1", "80"),
             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"),
+            // scheme based normalization https://datatracker.ietf.org/doc/html/rfc3986#section-6.2.3
+            Arguments.of("host:", "host", null),
             // Localhost tests
             Arguments.of("localhost:80", "localhost", "80"),
             Arguments.of("127.0.0.1:80", "127.0.0.1", "80"),
@@ -54,34 +56,49 @@ private static Stream<Arguments> validAuthorityProvider()
             Arguments.of("[1080:0:0:0:8:800:200C:417A]", "[1080:0:0:0:8:800:200C:417A]", null),
             Arguments.of("[3ffe:2a00:100:7031::1]", "[3ffe:2a00:100:7031::1]", null),
             Arguments.of("[1080::8:800:200C:417A]", "[1080::8:800:200C:417A]", null),
-            Arguments.of("[::192.9.5.5]", "[::192.9.5.5]", null),
-            Arguments.of("[::FFFF:129.144.52.38]:80", "[::FFFF:129.144.52.38]", "80"),
+            Arguments.of("[::192.9.5.5]", "[::192.9.5.5]", null), // TODO: IPv4 within IPv6 no longer specified in non-obsolete spec
+            Arguments.of("[::FFFF:129.144.52.38]:80", "[::FFFF:129.144.52.38]", "80"),  // TODO: IPv4 within IPv6 no longer specified in non-obsolete spec
             Arguments.of("[2010:836B:4179::836B:4179]", "[2010:836B:4179::836B:4179]", null),
             // Modified Examples from above, not using square brackets (valid, but should never have a port)
             Arguments.of("FEDC:BA98:7654:3210:FEDC:BA98:7654:3210", "[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]", null),
             Arguments.of("1080:0:0:0:8:800:200C:417A", "[1080:0:0:0:8:800:200C:417A]", null),
             Arguments.of("3ffe:2a00:100:7031::1", "[3ffe:2a00:100:7031::1]", null),
             Arguments.of("1080::8:800:200C:417A", "[1080::8:800:200C:417A]", null),
-            Arguments.of("::192.9.5.5", "[::192.9.5.5]", null),
-            Arguments.of("::FFFF:129.144.52.38", "[::FFFF:129.144.52.38]", null),
-            Arguments.of("2010:836B:4179::836B:4179", "[2010:836B:4179::836B:4179]", null)
+            Arguments.of("::192.9.5.5", "[::192.9.5.5]", null),  // TODO: IPv4 within IPv6 no longer specified in non-obsolete spec
+            Arguments.of("::FFFF:129.144.52.38", "[::FFFF:129.144.52.38]", null),  // TODO: IPv4 within IPv6 no longer specified in non-obsolete spec
+            Arguments.of("2010:836B:4179::836B:4179", "[2010:836B:4179::836B:4179]", null),
+            // IDN
+            Arguments.of("пример.рф", "пример.рф", null)
         );
     }
 
     private static Stream<Arguments> invalidAuthorityProvider()
     {
         return Stream.of(
-            null,
-            "host:",
-            "127.0.0.1:",
-            "[0::0::0::0::1]:",
-            "host:xxx",
-            "127.0.0.1:xxx",
-            "[0::0::0::0::1]:xxx",
-            "host:-80",
-            "127.0.0.1:-80",
-            "[0::0::0::0::1]:-80",
-            "127.0.0.1:65536"
+                null,
+                "", // TODO: if addressing edge case with absolute-uri and empty Host header
+                "host:xxx",
+                "127.0.0.1:xxx",
+                "[0::0::0::0::1]:xxx",
+                "host:-80",
+                "127.0.0.1:-80", // negative port
+                "[0::0::0::0::1]:-80", // negative port
+                "127.0.0.1:65536", // port too big
+                ":",
+                ":44",
+                "'eclipse.org:443'", // bad quoting that made it through
+                "eclipse.org:443\"", // bad end quoting that made it through
+                "':88'",
+                "jetty.eclipse.org:88007386429567428956488", // port too big
+                "this:that:or:the:other:222",  // multiple ':'
+                "[sheep:cheese:of:rome]:80", // invalid/mimic ipv6 with port
+                "[pecorino:romano]" // invalid/mimic ipv6 without port
+                // TODO: invalid host name checks? - (see issue #7269)
+            /*  "-",
+                "-:77",
+                "*",
+                "*:88",
+                "*.eclipse.org", */
             )
             .map(Arguments::of);
     }
@@ -96,7 +113,7 @@ public void testValidAuthority(String authority, String expectedHost, Integer ex
             assertThat(authority, hostPort.getHost(), is(expectedHost));
 
             if (expectedPort == null)
-                assertThat(authority, hostPort.getPort(), is(0));
+                assertThat(authority, hostPort.getPort(), is(HostPort.NO_PORT));
             else
                 assertThat(authority, hostPort.getPort(), is(expectedPort));
         }