test(signing): live-verifier integration for HTTPS stores#616
Merged
Conversation
Dev-only CI artifact. Fills the integration-test gap between the per-store unit tests (`request-signing-https-stores.test.js`) and the RFC 9421 conformance grader (`request-signing-grader-*.test.js`): the real HTTPS stores run inside the verifier pipeline across multi-request flows where the origin-side JWKS / revocation state changes mid-session. Scope: not a specialism grader. External agents have no way to let a remote tester drive their JWKS / revocation endpoints, so this file intentionally lives outside `@adcp/client/testing/storyboard/request-signing` (which stays pure spec-conformance) and is not exposed via any public barrel. Scenarios (8 tests): - Mid-run JWKS rotation — verifier picks up a rotated kid after cooldown without a process restart. - Mid-run revocation publish — freshly-revoked kid rejected at step 9 on the next verify past next_update. Hit-counter assertion pins the refresh actually fired (vs. passing coincidentally on replay-cache collision). - Stale-revocation fail-closed — origin silent past next_update + grace surfaces `request_signature_revocation_stale`. - Garbage-response-during-grace — origin serves 500/HTML after next_update. Within grace: cached snapshot still enforced. Past grace: fail-closed. The likeliest real incident (flaky CDN, wrong content-type) and the property that stops routine origin blips from cascading into cross-counterparty outages. - JWKS URL at IMDS → `SsrfRefusedError` surfaces from verifier. - Revocation URL at loopback without dev opt-in → `SsrfRefusedError` surfaces. - Middleware SSRF pass-through — `createExpressVerifier` sees the error at `next(err)`, NOT mapped to a 401. Locks in the invariant that SSRF refusals don't get laundered as transient auth failures a misbehaving caller might retry. - Combined rotation + revocation — one verifier, four sequential requests, exercises the cross-store cache / refresh interaction that can't surface when each store is tested in isolation. Expert review in-PR: - Extracted `startJwksServer` / `startRevocationServer` to `test/helpers/signing-origin-servers.js` instead of duplicating the existing unit-test copies. Shared helper adds a `responseOverride` hook needed by the garbage-response scenario. - Every scenario now wraps server lifecycle in `withServers()` (avoids a CI hang if setup throws before a try block would have been entered). - Tightened assertions: rotation and revocation tests assert request counts incremented, not just that the verdict matches. Loose shape assertions were letting regressions in adjacent paths pass by coincidence. - Middleware SSRF test added in response to the security reviewer's observation that `createExpressVerifier` handles SSRF differently from `verifyRequestSignature` and the file didn't pin that down. No library code changes. No changeset (test files only per project guidelines). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two follow-ups on PR #616: - Changeset CI gate fails on any non-doc file change without a corresponding changeset, even for test-only PRs. Adding an empty changeset to satisfy the check. - Security reviewer flagged the garbage-response-during-grace scenario for proving only that a non-revoked kid still verifies during grace — a regression that cleared the cached revocation list on refresh-failure would silently pass. Adding an `already-revoked-kid` to the JWKS and asserting it stays rejected at step 9 during grace closes that. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #609. Dev-only integration test — no library surface changes, no public barrel additions, no changeset.
Why
Existing signing test coverage splits into:
request-signing-https-stores.test.js):HttpsJwksResolverandHttpsRevocationStorein isolation.request-signing-grader-*.test.js): RFC 9421 vectors against aStaticJwksResolver+InMemoryRevocationStore.Neither runs the real HTTPS stores behind
verifyRequestSignatureacross a multi-step flow where the origin-side state changes mid-session. That's the wiring between PR #601's new moving parts and the verifier pipeline — pilots that depend on rotation/revocation working transparently need that integration covered.Scope / non-scope
HttpsJwksResolver+HttpsRevocationStorebehindverifyRequestSignature, with origin-side state transitions between requests. AlsocreateExpressVerifierso we pin middleware SSRF handling.@adcp/client/testing/storyboard/request-signing— the operator-facing grader stays pure spec-conformance.Scenarios (8 tests)
next_update + grace→request_signature_revocation_stalenext_update; within grace = cached snapshot enforced; past grace = fail-closedSsrfRefusedErrorwith codealways_blocked_addresssurfaces fromverifyRequestSignatureSsrfRefusedErrorwith codeprivate_addresssurfacescreateExpressVerifiersends the error tonext(err), NOT mapped to 401Expert-review fixes in-PR
Both code-reviewer and security-reviewer ran on the draft.
Code review Should-Fix → fixed:
startJwksServer/startRevocationServertotest/helpers/signing-origin-servers.jsinstead of duplicating from the existing unit-test file.withServers()(guarantees stop() runs even when setup throws — avoids a CI hang masking a real regression).err instanceof X && err.code === 'y').Code review Nice-to-Have → added:
Security review Should-Fix → fixed:
createExpressVerifierroutes SSRF refusals tonext(err), not 401. Future regression that wrapped SSRF asrequest_signature_key_unknown(a plausible shortcut in the JWKS resolver's catch path) would silently leak SSRF refusals as 401, train callers to retry, and shift attack surface. Pinning it down.Security review Consider → deferred:
maxValidityWindowSecondssurfaced through verifier → already covered by store unit tests, not load-bearing here.Test plan
npm run typecheckcleannpm run format:checkcleannpm run buildcleannode --test test/request-signing-live-verifier-integration.test.js— 8/8 passnpm test— 3923/3925 pass (2 pre-existing skips; the governance-e2e failures that were present before feat(governance): authority_level → reallocation_threshold / human_review_required (#576) #603 have since been fixed)🤖 Generated with Claude Code