fix: avoid redos on host and protocol getter #17
Conversation
WalkthroughThis pull request updates the header-processing logic in the Request classes and the body-handling logic in the Response class. A new function, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as Request Object
participant U as splitCommaSeparatedValues()
C->>R: Send request with headers (host, X-Forwarded-Proto, X-Forwarded-For)
R->>U: Call U with raw header value
U-->>R: Return trimmed value(s)
R-->>C: Return processed header value
sequenceDiagram
participant T as Test/Client
participant Res as Response Object
T->>Res: Set body with extensive leading whitespace + HTML
Res->>Res: Apply regex /^\s*</ to detect HTML marker
Res-->>T: Set content type to "html" if condition met
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17 +/- ##
==========================================
+ Coverage 98.44% 98.45% +0.01%
==========================================
Files 6 6
Lines 1924 1939 +15
Branches 366 368 +2
==========================================
+ Hits 1894 1909 +15
Misses 30 30 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
example/extend/Request.ts (1)
20-20: Consider using the new utility function.While the current implementation is secure, consider using the
splitCommaSeparatedValuesutility function for consistency with the main Request class.- this[HOST] = rawHost.split(/,/)[0].trim(); + this[HOST] = splitCommaSeparatedValues(rawHost, 1)[0];test/response/body.test.ts (1)
82-82: Consider reducing the whitespace length in the test.While stress testing is important, using 10 million spaces might significantly impact test execution time and memory usage. Consider using a smaller but still significant number (e.g., 10,000 spaces) that would still effectively test the functionality without excessive resource consumption.
- res.body = ' '.repeat(10000000) + '\t\r\n<h1>Tobi</h1>'; + res.body = ' '.repeat(10000) + '\t\r\n<h1>Tobi</h1>';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
example/extend/Request.ts(1 hunks)src/request.ts(5 hunks)src/response.ts(1 hunks)test/request/host.test.ts(1 hunks)test/request/ips.test.ts(1 hunks)test/response/body.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Node.js / Test (windows-latest, 23)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18.19.0)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (macos-latest, 23)
🔇 Additional comments (9)
src/request.ts (4)
215-217: LGTM! Improved host header processing.The changes properly handle malformed input by using the new utility function, which safely processes and sanitizes the header value.
347-351: LGTM! Enhanced protocol header security.The changes improve the handling of X-Forwarded-Proto header by using the new utility function, ensuring consistent and secure processing.
374-376: LGTM! Improved IP address processing.The changes enhance the handling of X-Forwarded-For header by using the new utility function, ensuring consistent processing across all proxy-related headers.
647-649: LGTM! Well-designed utility function.The new
splitCommaSeparatedValuesfunction effectively centralizes the comma-separated value processing logic, improving code maintainability and security.test/request/host.test.ts (1)
93-119: LGTM! Comprehensive test coverage for invalid headers.The new test cases effectively verify the security improvements by testing:
- Invalid x-forwarded-host handling
- Proper fallback behavior
- Both HTTP/1 and HTTP/2 scenarios
test/request/ips.test.ts (2)
24-25: LGTM! Good test for malformed X-Forwarded-For header.The test properly verifies that the code handles malformed headers with extra commas and spaces correctly.
28-33: LGTM! Important security test case.The test ensures that completely invalid IP addresses (only commas and spaces) result in an empty array, preventing potential security issues.
src/response.ts (2)
128-128: Great optimization for HTML detection!The non-greedy quantifier
?in the regex pattern^\s*?<helps prevent potential ReDoS issues by efficiently handling leading whitespace. This is a good security improvement.
128-128: Consider adding more robust HTML validation.While the current regex efficiently handles leading whitespace, it might incorrectly identify some strings as HTML. Consider using a more comprehensive check or adding a comment explaining the trade-off between performance and accuracy.
Let's check if there are any existing HTML validation utilities in the codebase:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for HTML validation related code rg -i "html.*valid|valid.*html" --type tsLength of output: 43
Additional HTML Validation Recommended
The current regex in
src/response.tsonly checks for leading whitespace before a<to classify content as HTML. Our search did not reveal any alternative or more robust HTML validation utilities elsewhere in the codebase. To prevent potential misclassification, consider either improving the validation logic (e.g., verifying closing tags or using a parser) or adding a comment to highlight the trade-offs between performance and accuracy.
[skip ci] ## [2.20.7](v2.20.6...v2.20.7) (2025-02-12) ### Bug Fixes * avoid redos on host and protocol getter ([#17](#17)) ([cd6b487](cd6b487))
GHSA-593f-38f6-jp5m
pick from koajs@5f294bb
Summary by CodeRabbit
Bug Fixes
Refactor