feat(oauth): Dynamic Client Registration (RFC 7591), open mode#155
feat(oauth): Dynamic Client Registration (RFC 7591), open mode#155
Conversation
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.
…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.
|
View your CI Pipeline Execution ↗ for commit 641dc7b
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Code Review
This pull request implements OAuth 2.0 Dynamic Client Registration (RFC 7591), introducing a rate-limited /register endpoint and realm-based scope allowlists. Feedback identifies several security and compliance improvements, including blocking dangerous URI schemes (e.g., javascript:), supporting the full 127.0.0.0/8 loopback range, and resolving a race condition in realm scope seeding. Additionally, it is recommended to avoid expensive hashing for public client sentinels to prevent DoS and to relax the contacts schema validation to better align with the RFC.
|
|
||
| // Non-http custom scheme (e.g. `com.example.app:/cb`) — allowed for native | ||
| // clients per RFC 8252. We don't attempt structural validation here. | ||
| if (/^[a-z][a-z0-9+\-.]*:$/i.test(parsed.protocol)) { |
There was a problem hiding this comment.
The current scheme validation is too permissive and allows dangerous protocols like javascript:, data:, or vbscript:. Allowing these as redirect URIs can lead to Cross-Site Scripting (XSS) vulnerabilities when the authorization server redirects the user back to the client. You should explicitly block these dangerous schemes.
const scheme = parsed.protocol.toLowerCase();
if (['javascript:', 'data:', 'vbscript:'].includes(scheme)) {
rejectRegistration('invalid_redirect_uri', 'dangerous scheme: ' + scheme);
}
if (/^[a-z][a-z0-9+\-.]*:$/i.test(scheme)) {
return;
}| clientSecretHash = await fastify.passwordHasher.hashPassword( | ||
| randomBytes(32).toString('hex') | ||
| ); |
There was a problem hiding this comment.
Computing an Argon2id hash for every public client registration is a significant performance bottleneck and a potential Denial of Service (DoS) vector. Since public clients do not use a secret for authentication, and the token_endpoint_auth_method is already public in the registration response, the 'length-oracle' protection is unnecessary here. Consider using a fixed sentinel string or making the database column nullable to avoid this expensive operation.
clientSecretHash = '$argon2id$v=19$m=65536,t=3,p=4$public-client-sentinel';| * recommends `127.0.0.1` / `[::1]`), but we accept it because many desktop | ||
| * tools still use it — we just require the URL parse as loopback. | ||
| */ | ||
| const LOOPBACK_HOSTS = new Set(['127.0.0.1', '[::1]', '::1', 'localhost']); |
There was a problem hiding this comment.
The loopback address check is too restrictive. RFC 8252 §7.3 requires that authorization servers allow any IP address in the loopback range, which is 127.0.0.0/8 for IPv4. The current implementation only allows 127.0.0.1.
const LOOPBACK_HOSTS = new Set(['[::1]', '::1', 'localhost']);
export function isLoopback(host: string): boolean {
return LOOPBACK_HOSTS.has(host) || /^127\.\d+\.\d+\.\d+$/.test(host);
}| await fastify.repositories.realms.update(realm.id, { | ||
| dynamicRegistrationAllowedScopes: allowedScopes, | ||
| }); |
There was a problem hiding this comment.
This read-then-update operation is subject to a race condition. If multiple registration requests for the same realm occur simultaneously when the allowlist is empty, they may all attempt to seed the defaults. Per the general rules, you should use an atomic update with a built-in check in the WHERE clause or wrap the operation in 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.
| logo_uri: z.url().max(2048).optional(), | ||
| tos_uri: z.url().max(2048).optional(), | ||
| policy_uri: z.url().max(2048).optional(), | ||
| contacts: z.array(z.email().max(255)).max(10).optional(), |
There was a problem hiding this comment.
The contacts field in RFC 7591 §2 is defined as an array of strings that are 'typically' email addresses, but not strictly required to be. Using z.email() may reject valid registration requests from clients that provide other forms of contact information (e.g., phone numbers or URIs).
| contacts: z.array(z.email().max(255)).max(10).optional(), | |
| contacts: z.array(z.string().max(255)).max(10).optional(), |
Closes #149.
Summary
POST /oauth/registerper RFC 7591 in open mode (noinitial_access_token). Validator/normalizer helpers inapps/auth-server/src/app/helpers/dynamic-client-registration.ts, request/response Zod schemas inapps/auth-server/src/app/schemas/oauth.ts.dynamic_registration_allowed_scopeslist (new jsonb column). Empty realms seeded on first/oauth/registerwith a tight default (openid profile email offline_access) — never overwrites an operator list. No admin/tenant scopes in defaults.token_endpoint_auth_methoddefaults tonone(OAuth 2.1 / MCP posture), not RFC 7591'sclient_secret_basic.require_pkce: trueforced on every dyn-reg client.client_secret;oauth_clients.client_secret_hashis stored as a sentinel argon2id hash of random bytes (column is NOT NULL — see follow-up below)./oauth/token. Knob:REGISTER_CLIENT_RATE_LIMIT.Caveats for reviewers
0002. Suggest landing order#151 (0002 tokens) → #150 (0003 consents) → this (0004); renumber this migration + snapshot + journal entry on rebase.oauth_clients.dynamic_registered_atexists and the DCR handler should writenow()on INSERT so the consent screen's dynamic-client badge is accurate. Not wired in this PR (column doesn't exist on this branch); add on rebase.client_secret_hashnullable and represent public clients asNULL./.well-known/*is handled by feat(oauth): authorization server metadata, JWKS, and OIDC discovery endpoints #148; this PR does not touch well-known.registration_access_token,registration_client_uri) intentionally out of scope for MVP.Safety story
Open DCR is only safe because #150 (consent screen) gates scope issuance. Prefer landing this last in the sequence so the defense-in-depth is in place before a dynamically-registered client can reach
/oauth/authorizeunchaperoned.Test plan
pnpm nx affected -t test— auth-server 103 passing (21 new across 2 files)pnpm nx affected -t lint typecheck— cleanpnpm nx build auth-server/drizzle-kit checkPOST /oauth/registerhappy path — public client, PKCE flow end-to-endmemory:admin) — expect rejection