Align OAuth2 implementation with RFC 6749 and RFC 6750 compliance#30
Align OAuth2 implementation with RFC 6749 and RFC 6750 compliance#30edimarlnx wants to merge 2 commits into
Conversation
Addresses 10 specification compliance issues: - Fix expires_in to return lifetime in seconds instead of Date object - Remove non-standard user param from authorization code redirect - Fix token_type to use capitalized Bearer per RFC 6750 - Include scope in token response when present - Only include state in redirect when provided by client - Propagate actual error codes in token endpoint instead of hardcoded unauthorized_client - Change HTTP 415 to 400 for unsupported_response_type - Fix getRefreshToken to return spec-compliant format (client/user objects) - Fix revokeToken to operate on RefreshTokens collection - Fix verifyScope to check subset instead of exact match - Make state required in authorize params to align with allowEmptyState: false Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fix: align OAuth2 implementation with RFC 6749 / RFC 6750
|
Hi, I recently used this package in an integration with Grafana, and I found some issues. I fixed these issues by following the OAuth 2.0 specifications in my fork (edimarlnx#1). |
|
@edimarlnx thank you for the PR and sorry for the delay. I'm coming back to this and prepare a review. I will also let copilot review this one but do not care too much about its comments, some are good some are meh. I added this to the roadmap for a v7 release, see #31 |
There was a problem hiding this comment.
Pull request overview
This PR tightens the package’s OAuth2 handling to better match RFC 6749/6750 and the @node-oauth/oauth2-server model expectations. It mainly updates authorization/token response behavior and the Meteor model adapters that back refresh-token flows.
Changes:
- Adjust authorization and token endpoint behavior (
state,token_type,expires_in, error propagation, redirect payloads). - Update model methods for refresh-token lookup/revocation and scope verification.
- Add a new markdown audit document describing the targeted OAuth2 compliance issues.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| OAUTH2_SPEC_ISSUES.md | Adds a written audit of OAuth2 spec deviations and intended fixes. |
| lib/validation/requiredAuthorizeGetParams.js | Makes state required during initial authorize request validation. |
| lib/oauth.js | Changes authorize/token endpoint validation, redirect construction, and token response/error formatting. |
| lib/model/model.js | Changes scope verification from exact-match to subset-style logic. |
| lib/model/meteor-model.js | Reshapes refresh-token retrieval output and changes refresh-token revocation behavior. |
Comments suppressed due to low confidence (1)
lib/oauth.js:232
validateResponseTypestill sends a direct JSON error response even though this code now receives the validated redirect URI. Forunsupported_response_type, once the client and redirect URI have been validated, RFC 6749 requires returning the error via redirect to the client'sredirect_uri; this change only adjusts the status code, so the compatibility issue described in the PR remains.
const validateResponseType = (req, res, validatedRedirectUri) => {
const responseType = req.method.toLowerCase() === 'get'
? req.query.response_type
: req.body.response_type
if (responseType !== 'code' && responseType !== 'token') {
return errorHandler(res, {
status: 400,
error: 'unsupported_response_type',
description: 'The response type is not supported by the authorization server.',
state: req.query.state,
debug: self.debug
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scope: Match.Maybe(String), | ||
| redirect_uri: isNonEmptyString, | ||
| state: Match.Maybe(String) | ||
| state: isNonEmptyString |
| code: code.authorizationCode | ||
| }) | ||
|
|
||
| if (req.body.state) { |
| const body = { | ||
| access_token: token.accessToken, | ||
| token_type: 'Bearer', | ||
| expires_in: Math.floor((token.accessTokenExpiresAt - Date.now()) / 1000), | ||
| refresh_token: token.refreshToken | ||
| } | ||
|
|
||
| if (token.scope) { | ||
| body.scope = Array.isArray(token.scope) ? token.scope.join(' ') : token.scope |
| if (!accessToken.scope) return false | ||
| return scope.every(s => accessToken.scope.includes(s)) |
| const doc = await collections.RefreshTokens.findOneAsync({ refreshToken }) | ||
| if (!doc) return doc | ||
|
|
||
| return { | ||
| refreshToken: doc.refreshToken, | ||
| refreshTokenExpiresAt: doc.expires, | ||
| client: { id: doc.clientId }, | ||
| user: { id: doc.userId } | ||
| } |
| **Spec:** RFC 6749 Section 10.12 | ||
|
|
||
| The default configuration sets `allowEmptyState: false`, which implies that | ||
| `state` is mandatory (recommended by the spec to prevent CSRF attacks). | ||
| However, the validation schema defines `state: Match.Maybe(String)`, | ||
| allowing `undefined` values. These two settings are contradictory. |
Thank you @jankapunkt I'll take a look at the comments. I'm going to use this package in this project: https://github.com/edimarlnx/samba-conductor |
Summary
expires_into return lifetime in seconds instead ofDateobjectuserparam leaked in authorization code redirecttoken_typeto use capitalizedBearerper RFC 6750scopein token response when present (RFC 6749 §5.1)statein redirect when provided by client (RFC 6749 §4.1.2)unauthorized_clientunsupported_response_typegetRefreshTokento return spec-compliant format withclient/userobjectsrevokeTokento operate onRefreshTokenscollection instead ofAccessTokensverifyScopeto check subset instead of exact matchstaterequired in authorize params to align withallowEmptyState: falseSee
OAUTH2_SPEC_ISSUES.mdfor the full analysis of each issue with spec references.Breaking changes
expires_inis now an integer (seconds) instead of a Date — clients following the spec are unaffecteduserparameter removed from authorization code redirect URL — non-standard clients that relied on it will need to updatestateis now required on/oauth/authorizeGET — clients must include astateparameter🤖 Generated with Claude Code