Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OAuth token revocation to ChangesToken Revocation Feature
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant ServerClient
participant AuthClient
participant Auth0
participant StateStore
Application->>ServerClient: revokeRefreshToken()
ServerClient->>StateStore: read session refresh token
StateStore-->>ServerClient: refresh token and domain
ServerClient->>AuthClient: revokeToken(token, refresh_token)
AuthClient->>Auth0: discover and revoke token
Auth0-->>AuthClient: success or OAuth error
AuthClient-->>ServerClient: completion or TokenRevocationError
ServerClient-->>Application: completion or error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/auth0-server-js/src/server-client.ts (1)
1098-1106: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate session-store read on the resolver-mode logout path.
revokeRefreshToken({}, storeOptions)internally fetchesstateData(per the sibling comment above), and thenlogout()fetches it again at line 1114 for resolver-mode domain checks — two round-trips to the store per logout call in multi-tenant setups. Also, thetry {} catch {}around revocation only bounds failures, not latency: a slow/hanging revocation endpoint will still stalllogout()since it's awaited before continuing, undermining the "best-effort" intent for responsiveness (not just correctness).Consider restructuring so
logout()fetchesstateDataonce and passes the already-known token/domain into the revocation call, and optionally bound the revoke call with a timeout so a slow endpoint can't delay the redirect response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/auth0-server-js/src/server-client.ts` around lines 1098 - 1106, `logout()` is doing duplicate session-state lookups on the resolver-mode path and still waits unbounded on refresh-token revocation. Update `logout()` in `server-client.ts` so it reads `stateData` once and reuses the existing token/domain information when calling `revokeRefreshToken`, instead of letting that method fetch the same store data again; use the `logout`/`revokeRefreshToken` flow to pass the known context through. Also make the best-effort revocation non-blocking for slow endpoints by adding a timeout around the awaited revoke call so logout can continue promptly even if revocation hangs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/auth0-server-js/src/server-client.ts`:
- Around line 1098-1106: `logout()` is doing duplicate session-state lookups on
the resolver-mode path and still waits unbounded on refresh-token revocation.
Update `logout()` in `server-client.ts` so it reads `stateData` once and reuses
the existing token/domain information when calling `revokeRefreshToken`, instead
of letting that method fetch the same store data again; use the
`logout`/`revokeRefreshToken` flow to pass the known context through. Also make
the best-effort revocation non-blocking for slow endpoints by adding a timeout
around the awaited revoke call so logout can continue promptly even if
revocation hangs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: db6538bf-5405-4bf0-bc00-fa7e5c296d8c
📒 Files selected for processing (9)
.npmrcpackages/auth0-auth-js/src/auth-client.spec.tspackages/auth0-auth-js/src/auth-client.tspackages/auth0-auth-js/src/errors.tspackages/auth0-auth-js/src/types.tspackages/auth0-server-js/src/index.tspackages/auth0-server-js/src/server-client.spec.tspackages/auth0-server-js/src/server-client.tspackages/auth0-server-js/src/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/auth0-server-js/EXAMPLES.md`:
- Around line 1509-1510: The logout documentation overstates cleanup behavior
after refresh token revocation failure. Update the wording around logout() and
revokeRefreshToken handling to say logout still proceeds best-effort, but local
session deletion only happens in resolver mode when the stored session domain
matches the resolved domain; reference logout() and the session
cleanup/resolver-mode flow so the docs match the actual implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e66dbcc5-6119-4c6c-9819-351891cca851
📒 Files selected for processing (4)
packages/auth0-auth-js/src/auth-client.spec.tspackages/auth0-server-js/EXAMPLES.mdpackages/auth0-server-js/src/server-client.spec.tspackages/auth0-server-js/src/server-client.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/auth0-auth-js/src/auth-client.spec.ts
- packages/auth0-server-js/src/server-client.ts
- packages/auth0-server-js/src/server-client.spec.ts
jd3vi1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/auth0-server-js/src/server-client.ts`:
- Around line 1132-1137: Update both logout revocation catch blocks in
server-client.ts (1132-1137 and 1155-1160) to silently ignore
MissingSessionError while retaining console.warn for all other revocation
failures. Apply the same error discrimination in both the non-resolver and
resolver logout branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 00fc5924-ceb8-49c7-a5c2-9d4afa79a32e
📒 Files selected for processing (5)
examples/example-express-web/src/auth0.tspackages/auth0-server-js/EXAMPLES.mdpackages/auth0-server-js/src/server-client.spec.tspackages/auth0-server-js/src/server-client.tspackages/auth0-server-js/src/types.ts
💤 Files with no reviewable changes (2)
- packages/auth0-server-js/src/types.ts
- examples/example-express-web/src/auth0.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/auth0-server-js/EXAMPLES.md
| try { | ||
| await this.revokeRefreshToken({}, storeOptions); | ||
| } catch (e) { | ||
| // best-effort: revocation failure must not block logout | ||
| console.warn('revokeRefreshToken failed during logout (swallowed):', e); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Prevent log pollution for sessions without a refresh token.
Because logout now attempts token revocation by default, users who logged in without requesting the offline_access scope will legitimately lack a refresh token. This predictably causes revokeRefreshToken to throw a MissingSessionError, which is caught and logged as a warning, resulting in log spam on every such logout.
Silently ignore MissingSessionError to avoid warning on this expected state, while preserving the warning for actual revocation failures (like network or configuration errors).
packages/auth0-server-js/src/server-client.ts#L1132-L1137: IgnoreMissingSessionErrorin the non-resolver modelogoutbranch.packages/auth0-server-js/src/server-client.ts#L1155-L1160: IgnoreMissingSessionErrorin the resolver modelogoutbranch.
🛠️ Proposed fix (applies to both locations)
try {
await this.revokeRefreshToken({}, storeOptions);
} catch (e) {
- // best-effort: revocation failure must not block logout
- console.warn('revokeRefreshToken failed during logout (swallowed):', e);
+ if (!(e instanceof MissingSessionError)) {
+ // best-effort: revocation failure must not block logout
+ console.warn('revokeRefreshToken failed during logout (swallowed):', e);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await this.revokeRefreshToken({}, storeOptions); | |
| } catch (e) { | |
| // best-effort: revocation failure must not block logout | |
| console.warn('revokeRefreshToken failed during logout (swallowed):', e); | |
| } | |
| try { | |
| await this.revokeRefreshToken({}, storeOptions); | |
| } catch (e) { | |
| if (!(e instanceof MissingSessionError)) { | |
| // best-effort: revocation failure must not block logout | |
| console.warn('revokeRefreshToken failed during logout (swallowed):', e); | |
| } | |
| } |
📍 Affects 1 file
packages/auth0-server-js/src/server-client.ts#L1132-L1137(this comment)packages/auth0-server-js/src/server-client.ts#L1155-L1160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/auth0-server-js/src/server-client.ts` around lines 1132 - 1137,
Update both logout revocation catch blocks in server-client.ts (1132-1137 and
1155-1160) to silently ignore MissingSessionError while retaining console.warn
for all other revocation failures. Apply the same error discrimination in both
the non-resolver and resolver logout branches.
3 participants
Summary
Adds token revocation support to
auth0-server-jsso a logout or explicit revoke actually invalidates the refresh token at Auth0, not just clears the local session.ServerClient.revokeRefreshToken(options?, storeOptions?)-- new public method that callsPOST /oauth/revoke. Reads the refresh token from the session store by default; accepts an explicit token viaoptions.tokento bypass the session lookup.logout({ revokeRefreshToken: true })-- convenience option that revokes before clearing the session. Best-effort: revocation failure never blocks logout.domainis used for the revocation request (preferred over calling the domain resolver), matching the behavior of the rest of the SDK.Changes
packages/auth0-auth-js/src/auth-client.ts-- addedrevokeToken()method callingPOST /oauth/revokepackages/auth0-auth-js/src/types.ts-- addedRevokeTokenOptions; fixed JSDoc citation to RFC 7009 onlypackages/auth0-server-js/src/server-client.ts-- addedrevokeRefreshToken()andrevokeRefreshToken: trueoption onlogout()packages/auth0-server-js/src/types.ts-- addedRevokeRefreshTokenOptionsandLogoutOptions.revokeRefreshTokenpackages/auth0-server-js/src/errors.ts-- addedTokenRevocationErrorpackages/auth0-server-js/EXAMPLES.md-- added "Revoking a Refresh Token" section covering all three usage patternsexamples/example-express-web-- wiredPOST /auth/revokeandGET /auth/logout-and-revokeroutes into the existing Express exampleTest plan
packages/auth0-auth-js: 360 tests pass (npm test)packages/auth0-server-js: 370 tests pass (npm test)revokeTokensuccess, error, client credentials;revokeRefreshTokenfrom session, explicit token, missing token, resolver mode;logout({ revokeRefreshToken: true })success, no-op, failure swallowed, resolver mode via logoutSummary by CodeRabbit
New Features
Bug Fixes
Documentation