-
Notifications
You must be signed in to change notification settings - Fork 10.5k
fix(SignalR): retry access token refresh on 401 in TS client #63740
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ient - Handle both HttpError.status and HttpResponse.statusCode - Normalize string "401" to number before comparison - Retry exactly once using _refreshTokenAndRetry - Skip retry when token factory returns empty - Preserve existing behavior for non-401 errors - Add unit tests for 401 response, HttpError, string "401", no-retry cases Fixes dotnet#56494
Thanks for your PR, @@daniloneto. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in the SignalR TypeScript client where access token refresh wasn't being retried properly on 401 errors. The fix handles both HTTP response objects and thrown errors, normalizes string status codes to numbers, and ensures exactly one retry attempt when the access token factory is available.
- Adds robust 401 error detection for both HttpResponse and HttpError cases
- Normalizes string "401" status codes to number format for consistent comparison
- Implements exactly one retry attempt with proper token refresh logic
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
src/SignalR/clients/ts/signalr/src/AccessTokenHttpClient.ts | Core implementation with helper functions for status normalization and retry logic |
src/SignalR/clients/ts/signalr/tests/AccessTokenHttpClient.test.ts | Comprehensive test coverage for all retry scenarios including edge cases |
src/SignalR/clients/ts/signalr/tests/AccessTokenHttpClient.test.ts
Outdated
Show resolved
Hide resolved
src/SignalR/clients/ts/signalr/tests/AccessTokenHttpClient.test.ts
Outdated
Show resolved
Hide resolved
@dotnet-policy-service agree |
Co-authored-by: Copilot <[email protected]>
…t.ts Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…t.ts Co-authored-by: Copilot <[email protected]>
…esends; shrink bundle fix(SignalR): ensure access token factory is always called and improve error handling in tests
35554db
to
75ca574
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
if (!t) { | ||
this._accessToken = undefined; | ||
if (request.headers) delete (request.headers as any).Authorization; | ||
if (request.abortSignal) return this._innerClient.send(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition request.abortSignal
checks for the existence of the signal, not if it's been aborted. This should likely check request.abortSignal?.aborted
to determine if the request was cancelled, or this logic may be incorrect for the intended behavior.
if (request.abortSignal) return this._innerClient.send(request); | |
if (request.abortSignal?.aborted) return this._innerClient.send(request); |
Copilot uses AI. Check for mistakes.
} | ||
} | ||
const h = request.headers || (request.headers = {}); | ||
if (this._accessToken) h.Authorization = 'Bearer ' + this._accessToken; else if (this._accessTokenFactory) delete h.Authorization; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This complex conditional logic should be split into multiple lines for better readability. Consider using separate if-else blocks to make the authorization header logic clearer.
if (this._accessToken) h.Authorization = 'Bearer ' + this._accessToken; else if (this._accessTokenFactory) delete h.Authorization; | |
if (this._accessToken) { | |
h.Authorization = 'Bearer ' + this._accessToken; | |
} else if (this._accessTokenFactory) { | |
delete h.Authorization; | |
} |
Copilot uses AI. Check for mistakes.
}); | ||
|
||
const request: HttpRequest = { method: "GET", url: "http://example.com/resource" }; | ||
const resp = await client.send(request); // send path with existing logic; allowRetry=false triggered by initial token acquisition above |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment mentions 'allowRetry=false triggered by initial token acquisition above' but there's no token acquisition visible in this test method above this line. The comment appears to be inaccurate or misleading.
const resp = await client.send(request); // send path with existing logic; allowRetry=false triggered by initial token acquisition above | |
const resp = await client.send(request); // Verifies that allowRetry=false prevents retry on initial token acquisition (401 response). |
Copilot uses AI. Check for mistakes.
fix(SignalR): retry access token refresh on 401 in TS client
Summary of the changes (Less than 80 chars)
Description
Fixes #56494