fix(auth): bind step-up challenge consumption to the acting Agent (HT-75) - #96

…-75)

Found by an independent Codex review of PR #94, after that PR merged.

`verifyAuthenticationCeremony` consumed the `webauthn_challenges` row
immediately after the token's signature/ceremony check, and only then
resolved the credential and applied `requireAgentId`. But `requireAgentId`
was compared against the CREDENTIAL's owner — never against the Agent the
challenge itself was minted for, even though both the signed token and the
DB row already carry that `agentId`.

Consequence: anyone holding a victim's step-up challenge token could spend
it from their own session with any garbage `response`. The consume
succeeded, verification then failed, and the victim's own legitimate verify
came back `challenge_expired`. Not an authentication bypass — the assertion
is still cryptographically verified and the credential-owner check still
holds — but a griefing DoS against a real user's step-up.

Fixed at both layers, matching spec §7's "two independent layers, not
duplicated logic":

- Application: reject when the token's `agentId` is not the acting Agent,
  BEFORE the consume runs.
- Database: `consumeChallenge` takes an optional `expectedAgentId` that
  adds `AND agent_id = $3` to the UPDATE predicate.

The login ceremony is deliberately unaffected: its challenges are minted
with `agent_id IS NULL` (discoverable credential, spec §4.3), so the
binding is conditional on `requireAgentId` rather than unconditional.

Regression test proven to catch the original bug: with both layers
reverted, it fails on the burn assertion (the victim's row is already
consumed). One pre-existing step-up test was minting its challenge with
`agentId: null`, which the new check would have short-circuited — it now
mints for the impersonator so it still exercises the credential-owner
check it is named for.

Gates: typecheck, lint, test:coverage all exit 0.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…me too

Codex review of the previous commit, findings 2 and 5.

Finding 5 (the real one): the step-up ceremony test could not fail if the
new `AND agent_id = $3` clause were deleted. The app-layer check returns
first, so the store's clause was never reached — dead weight no test would
notice losing. Two direct store-level tests added: a step-up row minted for
one Agent is not consumable under another (proven to fail when the clause is
removed), and a login row with `agent_id IS NULL` stays consumable when no
Agent is expected, which guards the SQL branch itself.

This also corrects an overclaim in the previous commit's PR description.
Disabling the app layer and watching the test fail showed the DB clause
firing in THAT configuration; it did not show the committed test covering
the clause. Those are different properties and only the first was verified.

Finding 2: registration's consume omitted the Agent argument, so its binding
rested solely on the app-level check above it. No live bug — that check is
correct — but the DB layer was not defense in depth the way step-up's now
is. Passed through for consistency.

Not addressed, deliberately: `verifyAuthenticationCeremony` still accepts
`ceremony: 'step-up'` with no `requireAgentId`, which would silently disable
the binding. No caller does this. Making it unrepresentable is a type-level
signature change beyond this fix's scope — noted in the PR for TJ.

Gates: typecheck, lint, test:coverage all exit 0.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>