Ignore illegal response header (#2439)#5
Conversation
* Ignore illegal response header
There was a problem hiding this comment.
Pull request overview
This PR implements validation and filtering of illegal response headers according to the Rack specification. The changes ensure that headers with invalid characters in keys or values, as well as banned header keys (Status and rack.*), are properly ignored instead of being sent to clients.
Changes:
- Implemented header key and value validation using regex patterns to detect illegal characters
- Added filtering logic to skip illegal headers in both regular responses and early hints
- Removed skip statements from previously disabled test cases to enable validation testing
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/puma/const.rb | Added regex patterns for detecting illegal header keys/values and banned header key pattern |
| lib/puma/request.rb | Renamed validation method and added header key validation, applying filters in early hints and regular response header processing |
| test/test_response_header.rb | Enabled previously skipped tests for illegal header validation |
| History.md | Added changelog entry for the bugfix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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.
The regex pattern is missing anchors and proper escaping. It will match any string containing 'rack.' or 'status' anywhere, not just headers that start with 'rack.' or exactly equal 'status'. The pattern should be /\Arack\.|status\z/i to match headers starting with 'rack.' (case-insensitive) or exactly 'status'.
| BANNED_HEADER_KEY = /rack.|status/.freeze | |
| BANNED_HEADER_KEY = /\Arack\.|status\z/i.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 |
There was a problem hiding this comment.
The pipe character '|' inside the character class is being treated as a literal character to match, not as an OR operator. Character classes don't need OR operators; all characters are automatically alternatives. Remove the pipe characters: /[\u0000-\u0025#{DQUOTE}#{HTTP_HEADER_DELIMITER}]/
| ILLEGAL_HEADER_KEY_REGEX = /[\u0000-\u0025|#{DQUOTE}|#{HTTP_HEADER_DELIMITER}]/.freeze | |
| ILLEGAL_HEADER_KEY_REGEX = /[\u0000-\u0025#{DQUOTE}#{HTTP_HEADER_DELIMITER}]/.freeze |
2 participants
PR_055