[DO NOT MERGE] bugfix: DPoP nonce retry on auth code callback #2390
Conversation
|
@claude please review this pr |
|
Claude finished @tusharpandey13's task —— View job PR Review CompleteI've completed the review of PR #2390. Here's my assessment: Overall Assessment❌ This PR is not ready to be merged. While the core fix is correct and RFC 9449 compliant, there are several issues that need to be addressed before merging. Critical Issues
Code Quality Notes
Recommendations
Once these issues are resolved, the core fix itself is solid and should be merged. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2390 +/- ##
==========================================
+ Coverage 87.65% 87.86% +0.20%
==========================================
Files 37 37
Lines 4084 4128 +44
Branches 809 822 +13
==========================================
+ Hits 3580 3627 +47
+ Misses 501 498 -3
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
tusharpandey13
changed the title
tusharpandey13
force-pushed
the
bugfix/dpop-retry-authcode
branch
2 times, most recently
from
7fd3cbd to
751e55d
Compare
|
@claude review this pr |
|
Claude finished @tusharpandey13's task —— View job Overall Assessment❌ This PR has a critical bug that breaks DPoP nonce retry for refresh token and connection token flows. The refactored |
src/utils/dpopUtils.ts
Outdated
| @@ -110,11 +137,12 @@ | |||
| // Delay before retry to avoid rapid successive requests | |||
| await new Promise((resolve) => setTimeout(resolve, actualDelay)); | |||
|
|
|||
| // The RS-signalled nonce is now cached, retrying | |||
| // Retry the request - the DPoP handle automatically learned the nonce | |||
| return await fn(); | |||
| } | |||
| throw error; | |||
| } | |||
|
|
|||
| return response; | |||
| } | |||
There was a problem hiding this comment.
The refactored withDPoPNonceRetry function has a critical flaw: it doesn't properly handle the case where fn() returns a processed token response object (not a raw Response).
Problem: When fn() wraps both the HTTP request AND response processing (like in refresh token flow at line 1241-1243), the returned value is a TokenEndpointResponse object, not a Response. The check response instanceof Response will be false, causing the function to skip retry logic entirely.
Example failure scenario:
// Refresh token flow (lines 1241-1243)
oauthRes = await withDPoPNonceRetry(
async () => {
const refreshTokenRes = await refreshTokenGrantRequestCall();
return await processRefreshTokenResponseCall(refreshTokenRes); // Returns TokenEndpointResponse
},
{ isDPoPEnabled: true }
);Here, processRefreshTokenResponseCall returns a TokenEndpointResponse object, not a Response. The retry logic will never trigger even if the server returns a DPoP nonce error.
Root cause: The previous implementation caught exceptions using isDPoPNonceError(), which worked for both Response and processed objects because oauth4webapi throws on errors. The new implementation only checks raw Response status codes.
Fix needed: Either:
- Revert to try-catch with
isDPoPNonceError()to handle both cases, OR - Ensure
fn()always returns raw Response objects and processing happens after retry (requires refactoring all call sites)
There was a problem hiding this comment.
added a try catch , removed extends Response | object
src/server/auth-client.ts
Outdated
|
|
||
| private dpopKeyPair?: DpopKeyPair; | ||
| private readonly useDPoP: boolean; | ||
| private defaultDPoPHandle?: ReturnType<typeof oauth.DPoP>; |
There was a problem hiding this comment.
Creating a single shared DPoP handle that's reused across multiple token requests is architecturally problematic for nonce management.
Issue: oauth4webapi's DPoP handle maintains internal state (learned nonces). When multiple concurrent token requests use the same handle:
- Request A fails with nonce error, handle learns nonce "ABC"
- Request B (concurrent) uses nonce "ABC" but server may have rotated to nonce "XYZ"
- Request B fails unnecessarily
RFC 9449 Section 8: "A new nonce value MUST be provisioned for each use" - the server can provide different nonces for different resources/endpoints.
Impact: While this approach works for sequential requests, it may cause issues with:
- Concurrent token refreshes (multiple tabs/windows)
- Parallel API calls with token refresh
- Server-side nonce rotation policies
Recommendation: Consider creating a fresh DPoP handle per token request (as the original code did), or implement per-endpoint nonce caching if performance is a concern.
There was a problem hiding this comment.
removed the global nonce id, reverted to original code
| } | ||
|
|
||
| // Second request: Return 200 with tokens | ||
| const idToken = await new jose.SignJWT({ | ||
| sid: DEFAULT.sid, | ||
| sub: DEFAULT.sub, | ||
| nonce: DEFAULT.nonce, | ||
| auth_time: Math.floor(Date.now() / 1000), | ||
| iss: `https://${DEFAULT.domain}/`, | ||
| aud: DEFAULT.clientId | ||
| }) | ||
| .setProtectedHeader({ alg: DEFAULT.alg }) | ||
| .setIssuedAt() | ||
| .setExpirationTime("1h") | ||
| .sign(keyPairParam.privateKey); | ||
|
|
||
| return HttpResponse.json({ | ||
| access_token: DEFAULT.accessToken, | ||
| refresh_token: DEFAULT.refreshToken, | ||
| id_token: idToken, | ||
| token_type: "Bearer", | ||
| expires_in: 86400 | ||
| } as oauth.TokenEndpointResponse); |
There was a problem hiding this comment.
The test validates HTTP-level retry behavior but doesn't verify the actual integration with handleCallback(). While the test confirms MSW mocking works, it doesn't prove the DPoP handle actually learns and applies the nonce.
Gap: The test creates request tracking state but never validates that:
- The DPoP JWT payload actually contains the nonce claim on the second request
- The nonce value matches what the server provided ("server_nonce_value_123")
Current validation at lines 286-307:
expect(tokenHandlerState.requests[1]).toMatchObject({
attempt: 2,
hasDPoP: true,
hasNonce: true // Only checks nonce EXISTS, not that it's correct
});Missing validation: The test should decode the DPoP JWT from the second request and verify:
const dpopJwt = tokenHandlerState.requests[1].dpopHeader;
const payload = jose.decodeJwt(dpopJwt);
expect(payload.nonce).toBe("server_nonce_value_123");This would prove the oauth4webapi DPoP handle correctly learned the nonce from the error response and injected it on retry.
| codeGrantResponse = await withDPoPNonceRetry( | ||
| authorizationCodeGrantRequestCall, | ||
| { | ||
| isDPoPEnabled: !!(this.useDPoP && this.dpopKeyPair), | ||
| ...this.dpopOptions?.retry | ||
| } | ||
| ); |
There was a problem hiding this comment.
The auth code callback flow passes a wrapper function that includes both HTTP request AND response processing to withDPoPNonceRetry(). This creates an inconsistency with how other token flows are structured.
Inconsistency:
- Auth code (lines 786-792): Passes only the HTTP request function, response processing happens after (line 807)
- Refresh token (lines 1240-1249): Passes BOTH HTTP request AND response processing in the wrapper
- Connection token (lines 1787-1793): Passes BOTH HTTP request AND response processing in the wrapper
Why this matters: The refactored withDPoPNonceRetry() at dpopUtils.ts:110-146 only handles Response objects (checks response instanceof Response). If the wrapper includes response processing, it returns a processed object and retry logic won't work.
Current state: Auth code flow works correctly because it only wraps the HTTP request. But this inconsistency with other flows is confusing and may lead to bugs if someone refactors it to match the pattern of refresh/connection flows.
Recommendation: Document why auth code flow must wrap only the HTTP request (not response processing), or refactor all flows to use the same pattern.
src/utils/dpopUtils.ts
Outdated
|
|
||
| // Check if this is a 400 error response with use_dpop_nonce | ||
| if (response instanceof Response && response.status === 400) { | ||
| const errorBody = await response.clone().json(); |
There was a problem hiding this comment.
Calling .json() on the response body without error handling could cause unhandled promise rejections if the response body is not valid JSON.
Scenario: A misconfigured server or proxy might return a 400 status with HTML error page instead of JSON. The .json() call would throw, but this error is not caught, causing the entire token operation to fail with a cryptic JSON parse error instead of the original HTTP error.
Impact: Poor error messages for users when servers misbehave.
| const errorBody = await response.clone().json(); | |
| // Check if this is a 400 error response with use_dpop_nonce | |
| if (response instanceof Response && response.status === 400) { | |
| try { | |
| const errorBody = await response.clone().json(); | |
| if (errorBody.error === "use_dpop_nonce") { | |
| // This is a DPoP nonce error, retry with delay and jitter | |
| const delay = config?.delay ?? 100; | |
| const jitter = config?.jitter ?? true; | |
| let actualDelay = delay; | |
| // Apply jitter if enabled (50-100% of original delay to prevent thundering herd) | |
| if (jitter) { | |
| actualDelay = delay * (0.5 + Math.random() * 0.5); | |
| } | |
| // Delay before retry to avoid rapid successive requests | |
| await new Promise((resolve) => setTimeout(resolve, actualDelay)); | |
| // Retry the request - the DPoP handle automatically learned the nonce | |
| return await fn(); | |
| } | |
| } catch { | |
| // If JSON parsing fails, it's not a DPoP nonce error - return original response | |
| } | |
| } |
There was a problem hiding this comment.
used your suggestion
src/server/auth-client.ts
Outdated
| // Initialize DPoP if enabled. Check useDPoP flag first to avoid timing attacks. | ||
| if ((options.useDPoP ?? false) && options.dpopKeyPair) { | ||
| this.dpopKeyPair = options.dpopKeyPair; | ||
| // Create DPoP handle once for reuse across all token grant operations | ||
| // oauth4webapi automatically learns nonces from error responses | ||
| this.defaultDPoPHandle = oauth.DPoP( | ||
| this.clientMetadata, |
There was a problem hiding this comment.
The DPoP handle is only created when both useDPoP flag AND dpopKeyPair are present, but later code only checks this.defaultDPoPHandle existence. This creates a subtle logic gap.
Issue: If someone mistakenly sets useDPoP: true without providing dpopKeyPair, or provides dpopKeyPair without setting useDPoP: true, the behavior is inconsistent:
- Constructor: Handle not created (correct)
- Token operations: Check
this.defaultDPoPHandle && { DPoP: this.defaultDPoPHandle }(works correctly, no DPoP used) withDPoPNonceRetry: ChecksisDPoPEnabled: !!(this.useDPoP && this.dpopKeyPair)(redundant check)
Redundancy: The spread operator ...(this.defaultDPoPHandle && { DPoP: this.defaultDPoPHandle }) already handles the case where handle doesn't exist. The isDPoPEnabled flag passed to withDPoPNonceRetry is redundant - you could simplify to checking if the response is from a DPoP-enabled request by inspecting whether the handle was actually used.
Simplification opportunity:
isDPoPEnabled: !!this.defaultDPoPHandleThis is clearer because it directly expresses "DPoP is enabled if we have a handle", rather than re-checking the original configuration flags.
There was a problem hiding this comment.
removed the default dpop handle
…ing finalizeSession
tusharpandey13
force-pushed
the
bugfix/dpop-retry-authcode
branch
from
751e55d to
3009c49
Compare
|
@claude review this pr |
|
Claude finished @tusharpandey13's task —— View job Overall Assessment✅ This PR successfully addresses the critical bugs from the previous review and implements DPoP nonce retry for auth code callbacks correctly. The core fix is sound and RFC 9449 compliant. The author has made significant improvements: removed the problematic shared DPoP handle, added proper retry configuration, enhanced test coverage to validate actual nonce values, and documented the intentional architectural differences between token flows. While there are minor code quality improvements that could be made (formatting, simplifying redundant checks, using jose for JWT parsing in tests), these are not blocking issues. The PR is ready to merge. |
| throw error; | ||
| else{ | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Formatting: Missing space before else keyword. Should be } else { instead of }else{.
| if (jitter) { | ||
| actualDelay = delay * (0.5 + Math.random() * 0.5); | ||
| // Check if this is a 400 error response with use_dpop_nonce | ||
| if (response instanceof Response && response.status === 400) { |
There was a problem hiding this comment.
The dual retry mechanism (checking both Response objects with status 400 AND catching isDPoPNonceError exceptions) creates inconsistent behavior across token flows.
Issue: The refresh token and connection token flows wrap both HTTP request AND response processing (lines 1260-1262 and 1805-1811 in auth-client.ts). When oauth.processRefreshTokenResponse() or oauth.processGenericTokenEndpointResponse() encounters a DPoP nonce error, oauth4webapi throws an exception rather than returning a 400 Response. The exception path (lines 150-159) handles this correctly with isDPoPNonceError().
However, the auth code callback flow only wraps the HTTP request (lines 796-802), so it returns a raw Response object. The Response path (lines 135-146) handles this by checking response.status === 400.
Why this is fragile: The retry logic now depends on how each flow is structured:
- Auth code: Relies on Response inspection (lines 135-146)
- Refresh/connection: Relies on exception catching (lines 150-159)
If someone refactors the auth code flow to match refresh/connection flows by including response processing in the wrapper, the Response path would break because processAuthorizationCodeResponse() throws on errors, not returns 400 Response.
Recommendation: Document this dual-path behavior clearly, or refactor all flows to use consistent wrapping (either all wrap only HTTP requests, or all wrap both HTTP + processing).
| return { hasNonce: false }; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
The nonce extraction logic parses the JWT manually using base64url decoding. While this works for testing purposes, it's fragile because it doesn't validate JWT structure or handle malformed tokens.
Potential issue: If the DPoP header is malformed or contains invalid base64url data, the try-catch will silently return { hasNonce: false } without indicating what went wrong.
Suggestion: Since jose is already imported, use jose.decodeJwt() which provides proper JWT parsing with error handling:
const extractDPoPNonce = (dpopHeader: string | null): { hasNonce: boolean; nonce?: string } => {
if (!dpopHeader || typeof dpopHeader !== "string") {
return { hasNonce: false };
}
try {
const payload = jose.decodeJwt(dpopHeader);
if ("nonce" in payload && typeof payload.nonce === "string") {
return { hasNonce: true, nonce: payload.nonce };
}
} catch {
// If JWT parsing fails, assume no nonce
}
return { hasNonce: false };
};This is more robust and leverages the existing library.
src/server/auth-client.ts
Outdated
| codeGrantResponse = await withDPoPNonceRetry( | ||
| authorizationCodeGrantRequestCall, | ||
| { | ||
| isDPoPEnabled: !!(this.useDPoP && this.dpopKeyPair), |
There was a problem hiding this comment.
The isDPoPEnabled flag is redundantly checking the same conditions used to create dpopHandle just a few lines above (lines 767-770). This creates maintenance burden if the DPoP enablement logic changes.
Simplification: Since dpopHandle is only defined when both useDPoP and dpopKeyPair are truthy, you can simplify to:
codeGrantResponse = await withDPoPNonceRetry(
authorizationCodeGrantRequestCall,
{
isDPoPEnabled: !!dpopHandle,
...this.dpopOptions?.retry
}
);This is more maintainable because it directly expresses "DPoP is enabled if we created a handle" rather than re-checking the original configuration conditions. Apply the same pattern to lines 1265 and 1819.
|
@claude review this pr |
|
Claude finished @tusharpandey13's task —— View job Overall Assessment✅ This PR successfully implements DPoP nonce retry for auth code callbacks and is ready to merge. The implementation correctly follows RFC 9449, addresses all critical issues from previous reviews, and includes comprehensive test coverage. The only suggestions are minor code style improvements that don't block merging. |
| codeGrantResponse = await withDPoPNonceRetry( | ||
| authorizationCodeGrantRequestCall, | ||
| { | ||
| isDPoPEnabled: !!dpopHandle, |
There was a problem hiding this comment.
The isDPoPEnabled flag redundantly checks the same condition used to create dpopHandle just above (lines 767-770). Since dpopHandle is only defined when both conditions are met, simplify to !!dpopHandle for better maintainability.
| isDPoPEnabled: !!dpopHandle, | |
| { | |
| isDPoPEnabled: !!dpopHandle, | |
| ...this.dpopOptions?.retry | |
| } |
tusharpandey13
force-pushed
the
bugfix/dpop-retry-authcode
branch
from
b34cfee to
6488a4a
Compare
tusharpandey13
changed the title
The auth code callback flow was missing the DPoP nonce retry pattern that already exists for:
refreshTokenGrantRequest)genericTokenEndpointRequest)When a server requires a DPoP nonce, it responds with HTTP 400 +
use_dpop_nonceerror +DPoP-Nonceheader. The client must retry with the provided nonce. Without this retry logic, the error propagates to the user and login fails.endpoint flows.
Changes
Wrapped the
authorizationCodeGrantRequestCallwithwithDPoPNonceRetry()when DPoP is enabled, matching the existing patterns for other token.src/server/auth-client.ts(lines 784-789): Added conditional DPoP nonce retry wrapper inhandleCallback()methodsrc/server/dpop-authcode-nonce-retry.test.ts: Added MSW-based HTTP flow tests validating:References