Fix SignalR JS: don't send Azure SignalR JWT to app server on reconnect

eng/scripts/CodeCheck.ps1

@@ -217,7 +217,7 @@ try {
         # 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
+            $hasOverride = git log origin/$targetBranch..HEAD --pretty=%B | Select-String -Pattern $signalrChangelogOverrideMarker -Quiet -SimpleMatch
 
             if (-not $hasOverride) {
                 LogError "Changes were made to 'src/SignalR/clients/ts/', but no update to 'CHANGELOG.md' was found."

src/SignalR/clients/ts/CHANGELOG.md

@@ -1,5 +1,9 @@
 Change log contains changes for both @microsoft/signalr and @microsoft/signalr-protocol-msgpack.
 
+## v11.0.0-preview.6
+
+- 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)
+
 ## v10.0.0-rc.1
 
 - Implemented fix for correctly reporting retries in the SignalR TS client. [#62812](https://github.com/dotnet/aspnetcore/pull/62812)

src/SignalR/clients/ts/signalr/src/HttpConnection.ts

@@ -57,7 +57,6 @@ export class HttpConnection implements IConnection {
     private _stopPromise?: Promise<void>;
     private _stopPromiseResolver: (value?: PromiseLike<void>) => void = () => {};
     private _stopError?: Error;
-    private _accessTokenFactory?: () => string | Promise<string>;
     private _sendQueue?: TransportSendQueue;
 
     public readonly features: any = {};
@@ -223,17 +222,18 @@ export class HttpConnection implements IConnection {
     }
 
     private async _startInternal(transferFormat: TransferFormat): Promise<void> {
-        // Store the original base url and the access token factory since they may change
-        // as part of negotiating
+        // Reset http client auth state for this start attempt. A previous start may have
+        // cached a redirect-scoped access token; that token must not be reused for the
+        // initial /negotiate request against the original (app) server.
         let url = this.baseUrl;
-        this._accessTokenFactory = this._options.accessTokenFactory;
-        this._httpClient._accessTokenFactory = this._accessTokenFactory;
+        this._httpClient._accessToken = undefined;
+        this._httpClient._accessTokenFactory = this._options.accessTokenFactory;
 
         try {
             if (this._options.skipNegotiation) {
                 if (this._options.transport === HttpTransportType.WebSockets) {
                     // No need to add a connection ID in this case
-                    this.transport = this._constructTransport(HttpTransportType.WebSockets);
+                    this.transport = this._constructTransport(HttpTransportType.WebSockets, undefined);
                     // We should just call connect directly in this case.
                     // No fallback or negotiate in this case.
                     await this._startTransport(url, transferFormat);
@@ -243,6 +243,10 @@ export class HttpConnection implements IConnection {
             } else {
                 let negotiateResponse: INegotiateResponse | null = null;
                 let redirects = 0;
+                // Tracks an access token returned by a negotiate redirect. It is scoped to the
+                // redirect target (e.g. Azure SignalR Service) and is bound to the transport
+                // created from this start attempt, never leaking back to the original server.
+                let redirectAccessToken: string | undefined;
 
                 do {
                     negotiateResponse = await this._getNegotiationResponse(url);
@@ -264,12 +268,11 @@ export class HttpConnection implements IConnection {
                     }
 
                     if (negotiateResponse.accessToken) {
-                        // Replace the current access token factory with one that uses
-                        // the returned access token
-                        const accessToken = negotiateResponse.accessToken;
-                        this._accessTokenFactory = () => accessToken;
-                        // set the factory to undefined so the AccessTokenHttpClient won't retry with the same token, since we know it won't change until a connection restart
-                        this._httpClient._accessToken = accessToken;
+                        redirectAccessToken = negotiateResponse.accessToken;
+                        // Set the cached token on the http client so the next /negotiate in the
+                        // redirect loop carries it. Clear the factory so AccessTokenHttpClient
+                        // won't retry with the same token - we know it won't change until restart.
+                        this._httpClient._accessToken = redirectAccessToken;
                         this._httpClient._accessTokenFactory = undefined;
                     }
 
@@ -281,7 +284,7 @@ export class HttpConnection implements IConnection {
                     throw new Error("Negotiate redirection limit exceeded.");
                 }
 
-                await this._createTransport(url, this._options.transport, negotiateResponse, transferFormat);
+                await this._createTransport(url, this._options.transport, negotiateResponse, transferFormat, redirectAccessToken);
             }
 
             if (this.transport instanceof LongPollingTransport) {
@@ -361,7 +364,7 @@ export class HttpConnection implements IConnection {
         return url + (url.indexOf("?") === -1 ? "?" : "&") + `id=${connectionToken}`;
     }
 
-    private async _createTransport(url: string, requestedTransport: HttpTransportType | ITransport | undefined, negotiateResponse: INegotiateResponse, requestedTransferFormat: TransferFormat): Promise<void> {
+    private async _createTransport(url: string, requestedTransport: HttpTransportType | ITransport | undefined, negotiateResponse: INegotiateResponse, requestedTransferFormat: TransferFormat, redirectAccessToken: string | undefined): Promise<void> {
         let connectUrl = this._createConnectUrl(url, negotiateResponse.connectionToken);
         if (this._isITransport(requestedTransport)) {
             this._logger.log(LogLevel.Debug, "Connection was provided an instance of ITransport, using that directly.");
@@ -377,7 +380,7 @@ export class HttpConnection implements IConnection {
         let negotiate: INegotiateResponse | undefined = negotiateResponse;
         for (const endpoint of transports) {
             const transportOrError = this._resolveTransportOrError(endpoint, requestedTransport, requestedTransferFormat,
-                negotiate?.useStatefulReconnect === true);
+                negotiate?.useStatefulReconnect === true, redirectAccessToken);
             if (transportOrError instanceof Error) {
                 // Store the error and continue, we don't want to cause a re-negotiate in these cases
                 transportExceptions.push(`${endpoint.transport} failed:`);
@@ -416,19 +419,30 @@ export class HttpConnection implements IConnection {
         return Promise.reject(new Error("None of the transports supported by the client are supported by the server."));
     }
 
-    private _constructTransport(transport: HttpTransportType): ITransport {
+    private _constructTransport(transport: HttpTransportType, redirectAccessToken: string | undefined): ITransport {
         switch (transport) {
-            case HttpTransportType.WebSockets:
+            case HttpTransportType.WebSockets: {
                 if (!this._options.WebSocket) {
                     throw new Error("'WebSocket' is not supported in your environment.");
                 }
-                return new WebSocketTransport(this._httpClient, this._accessTokenFactory, this._logger, this._options.logMessageContent!,
+                // If the negotiate redirect returned an access token, use a closure that returns it
+                // (matching the behavior expected by AccessTokenHttpClient). Otherwise, fall through
+                // to the original options factory so it can be re-invoked on token refresh.
+                const wsAccessTokenFactory = redirectAccessToken !== undefined
+                    ? () => redirectAccessToken
+                    : this._options.accessTokenFactory;
+                return new WebSocketTransport(this._httpClient, wsAccessTokenFactory, this._logger, this._options.logMessageContent!,
                     this._options.WebSocket, this._options.headers || {});
+            }
             case HttpTransportType.ServerSentEvents:
                 if (!this._options.EventSource) {
                     throw new Error("'EventSource' is not supported in your environment.");
                 }
-                return new ServerSentEventsTransport(this._httpClient, this._httpClient._accessToken, this._logger, this._options);
+                // Fall back to the token cached on the http client (populated from
+                // accessTokenFactory during the initial /negotiate) when no redirect
+                // token was returned, so SSE continues to authenticate with the user's
+                // access token in the access_token query string.
+                return new ServerSentEventsTransport(this._httpClient, redirectAccessToken ?? this._httpClient._accessToken, this._logger, this._options);
             case HttpTransportType.LongPolling:
                 return new LongPollingTransport(this._httpClient, this._logger, this._options);
             default:
@@ -465,7 +479,7 @@ export class HttpConnection implements IConnection {
     }
 
     private _resolveTransportOrError(endpoint: IAvailableTransport, requestedTransport: HttpTransportType | undefined,
-        requestedTransferFormat: TransferFormat, useStatefulReconnect: boolean): ITransport | Error | unknown {
+        requestedTransferFormat: TransferFormat, useStatefulReconnect: boolean, redirectAccessToken: string | undefined): ITransport | Error | unknown {
         const transport = HttpTransportType[endpoint.transport];
         if (transport === null || transport === undefined) {
             this._logger.log(LogLevel.Debug, `Skipping transport '${endpoint.transport}' because it is not supported by this client.`);
@@ -482,7 +496,7 @@ export class HttpConnection implements IConnection {
                         this._logger.log(LogLevel.Debug, `Selecting transport '${HttpTransportType[transport]}'.`);
                         try {
                             this.features.reconnect = transport === HttpTransportType.WebSockets ? useStatefulReconnect : undefined;
-                            return this._constructTransport(transport);
+                            return this._constructTransport(transport, redirectAccessToken);
                         } catch (ex) {
                             return ex;
                         }

src/SignalR/clients/ts/signalr/tests/HttpConnection.test.ts

@@ -729,6 +729,101 @@ describe("HttpConnection", () => {
         });
     });
 
+    it("does not send access token from redirect negotiate to original server on reconnect", async () => {
+        await VerifyLogger.run(async (logger) => {
+            let negotiateCount = 0;
+            let firstPoll = true;
+            const pollingPromiseSource = new PromiseSource();
+            const httpClient = new TestHttpClient()
+                .on("POST", /\/negotiate/, (r) => {
+                    negotiateCount++;
+                    if (negotiateCount === 1) {
+                        // First negotiate to app server - redirect to Azure SignalR with a token
+                        return { url: "https://azure.signalr.net/client/", accessToken: "azureSignalRToken" };
+                    }
+
+                    if (negotiateCount === 2) {
+                        // Second negotiate to Azure SignalR service (redirect target) - succeeds
+                        return {
+                            availableTransports: [{ transport: "LongPolling", transferFormats: ["Text"] }],
+                            connectionId: "0rge0d00-0040-0030-0r00-000q00r00e00",
+                        };
+                    }
+
+                    if (negotiateCount === 3) {
+                        // Third negotiate is a reconnect to the original app server
+                        // It should NOT include the Azure SignalR token
+                        if (r.headers && r.headers.Authorization === "Bearer azureSignalRToken") {
+                            fail("Reconnect negotiate to app server must not include the Azure SignalR access token.");
+                        }
+                        return { url: "https://azure.signalr.net/client/", accessToken: "azureSignalRToken2" };
+                    }
+
+                    // Fourth negotiate to Azure SignalR service on reconnect
+                    return {
+                        availableTransports: [{ transport: "LongPolling", transferFormats: ["Text"] }],
+                        connectionId: "1rge0d00-0040-0030-0r00-000q00r00e01",
+                    };
+                })
+                .on("GET", async (r) => {
+                    if (firstPoll) {
+                        firstPoll = false;
+                        return "";
+                    }
+                    await pollingPromiseSource.promise;
+                    return new HttpResponse(204, "No Content", "");
+                })
+                .on("DELETE", () => new HttpResponse(202));
+
+            const options: IHttpConnectionOptions = {
+                ...commonOptions,
+                // Intentionally omit accessTokenFactory: without the fix, providing
+                // `accessTokenFactory: () => null` would mask the bug because the factory
+                // would overwrite the stale _accessToken on the next negotiate. The bug
+                // surfaces when no factory is set and the only token in play came from a
+                // negotiate redirect on a previous start.
+                httpClient,
+                logger,
+                transport: HttpTransportType.LongPolling,
+            } as IHttpConnectionOptions;
+
+            const connection = new HttpConnection("http://tempuri.org", options);
+            try {
+                await connection.start(TransferFormat.Text);
+
+                // Verify first connection negotiated through the redirect
+                expect(negotiateCount).toBe(2);
+
+                pollingPromiseSource.resolve();
+            } finally {
+                await connection.stop();
+            }
+
+            // Reconnect - start again
+            firstPoll = true;
+            const pollingPromiseSource2 = new PromiseSource();
+            httpClient.on("GET", async (r) => {
+                if (firstPoll) {
+                    firstPoll = false;
+                    return "";
+                }
+                await pollingPromiseSource2.promise;
+                return new HttpResponse(204, "No Content", "");
+            });
+
+            try {
+                await connection.start(TransferFormat.Text);
+
+                // Verify reconnect also went through redirect (4 total negotiates)
+                expect(negotiateCount).toBe(4);
+
+                pollingPromiseSource2.resolve();
+            } finally {
+                await connection.stop();
+            }
+        });
+    });
+
     it("throws error if negotiate response has error", async () => {
         await VerifyLogger.run(async (logger) => {
             const httpClient = new TestHttpClient()