Tighten authority matching

Tighten up IPv6 parsing rules using regexp extracted from resolv in stdlib,
simplified to avoid creating additional groups.

Tighten up hostname matching to graphical characters, except square brackets
(so it doesn't overlap with IPv6 parsing).

Avoid unnecessary IPv4 matching, since anything that matches as an IPv4
address would match as a hostname.

Remove unnecessary named group creation.

Don't allow trailing newlines in host names.

Fixes #1607

Co-authored-by: Pieter van de Bruggen <pvande@gmail.com>

2 files changed, 104 insertions, 9 deletions

diff --git a/lib/rack/request.rb b/lib/rack/request.rb
index 4062cf46..4c133838 100644
--- a/lib/rack/request.rb
+++ b/lib/rack/request.rb
@@ -605,28 +605,48 @@ module Rack
         value ? value.strip.split(/[,\s]+/) : []
       end
 
+      # ipv6 extracted from resolv stdlib, simplified
+      # to remove numbered match group creation.
+      ipv6 = Regexp.union(
+        /(?:[0-9A-Fa-f]{1,4}:){7}
+         [0-9A-Fa-f]{1,4}/x,
+        /(?:[0-9A-Fa-f]{1,4}(?::[0-9A-Fa-f]{1,4})*)? ::
+         (?:[0-9A-Fa-f]{1,4}(?::[0-9A-Fa-f]{1,4})*)?/x,
+        /(?:[0-9A-Fa-f]{1,4}:){6,6}
+         \d+\.\d+\.\d+\.\d+/x,
+        /(?:[0-9A-Fa-f]{1,4}(?::[0-9A-Fa-f]{1,4})*)? ::
+         (?:[0-9A-Fa-f]{1,4}:)*
+         \d+\.\d+\.\d+\.\d+/x,
+        /[Ff][Ee]80
+         (?::[0-9A-Fa-f]{1,4}){7}
+         %[-0-9A-Za-z._~]+/x,
+        /[Ff][Ee]80:
+         (?:
+           (?:[0-9A-Fa-f]{1,4}(?::[0-9A-Fa-f]{1,4})*)? ::
+           (?:[0-9A-Fa-f]{1,4}(?::[0-9A-Fa-f]{1,4})*)?
+           |
+           :(?:[0-9A-Fa-f]{1,4}(?::[0-9A-Fa-f]{1,4})*)?
+         )?
+         :[0-9A-Fa-f]{1,4}%[-0-9A-Za-z._~]+/x)
+
       AUTHORITY = /
         \A
         (?<host>
           # Match IPv6 as a string of hex digits and colons in square brackets
-          \[(?<address>(?<ipv6>.*))\]
-          |
-          # Match IPv4 as four dot-separated groups of digits
-          (?<address>(?<ipv4>[\d.]{7,}))
+          \[(?<address>#{ipv6})\]
           |
-          # Match any other string as a hostname
-          (?<address>(?<hostname>.*?))
+          # Match any other printable string (except square brackets) as a hostname
+          (?<address>[[[:graph:]&&[^\[\]]]]*?)
         )
         (:(?<port>\d+))?
-        \Z
+        \z
       /x
 
       private_constant :AUTHORITY
 
       def split_authority(authority)
         return [] if authority.nil?
-
-        match = AUTHORITY.match(authority)
+        return [] unless match = AUTHORITY.match(authority)
         return match[:host], match[:address], match[:port]&.to_i
       end
 
diff --git a/test/spec_request.rb b/test/spec_request.rb
index 92316f38..fe1f56aa 100644
--- a/test/spec_request.rb
+++ b/test/spec_request.rb
@@ -158,6 +158,16 @@ class RackRequestTest < Minitest::Spec
     req.hostname.must_equal "technically_invalid.example.com"
 
     req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "trailing_newline.com\n")
+    req.host.must_be_nil
+    req.hostname.must_be_nil
+
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "really\nbad\ninput")
+    req.host.must_be_nil
+    req.hostname.must_be_nil
+
+    req = make_request \
       Rack::MockRequest.env_for("/", "SERVER_NAME" => "example.org", "SERVER_PORT" => "9292")
     req.host.must_equal "example.org"
     req.hostname.must_equal "example.org"
@@ -177,6 +187,71 @@ class RackRequestTest < Minitest::Spec
     req.host.must_equal "[2001:db8:cafe::17]"
     req.hostname.must_equal "2001:db8:cafe::17"
 
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "[::]:47011")
+    req.host.must_equal "[::]"
+    req.hostname.must_equal "::"
+
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "[1111:2222:3333:4444:5555:6666:123.123.123.123]")
+    req.host.must_equal "[1111:2222:3333:4444:5555:6666:123.123.123.123]"
+    req.hostname.must_equal "1111:2222:3333:4444:5555:6666:123.123.123.123"
+
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "[1111:2222:3333:4444:5555:6666:123.123.123.123]:47011")
+    req.host.must_equal "[1111:2222:3333:4444:5555:6666:123.123.123.123]"
+    req.hostname.must_equal "1111:2222:3333:4444:5555:6666:123.123.123.123"
+
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "0.0.0.0")
+    req.host.must_equal "0.0.0.0"
+    req.hostname.must_equal "0.0.0.0"
+
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "0.0.0.0:47011")
+    req.host.must_equal "0.0.0.0"
+    req.hostname.must_equal "0.0.0.0"
+
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "255.255.255.255")
+    req.host.must_equal "255.255.255.255"
+    req.hostname.must_equal "255.255.255.255"
+
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "255.255.255.255:47011")
+    req.host.must_equal "255.255.255.255"
+    req.hostname.must_equal "255.255.255.255"
+
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "really\nbad\ninput")
+    req.host.must_be_nil
+    req.hostname.must_be_nil
+
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "[0]")
+    req.host.must_be_nil
+    req.hostname.must_be_nil
+
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "[:::]")
+    req.host.must_be_nil
+    req.hostname.must_be_nil
+
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "[1111:2222:3333:4444:5555:6666:7777:88888]")
+    req.host.must_be_nil
+    req.hostname.must_be_nil
+
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "0.0..0.0")
+    req.host.must_equal '0.0..0.0'
+    req.hostname.must_equal '0.0..0.0'
+
+    req = make_request \
+      Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "255.255.255.0255")
+    req.host.must_equal "255.255.255.0255"
+    req.hostname.must_equal "255.255.255.0255"
+
     env = Rack::MockRequest.env_for("/")
     env.delete("SERVER_NAME")
     req = make_request(env)