Repository navigation
fix(http): reject HEAD on write-shaped GET routes - #135
Conversation
![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: f4b087c455
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Independent repro on current main (e6114b3, 0.9.3), still live today: HEAD executes all four GET-shaped write handlers, PUT/DELETE/PATCH correctly 405, and OPTIONS advertises Two impact details beyond the summary here, both confirmed against a running instance:
The WriteRoute gate plus the |
8da640d to
5a3ae91
Compare
Rebase onto main (flop-labs#259) and address review. - Keep the HEAD strip inline in app.py as `_get_write()` rather than a `WriteRoute` subclass in its own core file. Starlette adds HEAD to any GET, including when `methods=["GET"]` is passed (routing.py:237), so it has to be dropped after init either way — but a 4-line factory beside the route table is 2 core lines cheaper than a module, needs no import, and does not add a 5-line file to CORE_FILES. `core/app.py` goes 976 -> 979 and `core_total` follows, keeping the cap equal to the sum of the per-file caps. - Drop the `assert self.methods is not None` guard with it: `assert` is stripped under `python -O`, which is the wrong failure mode for a route-level gate. - Restore `MAX_HEADERS` / `MAX_HEADER_BYTES` as two lines. Collapsing them was an unrelated change made to fit under the old cap; CONTRIBUTING rejects code golf. - Cover the ownership lane's shared counter, as asked in review: a HEAD probe on `set-signed` moved `/kv/room-nonce/<room>` before the fix, which is visible to third parties and strands the real writer's captured URL. The message lane replays off the room's own records, so a count assertion pins that one. - Assert `Allow: GET` on all four write lanes, not two. Verified by un-fixing each route in turn: every one fails exactly two tests. - Drop the CHANGELOG.md edit. flop-labs#259 moved changelog assembly to merge/release time — the proposed release note is in the pull request body instead. Closes flop-labs#257 Co-Authored-By: Claude Opus 5 <[email protected]>
5a3ae91 to
6e26b72
Compare
_condition() returned on the if_absent branch before if= was ever read, so a write sent with both (?if=X&if_absent=1) silently ignored if=. When if= was the condition that could not hold, the request still answered 200 instead of 409 or a refusal -- a caller relying on if= for compare-and-set (task-state transitions per interop.md) could not tell its condition was never checked. The two forms are mutually exclusive by definition (if_absent: nothing is there; if=: something specific is there), so there is no correct value to pick silently. Both are now read, and sending both together raises StoreError, mapped to the existing 400 handler with a body naming the two colliding parameters -- matching the shape of every other input refusal rather than introducing a new exception type for one call site. core/app.py grows 981 -> 984 code lines for the extra branch and raise; sz-baseline.json's baseline and cap move up by the same 3 lines, following the precedent set by flop-labs#135. Regression test in tests/http/test_notes.py reproduces both directions from the issue (if= unsatisfiable + if_absent satisfiable, and the reverse) plus the POST lane, and fails against the pre-fix code (200 instead of 400). Fixes flop-labs#290 Co-Authored-By: Claude Fable 5 <[email protected]>
flop-labs#135 pinned the room half of the dual-lane paths; this pins /kv/<ns>/<key>: a HEAD there must answer from the reader (200 on a stored note, 404 on an absent one) rather than fall through to the POST writer. Requested in the close-out of flop-labs#258.
flop-labs#135 pinned the room half of the dual-lane paths; this pins /kv/<ns>/<key>: a HEAD there must answer from the reader (200 on a stored note, 404 on an absent one) rather than fall through to the POST writer. Requested in the close-out of flop-labs#258.
3 participants
Summary
HEADreaching the four GET-shaped mutation routesHEADon the 21 ordinary read-shaped GET routesAllowcontractCloses #257
Why
Starlette automatically adds
HEADto function routes registered forGET. On this service that meant a metadata-only request could append a message, write a note, or spend a signed nonce while discarding the response body. The documented GET write flow is unchanged; only the previously implicit HEAD behavior now returns 405 withAllow: GET._get_write()builds the route and dropsHEADafter init —Route.__init__force-adds it to any GET, including whenmethods=["GET"]is passed (routing.py:237).matches()reads that set, so a HEAD misses the route before the endpoint runs rather than executing and being stripped of its body afterwards, andallowed_methods()reads it too, so the 405'sAllowstays correct with no further change.It is a 4-line factory beside the route table rather than a
Routesubclass in its own module: the subclass bought nothing (nothing doesisinstance), cost 2 more core lines once the import and__init__/super()scaffold are counted, and added a 5-line file toCORE_FILES.No public surface or abuse surface is added.
Two replay counters, both covered
The service has two, and only one is derived from the room's own records:
/r/<room>/say-signed/…) replay against_last_nonce, scanned back off the room's records. A refused HEAD appends nothing, so nothing raises that floor — pinned with a count assertion./kv/<ns>/<key>/set-signed/…) replay against/kv/room-nonce/<room>, a server-written counter shared by every signer. Before the fix a HEAD probe moved it: verified locally that the counter went 1 → 2 on a HEAD, which is third-party-visible and strands the real writer's captured URL. This is the case @Jr-kenny asked to pin in #135 (comment); the test now asserts the counter is untouched by the refused HEAD and still spends on the following GET.Test coverage of the changed paths
Exactly four routes change behaviour, and each is asserted twice — a HEAD probe (405,
Allow: GET, and the write did not land) and theAllowcontract test. Confirmed by un-fixing each route in turn: every one fails exactly two tests.Allow: GET/r/{room}/say/…/r/{room}/say-signed/…/kv/{ns}/{key}/set/…/kv/{ns}/{key}/set-signed/…/kv/room-nonce/<room>unmovedRead-shaped GETs keeping
HEADis asserted too, and the GET write lanes are unchanged and still covered by the rest of the suite.Size caps
core/app.py976 → 979 for the factory, andcore_total2027 → 2030, keeping the cap equal to the sum of the per-file caps. No new core file. The earlierMAX_HEADERS/MAX_HEADER_BYTESline-collapse has been reverted — it was an unrelated change made to fit under the old cap.Proposed release note
Per #259 this PR does not edit
CHANGELOG.md. Suggested wording for[Unreleased]→### Fixed:Compatibility
No route, response shape or default moves. The only behaviour change a caller can observe is
HEADon the four write paths: previously it performed the write and returned 200 with no body, now it returns 405 withAllow: GET.HEADis undocumented on those paths in/openapi.json,README.md,SKILL.mdand the manual, so no documentation becomes inaccurate; the release note is proposed above rather than committed.Validation
Rebased onto
mainat 31eabb8 (#259).uv run ruff check .— passeduv run ruff format --check .— 55 files already formatteduv run ty check— passeduv run coverage run -m pytest tests -q— 393 passeduv run coverage report— 97.51% (app.py 98.87%; the factory is fully covered)uv run sz.py --check— ok, core code-lines 2028 <= baselineuv run sz.py --caps— app 979/979, total 2028/2030ci.ymlinvocation, incl.unsupported_methodandallow_header_conformance) — 876 generated, 876 passed, no issues