feat(api): pin the v2 HTTP contract and reject unknown request fields#242
Conversation
…chema Two of the three seams every PostGuard client depends on, as machine-checkable artifacts (EPIC #201, issues #209/#210): - pg-pkg/api-description.yaml: the full v2 OpenAPI (mirrors cryptify's api-description.yaml pattern) — endpoints, auth modes, request/response schemas incl. the ConItem condiscon shape, error envelope, rate limits. Lints clean (redocly). - pg-pkg/schemas/irma-auth-request.schema.json + canonical examples: the condiscon request pinned as JSON Schema, deliberately faithful to serde behavior (unknown fields ignored — documented). Examples cover the legacy flat shape, nested disjunctions, the optional empty-conjunction convention and its irmamobile#360 ordering workaround, plus the classic nesting mistakes as invalid vectors. - pg-pkg/tests/contract_examples.rs makes the examples EXECUTABLE: every valid example must parse as IrmaAuthRequest, every invalid one must be rejected — so the examples and the parser cannot drift apart silently. The e2e harness (postguard-e2e) will additionally validate live responses against the OpenAPI and the examples against the schema (issue #212).
|
Thanks for the PR, @rubenhensen! I'm on it — kicking off a review of the OpenAPI description and JSON Schema additions (closing #209 and #210) for correctness and rule-compliance, then I'll post a single consolidated review with a clear verdict. |
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thorough contract-pinning PR. The executable-examples approach (valid/ must parse, invalid/ must be rejected, with #212 closing the schema ↔ parser ↔ server loop) is a solid way to keep the seam honest. Tests pass (pg-pkg 57 + contract_examples 2) and redocly lints clean apart from the 4xx warnings noted below.
One blocking issue and two smaller notes, all verified against the handler source:
- bug:
/v2/request/jwt/{token}documents 502, but the server returns 503 on the upstream-error path (jwt.rs:21UpstreamError ->error.rs:83SERVICE_UNAVAILABLE; there is no 502 anywhere in the PKG). #212 validates live responses against this spec and would trip on the mismatch. The statusevents operation just below already documents 503 for the same error. - style:
jwtandstatuseventsboth return 400 for a malformed token (each has a passing*_rejects_malformed_token_with_400test) and 500 on an unreachable upstream; neither is documented. Two of the eight redocly warnings are a genuine missing 4xx, not house-style. - nit: the empty-alt-LAST note contradicts the server's own
optional:trueexpansion, which emits the empty conjunction first (start.rs:60).
Rule check (conventional-commit title, closing keywords, cargo-fmt, prose) is clean: the docs(pkg): title satisfies pr-title.yml, Closes #209 / closes #210 link correctly, and EPIC #201 is left open. The 502->503 fix and the missing 400 are small, concrete edits.
| schema: | ||
| type: string | ||
| description: "A compact JWS (three dot-separated segments)." | ||
| "502": |
There was a problem hiding this comment.
Wrong status code. /v2/request/jwt/{token} documents 502 here, but the handler maps this path to 503: jwt.rs:21 does .error_for_status().or(Err(Error::UpstreamError))?, and error.rs:83 maps UpstreamError -> SERVICE_UNAVAILABLE (503). There is no 502 path anywhere in the PKG, and the /statusevents/{token} operation just below correctly documents 503 for the same UpstreamError. Since #212 will validate live PKG responses against this spec, a real unknown-session lookup returns 503 and would mismatch the documented 502. Change this to "503".
| description: "The session token from `/v2/request/start`." | ||
| schema: | ||
| type: string | ||
| responses: |
There was a problem hiding this comment.
Missing documented error responses. Both jwt and statusevents validate {token} with is_valid_session_token and return Error::SessionTokenInvalid -> 400 for a malformed token (jwt.rs:11-13, statusevents.rs:25-27; each has a passing *_rejects_malformed_token_with_400 test), plus Error::Unexpected -> 500 when the upstream is unreachable. Neither 400 nor 500 is documented on these two operations. redocly's operation-4xx-response warnings [7]/[8] point at exactly these operations, so the PR description's claim that the remaining lint warnings are only house-style (license field, localhost server) isn't quite accurate: two of the eight flag a genuine missing 4xx. Please add the 400 (and ideally 500) responses here and on statusevents.
| }, | ||
| "conjunction": { | ||
| "title": "Conjunction", | ||
| "description": "An AND of attributes. An EMPTY conjunction inside a disjunction marks the whole disjunction as skippable (Yivi's optional-disjunction convention). Ordering note (irmamobile#360): deployed Yivi apps mis-render a disjunction whose empty alternative comes FIRST, so clients currently place the empty alternative LAST; the server accepts either order.", |
There was a problem hiding this comment.
nit (pre-existing, not introduced by this PR). This note, and the identical one at api-description.yaml:402, tells clients to place the empty alternative LAST as an irmamobile#360 workaround (apps mis-render an empty-first alternative). But the server's own optional:true expansion emits the empty conjunction FIRST: start.rs:60 vec![vec![], vec![ar]], with a test asserting dis[0].is_empty() ("empty alternative first"). If the #360 mis-render is real, the server-generated optional discon trips the same bug. The new contract docs make the contradiction explicit, so it's worth either scoping the note to client-supplied discons or reordering start.rs.
|
Reviewed this and posted a request changes review with 3 inline comments:
Everything else — commit title, closing keywords, formatting, prose — checked out clean, and the executable valid/invalid examples plus the #212 loop are a nice way to keep this contract honest. Details in the review: #242 (review) |
In a key-issuance request, a silently ignored misspelled field (`vaule` for
`v`, `optioanl` for `optional`) widens the disclosure the caller intended —
intent-weakening must be a 400, not a reinterpretation. Deployment reality
allows going strict directly: all clients are first-party.
- deny_unknown_fields on IrmaAuthRequest, DisclosureAttribute and
SigningKeyRequest. identity::Attribute stays lenient deliberately: it is
shared with the bincode wire format, the wasm/FFI seal boundary and
response policies, and its values are not load-bearing in request paths.
- ConItem's untagged Deserialize replaced with a manual visitor: the JSON
shape (object vs array) discriminates the variants, so the caller gets the
precise inner error ("unknown field `vaule`, expected one of `t`, `v`,
`optional`") instead of "did not match any variant".
- Body-deserialization failures now use the documented {error, message}
envelope (JsonConfig error_handler) instead of actix's plain-text default.
- Schema/OpenAPI flipped to additionalProperties: false; examples stripped of
$comment (which only ever parsed thanks to the old leniency — commentary
moved to examples/README.md); new executable invalid vectors for the typo
cases; tests assert the 400 names the offending field.
rubenhensen
changed the title
…sevents response docs
The contract docs surfaced a contradiction: clients place a discon's empty
alternative LAST to dodge irmamobile#360 (apps mis-render empty-first), but
the server's own optional:true expansion emitted it FIRST — tripping the very
bug the workaround exists for. The expansion now appends it last; either
order is still accepted on input, and the schema/OpenAPI notes say so.
Also per review: /v2/request/jwt/{token} maps upstream error statuses to 503
(UpstreamError), not the previously documented 502, and both jwt and
statusevents additionally return 400 (malformed session token) and 500
(upstream unreachable) — now documented.
|
All three addressed, /dobby:
|
|
Thanks for the update — I'm on it. Starting my review of the PR now, focusing on the JWT 502→503 fix, the new 400/500 documentation, and especially the server-side reordering of the empty alternative in |
The #360 mis-render of empty-first alternatives is fixed and shipped (irmamobile PRs #557/#558, released in v8.1.0 on 2026-07-10; issue closed 2026-07-14), and postguard-website has already reverted to the conventional leading-empty order. Prescribing empty-last in the new contract docs — and flipping the server's optional expansion to match — was codifying a workaround for a dead bug. - start.rs: optional expansion back to the conventional empty-first - schema/OpenAPI/api.rs: ordering documented as presentational with either order accepted; #360 kept only as a one-line historical note - examples: condiscon-name-disjunction.json now mirrors the website's actual current request verbatim (leading empty alternative, four name sources, optional phone/dob); optional-discon example renamed and reordered
|
Correction to my earlier '#360 workaround' change, prompted by review discussion: the workaround is obsolete and is now removed rather than codified. Verified timeline: fix merged upstream (irmamobile #557/#558, June 12/15), shipped in v8.1.0 (July 10), issue closed July 14 — and postguard-website has already reverted to the conventional leading-empty order in So the previous commit had it backwards (it moved the server toward the obsolete order). Now: the server's |
There was a problem hiding this comment.
Solid contract-pinning PR — the parser change is well-reasoned (a silently-dropped vaule widening a disclosure is exactly the kind of leniency a key-issuance API should not have), and the executable valid//invalid/ vectors plus the end-to-end 400-envelope test make it hard to regress. The three points from the last round are all addressed, and the empty-alternative fix (emit the empty option last, per irmamobile#360) is correct and now matches the client-side workaround.
Two non-blocking notes before merge:
-
PR title / release-plz semver.
deny_unknown_fieldsonIrmaAuthRequest/DisclosureAttribute/SigningKeyRequestis a genuine client-observable break — you already say so in the "Breaking note". Per this repo's CLAUDE.md, release-plz decides the bump from the squash-merge subject (the PR title), and a break needsfeat(api)!:plus aBREAKING CHANGE:footer; without the!this ships as a minor bump with no changelog warning. Recommend retitling tofeat(api)!:and adding the footer so the release reflects the break. -
One stale doc comment on
IrmaAuthRequest(inline).
Neither blocks — this is good to merge once the title reflects the break.
| @@ -45,6 +51,7 @@ pub struct DisclosureAttribute { | |||
| /// `[{t,v?,optional?}, ...]` JSON shape still deserialises, because | |||
| /// `ConItem` is `#[serde(untagged)]`. | |||
| #[derive(Debug, Serialize, Deserialize)] | |||
There was a problem hiding this comment.
Stale after this PR: the comment says the flat shape still deserialises because ConItem is #[serde(untagged)], but this PR replaces the untagged deserialize with a manual Deserialize impl (the visit_map/visit_seq visitor below) and drops Deserialize from ConItem's derive. #[serde(untagged)] now governs only Serialize. Anyone debugging a deserialize error will be misdirected — point this at the manual impl instead (which is what actually keeps the flat {t,v?,optional?} shape parsing, via visit_map).
| /// `ConItem` is `#[serde(untagged)]`. | ||
| #[derive(Debug, Serialize, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct IrmaAuthRequest { |
There was a problem hiding this comment.
This deny_unknown_fields (and the ones on DisclosureAttribute and SigningKeyRequest) is a client-observable breaking change: requests that previously succeeded with an extra/misspelled field now 400. That is the right behaviour, but per CLAUDE.md release-plz derives the version bump from the PR title, and feat(api): (no !) ships this as a minor bump with no BREAKING CHANGE: changelog entry. Recommend feat(api)!: + a BREAKING CHANGE: footer so the release matches the Breaking note in the PR body.
|
Nice, all three check out cleanly against HEAD — the 503 mapping, the 400/500 docs on |
1 participant
Closes #209, closes #210 (Phase 1 of EPIC #201) — and goes one step further than pinning the contract: it fixes the leniency the contract would otherwise have had to document as a flaw.
The behavioral change: unknown request fields are rejected
Previously the parser silently ignored unknown fields, so a misspelled
v(vaule) widened the disclosure the caller intended — a silent intent-weakening in a key-issuance API. Now:IrmaAuthRequest,DisclosureAttribute,SigningKeyRequestare#[serde(deny_unknown_fields)]→ a typo is a 400 naming the field, not a reinterpretation.ConItem's untagged deserialize is replaced with a manual visitor (object vs array discriminates the variants), so clients get serde's precise inner error — ``unknown fieldvaule, expected one of `t`, `v`, `optional``` — instead of "did not match any variant".{error, message}envelope (previously actix's plain-text default — the spec and reality disagreed).identity::Attributestays lenient deliberately: it's shared with the bincode wire format, the wasm/FFI seal boundary and response policies, and its values aren't load-bearing in request paths (documented in the spec).Breaking note: any client sending extra request fields gets 400s after this. All clients are first-party and none send extra fields (pg-js/dotnet/harness send exact shapes); the two PoC deployments should update/verify — the error message tells them exactly what to fix if anything surfaces.
The pinned contract
pg-pkg/api-description.yaml(PKG v2 OpenAPI spec #209): the full v2 OpenAPI (all endpoints incl./statusevents/{token}, both auth modes, error envelope, rate limits, the/v2/{irma|request}alias policy). Lints clean (redocly).pg-pkg/schemas/irma-auth-request.schema.json(Condiscon request JSON Schema + canonical examples #210): the condiscon request as JSON Schema, nowadditionalProperties: false— schema and parser agree with no caveats.pg-pkg/tests/contract_examples.rs): everyvalid/example must parse, everyinvalid/must be rejected — including the new typo vectors (unknown-attribute-field.json,unknown-top-level-field.json). Commentary lives inexamples/README.md(JSON comments would now rightly be rejected — the old$comments only parsed thanks to the leniency this PR removes). Covers the legacy flat shape, the website's name disjunction, the optional empty-conjunction convention with the irmamobile#360 ordering workaround, and the classic nesting mistakes.test_unknown_request_field_yields_400_envelopeproves the 400 envelope + field-naming end-to-end through the actix extractor.Tests: pg-core 57 + api tests, pg-pkg 58 + 2 contract tests — all green.
Follow-up (#212, postguard-e2e): validate live PKG responses against the OpenAPI and the examples against the schema, closing the loop schema ↔ parser ↔ running server.