Ignore illegal response header (#2439)

[MitchLewis930]

User description

PR_055

---

PR Type

Bug fix

---

Description

• Validate response headers against RFC specifications

• Reject headers with illegal characters in keys and values

• Block reserved header keys starting with "rack." and "status"

• Enable previously skipped header validation tests

---

Diagram Walkthrough

flowchart LR
  A["Response Headers"] --> B["Check Key Validity"]
  B --> C["Check Value Validity"]
  C --> D["Check Reserved Keys"]
  D --> E["Filter/Include Header"]
  B -.->|Illegal Key| E
  C -.->|Illegal Value| E
  D -.->|Reserved Key| E

Loading

File Walkthrough

	Relevant files
Enhancement	const.rb Add RFC-compliant header validation constants                        lib/puma/const.rb Removed old HTTP_INJECTION_REGEX constant Added ILLEGAL_HEADER_KEY_REGEX to detect non-compliant header keys Added ILLEGAL_HEADER_VALUE_REGEX to detect non-compliant header values Added BANNED_HEADER_KEY regex to block reserved "rack." and "status" keys +8/-1      request.rb Implement header validation and filtering logic                    lib/puma/request.rb Added illegal_header_key? method to validate header key format Renamed possible_header_injection? to illegal_header_value? for clarity Updated str_early_hints to filter illegal header keys and values Updated str_headers to filter illegal keys, values, and banned reserved keys +19/-6
Tests	test_response_header.rb Enable header validation test cases                                            test/test_response_header.rb Removed skip directives from 8 test cases Tests now validate illegal characters in header keys and values Tests cover regular headers, early hints, and content-length override scenarios Tests verify that reserved "rack." and "status" keys are filtered +0/-11
Documentation	History.md Document header validation bugfix                                                History.md Added entry documenting the illegal response header filtering bugfix References GitHub issue Ignore illegal response header puma/puma#2439 +2/-1

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Overbroad banned match: The banned-header detection regex /rack.|status/ is not anchored and does not escape the dot, causing unintended filtering of legitimate headers (e.g., any key containing status, or rack + any character) instead of only keys starting with rack. and status. Referred Code BANNED_HEADER_KEY = /rack.|status/.freeze end Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Incorrect reserved filtering: Response header-name validation for reserved keys relies on BANNED_HEADER_KEY matching substrings rather than only reserved keys starting with rack. or status, which can incorrectly drop valid externally-controlled header names and constitutes improper input handling. Referred Code when BANNED_HEADER_KEY next end Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Misleading constant name: BANNED_HEADER_KEY suggests an exact banned key list, but its unanchored regex will also match and ban non-reserved header names containing the substring status or rack followed by any character. Referred Code BANNED_HEADER_KEY = /rack.|status/.freeze end Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix regex to avoid rejecting valid headers Correct the ILLEGAL_HEADER_KEY_REGEX to avoid rejecting valid headers by adjusting the character range and removing the literal pipe character. lib/puma/const.rb [244] -ILLEGAL_HEADER_KEY_REGEX = /[\u0000-\u0025|#{DQUOTE}|#{HTTP_HEADER_DELIMITER}]/.freeze +ILLEGAL_HEADER_KEY_REGEX = /[\u0000-\u001F\u007F#{DQUOTE}#{HTTP_HEADER_DELIMITER}]/.freeze Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that the ILLEGAL_HEADER_KEY_REGEX is too broad and would reject valid HTTP headers, which is a significant bug in the new functionality.	Medium
	Skip entire header on illegal value Reject the entire header if its value contains any illegal characters, rather than just filtering individual lines of a multi-line header. lib/puma/request.rb [344-346] next if illegal_header_key?(k) +next if illegal_header_value?(vs) if vs.respond_to?(:to_s) && !vs.to_s.empty? Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion improves security by ensuring that if any part of a header value is invalid, the entire header is discarded, preventing partial and potentially malicious headers from being processed.	Low
General	Anchor and escape banned key regex Improve the BANNED_HEADER_KEY regex by escaping the dot and adding anchors to ensure it only matches the intended header keys. lib/puma/const.rb [248] -BANNED_HEADER_KEY = /rack.|status/.freeze +BANNED_HEADER_KEY = /\A(?:rack\.|status)\z/i.freeze Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that the BANNED_HEADER_KEY regex is too broad and could lead to false positives, and the proposed fix makes it more precise and robust.	Medium
	Avoid unnecessary string allocation in validation Optimize the illegal_header_key? method to avoid unnecessary string allocation by checking if the header key is already a string before calling to_s. lib/puma/request.rb [288-290] def illegal_header_key?(header_key) - !!(ILLEGAL_HEADER_KEY_REGEX =~ header_key.to_s) + key_s = header_key.is_a?(String) ? header_key : header_key.to_s + !!(ILLEGAL_HEADER_KEY_REGEX =~ key_s) end Apply / Chat Suggestion importance[1-10]: 3 __ Why: This is a minor performance optimization that avoids unnecessary string allocation, which is a valid but low-impact improvement.	Low
More