diff --git a/eng/scripts/CodeCheck.ps1 b/eng/scripts/CodeCheck.ps1 index f3ef5d320f6a..daab0b0a19b9 100644 --- a/eng/scripts/CodeCheck.ps1 +++ b/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." diff --git a/src/SignalR/clients/ts/CHANGELOG.md b/src/SignalR/clients/ts/CHANGELOG.md index 6fe59d38a6e4..bcf87cd6e101 100644 --- a/src/SignalR/clients/ts/CHANGELOG.md +++ b/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) diff --git a/src/SignalR/clients/ts/signalr/src/HttpConnection.ts b/src/SignalR/clients/ts/signalr/src/HttpConnection.ts index f8397f4d65b5..6123d86ab95e 100644 --- a/src/SignalR/clients/ts/signalr/src/HttpConnection.ts +++ b/src/SignalR/clients/ts/signalr/src/HttpConnection.ts @@ -57,7 +57,6 @@ export class HttpConnection implements IConnection { private _stopPromise?: Promise; private _stopPromiseResolver: (value?: PromiseLike) => void = () => {}; private _stopError?: Error; - private _accessTokenFactory?: () => string | Promise; private _sendQueue?: TransportSendQueue; public readonly features: any = {}; @@ -223,17 +222,18 @@ export class HttpConnection implements IConnection { } private async _startInternal(transferFormat: TransferFormat): Promise { - // 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 { + private async _createTransport(url: string, requestedTransport: HttpTransportType | ITransport | undefined, negotiateResponse: INegotiateResponse, requestedTransferFormat: TransferFormat, redirectAccessToken: string | undefined): Promise { 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; } diff --git a/src/SignalR/clients/ts/signalr/tests/HttpConnection.test.ts b/src/SignalR/clients/ts/signalr/tests/HttpConnection.test.ts index 76ff38e3357f..f436d2a60721 100644 --- a/src/SignalR/clients/ts/signalr/tests/HttpConnection.test.ts +++ b/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()