Ignore illegal response header (#2439)#5
Conversation
* Ignore illegal response header
📝 WalkthroughWalkthroughThis pull request implements header validation for response headers to comply with the Rack specification. New regex-based validation constants are introduced, and the request processing logic is enhanced with methods to check header key and value legality, replacing the previous header injection detection approach. Corresponding tests are enabled to verify the new validation behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/puma/request.rb (1)
341-355:⚠️ Potential issue | 🟡 MinorApply banned-header filtering to Early Hints too.
str_early_hintscurrently checksillegal_header_key?(k)but unlikestr_headers, it does not filterBANNED_HEADER_KEY(regex/rack.|status/). This creates an inconsistency where early hints responses could include Rack-reserved keys that are prohibited in regular response headers, violating Rack's response header contract.Proposed fix
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) + next if illegal_header_key?(k) || (BANNED_HEADER_KEY === k.to_s.downcase)
🤖 Fix all issues with AI agents
In `@lib/puma/const.rb`:
- Around line 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.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
History.mdlib/puma/const.rblib/puma/request.rbtest/test_response_header.rb
💤 Files with no reviewable changes (1)
- test/test_response_header.rb
🔇 Additional comments (3)
lib/puma/request.rb (2)
285-299: LGTM: helper methods are concise and clear.
410-435: Header filtering logic looks solid.History.md (1)
8-10: Release note addition looks good.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # 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 |
There was a problem hiding this comment.
🧩 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
EOFRepository: 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 rbRepository: 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 -10Repository: 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 -A3Repository: Signal65/puma-CodeRabbit
Length of output: 550
🏁 Script executed:
# Check test files for header validation
cat ./git/test/test_response_header.rbRepository: 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 -50Repository: 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 -20Repository: 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 -5Repository: Signal65/puma-CodeRabbit
Length of output: 199
🏁 Script executed:
# Look at the response header test file
cat -n ./test/test_response_header.rbRepository: 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.rbRepository: 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.rbRepository: 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.
| # 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.
2 participants
PR_055
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.