Repository navigation
fix(cadente): reject invalid request methods and targets, answer 431 on oversized headers - #52
Conversation
…on oversized headers Validate the request method as an RFC 9110 token and the request target as %x21-7E (RFC 9112 section 3.2) before the request reaches the core. Previously bytes such as 0x80, control characters and LF in the target were accepted, and invalid method tokens caused a FormatException in the core. Header blocks that exceed the read buffer now receive 431 Request Header Fields Too Large instead of a silent connection close.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11fc26cff8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if (totalRead >= bufferLength) { | ||
| Logger.LogInformation ( $"failed to parse HTTP request: headers too large" ); | ||
| await stream.WriteAsync ( "HTTP/1.1 431 Request Header Fields Too Large\r\nConnection: close\r\nContent-Length: 0\r\n\r\n"u8.ToArray (), cancellationToken ).ConfigureAwait ( false ); |
There was a problem hiding this comment.
When plain HTTP is sent to an HTTPS-configured listener, HttpHost.WritePlainHttpRedirectAsync calls this reader and treats every null result as a reason to call WriteHandshakeErrorAsync. An oversized header therefore writes this new 431 here and then immediately appends a complete 400 response on the same connection, despite the 431 declaring Content-Length: 0 and Connection: close. Clients that receive the first response can observe malformed extra response bytes; distinguish the oversized-header result or move the response decision to the caller.
Useful? React with 👍 / 👎.
…sponse on HTTPS redirect
1 participant
What
Three fixes in the Cadente HTTP/1.1 request parser:
%x21-7Eis now rejected (RFC 9112 §3.2). Previously bytes>= 0x80and control characters were accepted and decoded withEncoding.ASCII, soGET /admin\x80xwas served by the/adminroute, and a LF inside the target forged extra lines in the error log.HttpListeneranswers 400 to these.token(RFC 9110 §5.6.2). PreviouslyG\x01T,G@TorGET\x80reached the core, wherenew HttpMethod(...)threwFormatException, producing a 500, an[unavailable request]log entry and anOnExceptioncall.431 Request Header Fields Too LargewithConnection: closebefore closing, instead of dropping the connection silently.Invalid methods and targets keep the existing failure semantics (parse failure, connection closed), consistent with the other parse errors in
ParseHttpRequest.Tests
Added to
CadenteSecurityTests:InvalidRequestLineBytes_AreRejectedBeforeHandler(8 cases:0x80, ESC, LF, NUL,0x7Fin the target;0x01,@,0x80in the method)ValidRequestLine_StillReachesHandler(/a?b=c%20d, lowercaseget,M-SEARCH)OversizedHeaderBlock_RespondsWith431Before the fix, the 8 invalid cases and the 431 test fail; after it they pass.
Full
tests/Sisk.Coresuite, run locally:SISK_TEST_ENGINE=CadenteSISK_TEST_ENGINE=DefaultNotes