feat(oauth): implement RFC 8707 Resource Indicators#160
Conversation
|
View your CI Pipeline Execution ↗ for commit c675b66
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Code Review
This pull request implements RFC 8707 Resource Indicators, allowing clients to specify intended audiences for access tokens. Key changes include updating authorization and token routes to persist and validate resource parameters, advertising support in discovery metadata, and performing database migrations. The review feedback identifies a security vulnerability in the client credentials grant regarding resource validation, suggests including user IDs in audit logs for better traceability, and notes that error responses must use the invalid_target code to comply with the specification.
|
|
||
| // RFC 8707: machine clients MAY include `resource` to scope the minted | ||
| // token to a specific resource server (overrides client.audience). | ||
| const requestedResource = body.resource ?? []; |
There was a problem hiding this comment.
For the client_credentials grant, the requested resource must be validated against the client's pre-configured audience allowlist (client.audience). Currently, any requested resource is accepted and used as the token's audience, which could allow a machine client to escalate its access to unauthorized resource servers. If client.audience is configured, the request must be a subset of it; otherwise, it should default to the client's own ID.
const requestedResource = body.resource ?? [];
if (requestedResource.length > 0) {
const allowedAudience = client.audience ?? [client.clientId];
const extra = requestedResource.filter((r) => !allowedAudience.includes(r));
if (extra.length > 0) {
await fastify.repositories.auditLogs.create({
userId: null,
oauthClientId: client.id,
event: 'oauth.token.exchange.failure',
eventType: 'token',
success: false,
ipAddress: request.ip,
userAgent: request.headers['user-agent'] || null,
metadata: {
error: 'invalid_target: resource outside the client audience allowlist',
extra,
},
});
throw new BadRequestError('invalid_target');
}
}| await fastify.repositories.auditLogs.create({ | ||
| userId: null, | ||
| oauthClientId: client.id, | ||
| event: 'oauth.token.exchange.failure', | ||
| eventType: 'token', | ||
| success: false, | ||
| ipAddress: request.ip, | ||
| userAgent: request.headers['user-agent'] || null, | ||
| metadata: { | ||
| error: 'invalid_target: resource outside the authorization-code binding', | ||
| extra, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The audit log for a resource indicator validation failure should include the userId associated with the authorization code to provide a complete audit trail. We already have access to authCode.userId at this point in the handler.
| await fastify.repositories.auditLogs.create({ | |
| userId: null, | |
| oauthClientId: client.id, | |
| event: 'oauth.token.exchange.failure', | |
| eventType: 'token', | |
| success: false, | |
| ipAddress: request.ip, | |
| userAgent: request.headers['user-agent'] || null, | |
| metadata: { | |
| error: 'invalid_target: resource outside the authorization-code binding', | |
| extra, | |
| }, | |
| }); | |
| await fastify.repositories.auditLogs.create({ | |
| userId: authCode.userId, | |
| oauthClientId: client.id, | |
| event: 'oauth.token.exchange.failure', | |
| eventType: 'token', | |
| success: false, | |
| ipAddress: request.ip, | |
| userAgent: request.headers['user-agent'] || null, | |
| metadata: { | |
| error: 'invalid_target: resource outside the authorization-code binding', | |
| extra, | |
| }, | |
| }); |
| extra, | ||
| }, | ||
| }); | ||
| throw new InvalidGrantError('invalid_target'); |
There was a problem hiding this comment.
RFC 8707 §2.2 requires the error code invalid_target when a requested resource is invalid or unauthorized. Using InvalidGrantError will likely result in an invalid_grant error code being returned to the client, which violates the specification. Please use BadRequestError('invalid_target') or a specific error class that maps to the correct OAuth error code.
| throw new InvalidGrantError('invalid_target'); | |
| throw new BadRequestError('invalid_target'); |
| extra, | ||
| }, | ||
| }); | ||
| throw new InvalidGrantError('invalid_target'); |
There was a problem hiding this comment.
Similar to the authorization code exchange, a refresh token request that fails resource indicator validation must return the invalid_target error code per RFC 8707 §2.2. InvalidGrantError typically maps to invalid_grant.
| throw new InvalidGrantError('invalid_target'); | |
| throw new BadRequestError('invalid_target'); |
Four issues flagged on the initial RFC 8707 PR:
1. [SECURITY] `client_credentials` grant accepted any `resource` with no
validation — a compromised machine credential could mint tokens for
arbitrary resource URIs, including MCPs it was never configured to
reach. Now validates requested resource is a subset of the client's
pre-configured `audience` allowlist (falls back to `[client.clientId]`
when the client has no explicit audience).
2. Audit log for authorization_code resource-binding failures now
records `userId: authCode.userId` instead of `null` — we already have
the code row at that point.
3. RFC 8707 §2.2 mandates the OAuth error code `invalid_target` for
resource-binding failures. `InvalidGrantError` maps to `invalid_grant`
— not compliant. New `InvalidTargetError` class (same shape as
`InvalidGrantError`) produces `{error: "invalid_target",
error_description: "..."}` per RFC 6749 §5.2 wire format. Registered
in the shared `error-handler` plugin alongside the other OAuth errors
that carry `error_description`. Used for all three grant paths that
do resource validation (authorization_code, client_credentials,
refresh_token).
4. Refresh-token resource-narrowing now also throws `InvalidTargetError`
(was `InvalidGrantError`).
Tests:
- New `client_credentials` test: requested resource inside allowlist →
`aud` = requested resource.
- New `client_credentials` test: requested resource outside allowlist
→ `InvalidTargetError`, no token issued.
- Existing authorization_code + refresh_token narrowing tests updated
to expect `InvalidTargetError`.
171/171 auth-server tests pass.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds `resource` parameter support to /oauth/authorize and /oauth/token (all three grants), with persistence through the authorization code + refresh token and into the access token's `aud` claim. Without this, dynamically-registered public clients (`audience=null`) issued tokens with `aud=<client_id>` (a random UUID), which resource servers correctly rejected. RFC 8707 is the spec-compliant fix: the client names the resource, the AS binds the grant to it, and the issued access token's `aud` is the resource URI. Schemas - `resource` (string | string[] → string[]) optional on authorize query and on all three token grant bodies. URIs validated as absolute, no fragment, ≤2048 chars, ≤10 entries. DB - Migration 0003 adds `resource jsonb NOT NULL DEFAULT '[]'` to both `authorization_codes` and `refresh_tokens`. Drizzle schema + snapshot regenerated. Handlers - authorize.ts: persist `resource` onto the authorization code. - token.ts (authorization_code): read code.resource; if body.resource is present it MUST be a subset of the code's binding (invalid_target); use as `aud`; carry onto the refresh token. - token.ts (client_credentials): accept body.resource; validate it is a subset of client.audience allowlist (without this, a compromised machine credential could mint tokens for arbitrary resource servers); use as `aud` when inside allowlist. - token.ts (refresh_token): read storedToken.resource; body.resource MUST be a subset; rotated refresh token inherits the EFFECTIVE (not requested) binding so we never widen across a rotation. - resolveAudience: new optional `resource` parameter — precedence is resource > client.audience > clientId. - well-known: advertise `resource_indicators_supported: true` per RFC 8707 §3. Errors - New `InvalidTargetError` class (`invalid_target` per RFC 8707 §2.2 / RFC 6749 §5.2 wire format with optional `error_description`), registered in the error-handler plugin alongside the other OAuth errors that carry `error_description`. Backward compatibility - Clients that don't send `resource` are unaffected — empty array falls back to client.audience / clientId as before. Tests - authorize: persists `resource` onto the auth code row. - token (authorization_code): aud = resource; out-of-binding request → InvalidTargetError; also covers the public-client PKCE path (token_endpoint_auth_method=none, no client_secret). - token (client_credentials): aud = requested resource when inside allowlist; out-of-allowlist → InvalidTargetError, no token issued. - token (refresh_token): resource carries across rotation; out-of-binding request → InvalidTargetError. 171/171 auth-server tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
36aeece to
c675b66
Compare
Summary
Adds
resourceparameter support to/oauth/authorizeand/oauth/token(all three grants), with persistence through the authorization code → refresh token → access token'saudclaim.Without this, dynamically-registered public clients (
audience=null) were minting tokens withaud=<client_id>(a random UUID), which resource servers like majordomo's memory-mcp correctly rejected. RFC 8707 is the spec-compliant fix: the client names the resource, the AS binds the grant to it, and the issued access token'saudis the resource URI.Changes
resource(single URI or array, ≤10 entries, absolute URI, no fragment, ≤2048 chars) toauthorizeQuerySchema, all three token grant body schemas0003_rfc8707_resource_indicators.sql: addsresource jsonb NOT NULL DEFAULT '[]'toauthorization_codesandrefresh_tokens. Drizzle schema + snapshot regeneratedresourceonto the auth codecode.resource; request narrowing must be a subset (elseinvalid_target); use asaud; carry onto issued refresh tokenbody.resource; use asaud(overridesclient.audiencewhen present)storedToken.resource; request narrowing must be a subset; rotated refresh token inherits the effective (not requested) binding — we never widen across a rotationresourceparam precedence:resource > client.audience > clientIdresource_indicators_supported: trueper RFC 8707 §3Backward compatibility
Clients that don't send
resourceare unaffected — empty array falls back toclient.audience/clientIdas before. Pre-seeded machine clients (config/qauth/clients.json) keep working without any change.Test plan
authorize.test: persistsresourceonto the auth code rowtoken.test(authorization_code):aud= resource; narrowing outside the code's binding →InvalidGrantErrortoken.test(refresh_token): resource carries across rotation; narrowing outside stored binding →InvalidGrantErroraud=https://mcp.naqshi.net/mcp; memory-mcp accepts it once itsEXPECTED_AUDis flipped to the resource URL (separate majordomo PR)🤖 Generated with Claude Code