feat(oauth): user consent screen on /oauth/authorize#154
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 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).
|
View your CI Pipeline Execution ↗ for commit b553605
☁️ 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 introduces a browser-based session and consent flow for OAuth 2.1, including server-rendered login and consent pages, a revocation API, and a management UI in the developer portal. Feedback focuses on security and efficiency: a potential open-redirect bypass in the return URL validation was identified, along with brittle query string parsing in the authorization route. Additionally, the database repository for consents contains a race condition in its upsert logic, and the consent listing and revocation endpoints could be optimized to avoid N+1 queries and inefficient ownership checks.
| // Disallow absolute URLs, protocol-relative, or anything that doesn't | ||
| // start with a single `/`. This keeps us off the open-redirector list. | ||
| if (!value.startsWith('/')) return false; | ||
| if (value.startsWith('//')) return false; |
There was a problem hiding this comment.
The isSafeReturnTo check is vulnerable to an open redirect bypass in some browsers if the path starts with /\. For example, /\\example.com might be interpreted as //example.com by the browser, leading to a redirect to an external domain. It is safer to explicitly check for both // and /\.
| if (value.startsWith('//')) return false; | |
| if (value.startsWith('//') || value.startsWith('/\\')) return false; |
| client.id | ||
| ); | ||
| if (!canSkipConsent(existingConsent, client, scopes)) { | ||
| return reply.redirect(`/ui/consent${request.url.slice(request.url.indexOf('?'))}`, 302); |
There was a problem hiding this comment.
Extracting the query string using request.url.slice(request.url.indexOf('?')) is brittle. If the URL does not contain a ?, indexOf returns -1, and slice(-1) will return the last character of the path (e.g., /oauth/authorize becomes e), resulting in an invalid redirect to /ui/consente. While validation usually ensures parameters are present, the code should handle the absence of a query string gracefully.
const queryIndex = request.url.indexOf('?');
const queryString = queryIndex !== -1 ? request.url.slice(queryIndex) : '';
return reply.redirect('/ui/consent' + queryString, 302);| const [existing] = await invoker | ||
| .select() | ||
| .from(oauthConsents) | ||
| .where( | ||
| and( | ||
| eq(oauthConsents.userId, userId), | ||
| eq(oauthConsents.oauthClientId, oauthClientId), | ||
| isNull(oauthConsents.revokedAt) | ||
| ) | ||
| ) | ||
| .limit(1); |
There was a problem hiding this comment.
This implementation of upsertGrant performs a read followed by a conditional update or insert, which is susceptible to race conditions. If two concurrent requests attempt to grant consent for the same user and client, one may fail with a unique constraint violation or result in inconsistent state. Per the general rules, this should be handled using an atomic operation (e.g., PostgreSQL's ON CONFLICT) or wrapped in a database transaction with appropriate locking (e.g., FOR UPDATE).
References
- To prevent race conditions in read-then-update scenarios, 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.
| 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.
This loop performs an N+1 query by calling findById for each consent row. While the number of consents per user is likely small, it is better to optimize this by joining the oauth_clients table in the repository or using a single query to fetch all relevant clients in bulk.
| const rows = await fastify.repositories.oauthConsents.listActiveForUser(session.userId); | ||
| const owned = rows.find((r) => r.id === id); | ||
| if (!owned) { | ||
| throw new NotFoundError('OAuthConsent', id); | ||
| } |
There was a problem hiding this comment.
Fetching all active consents for a user to check ownership of a single ID is inefficient, especially as the number of grants grows. It is better to perform the ownership check directly in the revoke query by including userId in the WHERE clause (e.g., WHERE id = :id AND user_id = :userId).
1 participant
Closes #150.
Summary
/oauth/authorize(__Host-qauth_session, HttpOnly, SameSite=Lax, Secure, HMAC-SHA256) alongside the existing Bearer path.error=access_deniedand preservedstate(RFC 6749 §4.1.2.1).oauth_consents(user_id, oauth_client_id, realm_id, scopes[], granted_at, revoked_at)with partial unique index on active rows. "Allow forever" upserts and unions scopes; subsequent authorize calls with a subset skip the screen.oauth_clients.dynamic_registered_at(nullable bigint) powering the dynamic-client badge + forced re-consent window (DYNAMIC_CLIENT_BADGE_DAYS)./consentslist/delete with ownership checks; minimal developer-portal/consentspage.Caveats for reviewers
0002. Suggest landing order#151 (0002 tokens) → this (0003) → DCR (0004); renumber this migration + snapshot + journal entry on rebase.helpers/session-cookie.ts+routes/ui/consent.ts): double-submit token rotated per GET, stored in Redis session payload, timing-safe compare, burned on POST. Failure →BadRequestError('invalid_csrf_token').__Host-prefix enforces Secure + root path; env-gated for local dev.return_toinroutes/ui/login.ts(relative-path-onlyisSafeReturnTo).oauth_clients.dynamic_registered_atis the column DCR (feat(oauth): Dynamic Client Registration (RFC 7591), open mode #149) will populate; currently always null./oauth/authorizeretained for back-compat; should be removed once first-party callers migrate (comment in code).Scope cuts (follow-ups)
/ui/logoutendpoint yet — trivial follow-up viaclearSessionCookie./ui/consentHTML is utility-grade inline CSS; no@qauth-labs/uiyet./consentspage is bare-bones (no auth redirect, no pagination)..inject()test — route handlers covered directly.Test plan
pnpm nx test auth-server— 123 tests passing (13 new: session cookies, HTML escaping, consent logic, CSRF, allow/deny, authorize integration, consents JSON API)pnpm nx lintacross affected projects;pnpm nx build developer-portalerror=access_deniedwith preservedstate