From f196b23be24712fb8fb16051cc124798cc84f70e Mon Sep 17 00:00:00 2001
From: Evan Phoenix <[email protected]>
Date: Wed, 18 Sep 2024 21:56:07 -0700
Subject: [PATCH] Merge commit from fork

Origin: https://github.com/puma/puma/commit/f196b23be24712fb8fb16051cc124798cc84f70e

* Prevent underscores from clobbering hyphen headers

* Special case encoding headers to prevent app confusion

* Handle _ as , in jruby as well

* Silence RuboCop offense

---------

Co-authored-by: Patrik Ragnarsson <[email protected]>
---
 ext/puma_http11/org/jruby/puma/Http11.java |  2 +
 lib/puma/const.rb                          |  8 +++
 lib/puma/request.rb                        | 19 ++++++--
 test/test_normalize.rb                     | 57 ++++++++++++++++++++++
 test/test_request_invalid.rb               | 28 +++++++++++
 5 files changed, 111 insertions(+), 3 deletions(-)
 create mode 100644 test/test_normalize.rb

diff --git a/ext/puma_http11/org/jruby/puma/Http11.java b/ext/puma_http11/org/jruby/puma/Http11.java
index cd7a5d3bb0..0c4f79eee7 100644
--- a/ext/puma_http11/org/jruby/puma/Http11.java
+++ b/ext/puma_http11/org/jruby/puma/Http11.java
@@ -99,6 +99,8 @@ public static void http_field(Ruby runtime, RubyHash req, ByteList buffer, int f
             int bite = b.get(i) & 0xFF;
             if(bite == '-') {
                 b.set(i, (byte)'_');
+            } else if(bite == '_') {
+                b.set(i, (byte)',');
             } else {
                 b.set(i, (byte)Character.toUpperCase(bite));
             }
diff --git a/lib/puma/const.rb b/lib/puma/const.rb
index c4968f4ae8..451105e648 100644
--- a/lib/puma/const.rb
+++ b/lib/puma/const.rb
@@ -244,6 +244,14 @@ module Const
     # header values can contain HTAB?
     ILLEGAL_HEADER_VALUE_REGEX = /[\x00-\x08\x0A-\x1F]/.freeze
 
+    # The keys of headers that should not be convert to underscore
+    # normalized versions. These headers are ignored at the request reading layer,
+    # but if we normalize them after reading, it's just confusing for the application.
+    UNMASKABLE_HEADERS = {
+      "HTTP_TRANSFER,ENCODING" => true,
+      "HTTP_CONTENT,LENGTH" => true,
+    }
+
     # Banned keys of response header
     BANNED_HEADER_KEY = /\A(rack\.|status\z)/.freeze
 
diff --git a/lib/puma/request.rb b/lib/puma/request.rb
index 8c7b008ee8..86e2d467e4 100644
--- a/lib/puma/request.rb
+++ b/lib/puma/request.rb
@@ -318,6 +318,11 @@ def illegal_header_value?(header_value)
     # compatibility, we'll convert them back. This code is written to
     # avoid allocation in the common case (ie there are no headers
     # with `,` in their names), that's why it has the extra conditionals.
+    #
+    # @note If a normalized version of a `,` header already exists, we ignore
+    #       the `,` version. This prevents clobbering headers managed by proxies
+    #       but not by clients (Like X-Forwarded-For).
+    #
     # @param env [Hash] see Puma::Client#env, from request, modifies in place
     # @version 5.0.3
     #
@@ -326,23 +331,31 @@ def req_env_post_parse(env)
       to_add = nil
 
       env.each do |k,v|
-        if k.start_with?("HTTP_") and k.include?(",") and k != "HTTP_TRANSFER,ENCODING"
+        if k.start_with?("HTTP_") && k.include?(",") && !UNMASKABLE_HEADERS.key?(k)
           if to_delete
             to_delete << k
           else
             to_delete = [k]
           end
 
+          new_k = k.tr(",", "_")
+          if env.key?(new_k)
+            next
+          end
+
           unless to_add
             to_add = {}
           end
 
-          to_add[k.tr(",", "_")] = v
+          to_add[new_k] = v
         end
       end
 
-      if to_delete
+      if to_delete # rubocop:disable Style/SafeNavigation
         to_delete.each { |k| env.delete(k) }
+      end
+
+      if to_add
         env.merge! to_add
       end
     end
diff --git a/test/test_normalize.rb b/test/test_normalize.rb
new file mode 100644
index 0000000000..60e61c3dde
--- /dev/null
+++ b/test/test_normalize.rb
@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+require_relative "helper"
+
+require "puma/request"
+
+class TestNormalize < Minitest::Test
+  parallelize_me!
+
+  include Puma::Request
+
+  def test_comma_headers
+    env = {
+      "HTTP_X_FORWARDED_FOR" => "1.1.1.1",
+      "HTTP_X_FORWARDED,FOR" => "2.2.2.2",
+    }
+
+    req_env_post_parse env
+
+    expected = {
+      "HTTP_X_FORWARDED_FOR" => "1.1.1.1",
+    }
+
+    assert_equal expected, env
+
+    # Test that the iteration order doesn't matter
+
+    env = {
+      "HTTP_X_FORWARDED,FOR" => "2.2.2.2",
+      "HTTP_X_FORWARDED_FOR" => "1.1.1.1",
+    }
+
+    req_env_post_parse env
+
+    expected = {
+      "HTTP_X_FORWARDED_FOR" => "1.1.1.1",
+    }
+
+    assert_equal expected, env
+  end
+
+  def test_unmaskable_headers
+    env = {
+      "HTTP_CONTENT,LENGTH" => "100000",
+      "HTTP_TRANSFER,ENCODING" => "chunky"
+    }
+
+    req_env_post_parse env
+
+    expected = {
+      "HTTP_CONTENT,LENGTH" => "100000",
+      "HTTP_TRANSFER,ENCODING" => "chunky"
+    }
+
+    assert_equal expected, env
+  end
+end
diff --git a/test/test_request_invalid.rb b/test/test_request_invalid.rb
index 8e9295b592..c6aa91ab05 100644
--- a/test/test_request_invalid.rb
+++ b/test/test_request_invalid.rb
@@ -216,4 +216,32 @@ def test_chunked_size_mismatch_2
 
     assert_status data
   end
+
+  def test_underscore_header_1
+    hdrs = [
+      "X-FORWARDED-FOR: 1.1.1.1",  # proper
+      "X-FORWARDED-FOR: 2.2.2.2",  # proper
+      "X_FORWARDED-FOR: 3.3.3.3",  # invalid, contains underscore
+      "Content-Length: 5",
+    ].join "\r\n"
+
+    response = send_http_and_read "#{GET_PREFIX}#{hdrs}\r\n\r\nHello\r\n\r\n"
+
+    assert_includes response, "HTTP_X_FORWARDED_FOR = 1.1.1.1, 2.2.2.2"
+    refute_includes response, "3.3.3.3"
+  end
+
+  def test_underscore_header_2
+    hdrs = [
+      "X_FORWARDED-FOR: 3.3.3.3",  # invalid, contains underscore
+      "X-FORWARDED-FOR: 2.2.2.2",  # proper
+      "X-FORWARDED-FOR: 1.1.1.1",  # proper
+      "Content-Length: 5",
+    ].join "\r\n"
+
+    response = send_http_and_read "#{GET_PREFIX}#{hdrs}\r\n\r\nHello\r\n\r\n"
+
+    assert_includes response, "HTTP_X_FORWARDED_FOR = 2.2.2.2, 1.1.1.1"
+    refute_includes response, "3.3.3.3"
+  end
 end