integration: OAuth 2.1 discovery + dynamic registration + consent + refresh grant#156
Merged
integration: OAuth 2.1 discovery + dynamic registration + consent + refresh grant#156
Conversation
Introduces storage for user consent records (one active row per (user, client) pair, revocation preserved for audit) and a dynamic_registered_at column on oauth_clients consumed by the upcoming consent UI to badge newly registered clients. Required groundwork for the /oauth/authorize consent screen.
Adds `exportPublicJwk` to @qauth-labs/server-jwt so callers can convert imported EdDSA public keys into RFC 7517 JWKs with `use: 'sig'`, `alg: 'EdDSA'`, and an optional `kid`. Defensive strip of the `d` member prevents accidental private-key leakage if a caller passes the wrong key. Prepares the JWT plugin for serving `/.well-known/jwks.json` (issue #148).
Adds `getIssuer()` and `getJwks()` to the Fastify JWT plugin so routes can publish RFC 8414 / OIDC Discovery metadata and RFC 7517 JWKS without reaching into the plugin internals. An optional `keyId` plugin option feeds the JWKS `kid` member so future key rotation is a config change. Supports issue #148 (discovery endpoints).
Adds a jsonb column on realms holding the scope cap for RFC 7591 Dynamic Client Registration. Scopes requested at /oauth/register must be members of this list. Admin / tenant-scoped grants MUST NOT appear here.
- REGISTER_CLIENT_RATE_LIMIT / REGISTER_CLIENT_RATE_WINDOW: IP-scoped rate limit for /oauth/register, defaulting to match /oauth/token. - DEFAULT_DYNAMIC_REGISTRATION_SCOPES: seeds a realm's empty dynamic_registration_allowed_scopes list on first /oauth/register hit. Default is OIDC core only (openid profile email offline_access) — admin / tenant scopes must be added explicitly by an operator.
Publishes three unauthenticated well-known documents so clients (incl. MCP clients kicking off OAuth on 401) can auto-discover endpoints and signing keys: - GET /.well-known/oauth-authorization-server (RFC 8414) - GET /.well-known/openid-configuration (OIDC Discovery 1.0) - GET /.well-known/jwks.json (RFC 7517) All three return Cache-Control: public, max-age=3600, and the JWKS uses application/jwk-set+json. Endpoint URLs derive from JWT_ISSUER so operators configure discovery by setting the issuer, not per-endpoint env vars. Metadata advertises the OAuth 2.1 surface: response_types limited to code, code_challenge_methods limited to S256, grant_types including refresh_token (sibling branch) and registration_endpoint (#149) so discovery stays stable as those land. Closes #148.
…ration) Open-mode dynamic client registration. No initial_access_token required — defense-in-depth lives at the consent screen (#150). Hard limits enforced here: - IP-scoped rate limit (REGISTER_CLIENT_RATE_LIMIT), at least as strict as /oauth/token. - Requested scopes are capped to the realm's dynamic_registration_allowed_scopes list; any scope outside is rejected with invalid_client_metadata. - Public clients (token_endpoint_auth_method=none) receive no client_secret in the response; requirePkce is forced true on the DB row per OAuth 2.1 §4.1.3. - client_credentials grant is rejected with auth_method=none. - redirect_uri validation per OAuth 2.1 §10.3: http:// only for loopback, fragments rejected, custom schemes permitted for native apps per RFC 8252. Response shape follows RFC 7591 §3.2.1, with no-store caching headers. Registration events are written to the audit log.
Introduces a `family_id` column on `refresh_tokens` plus a covering index. Every rotation preserves the column so that replay of a revoked token can be attributed to — and revoke — an entire family atomically (OAuth 2.1 §4.3.1 / RFC 9700 §2.2.2). Repository gains two siblings to the existing revoke API: `findByTokenHashIncludingRevoked` (so replay is detectable at all) and `revokeFamily` (revokes every non-revoked row sharing a family_id in one statement, preserving the original revokedReason for audit).
Dispatches a third grant type at the OAuth token endpoint. New `handleRefreshToken` sibling to authorization_code/client_credentials: - Looks up the presented token regardless of revoked/expired state so replay is detectable. - Rejects cross-client presentation (invalid_grant) before any family revocation fires, so replay on one client can never fan out revocation on another. - Replay detection: a revoked-but-known token revokes its entire `family_id` (OAuth 2.1 §4.3.1, RFC 9700 §2.2.2) and the request fails invalid_grant. - Rotation: the presented token is revoked as 'rotated' BEFORE the new token is persisted — no two tokens in the same chain link can be live at once. - Down-scoping honoured (subset of original scopes); any scope outside the original set is rejected as invalid_scope. - Public clients (`token_endpoint_auth_method: 'none'`) present only client_id and are bound by refresh-token ownership; a new `authenticateClientForRefresh` helper handles the split. Audit logs carry `familyId` on success, and a security-event row is written when a family is revoked on replay. Authorization-code initial issuance and /auth/login now seed a new family_id per token so rotations can inherit it cleanly.
… surface `/auth/refresh` predated the OAuth-compliant token endpoint; it issued refresh tokens that /oauth/token could not accept, leaving a bifurcated refresh story. With `grant_type=refresh_token` landing at /oauth/token, the legacy endpoint is redundant — and keeping two surfaces with different rotation/ownership rules is a footgun. Audit confirmed no external callers (only the route's own tests + MVP docs mention the path). The now-unused `refreshSchema`/`refreshResponseSchema` Zod shapes are also removed. `REFRESH_RATE_LIMIT` env var stays in place for potential future use.
Adds the browser-driven authentication and consent flow required by issue #150 before dynamic client registration can ship: - Session cookie: __Host-qauth_session carries a signed session id (HMAC-SHA256) bound to a Redis-backed payload. HttpOnly, SameSite=Lax, Secure configurable for local dev. Timing-safe verification. - Login UI: GET/POST /ui/login renders a server-side form that sets the session cookie and redirects to a validated, same-origin return_to path (open-redirect-safe). Reuses the JSON login's password verification and audit log plumbing. - Consent UI: GET/POST /ui/consent renders the consent screen with client name, optional homepage_uri, scope descriptions, audience, a dynamic-client phishing-defense badge, "Allow forever" toggle, and timing-safe double-submit CSRF token rotated per render. - Authorize route: now accepts either the session cookie or the legacy Bearer token. Missing both → redirect to /ui/login with return_to; cookie + no matching consent → redirect to /ui/consent; cached consent that covers the requested scopes → issue code directly. - Scope-subset check and dynamic-client badge logic live in a dedicated helper with unit tests. Out of scope for this commit: revocation UI, DPoP, 2FA, audit-trail surfacing. Dynamic client registration will populate dynamicRegisteredAt in a sibling branch.
Adds the consent-management side of issue #150: users can see every active grant and revoke any of them. - Auth-server: GET /consents returns the authenticated user's active consents joined to client name/id; DELETE /consents/:id revokes one after an ownership check so a user cannot take back someone else's grant. Both endpoints require the __Host-qauth_session cookie. - Developer portal: a minimal /consents page lists each row (name, client_id, scopes, granted timestamp) with a single Revoke button. Uses credentials:'include' so the signed session cookie reaches the auth-server regardless of deployment origin. Scope intentionally kept minimal per issue guidance (list + revoke, no UX polish).
Drizzle-kit regen across the three parallel branches produces a single 0002_oauth_mcp_stack.sql covering: - refresh_tokens.family_id (#151) - oauth_consents table + oauth_clients.dynamic_registered_at (#150) - realms.dynamic_registration_allowed_scopes (#149) Also wires the cross-branch dependency: /oauth/register now stamps dynamic_registered_at on insert so the consent screen's dynamic-client badge surfaces correctly within DYNAMIC_CLIENT_BADGE_DAYS. Original per-branch migrations (0002_add_refresh_tokens_family_id, 0002_add_oauth_consents_and_dynamic_client_flag, 0002_add_dynamic_registration_allowed_scopes) are kept in their respective PRs (#152-#155); this consolidation exists only on the integration branch for immediate use downstream.
Drizzle-Zod typed-router enforces declared status codes on reply.code. The route returns 401 when the session cookie is missing, so add 401 to the response schema on both GET /consents and DELETE /consents/:id. Surfaced only on the integration branch because auth-server typecheck runs against the combined code base; individual PRs passed in isolation.
|
View your CI Pipeline Execution ↗ for commit 25ca8db
☁️ Nx Cloud last updated this comment at |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive OAuth 2.1 and OIDC stack, including browser-based session management via signed cookies, server-rendered login and consent pages, and dynamic client registration. It also introduces refresh token rotation with family-based replay detection and a consent revocation UI. Feedback focuses on improving security and robustness: specifically, using a Symbol for SafeHtml markers to prevent forgery, wrapping upsertGrant and refresh token rotation in database transactions to ensure atomicity and prevent race conditions, and optimizing the consents listing endpoint to avoid N+1 queries. Additionally, it is recommended to avoid rotating CSRF tokens on every GET request to improve the multi-tab user experience.
Comment on lines
+24
to
+36
| /** Marker wrapper for values that should NOT be HTML-escaped. */ | ||
| export interface SafeHtml { | ||
| readonly __safe: true; | ||
| readonly value: string; | ||
| } | ||
|
|
||
| export function safe(value: string): SafeHtml { | ||
| return { __safe: true, value }; | ||
| } | ||
|
|
||
| function isSafe(x: unknown): x is SafeHtml { | ||
| return typeof x === 'object' && x !== null && (x as SafeHtml).__safe === true; | ||
| } |
There was a problem hiding this comment.
The SafeHtml marker __safe: true is a plain property that can be easily forged if user-controlled objects (e.g., from a JSON body or a complex query string parser) are passed to the html template tag. Using a Symbol for the marker would make it impossible to forge via external input, as Symbols are not serializable.
const SAFE_MARKER = Symbol('SafeHtml');
/** Marker wrapper for values that should NOT be HTML-escaped. */
export interface SafeHtml {
readonly [SAFE_MARKER]: true;
readonly value: string;
}
export function safe(value: string): SafeHtml {
return { [SAFE_MARKER]: true, value } as SafeHtml;
}
function isSafe(x: unknown): x is SafeHtml {
return typeof x === 'object' && x !== null && (x as SafeHtml)[SAFE_MARKER] === true;
}
Comment on lines
+58
to
+104
| async upsertGrant( | ||
| userId: string, | ||
| oauthClientId: string, | ||
| realmId: string, | ||
| scopes: string[], | ||
| tx?: DbClient | ||
| ): Promise<OAuthConsent> { | ||
| const invoker = tx ?? defaultDb; | ||
| const now = Date.now(); | ||
|
|
||
| const [existing] = await invoker | ||
| .select() | ||
| .from(oauthConsents) | ||
| .where( | ||
| and( | ||
| eq(oauthConsents.userId, userId), | ||
| eq(oauthConsents.oauthClientId, oauthClientId), | ||
| isNull(oauthConsents.revokedAt) | ||
| ) | ||
| ) | ||
| .limit(1); | ||
|
|
||
| if (existing) { | ||
| // Union the scope sets so a narrower subsequent grant cannot silently | ||
| // remove previously-granted scopes. Callers rely on this to detect | ||
| // "already consented" for scope-subset checks. | ||
| const union = Array.from(new Set([...existing.scopes, ...scopes])).sort(); | ||
| const [updated] = await invoker | ||
| .update(oauthConsents) | ||
| .set({ scopes: union, grantedAt: now }) | ||
| .where(eq(oauthConsents.id, existing.id)) | ||
| .returning(); | ||
| return updated; | ||
| } | ||
|
|
||
| const [created] = await invoker | ||
| .insert(oauthConsents) | ||
| .values({ | ||
| userId, | ||
| oauthClientId, | ||
| realmId, | ||
| scopes: [...scopes].sort(), | ||
| grantedAt: now, | ||
| }) | ||
| .returning(); | ||
| return created; | ||
| }, |
There was a problem hiding this comment.
The upsertGrant method performs a read-then-update/insert operation without a transaction or row-level locking. This can lead to race conditions where concurrent requests for the same user and client result in lost updates to the granted scopes or duplicate insertion attempts. Wrapping the logic in a transaction with a FOR UPDATE lock on the existing row ensures atomicity.
async upsertGrant(
userId: string,
oauthClientId: string,
realmId: string,
scopes: string[],
tx?: DbClient
): Promise<OAuthConsent> {
const invoker = tx ?? defaultDb;
const now = Date.now();
return await invoker.transaction(async (innerTx) => {
const [existing] = await innerTx
.select()
.from(oauthConsents)
.where(
and(
eq(oauthConsents.userId, userId),
eq(oauthConsents.oauthClientId, oauthClientId),
isNull(oauthConsents.revokedAt)
)
)
.limit(1)
.for('update');
if (existing) {
const union = Array.from(new Set([...existing.scopes, ...scopes])).sort();
const [updated] = await innerTx
.update(oauthConsents)
.set({ scopes: union, grantedAt: now })
.where(eq(oauthConsents.id, existing.id))
.returning();
return updated;
}
const [created] = await innerTx
.insert(oauthConsents)
.values({
userId,
oauthClientId,
realmId,
scopes: [...scopes].sort(),
grantedAt: now,
})
.returning();
return created;
});
},References
- To prevent race conditions in read-then-update scenarios (e.g., checking and then using a token), either use an atomic update with built-in checks in the WHERE clause, or wrap the separate read and update operations in a database transaction.
Comment on lines
+271
to
+276
| const csrfToken = generateCsrfToken(); | ||
| await fastify.sessionUtils.setSession<BrowserSessionData>( | ||
| session.sessionId, | ||
| { ...session, csrfToken }, | ||
| env.SESSION_COOKIE_TTL | ||
| ); |
There was a problem hiding this comment.
Rotating the CSRF token on every GET request to the consent screen can break the user experience if they open the page in multiple tabs. Only the last tab opened will have a valid token, causing form submissions from earlier tabs to fail. It is safer to only generate and store a new token if one does not already exist in the session.
Suggested change
| const csrfToken = generateCsrfToken(); | |
| await fastify.sessionUtils.setSession<BrowserSessionData>( | |
| session.sessionId, | |
| { ...session, csrfToken }, | |
| env.SESSION_COOKIE_TTL | |
| ); | |
| const csrfToken = session.csrfToken ?? generateCsrfToken(); | |
| if (!session.csrfToken) { | |
| await fastify.sessionUtils.setSession<BrowserSessionData>( | |
| session.sessionId, | |
| { ...session, csrfToken }, | |
| env.SESSION_COOKIE_TTL | |
| ); | |
| } |
Comment on lines
+54
to
+70
| const rows = await fastify.repositories.oauthConsents.listActiveForUser(session.userId); | ||
|
|
||
| // N+1 is fine at the scale of "consents a user has granted". If this | ||
| // ever becomes a problem, add a joined `listActiveForUserWithClient` | ||
| // repo method. | ||
| const consents = await Promise.all( | ||
| rows.map(async (row) => { | ||
| const client = await fastify.repositories.oauthClients.findById(row.oauthClientId); | ||
| return { | ||
| id: row.id, | ||
| clientId: client?.clientId ?? 'unknown', | ||
| clientName: client?.name ?? 'Unknown application', | ||
| scopes: row.scopes, | ||
| grantedAt: row.grantedAt, | ||
| }; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
The GET /consents endpoint performs an N+1 query by fetching client details for each consent row in a loop. While the number of consents per user is likely small, this can be optimized by adding a joined query method to the repository (e.g., listActiveForUserWithClient) to fetch all necessary data in a single database round-trip.
Comment on lines
+646
to
+665
| // Rotate: revoke the presented token FIRST so a concurrent replay | ||
| // cannot also succeed. We then issue a new token carrying the same | ||
| // `familyId`. (The unique index on `token_hash` protects against the | ||
| // degenerate case of a collision.) | ||
| await fastify.repositories.refreshTokens.revoke(storedToken.id, 'rotated'); | ||
|
|
||
| const { token: newRefreshToken, tokenHash: newRefreshTokenHash } = | ||
| fastify.jwtUtils.generateRefreshToken(); | ||
| const refreshTokenExpiresAt = Date.now() + fastify.jwtUtils.getRefreshTokenLifespan() * 1000; | ||
|
|
||
| await fastify.repositories.refreshTokens.create({ | ||
| userId: user.id, | ||
| oauthClientId: client.id, | ||
| tokenHash: newRefreshTokenHash, | ||
| familyId: storedToken.familyId, | ||
| previousTokenHash: presentedHash, | ||
| expiresAt: refreshTokenExpiresAt, | ||
| scopes: grantedScopes, | ||
| }); | ||
|
|
There was a problem hiding this comment.
The refresh token rotation (revoking the old token and creating a new one) is performed as two separate database calls without an encompassing transaction. If the server crashes or the second call fails after the first one succeeds, the user will be left with a revoked token and no replacement, effectively logging them out. These operations should be performed atomically within a transaction.
References
- To prevent race conditions in read-then-update scenarios (e.g., checking and then using a token), either use an atomic update with built-in checks in the WHERE clause, or wrap the separate read and update operations in a database transaction.
- refresh rotation now atomic (token.ts): wrap revoke-old + insert-new in fastify.db.transaction so a crash between the two can't leave the user with a revoked token and no replacement. - upsertGrant now atomic (oauth-consents.repository.ts): select ... FOR UPDATE inside a transaction so concurrent 'allow forever' clicks can't race and drop one side's scope union. - SafeHtml marker is now a module-private Symbol (html.ts): forged user-input JSON cannot bypass escaping via a __safe: true key. - CSRF token reused across tabs (consent.ts): only mint a fresh token when the session has none; the token is still burned on successful POST. Fixes multi-tab UX breakage. - GET /consents now joins oauth_consents to oauth_clients in a single query (listActiveForUserWithClient) — no more per-row N+1 findById. Type plumbing: split DbClient (db | transaction union for repo params) from Database (top-level drizzle root) so fastify's db decorator stays typed precisely and repositories can participate in transactions. Test updates: token.test.ts expects tx arg on refresh rotation calls; consents.test.ts mocks the new joined-list method.
3 tasks
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.
Summary
Bundle of four issues delivering end-to-end OAuth 2.1 capability for first-class MCP / third-party client support. Merge this as a single integration step; individual feat PRs (#152–#155) stay open for focused review context but this branch is what ships.
Closes #148, closes #149, closes #150, closes #151.
What ships
/.well-known/oauth-authorization-server,/.well-known/openid-configuration,/.well-known/jwks.jsonapps/auth-server/src/app/routes/well-known.ts,libs/server/jwt/src/lib/jwks.tsPOST /oauth/register(RFC 7591, open mode + scope cap + rate limit)apps/auth-server/src/app/routes/oauth/register.ts,realms.dynamic_registration_allowed_scopescolumnoauth_consentstable, revocation UI in the developer portalapps/auth-server/src/app/routes/ui/{login,consent}.ts,apps/auth-server/src/app/routes/consents/index.ts,apps/developer-portal/src/routes/consents.tsxgrant_type=refresh_tokenat/oauth/token, rotation + family-level replay detection, removes the old/auth/refreshsurfaceapps/auth-server/src/app/routes/oauth/token.ts,refresh_tokens.family_idcolumnIntegration-only changes
0002_oauth_mcp_stack.sqlreplaces what would have been 0002/0003/0004/0005 had each PR merged separately. Clean single migration for operators applying this upgrade.ea7cb21) — surfaced only once the consent UI stack was integrated.Cross-links in the AS metadata
/.well-known/oauth-authorization-servernow advertises:grant_types_supported: ["authorization_code", "client_credentials", "refresh_token"]— all three backed by real dispatch at/oauth/tokenregistration_endpoint: ".../oauth/register"— matches the RFC 7591 routecode_challenge_methods_supported: ["S256"]— PKCE is enforced for public clientstoken_endpoint_auth_methods_supported: ["client_secret_basic", "client_secret_post", "none"]—noneis what public clients (Claude Code, claude.ai) use with PKCESafety posture
Open dynamic registration is only safe because the consent screen gates scope grants. Both land in the same merge so there is no window where dyn-reg exists without consent.
The consent screen also replaces the MVP "bearer in the Authorization header" user-auth path on
/oauth/authorizewith proper session cookies — that was unsuited to browser flows anyway.Test plan
Unit:
pnpm nx run-many -t test --projects=auth-server,server-jwt,infra-db— all greenpnpm nx run-many -t typecheck— all greenManual (post-merge on the VPS):
curl https://qauth.naqshi.net/.well-known/oauth-authorization-serverreturns a valid RFC 8414 body./.well-known/jwks.json.POST /oauth/registerwith a Claude-Code-style public-client body returns aclient_idand noclient_secret.{"type":"http","url":"..."}triggers the full on-401 discovery → dyn-reg → auth_code + PKCE → token flow. User sees the consent screen, approves, memory-mcp accepts the resulting bearer.Follow-up (not blocking this merge)