Fix SignalR JS: don't send Azure SignalR JWT to app server on reconnect#65638
Fix SignalR JS: don't send Azure SignalR JWT to app server on reconnect#65638Copilot wants to merge 7 commits into
Conversation
…alR JWT leaking to app server Co-authored-by: BrennanConroy <7574801+BrennanConroy@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a SignalR TypeScript client reconnect edge case where an Azure SignalR Service redirect token (JWT) could be erroneously re-used as a Bearer token when renegotiating against the original app server.
Changes:
- Reset the cached
_httpClient._accessTokenat the start of_startInternalto avoid leaking redirect-scoped tokens on reconnect. - Add a regression test covering redirected negotiate followed by a stop/start reconnect.
- Update SignalR TS changelog and tighten the
[no changelog]override detection inCodeCheck.ps1.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/SignalR/clients/ts/signalr/src/HttpConnection.ts | Clears cached access token on (re)start so redirect tokens don’t get sent to the original negotiate endpoint. |
| src/SignalR/clients/ts/signalr/tests/HttpConnection.test.ts | Adds coverage to ensure reconnect negotiate to the app server doesn’t include the redirect token. |
| src/SignalR/clients/ts/CHANGELOG.md | Documents the client behavior change for the next preview release. |
| eng/scripts/CodeCheck.ps1 | Uses literal string matching for the [no changelog] override marker. |
Comments suppressed due to low confidence (1)
src/SignalR/clients/ts/signalr/tests/HttpConnection.test.ts:782
- There’s commented-out code in the options object (
accessTokenFactory: () => null). Since this test is intentionally omitting the option, it would be clearer to remove the commented line and keep the intent in a short comment (or set the property toundefinedexplicitly if needed).
...commonOptions,
// explicitly do not provide an access token factory to verify we correctly reset state
// accessTokenFactory: () => null,
httpClient,
|
|
||
| ## v11.0.0-preview.3 | ||
|
|
||
| - Don't send Azure SignalR Service access token to app server on reconnect when accessTokenFactory is null. [#65638](https://github.com/dotnet/aspnetcore/pull/65638) |
There was a problem hiding this comment.
The changelog entry is a bit misleading: this fix is about clearing a cached redirect-scoped access token so it isn’t sent to the original app server on a subsequent start/reconnect. It’s not specific to accessTokenFactory being null (the option is typically undefined when not provided, and the bug is triggered by a negotiate redirect returning an access token). Consider rewording the bullet to describe the redirected-negotiate/reconnect behavior rather than accessTokenFactory being null.
| - Don't send Azure SignalR Service access token to app server on reconnect when accessTokenFactory is null. [#65638](https://github.com/dotnet/aspnetcore/pull/65638) | |
| - Don't send a redirect-scoped Azure SignalR Service access token obtained from a negotiate redirect to the original app server on subsequent start/reconnect. [#65638](https://github.com/dotnet/aspnetcore/pull/65638) |
|
Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. |
| @@ -1,5 +1,9 @@ | |||
| Change log contains changes for both @microsoft/signalr and @microsoft/signalr-protocol-msgpack. | |||
|
|
|||
| ## v11.0.0-preview.3 | |||
There was a problem hiding this comment.
| ## v11.0.0-preview.3 | |
| ## v11.0.0-preview.5 |
| # Only enforce changelog rule if there are relevant TS changes | ||
| if ($tsChanges.Count -gt 0 -and -not $changelogChanged) { | ||
| # Check if the override marker exists in recent commit messages | ||
| $hasOverride = git log origin/$targetBranch..HEAD --pretty=%B | Select-String -Pattern $signalrChangelogOverrideMarker -Quiet |
There was a problem hiding this comment.
Was not using -SimpleMatch breaking anything?
There was a problem hiding this comment.
Yeah, it doesn't treat -Pattern as a regex
https://learn.microsoft.com/powershell/module/microsoft.powershell.utility/select-string?view=powershell-7.6#-simplematch
| // as part of negotiating | ||
| let url = this.baseUrl; | ||
| this._accessTokenFactory = this._options.accessTokenFactory; | ||
| this._httpClient._accessToken = undefined; |
There was a problem hiding this comment.
Clearing out stale state at startup feels like a bit of an antipattern. I wonder if we could clear out the _httpClient._accessToken sooner as soon as we're done with the negotiate redirects.
I think the cleaner thing would be to pass the accessToken as an explicit parameter to _createTransport. Therefore, the transport and the access token always get cleaned up together making it impossible to get out of sync.
Also, I know it's not new to this PR, but do we need this._httpClient._accessTokenFactory and this._accessTokenFactory. I get that the options could be rewritten, but the closure could be modified too. I doubt anyone is really changing the reference, and it's just more state we need to keep track of.
There was a problem hiding this comment.
Refactored in 493ce77: redirect-scoped access token is now plumbed explicitly through _createTransport / _constructTransport (and _resolveTransportOrError), and the duplicate HttpConnection._accessTokenFactory field is gone. _httpClient._accessToken still has to be set during the negotiate redirect loop (so the next negotiate carries it) and is left on the client for the LongPollingTransport's lifetime, but the _createTransport/transport pairing is now the single thing that "owns" the token, and the reset at the top of _startInternal is the symmetric clear for restart. Let me know if you wanted me to go further (e.g. pull _accessToken off AccessTokenHttpClient entirely).
| const options: IHttpConnectionOptions = { | ||
| ...commonOptions, | ||
| // explicitly do not provide an access token factory to verify we correctly reset state | ||
| // accessTokenFactory: () => null, |
There was a problem hiding this comment.
So is the above a workaround that clears out the access token even without this fix?
There was a problem hiding this comment.
Yes ΓÇö setting accessTokenFactory: () => null is the workaround I suggested on the issue, because the factory overwrites the cached _accessToken on the next negotiate. The test intentionally leaves the factory unset so the regression actually reproduces without the fix. Replaced the dead commented-out line with a real comment explaining that in 493ce77.
| // accessTokenFactory: () => null, | ||
| httpClient, | ||
| logger, | ||
| transport: HttpTransportType.LongPolling, |
There was a problem hiding this comment.
Should we test any other transports?
There was a problem hiding this comment.
I don't think it adds coverage: the bug is in the /negotiate request path on the second start(), which goes through AccessTokenHttpClient.send regardless of which transport ends up being selected. The transport isn't even constructed until after both negotiates are done. Happy to add WS/SSE variants if you'd still like, but they'd be testing the same code path.
… up test Addresses review feedback on #65638: - Pass redirect-scoped access token explicitly through _createTransport/_constructTransport instead of relying on stale mutable state on HttpConnection / AccessTokenHttpClient - Drop the duplicate HttpConnection._accessTokenFactory field; rely on _httpClient - Reword CHANGELOG entry to describe the actual scenario and bump to preview.5 - Replace dead commented-out option in test with a real explanatory comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The refactor in 493ce77 changed _constructTransport's SSE branch to pass only redirectAccessToken to ServerSentEventsTransport. When there's no negotiate redirect, redirectAccessToken is undefined but the user's accessTokenFactory has already populated _httpClient._accessToken during the initial /negotiate. The original code passed _httpClient._accessToken so SSE got the user's token in the access_token query string. Restore that fallback so the TypeScript functional test 'hubConnection over ServerSentEvents transport can connect to hub with authorization' passes again. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When connecting through Azure SignalR Service, the negotiate redirect response returns an access token (Azure SignalR JWT) that gets stored in
_httpClient._accessToken. On reconnect,_startInternalwas resetting_accessTokenFactoryand_httpClient._accessTokenFactoryback to the original options values, but not_httpClient._accessToken— causing the stale Azure SignalR JWT to be sent as aBearertoken to the app server's/negotiateendpoint.Description
this._httpClient._accessToken = undefinedat the top of_startInternal, alongside the existing factory resets.HttpConnection.test.tscase verifying that after a redirected negotiate (with returned access token), the reconnect negotiate to the original app server does not include the redirect-scoped token.Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.