Skip to content

Ignore illegal response header (#2439) #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MitchLewis930 wants to merge 1 commit into
base: pr_055_before
Choose a base branch
from
Open

Ignore illegal response header (#2439) #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion History.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

* Bugfixes
* Your bugfix goes here <Most recent on the top, like GitHub> (#Github Number)

* Ignore illegal (by Rack spec) response header (#2439)

## 5.0.3 / 2020-10-26

* Bugfixes
Expand Down
9 changes: 8 additions & 1 deletion lib/puma/const.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,6 @@ module Const
COLON = ": ".freeze

NEWLINE = "\n".freeze
HTTP_INJECTION_REGEX = /[\r\n]/.freeze

HIJACK_P = "rack.hijack?".freeze
HIJACK = "rack.hijack".freeze
Expand All @@ -239,5 +238,13 @@ module Const
# Mininum interval to checks worker health
WORKER_CHECK_INTERVAL = 5

# Illegal character in the key or value of response header
DQUOTE = "\"".freeze
HTTP_HEADER_DELIMITER = Regexp.escape("(),/:;<=>?@[]{}").freeze
ILLEGAL_HEADER_KEY_REGEX = /[\u0000-\u0025|#{DQUOTE}|#{HTTP_HEADER_DELIMITER}]/.freeze
ILLEGAL_HEADER_VALUE_REGEX = /[\000-\037]/.freeze

# Banned keys of response header
BANNED_HEADER_KEY = /rack.|status/.freeze
Comment on lines +241 to +248
Copy link

@coderabbitai coderabbitai bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Test the regex patterns with Ruby
ruby <<'EOF'
# Test current regex
current_regex = /rack.|status/

# Test proposed regex
proposed_regex = /\A(?:rack\..*|status)\z/

test_headers = [
  "rack.version",           # Should be banned (rack.)
  "rack.input",             # Should be banned (rack.)
  "status",                 # Should be banned (exact match)
  "x-status-code",          # Should NOT be banned
  "x-tracking-id",          # Should NOT be banned
  "x-rack-id",              # Should NOT be banned
  "content-status",         # Should NOT be banned
  "my-rack-value",          # Should NOT be banned (rack. not literal)
  "Status",                 # Should NOT be banned (case matters)
  "rack.",                  # Edge case - bare prefix
]

puts "=== Current Regex: /rack.|status/ ==="
test_headers.each do |header|
  match = current_regex.match(header)
  puts "#{header.ljust(20)} => #{match ? 'BANNED' : 'allowed'}"
end

puts "\n=== Proposed Regex: /\\A(?:rack\\..*|status)\\z/ ==="
test_headers.each do |header|
  match = proposed_regex.match(header)
  puts "#{header.ljust(20)} => #{match ? 'BANNED' : 'allowed'}"
end
EOF

Repository: Signal65/puma-CodeRabbit

Length of output: 783

🏁 Script executed:

# Check actual usage and tests of BANNED_HEADER_KEY
cd lib && rg -A5 -B5 "BANNED_HEADER_KEY" --type rb

Repository: Signal65/puma-CodeRabbit

Length of output: 94

🏁 Script executed:

# Search for tests related to header validation
cd .. && find . -name "*test*" -o -name "*spec*" | grep -E "\.(rb)$" | xargs rg -l "header|BANNED_HEADER" 2>/dev/null | head -10

Repository: Signal65/puma-CodeRabbit

Length of output: 219

🏁 Script executed:

# Look at the actual usage of BANNED_HEADER_KEY in the codebase
cd lib && rg "BANNED_HEADER_KEY" -B3 -A3

Repository: Signal65/puma-CodeRabbit

Length of output: 550

🏁 Script executed:

# Check test files for header validation
cat ./git/test/test_response_header.rb

Repository: Signal65/puma-CodeRabbit

Length of output: 134

🏁 Script executed:

# Also check the context where BANNED_HEADER_KEY is used
cat -n lib/puma/response.rb | head -50

Repository: Signal65/puma-CodeRabbit

Length of output: 120

🏁 Script executed:

# Search for where headers are validated using this constant
rg "BANNED_HEADER_KEY" lib/

Repository: Signal65/puma-CodeRabbit

Length of output: 182

🏁 Script executed:

# Find test files
find . -path "*/test*" -name "*.rb" | head -20

Repository: Signal65/puma-CodeRabbit

Length of output: 772

🏁 Script executed:

# Look at the request.rb usage context
cat -n lib/puma/request.rb | grep -A10 -B10 "BANNED_HEADER_KEY"

Repository: Signal65/puma-CodeRabbit

Length of output: 859

🏁 Script executed:

# Check if there are any tests for header validation
find . -name "*.rb" -path "*/test*" | xargs rg -l "header" 2>/dev/null | head -5

Repository: Signal65/puma-CodeRabbit

Length of output: 199

🏁 Script executed:

# Look at the response header test file
cat -n ./test/test_response_header.rb

Repository: Signal65/puma-CodeRabbit

Length of output: 4652

🏁 Script executed:

# Also check what the purpose of BANNED_HEADER_KEY is - search for RFC or comments about Rack headers
rg -B5 "BANNED_HEADER_KEY|rack\." lib/puma/const.rb

Repository: Signal65/puma-CodeRabbit

Length of output: 807

🏁 Script executed:

# Check if there are any tests that verify the banned header behavior
rg -B5 -A5 "banned|BANNED|rack\." ./test/test_response_header.rb

Repository: Signal65/puma-CodeRabbit

Length of output: 1012

BANNED_HEADER_KEY regex is too broad and silently drops valid headers.

The pattern /rack.|status/ matches any header containing status or rack followed by any character anywhere in the key. This causes valid headers like x-status-code, x-tracking-id, x-rack-id, and content-status to be incorrectly filtered out.

The regex should anchor to the full string and use a literal dot to match only rack. prefixed headers (per Rack specification) and the exact status header:

Proposed fix 
-    BANNED_HEADER_KEY = /rack.|status/.freeze
+    BANNED_HEADER_KEY = /\A(?:rack\..*|status)\z/.freeze
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Illegal character in the key or value of response header
DQUOTE = "\"".freeze
HTTP_HEADER_DELIMITER = Regexp.escape("(),/:;<=>?@[]{}").freeze
ILLEGAL_HEADER_KEY_REGEX = /[\u0000-\u0025|#{DQUOTE}|#{HTTP_HEADER_DELIMITER}]/.freeze
ILLEGAL_HEADER_VALUE_REGEX = /[\000-\037]/.freeze
# Banned keys of response header
BANNED_HEADER_KEY = /rack.|status/.freeze
# Illegal character in the key or value of response header
DQUOTE = "\"".freeze
HTTP_HEADER_DELIMITER = Regexp.escape("(),/:;<=>?@[]{}").freeze
ILLEGAL_HEADER_KEY_REGEX = /[\u0000-\u0025|#{DQUOTE}|#{HTTP_HEADER_DELIMITER}]/.freeze
ILLEGAL_HEADER_VALUE_REGEX = /[\000-\037]/.freeze
# Banned keys of response header
BANNED_HEADER_KEY = /\A(?:rack\..*|status)\z/.freeze
🤖 Prompt for AI Agents 
In `@lib/puma/const.rb` around lines 241 - 248, BANNED_HEADER_KEY is too
permissive and matches substrings like "x-status-code" or "x-rack-id"; update
the BANNED_HEADER_KEY constant so it only matches header keys that are exactly
"status" or that start with the literal "rack." prefix per Rack spec (i.e.
anchor the start and end and use a literal dot for the prefix). Replace the
existing /rack.|status/ pattern with a regex that anchors and distinguishes the
two cases (for example a pattern that matches ^rack\..* for prefixed keys and
^status$ for the exact key) in the BANNED_HEADER_KEY constant.

end
end
25 changes: 19 additions & 6 deletions lib/puma/request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,13 +282,20 @@ def normalize_env(env, client)
end
# private :normalize_env

# @param header_key [#to_s]
# @return [Boolean]
#
def illegal_header_key?(header_key)
!!(ILLEGAL_HEADER_KEY_REGEX =~ header_key.to_s)
end

# @param header_value [#to_s]
# @return [Boolean]
#
def possible_header_injection?(header_value)
!!(HTTP_INJECTION_REGEX =~ header_value.to_s)
def illegal_header_value?(header_value)
!!(ILLEGAL_HEADER_VALUE_REGEX =~ header_value.to_s)
end
private :possible_header_injection?
private :illegal_header_key?, :illegal_header_value?

# Fixup any headers with `,` in the name to have `_` now. We emit
# headers with `,` in them during the parse phase to avoid ambiguity
Expand Down Expand Up @@ -334,9 +341,11 @@ def req_env_post_parse(env)
def str_early_hints(headers)
eh_str = "HTTP/1.1 103 Early Hints\r\n".dup
headers.each_pair do |k, vs|
next if illegal_header_key?(k)

if vs.respond_to?(:to_s) && !vs.to_s.empty?
vs.to_s.split(NEWLINE).each do |v|
next if possible_header_injection?(v)
next if illegal_header_value?(v)
eh_str << "#{k}: #{v}\r\n"
end
else
Expand Down Expand Up @@ -399,9 +408,11 @@ def str_headers(env, status, headers, res_info, lines)
res_info[:response_hijack] = nil

headers.each do |k, vs|
next if illegal_header_key?(k)

case k.downcase
when CONTENT_LENGTH2
next if possible_header_injection?(vs)
next if illegal_header_value?(vs)
res_info[:content_length] = vs
next
when TRANSFER_ENCODING
Expand All @@ -410,11 +421,13 @@ def str_headers(env, status, headers, res_info, lines)
when HIJACK
res_info[:response_hijack] = vs
next
when BANNED_HEADER_KEY
next
end

if vs.respond_to?(:to_s) && !vs.to_s.empty?
vs.to_s.split(NEWLINE).each do |v|
next if possible_header_injection?(v)
next if illegal_header_value?(v)
lines.append k, colon, v, line_ending
end
else
Expand Down
11 changes: 0 additions & 11 deletions test/test_response_header.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,11 @@ def assert_ignore_header(name, value, opts={})

# The header must not contain a Status key.
def test_status_key
skip 'implement later'

assert_ignore_header("Status", "500")
end

# Special headers starting “rack.” are for communicating with the server, and must not be sent back to the client.
def test_rack_key
skip 'implement later'

assert_ignore_header("rack.command_to_server_only", "work")
end

Expand All @@ -102,36 +98,29 @@ def test_rack_key
# Header keys will be set through two ways: Regular and early hints.

def test_illegal_character_in_key
skip 'implement later'
assert_ignore_header("\"F\u0000o\u0025(@o}", "Boo")
end

def test_illegal_character_in_key_when_early_hints
skip 'implement later'
assert_ignore_header("\"F\u0000o\u0025(@o}", "Boo", early_hints: true)
end

# testing header value can be separated by \n into line, and each line must not contain characters below 037
# Header values can be set through three ways: Regular, early hints and a special case for overriding content-length

def test_illegal_character_in_value
skip 'implement later'
assert_ignore_header("X-header", "First \000Lin\037e")
end

def test_illegal_character_in_value_when_early_hints
skip 'implement later'
assert_ignore_header("X-header", "First \000Lin\037e", early_hints: true)
end

def test_illegal_character_in_value_when_override_content_length
skip 'implement later'
assert_ignore_header("Content-Length", "\037")
end

def test_illegal_character_in_value_when_newline
skip 'implement later'

server_run app: ->(env) { [200, {'X-header' => "First\000 line\nSecond Lin\037e"}, ["Hello"]] }
data = send_http_and_read "GET / HTTP/1.0\r\n\r\n"

Expand Down