Skip to content

refactor(auth): make step-up-without-Agent-binding unrepresentable (HT-75) #97

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zaridan merged 1 commit into from
Jul 20, 2026
Merged

refactor(auth): make step-up-without-Agent-binding unrepresentable (HT-75) #97

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions src/auth/webauthn-ceremony.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,16 +75,40 @@ export interface WebAuthnCeremonyDeps {
rp: WebAuthnRpConfig
}

export interface VerifyAuthenticationCeremonyParams {
/** Which ceremony this verify call expects — checked against the token's OWN `ceremony` field before anything else runs (spec §7's application-level discriminator check). */
ceremony: 'authentication' | 'step-up'
interface VerifyAuthenticationCeremonyParamsBase {
/** The raw, untrusted request-body `response` field. */
responseJson: unknown
challengeToken: string
/** Step-up only (spec §5.1): the resolved credential's `agent_id` must equal this, or the ceremony is rejected — proving a factor for a DIFFERENT Agent does not step up THIS session. */
requireAgentId?: string
}

/**
* A discriminated union rather than one shape with an optional
* `requireAgentId`, so that "step-up without an Agent to bind to" cannot be
* written at all.
*
* That combination silently disabled BOTH halves of step-up's Agent binding
* — the pre-consume check here and `consumeChallenge`'s `AND agent_id = $3`
* — reopening the challenge-burn DoS the binding exists to close. No caller
* ever did it, but nothing except this type stopped one from starting
* (Codex review, PR #96). `authentication` takes `requireAgentId?: never`
* for the mirror-image reason: its challenges are minted with
* `agent_id IS NULL` (discoverable credential, spec §4.3), so binding one
* would match no row and break every login.
*/
export type VerifyAuthenticationCeremonyParams = VerifyAuthenticationCeremonyParamsBase &
(
| {
/** Checked against the token's OWN `ceremony` field before anything else runs (spec §7's application-level discriminator check). */
ceremony: 'authentication'
requireAgentId?: never
}
Comment on lines +98 to +104

@coderabbitai coderabbitai Bot Jul 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n 'exactOptionalPropertyTypes' --glob 'tsconfig*.json' --glob '*.json' .
rg -n 'requireAgentId' --glob '*test*.ts' --glob '*test*.tsx' .

Repository: Helpthread/helpthread

Length of output: 159

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== tsconfig files ==\n'
git ls-files 'tsconfig*.json' '**/tsconfig*.json'

printf '\n== exactOptionalPropertyTypes occurrences ==\n'
rg -n 'exactOptionalPropertyTypes' --glob 'tsconfig*.json' --glob '**/tsconfig*.json' --glob '*.json' .

printf '\n== webauthn ceremony file map ==\n'
ast-grep outline src/auth/webauthn-ceremony.ts --view expanded || true

printf '\n== requireAgentId test/type-test occurrences ==\n'
rg -n 'requireAgentId' --glob '*test*.ts' --glob '*test*.tsx' --glob '*.ts' --glob '*.tsx' src test tests .

printf '\n== nearby source context ==\n'
sed -n '1,220p' src/auth/webauthn-ceremony.ts

Repository: Helpthread/helpthread

Length of output: 259

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== tsconfig.json ==\n'
cat -n tsconfig.json

printf '\n== web/tsconfig.json ==\n'
cat -n web/tsconfig.json

printf '\n== src/auth/webauthn-ceremony.ts (1-180) ==\n'
sed -n '1,180p' src/auth/webauthn-ceremony.ts

printf '\n== requireAgentId mentions in repo ==\n'
rg -n 'requireAgentId' src web . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'

Repository: Helpthread/helpthread

Length of output: 14190

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== type-test / expect-error occurrences ==\n'
rg -n '`@ts-expect-error`|tsd|expectType|satisfies VerifyAuthenticationCeremonyParams|VerifyAuthenticationCeremonyParams' src tests .

printf '\n== src/auth/webauthn-ceremony.test.ts (160-260) ==\n'
sed -n '160,260p' src/auth/webauthn-ceremony.test.ts

printf '\n== package.json ==\n'
cat -n package.json

Repository: Helpthread/helpthread

Length of output: 6840

Tighten the authentication branch’s requireAgentId restriction at src/auth/webauthn-ceremony.ts:98-104
requireAgentId?: never still accepts { ceremony: 'authentication', requireAgentId: undefined } under the current TypeScript config, since neither tsconfig.json nor web/tsconfig.json enables exactOptionalPropertyTypes. Turn that on for the checked-in configs and add a negative type test for the explicit undefined case.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/auth/webauthn-ceremony.ts` around lines 98 - 104, Enable
exactOptionalPropertyTypes in both checked-in TypeScript configurations, then
add a negative type test covering `{ ceremony: 'authentication', requireAgentId:
undefined }` against VerifyAuthenticationCeremonyParams. Keep the authentication
branch’s requireAgentId?: never restriction unchanged and ensure the test
verifies explicit undefined is rejected.

| {
ceremony: 'step-up'
/** Required (spec §5.1): the challenge must have been minted for this Agent, AND the resolved credential's `agent_id` must equal it — proving a factor for a DIFFERENT Agent does not step up THIS session. */
requireAgentId: string
}
)

/** Every rejection this function can return collapses to one of two client-visible outcomes (spec §4.3, §6.2): `'challenge_expired'` (the one deliberate, safe exception to uniform 401 — see webauthn-provider.ts) or `'invalid'` (everything else, including a ceremony mismatch, unknown credential, bad signature, counter regression, inactive Agent, or userHandle mismatch — no finer distinction is ever surfaced). */
export type CeremonyVerifyResult =
| { ok: true; agentId: string }
Expand Down