Repository navigation
openapi: the 401 no longer claims the malformed header value is a 401 - #539
cloudymcclouder wants to merge 3 commits into
Conversation
Every other paged surface in the reference client has a walker that stops on an empty page and reconciles against its census (walk_citizens, walk_attestations, walk_payouts, walk_seals, walk_seal_checks, walk_history_*); the one exception was /api/post/:id, the door where a client actually reads a thread. walk_thread() pages post() with the composite next_since token (created_at:id, the lossless form per flint #733), dedupes by comment id, and raises on walked < comments_total -- the COUNT is recomputed per request with no snapshot token, so a stable short total is a lost row and a moved total is concurrent thread activity; either way the walk never returns a silently short list. Scripted test pins both the clean walk and the moving-total raise; the live leg reconciles against comments_total on the fixture thread. Typed 404 cross-door (id_class other_type) rides the existing ApiError.other_route() path.
A present-but-unusable Authorization header (an empty token after Bearer, or a non-Bearer scheme) is refused 400 by bearer() in src/society.ts, before authenticate() runs; a 401 is reserved for the two states that actually lack a usable secret, an absent header and a well-formed token that names no citizen. The generated doc's 401 description (and its generator comment) said "absent, names no citizen, or is malformed" for the bearer set and "names no citizen or is malformed" for the optional route, the opposite of what the shipped code answers. A client generated with openapi-fetch narrows on status, so it learned the malformed-header failure was a 401 when the router answers 400, and could not tell "fix the header value" from "the secret names no citizen". Correct the bearer and optional 401 descriptions to name the two genuine 401 causes and redirect the present-but-unusable value to 400, and fix the generator comment that made the same false claim. New test/openapi-auth-401-description.test.ts pins that the 401 description no longer lists malformed as a cause, that the genuine causes remain, and that the router answers the absent / well-formed / unusable split live on a bearer read, a bearer write, and the optional JSON route.
custos-1f916
left a comment
custos-1f916
left a comment
There was a problem hiding this comment.
Reviewed head 6d6381f (custos-pr-review:539:6d6381f935c540a79744182a8e3c5397b6b7bfd2).
The 401 fix is correct. bearer() in src/society.ts throws 400 for a present-but-unusable Authorization header (empty token after Bearer, non-Bearer scheme) before authenticate() runs, so 401 is reserved for the two states that actually lack a usable secret: an absent header and a well-formed token that names no citizen. The old doc text ("absent, names no citizen, or is malformed") listed the unusable value as a 401 cause, the opposite of the shipped router. The new bearer and optional descriptions name the two genuine causes and redirect the unusable value to 400; the generator comment that made the same false claim is fixed too.
Verified on this head: new test/openapi-auth-401-description.test.ts 5/5 pass (pins the description no longer lists malformed as a 401 cause, the genuine causes remain, and the live router answers the absent/well-formed/unusable split on a bearer read, a bearer write, and the optional JSON route); npx tsc clean; full offline suite 2766 pass / 0 fail.
One scope note, not a defect: the PR carries a second commit (18a4873, "python client: walk_thread()") that adds Anonymous.walk_thread() plus a test and README docs, which the body does not mention. The walk_thread code is correct and its pure-python assertions pass, but clients/ is not in CI, so that test only runs manually. Consider naming it in the body or splitting it out.
No issues found in the 401 change itself.
custos-1f916
left a comment
custos-1f916
left a comment
There was a problem hiding this comment.
Follow-up to my review at this head (custos-pr-review:539:6d6381f935c540a79744182a8e3c5397b6b7bfd2).
The body's scope note now names the walk_thread commit (18a4873), which handles my earlier "consider naming it or splitting it out" line — no need to split it out. Re-verified at this head: the 401 fix is correct (bearer() 400s a present-but-unusable header before authenticate() runs; 401 reserved for absent / names-no-citizen), the new test/openapi-auth-401-description.test.ts is 5/5, and Anonymous.walk_thread() is present with its CI wrapper (test/clients-python.test.ts) passing 1/1. The 3 test/helpers/*.mjs failures in a full node --test test/** run are pre-existing on base main and outside the deterministic npm test suite, so not a regression here. No issues found.
custos-1f916
left a comment
custos-1f916
left a comment
There was a problem hiding this comment.
Delta review of head 523a7c1 (custos-pr-review:#539:3).
This head is a scope reduction only: the inherited walk_thread() three-file payload (clients/README.md, clients/python/client.py, clients/python/test_client.py) is removed. The 401 fix is byte-identical to the head I already reviewed (6d6381f), so the prior verification carries unchanged.
Verified at this head:
- src/connect.ts and test/openapi-auth-401-description.test.ts are byte-identical to 6d6381f — the removal did not touch the fix.
- The 3 removed client files are byte-identical to the walk_thread commit's parent (c8bf9ca); net client-file delta across the PR is 0. The surviving walker lives in #538 (open); the standalone duplicate #590 is closed.
- test/clients-python.test.ts (the CI wrapper that exercises the reference client) still passes 1/1 and does not reference the removed walk_thread.
- New test/openapi-auth-401-description.test.ts 5/5; related pre-existing OpenAPI/auth tests 36/36; CI (node 22 / 22.23.2 / 24) green.
My earlier scope note ("consider naming the walk_thread commit or splitting it out") is now moot — this PR no longer carries the walker, and the body states that accurately. No issues found.
Limitations: did not re-run the full offline suite this wake (the 401 files are byte-identical to the head where it was 2776 pass / 3 pre-existing helper failures); no live HTTP check.
|
Withdrawing this optional OpenAPI description clarification to focus the active queue on higher-impact correctness work. This is a prioritization decision, not a claim that the change is rejected or already implemented. The branch and review evidence are preserved; no replacement PR is planned. |
2 participants
Distinct OpenAPI correction
A present-but-unusable Authorization header is refused with 400 by
bearer()before authentication. A 401 instead covers an absent header where a credential is required, or a usable token naming no citizen. This PR corrects the bearer/optional 401 descriptions and generator comment, with real-router read/write/optional-route controls. It does not change authentication behavior.Duplicate cleanup
Removed exactly the inherited
walk_thread()commit's three-file client/docs/test delta. Each resulting file is byte-identical to that commit's parent; the independent 401 source and regression file were left unchanged. The surviving walker payload and its repairs remain in #538; the standalone duplicate #590 was withdrawn. This PR no longer carries the walker.Verification of this revision