Repository navigation
Read a refused POST's body before answering, so a late body cannot reset the refusal away (FM-49) - #177
Conversation
…deliver the refusal (FM-49) The Windows CI leg flakes in test_preview_appends_nothing_for_anyone with WinError 10053. The server answers an unauthenticated POST without reading its body; when the body lands after the answer, closing the socket resets the connection and Windows discards the refusal the client had not read. late_body_post() forces that gap on every run with a raw socket. The new ConsoleRefusals tests assert the refusal arrives whole, and that a refused body too large or of unknown length is not read and the answer says Connection: close. test_preview_appends_nothing_for_anyone now asserts the status of every answer and turns a broken connection into a failure. These fail on main; the server fix follows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…set the refusal away (FM-49) do_POST checked the token before reading the body. A refusal was sent and the socket closed with the body unread; a body that landed late made the close reset the connection, and Windows discarded the refusal the client had not read yet (WinError 10053, the Windows CI flake in test_preview_appends_nothing_for_anyone). Every do_POST path that answers before the body is read now handles it: - the token refusal, and any unexpected error while resolving the token: the body is read and dropped first, up to 1 MB and 5 s in total (one receive per read, so a trickling client cannot stretch the wait); - a Content-Length that is missing, malformed or over 1 MB, or a body held back for Expect: 100-continue: nothing is read and the refusal says Connection: close; - _body's own refusals (malformed or over-limit length, and an authenticated POST with no Content-Length): Connection: close. Status codes and error bodies are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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: 859cb693ff
ℹ️ 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".
| self.unread_body = True | ||
| break |
There was a problem hiding this comment.
If an unauthenticated POST declares a valid body but sends it more slowly than the five-second limit, this branch leaves part of the body unread and the handler still sends the refusal before closing. That recreates the unread-request close that can reset the response on Windows, so the client may still lose the refusal; the byte-at-a-time test checks only that the socket becomes readable, not that the full response arrives.
AGENTS.md reference: AGENTS.md:L157-L161
Useful? React with 👍 / 👎.
| raw = self.headers.get("Content-Length", "").strip() | ||
| length = int(raw) if raw.isascii() and raw.isdigit() else -1 |
There was a problem hiding this comment.
On the project's default Python 3.11+ configuration, a Content-Length with more than 4,300 ASCII digits passes isdigit() but makes int(raw) raise ValueError. That happens before unread_body is set, so the outer handler returns invalid_request without Connection: close while request bytes may still be pending, reopening the refused-body reset path on Windows.
AGENTS.md reference: AGENTS.md:L157-L161
Useful? React with 👍 / 👎.
|
Independent review (OpenAI gpt-6-sol): CHANGES REQUESTED at The refusal fix is covered for unauthorized POSTs, but one changed error path still needs a regression test.
Response |
…y and answers 500 whole (FM-49) do_POST drains the body when _user() raises anything, not only the unauthorized refusal. This test patches _user() on the handler class to raise RuntimeError, sends the body late, and checks the complete 500 internal_error answer arrives with no reset, after the server read the body. Against main's server it fails on Windows by reset (WinError 10053) and on every platform by the wait check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (OpenAI gpt-6-sol): APPROVED at The change reads bounded, length-delimited bodies before answering refused POSTs and marks requests that cannot be drained for connection closure. The added tests cover late bodies, refusal responses, and bounded waits. No blocking finding in the provided diff. No findings. Response |
Summary
Fixes FM-49, the Windows CI flake in
tests/test_console.py.do_POSTinsyberwork/server.pychecked the bearer token before it read the request body. An unauthenticated POST was refused and the socket was closed with the body still unread. If the body arrived after the server had read the headers, closing the socket sent a TCP reset. On Windows the reset throws away the refusal the client has not read yet, and the client seesConnectionAbortedError: [WinError 10053].http.clientwrites the headers and the body in two sends. The console tests run the client and the server as threads in one process, so under CI load the server thread can read the headers, refuse and close before the client sends the body.test_preview_appends_nothing_for_anyonesends nine unauthenticated POSTs. It's the console test that hit this.This is a server defect, not just a test-harness problem: a real client posting with a missing or expired token can lose the refusal the same way.
Evidence
The same test failed with the same error in three Windows CI runs:
Forced-gap repro: an unauthenticated POST to a test
Cell, with a sleep between the header send and the body send (Windows 11, Python 3.12.6):unauthorizedRefusal paths covered (every
do_POSTpath that answers before the body is read)_user()refuses the token (missing or wrong)_user()raises something unexpected (answered 500)Connection: closeExpect: 100-continue(this HTTP/1.0 server never sends 100)Connection: close_body(): Content-Length malformed (invalid_request) or over 1 MB (payload_size)Connection: close_body(): authenticated POST with no Content-Length (for example chunked)Connection: closeStatus codes and error bodies are unchanged. An unauthenticated POST is still answered
409 {"error": "unauthorized"}, as it is on main. Every route check runs after_body(), so no other path answers early.Exposure note: an unauthenticated client can now make one server thread read up to 1 MB, for up to 5 s in total, before it gets its refusal. That's bounded, and the server already has no per-connection timeout for slow headers.
Tests
tests/test_console.py:late_body_post()sends a POST over a raw socket and holds the body back until the server answers, or for 1 s. If the server answers first, the helper waits 0.2 s so the server has closed before the body is sent. It reads the response to the end of the stream and records anyOSError.ConsoleRefusals, 5 tests:test_an_unauthenticated_post_whose_body_arrives_late_still_gets_its_refusal: no token and a wrong token. Checks the refusal arrives whole (noOSError, body length matches Content-Length, 409unauthorized) and that the server waited for the body. The waiting check makes the test fail on main on every platform, not only where the reset shows.test_an_unexpected_failure_resolving_the_token_drains_a_late_body_and_answers_500_whole(fix round 1): patches_user()on the handler class, for this test only, to raiseRuntimeError, then sends the body late. Checks the complete500 {"error": "internal_error"}answer arrives with no reset, after the server read the body. Against main's server it fails on Windows by reset, 3 of 3 local runs (ConnectionAbortedError(10053, ...) is not None). It fails on every platform by the wait check, because main answers before reading. With the fix it passes 10 of 10.test_a_refused_body_too_large_or_of_unknown_length_is_not_read_and_the_connection_is_closed: 7 subtests. The response comes at once and carriesConnection: close.test_a_refused_body_that_never_arrives_is_waited_for_a_bounded_timetest_a_refused_body_sent_a_byte_at_a_time_is_waited_for_in_total_not_per_bytetest_preview_appends_nothing_for_anyonenow checks the status of every answer: (409,unauthorized) without a token, otherwise 200 or 409 and neverinternal_error. A broken connection becomes a namedself.failinstead of an error.Expect/no-length subtests, and the two bounded-wait tests. No test that exists on main was weakened. Commit 3 (9bd6123, fix round 1) adds the 500-path test.Failing first, locally on Windows against main (commit-1 tests):
FAILED (failures=7). Both late-body subtests failed withAssertionError: ConnectionAbortedError(10053, ...) is not None : the connection broke before the refusal was read, in 10 of 10 repeated runs. The five Connection-close subtests failed withNone != 'close'. Final tests against main's server:FAILED (failures=9, errors=2). The 2 errors are the bounded-wait tests: theREFUSED_BODY_SECONDSconstant they patch doesn't exist on main.After the fix, locally on Windows:
ConsoleRefusals: 10/10 runs OKtests.test_console: 102 tests OK (1 skipped)tests.test_server: 6 OKtests.test_security_review: 35 OK (4 skipped)Before the fix, 20 runs of the whole
tests.test_consolefile, 4 at a time, all passed (20/20). The race window is too narrow to hit on this machine without forcing it; the forced-gap repro above is the reproduction.CI
Failing first, tests only (f335e42), run 41944655147: failed as intended.
FAILED (failures=7): both late-body subtests failed withAssertionError: ConnectionAbortedError(10053, 'An established connection was aborted by the software in your host machine', ...) is not None : the connection broke before the refusal was read, plus the 5 Connection-close subtestsFAILED (failures=5): only the 5 Connection-close subtests failedSo on Linux the reset does not show: the late-body test as written in commit 1 passed there. That's why commit 2 adds the "the server waited for the body" check, which fails on main on every platform.
With the fix (859cb69): all green. Push run 41946160945 and pull_request run 41946284344: windows, test (3.11), (3.12), (3.13), compose, G8 Existing suite (strict, 3.12) and G8 all pass. G2–G6, admit and the GitHits dependency policy check also pass.
Fix round 1 (9bd6123): all green. Push run 41949236152 and pull_request run 41949242637: windows, test (3.11), (3.12), (3.13), compose, G8 Existing suite (strict, 3.12) and G8 all pass. G2–G6, admit and the GitHits dependency policy check also pass.
Not explained
ConsoleWitness.test_a_signed_head_is_witnessed_through_its_count_and_later_events_are_notfailed once withTimeoutError: timed out(run 41871105046, attempt 1, line 691). It was a 10 s client timeout on an authenticated POST that takes about 56 ms locally. That shard ran 1198 s against a planned 314 s. I found no harness cause:port=0);SO_REUSEADDR;It is left as runner load and is not claimed fixed. The
test_builder_httptimeout in run 41780132804 looks similar and is out of scope.Rollback
A plain revert of 859cb69 restores the old server behavior. Reverting f335e42 as well removes the tests.
Review notes
A fresh-context read-only review ran on the diff. Accepted and fixed:
Expect: 100-continue;Connection: closefor an authenticated POST with no Content-Length.Declined: making
_body()'s Content-Length parsing as strict as_discard_body()'s. It would change which inputs an authenticated POST accepts, and that is outside this fix.Files outside the allowlist: none (
syberwork/server.py,tests/test_console.py).Admission
Class: R
Class-computed-by:
git diff --name-only origin/main...HEADlistssyberwork/server.pyandtests/test_console.py. Neither matches a rule in SyberWork'spolicy.json, so the floor is auto. AGENTS.md §1 classes a bug fix with its test, revertible by one commit, as R.Agent-platform: claude
Reviewed-by: openai:gpt-6-sol (response resp_0b375a196acef471006aebe152842487d090e6b84fa5703cfd, head 9bd6123, tier medium, run by the PM (Claude) on behalf of sykosyber)
🤖 Generated with Claude Code