From 142a062559910d043654de279c1404419b5f2fdf Mon Sep 17 00:00:00 2001 From: "Mars.P" Date: Tue, 13 Oct 2026 11:56:04 +0800 Subject: [PATCH] Connect OAuth without App:PublicBaseUrl and across sites #2104 made OAuthFlowService inject PublicBaseUrlSetting, whose constructor throws outside Development when App:PublicBaseUrl is unset. Every OAuth start and every callback then answered 500 on such a host, whatever the return address; the shipped docker-compose API is one. An absolute return address also had to equal that one origin, so a page opened on an alias or on 127.0.0.1 could not start a connect. OAuth now reads a non-throwing set of return origins: App:PublicBaseUrl when set, every Cors:AllowedOrigins entry, OAuth:CallbackUrl's own origin and, in Development, the dev SPA origins the CORS policy adds. A path or no return address needs no origin; an absolute one outside the set is a 400 invalid_request naming both settings. Invitation and reset links still fail fast without the key. The binding cookie from #2104 is SameSite=Lax and host-only. A browser never keeps such a cookie from a cross-site fetch, and never sends it to another host, so with the SPA and API on different sites, or a connect started on another host than OAuth:CallbackUrl names, every callback arrived without it and was refused. A callback without a matching nonce now consumes nothing and redirects to the SPA's /oauth-callback with the code and state in the fragment, which reaches no server log. The signed-in SPA finishes it with POST /api/credentials/oauth/complete, which consumes the state only for its InitiatorUserId. A forwarded authorize link lands on the victim's session, which is not the starter's, so it still completes nothing. The cookie remains the fast path. The hand-off lands on the origin the connect named, and only the page that started the connect holds the session and the opener that finish it. Vite prints http://127.0.0.1:5180 while the callback names localhost:5180, so the SPA names its own origin first, and asks again with a path only when the API answers on the page's origin and refuses that with invalid_request: an alias the API does not list, or an older pod that matched App:PublicBaseUrl alone. A popup that lands where no opener on its origin can hear it stays open with the outcome instead of closing into a reported cancel. No schema change: a state row either build writes is finished by the other. The page retries the POST on a 404, the answer of a pod that predates the endpoint, which consumes nothing. --- .../Controllers/CredentialOAuthController.cs | 23 +- .../Extensions/CorsPolicyExtension.cs | 5 +- backend/src/CodeSpace.Api/appsettings.json | 2 +- ...redentialOAuthAsInitiatorCommandHandler.cs | 15 ++ .../Persistence/Entities/OAuthPendingState.cs | 7 +- .../Services/OAuth/IOAuthFlowService.cs | 7 +- .../Services/OAuth/IOAuthStateStore.cs | 9 + .../Services/OAuth/OAuthFlowService.cs | 71 ++++-- .../Services/OAuth/OAuthReturnUrl.cs | 42 +++- .../Services/OAuth/OAuthStateStore.cs | 25 +- .../CorsPolicy/AllowableCorsOriginsSetting.cs | 6 + .../OAuth/OAuthReturnOriginsSetting.cs | 61 +++++ ...mpleteCredentialOAuthAsInitiatorCommand.cs | 19 ++ .../CompleteCredentialOAuthCommand.cs | 17 +- .../Credentials/InitCredentialOAuthCommand.cs | 6 +- .../OAuthConnectBindingE2ETests.cs | 119 ++++++++- ...AuthConnectWithoutPublicBaseUrlE2ETests.cs | 135 +++++++++++ .../Infrastructure/GitWireApiFactory.cs | 23 ++ .../Credentials/CredentialOwnerReachTests.cs | 2 +- .../DepartedMemberCredentialTests.cs | 4 +- .../OAuthConnectWithoutPublicBaseUrlTests.cs | 171 +++++++++++++ .../OAuth/OAuthFlowTests.cs | 227 +++++++++++++++++- .../OAuth/OAuthStateCleanupTests.cs | 4 +- .../OAuth/ProviderReconnectFlowTests.cs | 2 +- .../RequestAuthorizationInventoryTests.cs | 2 +- .../OAuth/OAuthReturnUrlTests.cs | 73 +++++- .../OAuthReturnOriginsSettingTests.cs | 92 +++++++ frontend/.env.example | 23 +- frontend/src/api/oauth.test.ts | 22 ++ frontend/src/api/oauth.ts | 9 +- frontend/src/api/types.ts | 4 + frontend/src/hooks/use-oauth-flow.test.ts | 104 ++++++++ frontend/src/hooks/use-oauth-flow.ts | 64 ++++- frontend/src/routes/oauth-callback.test.tsx | 170 +++++++++++++ frontend/src/routes/oauth-callback.tsx | 168 ++++++++++--- 35 files changed, 1624 insertions(+), 109 deletions(-) create mode 100644 backend/src/CodeSpace.Core/Handlers/CommandHandlers/Credentials/CompleteCredentialOAuthAsInitiatorCommandHandler.cs create mode 100644 backend/src/CodeSpace.Core/Settings/OAuth/OAuthReturnOriginsSetting.cs create mode 100644 backend/src/CodeSpace.Messages/Commands/Credentials/CompleteCredentialOAuthAsInitiatorCommand.cs create mode 100644 backend/tests/CodeSpace.E2ETests/Credentials/OAuthConnectWithoutPublicBaseUrlE2ETests.cs create mode 100644 backend/tests/CodeSpace.IntegrationTests/OAuth/OAuthConnectWithoutPublicBaseUrlTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Settings/OAuthReturnOriginsSettingTests.cs create mode 100644 frontend/src/hooks/use-oauth-flow.test.ts create mode 100644 frontend/src/routes/oauth-callback.test.tsx diff --git a/backend/src/CodeSpace.Api/Controllers/CredentialOAuthController.cs b/backend/src/CodeSpace.Api/Controllers/CredentialOAuthController.cs index ca05d6a91..50f794aab 100644 --- a/backend/src/CodeSpace.Api/Controllers/CredentialOAuthController.cs +++ b/backend/src/CodeSpace.Api/Controllers/CredentialOAuthController.cs @@ -1,3 +1,4 @@ +using CodeSpace.Core.Services.OAuth; using CodeSpace.Messages.Commands.Credentials; using MediatR; using Microsoft.AspNetCore.Authorization; @@ -13,6 +14,11 @@ namespace CodeSpace.Api.Controllers; /// is two halves: the state (32-byte CSPRNG, one-time use, 10-min TTL, PK-indexed) and a nonce /// cookie /init set in the browser that started the flow. State alone let whichever browser /// presented it finish someone else's connect. +/// +/// A browser cannot always keep that cookie: a cross-site fetch response's SameSite=Lax cookie is never stored, +/// so with the SPA and the API on different sites — and a connect started on another host than OAuth:CallbackUrl +/// names — the callback arrives without it. It then consumes nothing and hands the code and state to the SPA's +/// completion page, which finishes the connect through POST /complete as the signed-in person who started it. /// [ApiController] [Route("api/credentials/oauth")] @@ -47,10 +53,25 @@ public async Task Callback([FromQuery] OAuthCallbackQuery query, var result = await _mediator.Send(new CompleteCredentialOAuthCommand { Code = query.Code, State = query.State, BrowserNonce = Request.Cookies[BrowserNonceCookie] }, cancellationToken).ConfigureAwait(false); + // Handed off: nothing was consumed, and a cookie this browser does hold belongs to another connect, so it stays. + if (result.CredentialId is not { } credentialId) return Redirect(OAuthReturnUrl.HandOff(result.ReturnUrl, query.Code, query.State)); + // Spent with the state it proved. Response.Cookies.Delete(BrowserNonceCookie, BrowserNonceCookieOptions(result.BrowserNonceCookiePath, lifetime: null)); - return Redirect(BuildSuccessReturn(result.ReturnUrl, result.CredentialId)); + return Redirect(BuildSuccessReturn(result.ReturnUrl, credentialId)); + } + + /// + /// The SPA's completion page finishing a connect the callback handed off. Authenticated — the fallback policy — and + /// accepted only from the person who started the connect. + /// + [HttpPost("complete")] + public async Task Complete([FromBody] CompleteCredentialOAuthAsInitiatorCommand command, CancellationToken cancellationToken) + { + var result = await _mediator.Send(command, cancellationToken).ConfigureAwait(false); + + return Ok(new { credentialId = result.CredentialId }); } /// diff --git a/backend/src/CodeSpace.Api/Extensions/CorsPolicyExtension.cs b/backend/src/CodeSpace.Api/Extensions/CorsPolicyExtension.cs index adad9ca19..35c58ab8f 100644 --- a/backend/src/CodeSpace.Api/Extensions/CorsPolicyExtension.cs +++ b/backend/src/CodeSpace.Api/Extensions/CorsPolicyExtension.cs @@ -15,9 +15,6 @@ public static class CorsPolicyExtension /// Named rather than the default policy so UseCors states which policy it is applying, and a second policy (an admin surface, a webhook origin) can be added later without silently widening this one. public const string PolicyName = "CodeSpaceSpa"; - /// The Vite dev-server origins, added in Development only. The port is pinned by vite.config.ts — keep these in sync. - private static readonly string[] DevelopmentOrigins = { "http://localhost:5180", "http://127.0.0.1:5180" }; - public static IServiceCollection AddCorsPolicy(this IServiceCollection services, IConfiguration configuration, IHostEnvironment environment) { var origins = ResolveOrigins(configuration, environment); @@ -46,6 +43,6 @@ internal static string[] ResolveOrigins(IConfiguration configuration, IHostEnvir if (!environment.IsDevelopment()) return configured; - return configured.Concat(DevelopmentOrigins).Distinct(StringComparer.OrdinalIgnoreCase).ToArray(); + return configured.Concat(AllowableCorsOriginsSetting.DevelopmentOrigins).Distinct(StringComparer.OrdinalIgnoreCase).ToArray(); } } diff --git a/backend/src/CodeSpace.Api/appsettings.json b/backend/src/CodeSpace.Api/appsettings.json index 866bc98fb..713ca45e0 100644 --- a/backend/src/CodeSpace.Api/appsettings.json +++ b/backend/src/CodeSpace.Api/appsettings.json @@ -37,7 +37,7 @@ "CallbackUrl": "http://localhost:5180/api/credentials/oauth/callback" }, "Cors": { - "_doc": "Origins allowed to call this API cross-origin (when the SPA bypasses the Vite /api proxy). localhost:5180 is added automatically in Development (matches vite.config.ts server.port); production deployments must list their SPA host explicitly.", + "_doc": "Origins allowed to call this API cross-origin (when the SPA bypasses the Vite /api proxy). localhost:5180 and 127.0.0.1:5180 are added automatically in Development (matches vite.config.ts server.port); production deployments must list their SPA host explicitly. An OAuth connect may also return the browser to any origin listed here, as to App:PublicBaseUrl and to OAuth:CallbackUrl's own origin.", "AllowedOrigins": [] }, "Logging": { diff --git a/backend/src/CodeSpace.Core/Handlers/CommandHandlers/Credentials/CompleteCredentialOAuthAsInitiatorCommandHandler.cs b/backend/src/CodeSpace.Core/Handlers/CommandHandlers/Credentials/CompleteCredentialOAuthAsInitiatorCommandHandler.cs new file mode 100644 index 000000000..9b22eb876 --- /dev/null +++ b/backend/src/CodeSpace.Core/Handlers/CommandHandlers/Credentials/CompleteCredentialOAuthAsInitiatorCommandHandler.cs @@ -0,0 +1,15 @@ +using CodeSpace.Core.Services.OAuth; +using CodeSpace.Messages.Commands.Credentials; +using MediatR; + +namespace CodeSpace.Core.Handlers.CommandHandlers.Credentials; + +public sealed class CompleteCredentialOAuthAsInitiatorCommandHandler : IRequestHandler +{ + private readonly IOAuthFlowService _flow; + + public CompleteCredentialOAuthAsInitiatorCommandHandler(IOAuthFlowService flow) { _flow = flow; } + + public async Task Handle(CompleteCredentialOAuthAsInitiatorCommand request, CancellationToken cancellationToken) => + await _flow.CompleteAsInitiatorAsync(request.State, request.Code, cancellationToken).ConfigureAwait(false); +} diff --git a/backend/src/CodeSpace.Core/Persistence/Entities/OAuthPendingState.cs b/backend/src/CodeSpace.Core/Persistence/Entities/OAuthPendingState.cs index b03dce42d..81e1ee324 100644 --- a/backend/src/CodeSpace.Core/Persistence/Entities/OAuthPendingState.cs +++ b/backend/src/CodeSpace.Core/Persistence/Entities/OAuthPendingState.cs @@ -19,9 +19,10 @@ public class OAuthPendingState : IAuditable public string? ReturnUrl { get; set; } /// - /// SHA-256 of the nonce the starting browser holds in its binding cookie. The callback must present that nonce, - /// so only the browser that started a connect can finish it. Null only on a row a previous version wrote, which - /// therefore can never be completed (migration 0246). + /// SHA-256 of the nonce the starting browser holds in its binding cookie. A callback that presents that nonce finishes + /// the connect on the spot; one that does not consumes nothing and is handed to the SPA, where only + /// , signed in, can finish it. Null only on a row a version before migration 0246 wrote, + /// which therefore only that hand-off can complete. /// public string? BrowserNonceHash { get; set; } public List? RequestedScopes { get; set; } diff --git a/backend/src/CodeSpace.Core/Services/OAuth/IOAuthFlowService.cs b/backend/src/CodeSpace.Core/Services/OAuth/IOAuthFlowService.cs index 5d93c62ac..2e18987e1 100644 --- a/backend/src/CodeSpace.Core/Services/OAuth/IOAuthFlowService.cs +++ b/backend/src/CodeSpace.Core/Services/OAuth/IOAuthFlowService.cs @@ -8,10 +8,15 @@ namespace CodeSpace.Core.Services.OAuth; /// provider's authorize URL the SPA opens in a popup. /// 2. runs from the callback redirect — anonymous, with the /// state token and the starting browser's nonce as proof — consumes the state, -/// exchanges the auth code, and persists a Credential. +/// exchanges the auth code, and persists a Credential. Without the nonce it consumes +/// nothing and hands the callback to the SPA's completion page instead. +/// 3. is that page finishing it: the same consume, +/// exchange and persist, with the signed-in caller — who must have started the +/// connect — as the proof in place of the nonce. /// public interface IOAuthFlowService { Task InitAsync(Guid providerInstanceId, string displayName, Guid? intendedOwnerUserId, string? returnUrl, IReadOnlyList? scopes, CancellationToken cancellationToken); Task CompleteAsync(string state, string code, string? browserNonce, CancellationToken cancellationToken); + Task CompleteAsInitiatorAsync(string state, string code, CancellationToken cancellationToken); } diff --git a/backend/src/CodeSpace.Core/Services/OAuth/IOAuthStateStore.cs b/backend/src/CodeSpace.Core/Services/OAuth/IOAuthStateStore.cs index 23285412a..c0edbb147 100644 --- a/backend/src/CodeSpace.Core/Services/OAuth/IOAuthStateStore.cs +++ b/backend/src/CodeSpace.Core/Services/OAuth/IOAuthStateStore.cs @@ -18,6 +18,15 @@ public interface IOAuthStateStore /// browser that started the connect can still finish it. /// Task ConsumeAsync(string state, string browserNonceHash, CancellationToken cancellationToken); + + /// + /// The same one-time consume, bound to the person instead of the browser: only the row whose state matches AND + /// whose InitiatorUserId is the caller. Anyone else presenting the state matches nothing and consumes nothing. + /// + Task ConsumeForInitiatorAsync(string state, Guid initiatorUserId, CancellationToken cancellationToken); + + /// The unexpired row for this state, read without consuming it. Null when there is none. + Task FindLiveAsync(string state, CancellationToken cancellationToken); } public sealed record OAuthPendingStateInput diff --git a/backend/src/CodeSpace.Core/Services/OAuth/OAuthFlowService.cs b/backend/src/CodeSpace.Core/Services/OAuth/OAuthFlowService.cs index 63760c455..017ccdd26 100644 --- a/backend/src/CodeSpace.Core/Services/OAuth/OAuthFlowService.cs +++ b/backend/src/CodeSpace.Core/Services/OAuth/OAuthFlowService.cs @@ -7,6 +7,7 @@ using CodeSpace.Core.Services.Providers.Identity; using CodeSpace.Core.Services.Teams; using CodeSpace.Core.Settings.Application; +using CodeSpace.Core.Settings.CorsPolicy; using CodeSpace.Core.Settings.OAuth; using CodeSpace.Messages.Commands.Credentials; using CodeSpace.Messages.Credentials; @@ -34,9 +35,9 @@ public sealed class OAuthFlowService : IOAuthFlowService, IScopedDependency private readonly ICredentialSuccessionService _succession; private readonly ITeamMemberService _members; private readonly OAuthCallbackUrlSetting _callbackUrlSetting; - private readonly PublicBaseUrlSetting _publicBaseUrl; + private readonly OAuthReturnOriginsSetting _returnOrigins; - public OAuthFlowService(CodeSpaceDbContext db, IOAuthClientRegistry oauthClients, IOAuthStateStore stateStore, IPkceGenerator pkce, IPayloadEncryptor encryptor, ICredentialPayloadSerializer serializer, ICurrentTeam currentTeam, ICurrentUser currentUser, IUserProviderIdentityService identityService, ICredentialSuccessionService succession, ITeamMemberService members, OAuthCallbackUrlSetting callbackUrlSetting, PublicBaseUrlSetting publicBaseUrl) + public OAuthFlowService(CodeSpaceDbContext db, IOAuthClientRegistry oauthClients, IOAuthStateStore stateStore, IPkceGenerator pkce, IPayloadEncryptor encryptor, ICredentialPayloadSerializer serializer, ICurrentTeam currentTeam, ICurrentUser currentUser, IUserProviderIdentityService identityService, ICredentialSuccessionService succession, ITeamMemberService members, OAuthCallbackUrlSetting callbackUrlSetting, OAuthReturnOriginsSetting returnOrigins) { _db = db; _oauthClients = oauthClients; @@ -50,7 +51,7 @@ public OAuthFlowService(CodeSpaceDbContext db, IOAuthClientRegistry oauthClients _succession = succession; _members = members; _callbackUrlSetting = callbackUrlSetting; - _publicBaseUrl = publicBaseUrl; + _returnOrigins = returnOrigins; } public async Task InitAsync(Guid providerInstanceId, string displayName, Guid? intendedOwnerUserId, string? returnUrl, IReadOnlyList? scopes, CancellationToken cancellationToken) @@ -95,7 +96,23 @@ public async Task InitAsync(Guid providerInstanceId, public async Task CompleteAsync(string state, string code, string? browserNonce, CancellationToken cancellationToken) { - var pending = await ConsumeStateAsync(state, browserNonce, cancellationToken).ConfigureAwait(false); + var pending = await ConsumeBrowserBoundStateAsync(state, browserNonce, cancellationToken).ConfigureAwait(false); + + if (pending == null) return await HandOffToInitiatorAsync(state, cancellationToken).ConfigureAwait(false); + + return await FinishAsync(pending, code, cancellationToken).ConfigureAwait(false); + } + + public async Task CompleteAsInitiatorAsync(string state, string code, CancellationToken cancellationToken) + { + var pending = await ConsumeInitiatorBoundStateAsync(state, cancellationToken).ConfigureAwait(false); + + return await FinishAsync(pending, code, cancellationToken).ConfigureAwait(false); + } + + /// Exchanges the code for the consumed state and keeps the token — whichever proof consumed it. + private async Task FinishAsync(OAuthPendingState pending, string code, CancellationToken cancellationToken) + { var instance = await LoadInstanceAsync(pending.ProviderInstanceId, cancellationToken).ConfigureAwait(false); var (clientId, clientSecret) = ReadOAuthClientCredentials(instance); @@ -116,8 +133,8 @@ public async Task CompleteAsync(string state, str { CredentialId = credential.Id, TeamId = pending.TeamId, - // Checked again rather than trusted from init: the public origin can change while a connect is pending. - ReturnUrl = OAuthReturnUrl.AllowedOrFallback(pending.ReturnUrl, _publicBaseUrl.Value), + // Checked again rather than trusted from init: the configured origins can change while a connect is pending. + ReturnUrl = OAuthReturnUrl.AllowedOrFallback(pending.ReturnUrl, _returnOrigins.Value), BrowserNonceCookiePath = ResolveCallbackUri().AbsolutePath }; } @@ -129,9 +146,9 @@ public async Task CompleteAsync(string state, str /// private void EnsureReturnUrlAllowed(string? returnUrl) { - if (returnUrl == null || OAuthReturnUrl.IsAllowed(returnUrl, _publicBaseUrl.Value)) return; + if (returnUrl == null || OAuthReturnUrl.IsAllowed(returnUrl, _returnOrigins.Value)) return; - throw new ArgumentException("returnUrl must be a path on this site or an address on its public origin.", nameof(returnUrl)); + throw new ArgumentException($"returnUrl must be a path on this site, or an address on an origin this deployment serves its web app from — {PublicBaseUrlSetting.ConfigurationKey}, or one listed in {AllowableCorsOriginsSetting.ConfigurationKey}.", nameof(returnUrl)); } /// @@ -214,23 +231,43 @@ private async Task LoadOwnedInstanceAsync(Guid id, Cancellatio // Complete-internals (callback runs anonymous, no current-team check). /// - /// Consumes the state only together with the nonce of the browser that started the flow. A callback from any - /// other browser — the person an authorize link was forwarded to, or nobody at all — matches no row, so nothing - /// is consumed, exchanged or persisted and the starting browser can still finish. One message for every refusal: - /// telling "wrong browser" from "no such state" would only confirm which states are live. + /// The fast path: consumes the state only together with the nonce of the browser that started the flow. A callback + /// from any other browser — the person an authorize link was forwarded to, a browser that could not keep the cookie, + /// or nobody at all — matches no row, so nothing is consumed, exchanged or persisted here. /// - private async Task ConsumeStateAsync(string state, string? browserNonce, CancellationToken cancellationToken) + private async Task ConsumeBrowserBoundStateAsync(string state, string? browserNonce, CancellationToken cancellationToken) { - if (string.IsNullOrEmpty(browserNonce)) - throw UnusableState(); + if (string.IsNullOrEmpty(browserNonce)) return null; - var row = await _stateStore.ConsumeAsync(state, OAuthBrowserNonce.Hash(browserNonce), cancellationToken).ConfigureAwait(false); + return await _stateStore.ConsumeAsync(state, OAuthBrowserNonce.Hash(browserNonce), cancellationToken).ConfigureAwait(false); + } + + /// + /// A callback with no binding for its state still consumes nothing: it is handed to the SPA's completion page, where + /// the signed-in person who started the connect finishes it (). That page is on + /// an origin the deployment serves its web app from, never wherever the starter asked — the code rides along, and a + /// forwarded authorize link must not deliver someone's code to its starter. A state that is not live is refused with + /// the one message every refusal shares: telling "wrong browser" from "no such state" would only confirm which + /// states are live. + /// + private async Task HandOffToInitiatorAsync(string state, CancellationToken cancellationToken) + { + var pending = await _stateStore.FindLiveAsync(state, cancellationToken).ConfigureAwait(false) ?? throw UnusableState(); - return row ?? throw UnusableState(); + return new CompleteCredentialOAuthResult { ReturnUrl = OAuthReturnUrl.CompletionPageFor(pending.ReturnUrl, _returnOrigins.Value), BrowserNonceCookiePath = ResolveCallbackUri().AbsolutePath }; } + /// + /// Consumes the state only for the person who started the connect. Anyone else — the signed-in victim of a forwarded + /// authorize link — matches no row, so nothing is consumed and the starter can still finish. + /// + private async Task ConsumeInitiatorBoundStateAsync(string state, CancellationToken cancellationToken) => + await _stateStore.ConsumeForInitiatorAsync(state, _currentUser.Id!.Value, cancellationToken).ConfigureAwait(false) ?? throw NotStartedByCaller(); + private static OAuthCallbackException UnusableState() => new("state token missing, expired, already consumed, or not started in this browser"); + private static OAuthCallbackException NotStartedByCaller() => new("state token missing, expired, already consumed, or not started by you"); + private async Task LoadInstanceAsync(Guid id, CancellationToken cancellationToken) => await _db.ProviderInstance.FirstOrDefaultAsync(p => p.Id == id && p.DeletedDate == null, cancellationToken).ConfigureAwait(false) ?? throw new OAuthCallbackException($"provider instance {id} no longer exists"); diff --git a/backend/src/CodeSpace.Core/Services/OAuth/OAuthReturnUrl.cs b/backend/src/CodeSpace.Core/Services/OAuth/OAuthReturnUrl.cs index 28dffaa39..97568549d 100644 --- a/backend/src/CodeSpace.Core/Services/OAuth/OAuthReturnUrl.cs +++ b/backend/src/CodeSpace.Core/Services/OAuth/OAuthReturnUrl.cs @@ -1,13 +1,14 @@ namespace CodeSpace.Core.Services.OAuth; /// -/// Where a finished OAuth connect may send the browser: a path on this site, or an address on its public origin. +/// Where a finished OAuth connect may send the browser: a path on this site, or an address on an origin this deployment +/// serves its web app from (OAuthReturnOriginsSetting). /// /// The return address comes from whoever starts the connect and used to be followed verbatim, so a completed /// connect could hand the browser to any page on the internet — straight after a consent screen that made the whole /// trip look legitimate. A path is accepted only with a single leading '/': two, or '/' then '\', is an address on /// another host to a browser. Control characters are refused anywhere, since browsers strip them and can turn -/// / /host into //host. An absolute address must be http(s) on exactly the public origin and carry +/// / /host into //host. An absolute address must be http(s) on exactly one of the origins and carry /// no user-info. Mirrors ASP.NET Core's IsLocalUrl for the path half. /// public static class OAuthReturnUrl @@ -15,18 +16,45 @@ public static class OAuthReturnUrl /// Where a connect lands when it was given no return address, or one that no longer passes. public const string Fallback = "/"; - public static bool IsAllowed(string returnUrl, string publicBaseUrl) => !HasControlCharacter(returnUrl) && (IsSingleSlashPath(returnUrl) || IsOnOrigin(returnUrl, publicBaseUrl)); + /// + /// The SPA's own route that finishes a connect in the browser. Ours, like /invite/… — the frontend's route, + /// never an operator's setting. + /// + public const string CompletionPage = "/oauth-callback"; - public static string AllowedOrFallback(string? returnUrl, string publicBaseUrl) => returnUrl != null && IsAllowed(returnUrl, publicBaseUrl) ? returnUrl : Fallback; + /// The fragment keys the completion page (frontend/src/routes/oauth-callback.tsx) reads a handed-off code and state from. + public const string HandOffCodeKey = "oauthCode"; + + public const string HandOffStateKey = "oauthState"; + + public static bool IsAllowed(string returnUrl, IReadOnlyCollection origins) => !HasControlCharacter(returnUrl) && (IsSingleSlashPath(returnUrl) || origins.Any(origin => IsOnOrigin(returnUrl, origin))); + + public static string AllowedOrFallback(string? returnUrl, IReadOnlyCollection origins) => returnUrl != null && IsAllowed(returnUrl, origins) ? returnUrl : Fallback; + + /// + /// Where a connect handed to the SPA finishes: the completion page on the origin the return address named, when that + /// origin still passes, or on this site otherwise. The provider's code travels with this redirect, so it can never + /// name an origin outside the set — a forwarded authorize link must not deliver someone's code to its starter's page. + /// + public static string CompletionPageFor(string? returnUrl, IReadOnlyCollection origins) => OriginOf(AllowedOrFallback(returnUrl, origins)) + CompletionPage; + + /// + /// The redirect that hands a connect to the completion page. The code and state ride in the fragment, which a browser + /// never sends to any server: the SPA host's access log, a proxy in front of it and a Referer all see the path alone. + /// + public static string HandOff(string completionPage, string code, string state) => $"{completionPage}#{HandOffCodeKey}={Uri.EscapeDataString(code)}&{HandOffStateKey}={Uri.EscapeDataString(state)}"; private static bool IsSingleSlashPath(string url) => url.StartsWith('/') && (url.Length == 1 || (url[1] != '/' && url[1] != '\\')); - private static bool IsOnOrigin(string url, string publicBaseUrl) => + private static bool IsOnOrigin(string url, string origin) => Uri.TryCreate(url, UriKind.Absolute, out var target) - && Uri.TryCreate(publicBaseUrl, UriKind.Absolute, out var origin) + && Uri.TryCreate(origin, UriKind.Absolute, out var allowed) && (target.Scheme == Uri.UriSchemeHttps || target.Scheme == Uri.UriSchemeHttp) && string.IsNullOrEmpty(target.UserInfo) - && Uri.Compare(target, origin, UriComponents.SchemeAndServer, UriFormat.Unescaped, StringComparison.OrdinalIgnoreCase) == 0; + && Uri.Compare(target, allowed, UriComponents.SchemeAndServer, UriFormat.Unescaped, StringComparison.OrdinalIgnoreCase) == 0; + + /// The scheme and authority of an allowed absolute address; empty for a path, which stays on this site. + private static string OriginOf(string allowedReturnUrl) => IsSingleSlashPath(allowedReturnUrl) ? string.Empty : new Uri(allowedReturnUrl).GetLeftPart(UriPartial.Authority); private static bool HasControlCharacter(string url) => url.Any(char.IsControl); } diff --git a/backend/src/CodeSpace.Core/Services/OAuth/OAuthStateStore.cs b/backend/src/CodeSpace.Core/Services/OAuth/OAuthStateStore.cs index 473aefdbc..54edbcf3d 100644 --- a/backend/src/CodeSpace.Core/Services/OAuth/OAuthStateStore.cs +++ b/backend/src/CodeSpace.Core/Services/OAuth/OAuthStateStore.cs @@ -38,10 +38,31 @@ public async Task CreateAsync(OAuthPendingStateInput input, C { var row = await _db.OAuthPendingState.FirstOrDefaultAsync(r => r.State == state && r.BrowserNonceHash == browserNonceHash, cancellationToken).ConfigureAwait(false); + return Spend(row); + } + + public async Task ConsumeForInitiatorAsync(string state, Guid initiatorUserId, CancellationToken cancellationToken) + { + var row = await _db.OAuthPendingState.FirstOrDefaultAsync(r => r.State == state && r.InitiatorUserId == initiatorUserId, cancellationToken).ConfigureAwait(false); + + return Spend(row); + } + + public async Task FindLiveAsync(string state, CancellationToken cancellationToken) + { + var now = _clock.GetUtcNow(); + + return await _db.OAuthPendingState.AsNoTracking().FirstOrDefaultAsync(r => r.State == state && r.ExpiresDate > now, cancellationToken).ConfigureAwait(false); + } + + /// + /// Always delete on read — one-time-use enforcement. If expired we return null but + /// still drop the row so it doesn't accumulate. + /// + private OAuthPendingState? Spend(OAuthPendingState? row) + { if (row == null) return null; - // Always delete on read — one-time-use enforcement. If expired we return null but - // still drop the row so it doesn't accumulate. _db.OAuthPendingState.Remove(row); if (row.ExpiresDate < _clock.GetUtcNow()) return null; diff --git a/backend/src/CodeSpace.Core/Settings/CorsPolicy/AllowableCorsOriginsSetting.cs b/backend/src/CodeSpace.Core/Settings/CorsPolicy/AllowableCorsOriginsSetting.cs index 0d1fba82d..61b4adb68 100644 --- a/backend/src/CodeSpace.Core/Settings/CorsPolicy/AllowableCorsOriginsSetting.cs +++ b/backend/src/CodeSpace.Core/Settings/CorsPolicy/AllowableCorsOriginsSetting.cs @@ -16,6 +16,12 @@ public class AllowableCorsOriginsSetting : IConfigurationSetting { public const string ConfigurationKey = "Cors:AllowedOrigins"; + /// + /// The Vite dev-server origins, which the CORS policy adds in Development only. The port is pinned by vite.config.ts + /// — keep these in sync. Here rather than beside the policy so an OAuth connect may return to exactly the same ones. + /// + public static readonly string[] DevelopmentOrigins = { "http://localhost:5180", "http://127.0.0.1:5180" }; + public AllowableCorsOriginsSetting(IConfiguration configuration) { Value = Resolve(configuration); diff --git a/backend/src/CodeSpace.Core/Settings/OAuth/OAuthReturnOriginsSetting.cs b/backend/src/CodeSpace.Core/Settings/OAuth/OAuthReturnOriginsSetting.cs new file mode 100644 index 000000000..aed9e8a3b --- /dev/null +++ b/backend/src/CodeSpace.Core/Settings/OAuth/OAuthReturnOriginsSetting.cs @@ -0,0 +1,61 @@ +using CodeSpace.Core.Settings.Application; +using CodeSpace.Core.Settings.CorsPolicy; +using Microsoft.Extensions.Configuration; + +namespace CodeSpace.Core.Settings.OAuth; + +/// +/// The origins an OAuth connect may send the browser back to — every origin configuration says this deployment's web app +/// answers on: App:PublicBaseUrl when it is set, every Cors:AllowedOrigins entry (an SPA that calls the API +/// directly is listed there, or it could not call at all), the origin of OAuth:CallbackUrl (the provider already +/// sends the browser to that host, and in a same-origin deployment it is where the SPA answers), and in Development the +/// dev SPA origins the CORS policy adds. +/// +/// Never throws. does outside Development when its key is unset — rightly: a +/// link minted for someone else must open somewhere real. A connect is different. It returns to the browser that started +/// it, usually by a path that needs no origin at all, so an unset key only narrows what an absolute return address may +/// name. Injecting the throwing setting into the OAuth service made every connect answer 500 — path or not, start or +/// callback — on a host that never set the key, the shipped docker-compose API among them. +/// +public class OAuthReturnOriginsSetting : IConfigurationSetting +{ + public OAuthReturnOriginsSetting(IConfiguration configuration) + { + Value = Resolve(configuration); + } + + public string[] Value { get; set; } + + private static string[] Resolve(IConfiguration configuration) => + PublicBaseUrl(configuration) + .Concat(new AllowableCorsOriginsSetting(configuration).Value) + .Concat(CallbackOrigin(configuration)) + .Concat(IsDevelopment(configuration) ? AllowableCorsOriginsSetting.DevelopmentOrigins : Array.Empty()) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToArray(); + + /// The raw key, read the way normalises it, but with no fallback and no throw. + private static IEnumerable PublicBaseUrl(IConfiguration configuration) + { + var configured = configuration[PublicBaseUrlSetting.ConfigurationKey]; + + return string.IsNullOrWhiteSpace(configured) ? Array.Empty() : new[] { configured.Trim().TrimEnd('/') }; + } + + private static IEnumerable CallbackOrigin(IConfiguration configuration) + { + var callbackUrl = new OAuthCallbackUrlSetting(configuration).Value; + + if (!Uri.TryCreate(callbackUrl, UriKind.Absolute, out var callback) || (callback.Scheme != Uri.UriSchemeHttps && callback.Scheme != Uri.UriSchemeHttp)) return Array.Empty(); + + return new[] { callback.GetLeftPart(UriPartial.Authority) }; + } + + /// The same reading of the environment makes, so the two never disagree about what Development is. + private static bool IsDevelopment(IConfiguration configuration) + { + var environment = configuration["ASPNETCORE_ENVIRONMENT"] ?? configuration["DOTNET_ENVIRONMENT"]; + + return string.IsNullOrWhiteSpace(environment) || string.Equals(environment, "Development", StringComparison.OrdinalIgnoreCase); + } +} diff --git a/backend/src/CodeSpace.Messages/Commands/Credentials/CompleteCredentialOAuthAsInitiatorCommand.cs b/backend/src/CodeSpace.Messages/Commands/Credentials/CompleteCredentialOAuthAsInitiatorCommand.cs new file mode 100644 index 000000000..193f4305a --- /dev/null +++ b/backend/src/CodeSpace.Messages/Commands/Credentials/CompleteCredentialOAuthAsInitiatorCommand.cs @@ -0,0 +1,19 @@ +using CodeSpace.Messages.Authorization; +using CodeSpace.Messages.Mediation; + +namespace CodeSpace.Messages.Commands.Credentials; + +/// +/// Finishes a connect whose callback arrived without the starting browser's binding cookie — the browser could not keep +/// it (the SPA and the API on different sites), or the connect was started on another host than OAuth:CallbackUrl +/// names. The callback hands the code and state to the SPA's completion page, and the signed-in SPA posts them here. +/// +/// Bound to the person instead of the browser: only the caller who started the connect consumes its state. A +/// forwarded authorize link lands on the victim's own session, which is not the starter's, so it completes nothing and +/// consumes nothing — the same refusal the binding cookie gives. +/// +public sealed record CompleteCredentialOAuthAsInitiatorCommand : ICommand, IRequireAuthenticatedUser +{ + public required string Code { get; init; } + public required string State { get; init; } +} diff --git a/backend/src/CodeSpace.Messages/Commands/Credentials/CompleteCredentialOAuthCommand.cs b/backend/src/CodeSpace.Messages/Commands/Credentials/CompleteCredentialOAuthCommand.cs index c6a8c3514..5cd3ba255 100644 --- a/backend/src/CodeSpace.Messages/Commands/Credentials/CompleteCredentialOAuthCommand.cs +++ b/backend/src/CodeSpace.Messages/Commands/Credentials/CompleteCredentialOAuthCommand.cs @@ -7,6 +7,8 @@ namespace CodeSpace.Messages.Commands.Credentials; /// callback endpoint. INTENTIONALLY has no authorization marker — the OAuth provider's /// redirect doesn't carry our JWT. The proof is the state row (one-time use + TTL) TOGETHER /// with , which only the browser that started the flow holds. +/// Without it nothing is consumed: the callback is handed to the SPA, where only the person +/// who started the connect can finish it (). /// Do NOT add IRequireTeamMembership here. /// public sealed record CompleteCredentialOAuthCommand : ICommand @@ -16,15 +18,24 @@ public sealed record CompleteCredentialOAuthCommand : ICommand /// The nonce from the starting browser's binding cookie, read by the controller. Null when the browser sent - /// none — a callback from any browser but the one that started the flow — and then nothing is consumed. + /// none — a browser that could not keep the cookie (the API on another site), or any browser but the one that + /// started the flow — and then nothing is consumed. /// public string? BrowserNonce { get; init; } } public sealed record CompleteCredentialOAuthResult { - public required Guid CredentialId { get; init; } - public required Guid TeamId { get; init; } + /// + /// The credential the connect produced. Null when the callback carried no binding for its state and was handed off + /// instead: nothing was consumed, and is the SPA's completion page, where the signed-in + /// person who started the connect finishes it. + /// + public Guid? CredentialId { get; init; } + + public Guid? TeamId { get; init; } + + /// Where the browser goes next: the connect's own return address once it completed, or the SPA's completion page when it was handed off. public required string ReturnUrl { get; init; } /// The path the spent binding cookie was set on, which clearing it must name — see InitCredentialOAuthResult. diff --git a/backend/src/CodeSpace.Messages/Commands/Credentials/InitCredentialOAuthCommand.cs b/backend/src/CodeSpace.Messages/Commands/Credentials/InitCredentialOAuthCommand.cs index 1b2e0e87b..7a8f810da 100644 --- a/backend/src/CodeSpace.Messages/Commands/Credentials/InitCredentialOAuthCommand.cs +++ b/backend/src/CodeSpace.Messages/Commands/Credentials/InitCredentialOAuthCommand.cs @@ -26,8 +26,10 @@ public sealed record InitCredentialOAuthCommand : ICommand /// Where to redirect the browser after success, with query params appended. Must be a path on this site - /// (a single leading '/') or an address on the deployment's public origin; anything else is refused here, - /// before a flow starts, and checked again at the callback. + /// (a single leading '/') or an address on an origin this deployment serves its web app from + /// (OAuthReturnOriginsSetting); anything else is refused here with 400 invalid_request, before a + /// flow starts, and checked again at the callback. The SPA names its own origin first and asks again with a path + /// on that refusal. /// public string? ReturnUrl { get; init; } diff --git a/backend/tests/CodeSpace.E2ETests/Credentials/OAuthConnectBindingE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Credentials/OAuthConnectBindingE2ETests.cs index bdad4bf0c..471c16633 100644 --- a/backend/tests/CodeSpace.E2ETests/Credentials/OAuthConnectBindingE2ETests.cs +++ b/backend/tests/CodeSpace.E2ETests/Credentials/OAuthConnectBindingE2ETests.cs @@ -24,7 +24,10 @@ namespace CodeSpace.E2ETests.Credentials; /// so the victim's browser then left for the attacker's page. /// /// Now the start of a connect sets an HttpOnly cookie in the starting browser and stores only its hash on the -/// pending state; the callback must present it before anything is consumed, exchanged or persisted. +/// pending state; the callback must present it before anything is consumed, exchanged or persisted. A callback without +/// it — a forwarded link, or a browser that could not keep the cookie because the SPA is on another site — consumes +/// nothing and is handed to the SPA's completion page, which finishes the connect only as the signed-in person who +/// started it. /// [Trait("Category", "E2E")] [Trait("Surface", "Http")] @@ -32,6 +35,10 @@ public sealed class OAuthConnectBindingE2ETests : IClassFixtureA dev SPA origin on another site than the API's http://localhost — on the CORS list in Development. + private const string CrossSiteSpa = "http://127.0.0.1:5180"; private readonly GitWireApiFactory _factory; private readonly TenantHarness _tenants; @@ -94,12 +101,15 @@ public async Task A_callback_from_a_browser_that_did_not_start_the_connect_compl var attacker = await SeedConnectorAsync(); var victim = await SeedConnectorAsync(); var state = await StateOfAsync(await InitAsync(attacker, returnUrl: null)); + var code = $"victim-code-{Guid.NewGuid():N}"; - // The victim's browser: the victim's own session and team, and no binding for this flow. - var callback = await CallbackAsync(state, code: $"victim-code-{Guid.NewGuid():N}", bindingCookie: null, victim); + // The victim's browser: the victim's own session and team, and no binding for this flow. It is handed to the + // SPA, where the victim — signed in, but not the starter — finishes nothing. + var callback = await CallbackAsync(state, code, bindingCookie: null, victim); + var (handedCode, handedState) = HandOffOf(callback, expectedPage: "/oauth-callback"); + var finish = await CompleteAsync(victim, handedCode, handedState); - callback.StatusCode.ShouldBe(HttpStatusCode.BadRequest, await TenantHarness.DescribeAsync(callback)); - callback.Headers.Location.ShouldBeNull("a refused callback redirects nowhere"); + finish.StatusCode.ShouldBe(HttpStatusCode.BadRequest, await TenantHarness.DescribeAsync(finish)); await ShouldHaveCompletedNothingAsync(attacker, state); } @@ -112,11 +122,86 @@ public async Task A_callback_carrying_the_binding_of_a_different_connect_complet var victimCookie = CookieValue(BindingCookieOf(await InitAsync(victim, returnUrl: null))!); var callback = await CallbackAsync(attackerState, code: $"victim-code-{Guid.NewGuid():N}", victimCookie, victim); + var (handedCode, handedState) = HandOffOf(callback, expectedPage: "/oauth-callback"); + SetCookies(callback).ShouldNotContain(c => c.StartsWith($"{BindingCookie}=;", StringComparison.Ordinal), "the cookie this browser holds belongs to its own connect and stays"); + var finish = await CompleteAsync(victim, handedCode, handedState); - callback.StatusCode.ShouldBe(HttpStatusCode.BadRequest, await TenantHarness.DescribeAsync(callback)); + finish.StatusCode.ShouldBe(HttpStatusCode.BadRequest, await TenantHarness.DescribeAsync(finish)); await ShouldHaveCompletedNothingAsync(attacker, attackerState); } + /// + /// The SPA on one site and the API on another (here http://127.0.0.1:5180 against http://localhost): a cross-site + /// fetch response's SameSite=Lax cookie is never stored, so the callback arrives without it. It hands the code to + /// the SPA origin the connect named — in the fragment, which no server or log sees — and the signed-in starter's + /// SPA finishes it. + /// + [Fact] + public async Task A_cross_site_connect_is_finished_by_its_starters_spa() + { + var initiator = await SeedConnectorAsync(); + var init = await InitAsync(initiator, $"{CrossSiteSpa}/oauth-callback"); + var state = await StateOfAsync(init); + var code = $"cross-site-code-{Guid.NewGuid():N}"; + + var callback = await CallbackAsync(state, code, bindingCookie: null, caller: null); + + var (handedCode, handedState) = HandOffOf(callback, expectedPage: $"{CrossSiteSpa}/oauth-callback"); + (handedCode, handedState).ShouldBe((code, state)); + _factory.Wire.ExchangedCodes.ShouldNotContain(code, "handing off exchanges nothing"); + + var finish = await CompleteAsync(initiator, handedCode, handedState); + + finish.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(finish)); + var credentialId = JsonDocument.Parse(await finish.Content.ReadAsStringAsync()).RootElement.GetProperty("credentialId").GetGuid(); + var credential = await _tenants.DbAsync(db => db.Credential.AsNoTracking().SingleAsync(c => c.ProviderInstanceId == initiator.InstanceId)); + + credential.Id.ShouldBe(credentialId); + credential.OwnerUserId.ShouldBe(initiator.UserId); + using var scope = _factory.Services.CreateScope(); + scope.ServiceProvider.GetRequiredService().Decrypt(credential.EncryptedPayload).ShouldContain($"token-for-{code}"); + (await PendingCountAsync(initiator.InstanceId)).ShouldBe(0, "finishing spends the state"); + } + + [Fact] + public async Task A_forwarded_cross_site_connect_is_refused_to_everyone_but_its_starter() + { + // Same team, same SPA, an Owner: the victim's session still is not the starter's. + var attacker = await SeedConnectorAsync(); + var victim = await _tenants.AccountAsync(); + await _tenants.AddMemberAsync(attacker.TeamId, victim, TeamRole.Owner); + var state = await StateOfAsync(await InitAsync(attacker, $"{CrossSiteSpa}/oauth-callback")); + + var callback = await CallbackAsync(state, $"victim-code-{Guid.NewGuid():N}", bindingCookie: null, caller: null); + var (handedCode, handedState) = HandOffOf(callback, expectedPage: $"{CrossSiteSpa}/oauth-callback"); + var finish = await CompleteAsync(new Connector(victim, attacker.TeamId, attacker.InstanceId), handedCode, handedState); + + finish.StatusCode.ShouldBe(HttpStatusCode.BadRequest, await TenantHarness.DescribeAsync(finish)); + await ShouldHaveCompletedNothingAsync(attacker, state); + } + + [Fact] + public async Task A_handed_off_connect_is_not_finished_without_a_session() + { + var initiator = await SeedConnectorAsync(); + var state = await StateOfAsync(await InitAsync(initiator, returnUrl: null)); + var (handedCode, handedState) = HandOffOf(await CallbackAsync(state, $"code-{Guid.NewGuid():N}", bindingCookie: null, caller: null), expectedPage: "/oauth-callback"); + + var finish = await _tenants.Client().SendAsync(_tenants.Request(HttpMethod.Post, CompletePath, userId: null, teamId: null, new { code = handedCode, state = handedState })); + + finish.StatusCode.ShouldBe(HttpStatusCode.Unauthorized, await TenantHarness.DescribeAsync(finish)); + (await PendingCountAsync(initiator.InstanceId)).ShouldBe(1); + } + + [Fact] + public async Task A_callback_for_a_state_that_is_not_live_is_refused_and_hands_off_nothing() + { + var callback = await CallbackAsync("no-such-state", $"code-{Guid.NewGuid():N}", bindingCookie: null, caller: null); + + callback.StatusCode.ShouldBe(HttpStatusCode.BadRequest, await TenantHarness.DescribeAsync(callback)); + callback.Headers.Location.ShouldBeNull("a refused callback redirects nowhere"); + } + [Fact] public async Task The_starting_browser_completes_the_connect_and_returns_to_the_public_origin() { @@ -152,6 +237,7 @@ public async Task A_return_address_off_this_site_is_refused_before_a_connect_sta var init = await InitAsync(initiator, returnUrl); init.StatusCode.ShouldBe(HttpStatusCode.BadRequest, await TenantHarness.DescribeAsync(init)); + JsonDocument.Parse(await init.Content.ReadAsStringAsync()).RootElement.GetProperty("code").GetString().ShouldBe("invalid_request", "the SPA asks again with a path on exactly this answer (frontend/src/hooks/use-oauth-flow.ts)"); (await PendingCountAsync(initiator.InstanceId)).ShouldBe(0, "a refused start leaves no pending connect behind"); } @@ -199,6 +285,27 @@ private async Task CallbackAsync(string state, string code, return await _tenants.Client(origin).SendAsync(request); } + private async Task CompleteAsync(Connector caller, string code, string state) => + await _tenants.Client().SendAsync(_tenants.Request(HttpMethod.Post, CompletePath, caller.UserId, caller.TeamId, new { code, state })); + + /// + /// A hand-off is a redirect to the SPA's completion page with the code and state in the fragment — never the query, + /// which the SPA host's access log would keep — and it spends nothing, so no binding cookie is cleared. + /// + private static (string Code, string State) HandOffOf(HttpResponseMessage callback, string expectedPage) + { + callback.StatusCode.ShouldBe(HttpStatusCode.Redirect); + var location = callback.Headers.Location.ShouldNotBeNull().OriginalString; + + location.ShouldStartWith($"{expectedPage}#oauthCode="); + location.ShouldNotContain("?", customMessage: "nothing of the hand-off rides in a query string"); + SetCookies(callback).ShouldNotContain(c => c.StartsWith($"{BindingCookie}=;", StringComparison.Ordinal)); + + var fragment = location[(location.IndexOf('#') + 1)..].Split('&').Select(p => p.Split('=', 2)).ToDictionary(p => p[0], p => Uri.UnescapeDataString(p[1])); + + return (fragment["oauthCode"], fragment["oauthState"]); + } + private async Task ShouldHaveCompletedNothingAsync(Connector attacker, string state) { _factory.Wire.ExchangedCodes.ShouldNotContain(c => c.StartsWith("victim-code-"), "the victim's code must never reach the token endpoint"); diff --git a/backend/tests/CodeSpace.E2ETests/Credentials/OAuthConnectWithoutPublicBaseUrlE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Credentials/OAuthConnectWithoutPublicBaseUrlE2ETests.cs new file mode 100644 index 000000000..0f3e30124 --- /dev/null +++ b/backend/tests/CodeSpace.E2ETests/Credentials/OAuthConnectWithoutPublicBaseUrlE2ETests.cs @@ -0,0 +1,135 @@ +using System.Net; +using System.Text.Json; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Credentials; +using CodeSpace.E2ETests.Infrastructure; +using CodeSpace.Messages.Constants; +using CodeSpace.Messages.Enums; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using Shouldly; + +namespace CodeSpace.E2ETests.Credentials; + +/// +/// 🟢 High fidelity (real HTTP pipeline in the Production environment, Postgres; only the provider's token endpoint is a +/// stub): a host outside Development that never set App:PublicBaseUrl. #2104 left every OAuth start and every +/// callback on such a host answering 500, because the OAuth service injected the setting that throws when the key is +/// unset. A connect needs no origin to return to a path, and may name any origin the deployment serves its SPA from; +/// only the invitation link, which someone else must be able to open, still refuses to be minted. +/// +/// Diagnose a red here by starting the API with ASPNETCORE_ENVIRONMENT=Production and no +/// App__PublicBaseUrl, then POST /api/credentials/oauth/init: a 500 with "App:PublicBaseUrl is required" +/// in the log means something in the OAuth path resolves PublicBaseUrlSetting again. +/// +[Trait("Category", "E2E")] +[Trait("Surface", "Http")] +public sealed class OAuthConnectWithoutPublicBaseUrlE2ETests : IClassFixture +{ + private const string BindingCookie = "codespace_oauth_nonce"; + + private readonly ProductionWithoutPublicBaseUrlGitWireApiFactory _factory; + private readonly TenantHarness _tenants; + + public OAuthConnectWithoutPublicBaseUrlE2ETests(ProductionWithoutPublicBaseUrlGitWireApiFactory factory) + { + _factory = factory; + _tenants = new TenantHarness(factory); + } + + [Theory] + [InlineData("/oauth-callback", "/oauth-callback?oauthCredentialId=")] + [InlineData(null, "/?oauthCredentialId=")] + public async Task A_connect_returning_to_a_path_starts_and_completes(string? returnUrl, string landing) + { + var (userId, teamId, instanceId) = await SeedConnectorAsync(); + + var init = await InitAsync(userId, teamId, instanceId, returnUrl); + + init.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(init)); + var callback = await CallbackAsync(await StateOfAsync(init), $"code-{Guid.NewGuid():N}", BindingCookieValueOf(init)); + + callback.StatusCode.ShouldBe(HttpStatusCode.Redirect, await TenantHarness.DescribeAsync(callback)); + callback.Headers.Location!.OriginalString.ShouldStartWith(landing); + } + + [Fact] + public async Task A_connect_returning_to_a_cors_origin_starts() + { + var (userId, teamId, instanceId) = await SeedConnectorAsync(); + + var init = await InitAsync(userId, teamId, instanceId, $"{ProductionWithoutPublicBaseUrlGitWireApiFactory.CorsOrigin}/oauth-callback"); + + init.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(init)); + } + + [Fact] + public async Task A_connect_returning_anywhere_else_is_a_400_naming_both_settings() + { + var (userId, teamId, instanceId) = await SeedConnectorAsync(); + + var init = await InitAsync(userId, teamId, instanceId, "https://elsewhere.example/oauth-callback"); + + init.StatusCode.ShouldBe(HttpStatusCode.BadRequest, await TenantHarness.DescribeAsync(init)); + var body = JsonDocument.Parse(await init.Content.ReadAsStringAsync()).RootElement; + var message = body.GetProperty("message").GetString(); + message.ShouldNotBeNull().ShouldContain("App:PublicBaseUrl"); + message.ShouldContain("Cors:AllowedOrigins"); + body.GetProperty("code").GetString().ShouldBe("invalid_request", "the SPA asks again with a path on exactly this answer (frontend/src/hooks/use-oauth-flow.ts), so a page on an origin the API does not list can still connect"); + (await _tenants.DbAsync(db => db.OAuthPendingState.AsNoTracking().CountAsync(s => s.ProviderInstanceId == instanceId))).ShouldBe(0, "asking again must not leave the refused start behind"); + } + + [Fact] + public async Task A_callback_for_an_unknown_state_is_a_400_not_a_500() + { + var callback = await CallbackAsync("no-such-state", "code", bindingCookie: null); + + callback.StatusCode.ShouldBe(HttpStatusCode.BadRequest, await TenantHarness.DescribeAsync(callback)); + } + + [Fact] + public async Task An_invitation_still_fails_fast_without_the_key() + { + // Unchanged on purpose: an invite is a link handed to someone else, and the built-in default is the dev SPA. + var (userId, teamId, _) = await SeedConnectorAsync(); + + var invite = await _tenants.SendAsync(HttpMethod.Post, "/api/teams/invitations", userId, teamId, new { email = $"invitee-{Guid.NewGuid():N}@tenant.test", role = "Member" }); + + invite.StatusCode.ShouldBe(HttpStatusCode.InternalServerError, await TenantHarness.DescribeAsync(invite)); + } + + private async Task<(Guid UserId, Guid TeamId, Guid InstanceId)> SeedConnectorAsync() + { + var admin = await _tenants.AccountAsync(); + var teamId = await _tenants.TeamAsync((admin, TeamRole.Owner)); + + using var scope = _factory.Services.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + var encryptor = scope.ServiceProvider.GetRequiredService(); + var instance = new ProviderInstance { Id = Guid.NewGuid(), TeamId = teamId, Provider = ProviderKind.Git, DisplayName = "OAuth app", BaseUrl = $"https://git.stub.test/{Guid.NewGuid():N}", OauthClientId = "client", OauthClientSecretEnc = encryptor.Encrypt("secret"), CreatedBy = SystemUsers.SeederId, LastModifiedBy = SystemUsers.SeederId }; + + db.ProviderInstance.Add(instance); + await db.SaveChangesAsync(); + + return (admin, teamId, instance.Id); + } + + private async Task InitAsync(Guid userId, Guid teamId, Guid instanceId, string? returnUrl) => + await _tenants.SendAsync(HttpMethod.Post, "/api/credentials/oauth/init", userId, teamId, new { providerInstanceId = instanceId, displayName = "connect", returnUrl }); + + private async Task CallbackAsync(string state, string code, string? bindingCookie) + { + var request = _tenants.Request(HttpMethod.Get, $"/api/credentials/oauth/callback?code={Uri.EscapeDataString(code)}&state={Uri.EscapeDataString(state)}", userId: null, teamId: null); + + if (bindingCookie != null) request.Headers.Add("Cookie", $"{BindingCookie}={bindingCookie}"); + + return await _tenants.Client().SendAsync(request); + } + + private static async Task StateOfAsync(HttpResponseMessage init) => + JsonDocument.Parse(await init.Content.ReadAsStringAsync()).RootElement.GetProperty("state").GetString()!; + + private static string BindingCookieValueOf(HttpResponseMessage init) => + init.Headers.GetValues("Set-Cookie").Single(c => c.StartsWith($"{BindingCookie}=", StringComparison.Ordinal)).Split(';')[0][(BindingCookie.Length + 1)..]; +} diff --git a/backend/tests/CodeSpace.E2ETests/Infrastructure/GitWireApiFactory.cs b/backend/tests/CodeSpace.E2ETests/Infrastructure/GitWireApiFactory.cs index 284cc2934..7436433a2 100644 --- a/backend/tests/CodeSpace.E2ETests/Infrastructure/GitWireApiFactory.cs +++ b/backend/tests/CodeSpace.E2ETests/Infrastructure/GitWireApiFactory.cs @@ -71,3 +71,26 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) builder.ConfigureAppConfiguration((_, cfg) => cfg.AddInMemoryCollection(new Dictionary { ["ForwardedHeaders_Enabled"] = "true" })); } } + +/// +/// The same host as an operator runs it outside Development without App:PublicBaseUrl — the shipped +/// docker-compose API: ASPNETCORE_ENVIRONMENT=Production (both the host environment and the configuration key the +/// settings read), the key explicitly blank, and one SPA origin on the CORS list as a direct-mode deployment has. +/// +public sealed class ProductionWithoutPublicBaseUrlGitWireApiFactory : GitWireApiFactory +{ + public const string CorsOrigin = "https://spa.example"; + + protected override void ConfigureWebHost(IWebHostBuilder builder) + { + base.ConfigureWebHost(builder); + + builder.UseEnvironment("Production"); + builder.ConfigureAppConfiguration((_, cfg) => cfg.AddInMemoryCollection(new Dictionary + { + ["ASPNETCORE_ENVIRONMENT"] = "Production", + ["App:PublicBaseUrl"] = "", + ["Cors:AllowedOrigins"] = CorsOrigin, + })); + } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/Credentials/CredentialOwnerReachTests.cs b/backend/tests/CodeSpace.IntegrationTests/Credentials/CredentialOwnerReachTests.cs index 272aea122..397b1948d 100644 --- a/backend/tests/CodeSpace.IntegrationTests/Credentials/CredentialOwnerReachTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/Credentials/CredentialOwnerReachTests.cs @@ -160,7 +160,7 @@ private async Task ConnectOAuthAsync(World world, Guid actorId, Guid owner using var callback = OAuthScope(userId: null, teamId: null); - return (await callback.Resolve().Send(new CompleteCredentialOAuthCommand { State = init.State, Code = "code", BrowserNonce = init.BrowserNonce })).CredentialId; + return (await callback.Resolve().Send(new CompleteCredentialOAuthCommand { State = init.State, Code = "code", BrowserNonce = init.BrowserNonce })).CredentialId.ShouldNotBeNull(); } private async Task LinkOwnTokenAsync(World world, Guid userId) diff --git a/backend/tests/CodeSpace.IntegrationTests/Credentials/DepartedMemberCredentialTests.cs b/backend/tests/CodeSpace.IntegrationTests/Credentials/DepartedMemberCredentialTests.cs index 5b0230485..86aeb9e2f 100644 --- a/backend/tests/CodeSpace.IntegrationTests/Credentials/DepartedMemberCredentialTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/Credentials/DepartedMemberCredentialTests.cs @@ -445,12 +445,12 @@ public async Task An_instance_admin_outside_the_team_connects_their_own_account_ using (var callback = OAuthScope(oauth, userId: null, teamId: null)) completed = await callback.Resolve().Send(new CompleteCredentialOAuthCommand { State = init.State, Code = "code", BrowserNonce = init.BrowserNonce }); - var token = await LoadCredentialAsync(completed.CredentialId); + var token = await LoadCredentialAsync(completed.CredentialId.ShouldNotBeNull("the starting browser's callback completes the connect")); token.OwnerUserId.ShouldBe(adminId, "the connect is the instance Admin's own"); token.Status.ShouldBe(CredentialStatus.Active, "an instance Admin has standing in the team, so the callback keeps their token"); var identity = await ResolveAsync(adminId, world.InstanceA); identity.ShouldNotBeNull("the connect links the instance Admin's act-as identity in the team"); - identity.CredentialId.ShouldBe(completed.CredentialId, "through the token they just connected"); + identity.CredentialId.ShouldBe(token.Id, "through the token they just connected"); } [Fact] diff --git a/backend/tests/CodeSpace.IntegrationTests/OAuth/OAuthConnectWithoutPublicBaseUrlTests.cs b/backend/tests/CodeSpace.IntegrationTests/OAuth/OAuthConnectWithoutPublicBaseUrlTests.cs new file mode 100644 index 000000000..3ecf35426 --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/OAuth/OAuthConnectWithoutPublicBaseUrlTests.cs @@ -0,0 +1,171 @@ +using Autofac; +using CodeSpace.Core.Failures; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Credentials; +using CodeSpace.Core.Services.OAuth; +using CodeSpace.Core.Settings.Application; +using CodeSpace.Core.Settings.OAuth; +using CodeSpace.IntegrationTests.Infrastructure; +using CodeSpace.Messages.Commands.Credentials; +using CodeSpace.Messages.Commands.Invitations; +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Failures; +using MediatR; +using Microsoft.Extensions.Configuration; +using Shouldly; + +namespace CodeSpace.IntegrationTests.OAuth; + +/// +/// 🟢 Integration (real container, real handlers, Postgres): a host outside Development that never set +/// App:PublicBaseUrl — the shipped docker-compose API is one. #2104 made the OAuth service inject +/// PublicBaseUrlSetting, whose constructor throws on such a host, so every connect answered 500 whatever return +/// address it asked for, and so did every callback. OAuth now reads the non-throwing return-origin set; the invitation +/// link, which must open somewhere real for someone else, still refuses exactly as before. +/// +/// The settings are built from a Production configuration the way the container builds them — the OAuth set +/// through its own constructor, PublicBaseUrlSetting as a registration that throws on resolve — so a service that +/// went back to the throwing setting fails here. +/// +[Collection(PostgresCollection.Name)] +[Trait("Category", "Integration")] +public class OAuthConnectWithoutPublicBaseUrlTests +{ + private const string CorsOrigin = "https://spa.example"; + + private static readonly IConfiguration Production = new ConfigurationBuilder().AddInMemoryCollection(new Dictionary + { + ["ASPNETCORE_ENVIRONMENT"] = "Production", + ["Cors:AllowedOrigins"] = CorsOrigin, + ["OAuth:CallbackUrl"] = "https://api.example/api/credentials/oauth/callback", + }).Build(); + + private readonly PostgresFixture _fixture; + + public OAuthConnectWithoutPublicBaseUrlTests(PostgresFixture fixture) { _fixture = fixture; } + + [Fact] + public void The_host_under_test_cannot_mint_a_link() + { + using var scope = ProductionScope(caller: null, stub: null); + + Should.Throw(() => scope.Resolve()).GetBaseException().Message.ShouldContain(PublicBaseUrlSetting.ConfigurationKey); + } + + [Theory] + [InlineData("/oauth-callback", "/oauth-callback")] + [InlineData(null, "/")] + public async Task A_connect_returning_to_a_path_starts_and_finishes(string? returnUrl, string landing) + { + var connector = await SeedConnectorAsync().ConfigureAwait(false); + var stub = new StubOAuthClient(ProviderKind.GitLab, new OAuthTokenResponse { AccessToken = "at" }); + + var init = await InitAsync(connector, stub, returnUrl).ConfigureAwait(false); + + using var callback = ProductionScope(caller: null, stub); + var completed = await callback.Resolve().Send(new CompleteCredentialOAuthCommand { State = init.State, Code = "code", BrowserNonce = init.BrowserNonce }).ConfigureAwait(false); + + completed.CredentialId.ShouldNotBeNull(); + completed.ReturnUrl.ShouldBe(landing); + } + + [Fact] + public async Task A_connect_returning_to_a_cors_origin_starts_and_hands_off_there() + { + // Direct mode: the SPA calls the API from an origin on the CORS list, so it may name that origin to come back to. + var connector = await SeedConnectorAsync().ConfigureAwait(false); + var stub = new StubOAuthClient(ProviderKind.GitLab, new OAuthTokenResponse { AccessToken = "at" }); + + var init = await InitAsync(connector, stub, $"{CorsOrigin}/oauth-callback").ConfigureAwait(false); + + using var callback = ProductionScope(caller: null, stub); + var handedOff = await callback.Resolve().Send(new CompleteCredentialOAuthCommand { State = init.State, Code = "code" }).ConfigureAwait(false); + + handedOff.ReturnUrl.ShouldBe($"{CorsOrigin}/oauth-callback"); + } + + [Fact] + public async Task A_connect_returning_anywhere_else_is_refused_by_naming_both_settings() + { + var connector = await SeedConnectorAsync().ConfigureAwait(false); + var stub = new StubOAuthClient(ProviderKind.GitLab, new OAuthTokenResponse { AccessToken = "at" }); + + var refused = await Should.ThrowAsync(async () => await InitAsync(connector, stub, "https://elsewhere.example/oauth-callback").ConfigureAwait(false)).ConfigureAwait(false); + + refused.Message.ShouldContain("App:PublicBaseUrl"); + refused.Message.ShouldContain("Cors:AllowedOrigins"); + FailureClassifier.Classify(refused).Kind.ShouldBe(FailureKind.Invalid, "a 400 the operator can act on, not a 500"); + } + + [Fact] + public async Task A_callback_for_an_unknown_state_is_a_refusal_not_a_failure() + { + using var callback = ProductionScope(caller: null, new StubOAuthClient(ProviderKind.GitLab, new OAuthTokenResponse { AccessToken = "at" })); + + var refused = await Should.ThrowAsync(async () => await callback.Resolve().Send(new CompleteCredentialOAuthCommand { State = "no-such-state", Code = "code" }).ConfigureAwait(false)).ConfigureAwait(false); + + FailureClassifier.Classify(refused).Kind.ShouldBe(FailureKind.Invalid); + } + + [Fact] + public async Task An_invitation_still_refuses_to_mint_a_link_nobody_could_open() + { + var connector = await SeedConnectorAsync().ConfigureAwait(false); + using var scope = ProductionScope(connector, stub: null); + + var refused = await Should.ThrowAsync(async () => await scope.Resolve().Send(new CreateTeamInvitationCommand { Email = $"invitee-{Guid.NewGuid():N}@x", Role = TeamRole.Member }).ConfigureAwait(false)).ConfigureAwait(false); + + refused.GetBaseException().ShouldBeOfType().Message.ShouldContain(PublicBaseUrlSetting.ConfigurationKey); + } + + // ── Plumbing ──────────────────────────────────────────────────────────────────── + + private sealed record Connector(Guid UserId, Guid TeamId, Guid InstanceId); + + private ILifetimeScope ProductionScope(Connector? caller, StubOAuthClient? stub) => _fixture.BeginScope(b => + { + b.Register(_ => new OAuthReturnOriginsSetting(Production)).AsSelf().SingleInstance(); + b.Register(_ => new OAuthCallbackUrlSetting(Production)).AsSelf().SingleInstance(); + b.Register(_ => new PublicBaseUrlSetting(Production)).AsSelf(); + + if (stub != null) b.RegisterInstance(new SingleProviderRegistry(stub)).SingleInstance(); + + if (caller == null) return; + + b.RegisterInstance(new TestCurrentUser(caller.UserId, "tester")).As().SingleInstance(); + b.RegisterInstance(new TestCurrentTeam(caller.TeamId)).As().SingleInstance(); + }); + + private async Task InitAsync(Connector connector, StubOAuthClient stub, string? returnUrl) + { + using var scope = ProductionScope(connector, stub); + + return await scope.Resolve().Send(new InitCredentialOAuthCommand { ProviderInstanceId = connector.InstanceId, DisplayName = "connect", ReturnUrl = returnUrl }).ConfigureAwait(false); + } + + private async Task SeedConnectorAsync() + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + var suffix = Guid.NewGuid().ToString("N")[..8]; + var user = new User { Id = Guid.NewGuid(), Email = $"u-{suffix}@x", Name = $"owner-{suffix}" }; + var team = new Team { Id = Guid.NewGuid(), Slug = $"t-{suffix}", Name = "Team" }; + var instance = new ProviderInstance { Id = Guid.NewGuid(), TeamId = team.Id, Provider = ProviderKind.GitLab, DisplayName = "OAuth-capable", BaseUrl = $"https://gitlab-{suffix}.local", OauthClientId = "test-client", OauthClientSecretEnc = scope.Resolve().Encrypt("test-secret") }; + + db.User.Add(user); + db.Team.Add(team); + db.TeamMembership.Add(new TeamMembership { Id = Guid.NewGuid(), TeamId = team.Id, UserId = user.Id, Role = TeamRole.Owner }); + db.ProviderInstance.Add(instance); + await db.SaveChangesAsync().ConfigureAwait(false); + + return new Connector(user.Id, team.Id, instance.Id); + } + + private sealed class SingleProviderRegistry : IOAuthClientRegistry + { + private readonly IOAuthClient _client; + public SingleProviderRegistry(IOAuthClient client) { _client = client; } + public IOAuthClient Get(ProviderKind kind) => _client; + } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/OAuth/OAuthFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/OAuth/OAuthFlowTests.cs index 4ae2fa2bd..397a6f96c 100644 --- a/backend/tests/CodeSpace.IntegrationTests/OAuth/OAuthFlowTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/OAuth/OAuthFlowTests.cs @@ -114,7 +114,7 @@ public async Task Complete_consumes_state_and_persists_credential() } completeResult.TeamId.ShouldBe(teamId); - completeResult.CredentialId.ShouldNotBe(Guid.Empty); + var credentialId = completeResult.CredentialId.ShouldNotBeNull(); completeResult.ReturnUrl.ShouldBe("/"); // Verify the Credential row was persisted with the right shape, and that the @@ -122,7 +122,7 @@ public async Task Complete_consumes_state_and_persists_credential() using var verify = _fixture.BeginScope(); var db = verify.Resolve(); - var cred = await db.Credential.AsNoTracking().SingleAsync(c => c.Id == completeResult.CredentialId).ConfigureAwait(false); + var cred = await db.Credential.AsNoTracking().SingleAsync(c => c.Id == credentialId).ConfigureAwait(false); cred.TeamId.ShouldBe(teamId); cred.ProviderInstanceId.ShouldBe(instanceId); cred.OwnerUserId.ShouldBe(userId); @@ -200,21 +200,22 @@ public async Task Complete_twice_with_same_state_rejects_replay() [Theory] [InlineData(null)] [InlineData("a-nonce-this-browser-never-received")] - public async Task Complete_from_a_browser_that_did_not_start_the_flow_consumes_nothing(string? browserNonce) + public async Task Complete_from_a_browser_that_did_not_start_the_flow_consumes_nothing_and_is_handed_to_the_spa(string? browserNonce) { // The capture this pins: an admin of one team starts a connect on their own OAuth app and forwards the // authorize link; whoever's browser finishes it used to hand that team their provider token. Without the - // starting browser's nonce, nothing is consumed, exchanged or persisted — and the starter can still finish. + // starting browser's nonce, nothing is consumed, exchanged or persisted — the callback is only pointed at the + // SPA's completion page, where the starter alone can finish it — and the starter can still finish. var (userId, teamId, instanceId) = await SeedOAuthCapableInstanceAsync(ProviderKind.GitLab).ConfigureAwait(false); var stub = new StubOAuthClient(ProviderKind.GitLab, BuildToken("victim-token")); var initResult = await InitAsync(userId, teamId, instanceId, stub, returnUrl: null).ConfigureAwait(false); using (var scope = _fixture.BeginScope(b => b.RegisterInstance(new SingleProviderRegistry(stub)).SingleInstance())) { - var act = async () => await scope.Resolve().Send(new CompleteCredentialOAuthCommand { Code = "victim-code", State = initResult.State, BrowserNonce = browserNonce }).ConfigureAwait(false); + var handedOff = await scope.Resolve().Send(new CompleteCredentialOAuthCommand { Code = "victim-code", State = initResult.State, BrowserNonce = browserNonce }).ConfigureAwait(false); - var refused = await act.ShouldThrowAsync().ConfigureAwait(false); - refused.Reason.ShouldContain("not started in this browser"); + handedOff.CredentialId.ShouldBeNull("a callback without the starting browser's nonce completes nothing"); + handedOff.ReturnUrl.ShouldBe(OAuthReturnUrl.CompletionPage, "handed to the SPA's completion page on this site"); } stub.LastExchange.ShouldBeNull("the code must never reach the token endpoint"); @@ -343,6 +344,167 @@ public async Task Reconnecting_via_oauth_repoints_the_single_identity_and_keeps_ first.DeletedDate.ShouldBeNull(); } + // ── A callback without the binding cookie: handed to the SPA, finished by the starter signed in ───────── + + [Fact] + public async Task A_handed_off_connect_is_finished_by_the_person_who_started_it() + { + // The SPA and the API on different sites: the browser never keeps the binding cookie, so every callback arrives + // without it. The SPA's completion page then finishes the connect as its signed-in starter. + var (userId, teamId, instanceId) = await SeedOAuthCapableInstanceAsync(ProviderKind.GitLab).ConfigureAwait(false); + var stub = new StubOAuthClient(ProviderKind.GitLab, BuildToken("access-from-provider", "refresh-from-provider")); + var init = await InitAsync(userId, teamId, instanceId, stub, returnUrl: "http://127.0.0.1:5180/oauth-callback").ConfigureAwait(false); + + var handedOff = await CallbackAsync(init.State, "code-from-provider", browserNonce: null, stub).ConfigureAwait(false); + handedOff.ReturnUrl.ShouldBe("http://127.0.0.1:5180/oauth-callback", "back to the SPA origin that started it"); + + var completed = await CompleteAsInitiatorAsync(userId, init.State, "code-from-provider", stub).ConfigureAwait(false); + + var credentialId = completed.CredentialId.ShouldNotBeNull(); + completed.TeamId.ShouldBe(teamId); + stub.LastExchange.ShouldNotBeNull().Code.ShouldBe("code-from-provider"); + + using var verify = _fixture.BeginScope(); + var db = verify.Resolve(); + var cred = await db.Credential.AsNoTracking().SingleAsync(c => c.Id == credentialId).ConfigureAwait(false); + + cred.TeamId.ShouldBe(teamId); + cred.OwnerUserId.ShouldBe(userId); + cred.CreatedBy.ShouldBe(userId); + ((OAuthPayload)verify.Resolve().Deserialize(AuthType.OAuth, verify.Resolve().Decrypt(cred.EncryptedPayload))).AccessToken.ShouldBe("access-from-provider"); + (await db.OAuthPendingState.AsNoTracking().AnyAsync(s => s.State == init.State).ConfigureAwait(false)).ShouldBeFalse("finishing spends the state"); + } + + [Fact] + public async Task Nobody_but_the_starter_finishes_a_handed_off_connect_and_the_starter_still_can() + { + // The forwarded-link capture again, one step further: the victim's browser was handed to the SPA, and the + // victim is signed in there — in the very team, as an Owner. Their session is not the starter's, so nothing is + // consumed, exchanged or persisted, and the connect stays the starter's to finish. + var (userId, teamId, instanceId) = await SeedOAuthCapableInstanceAsync(ProviderKind.GitLab).ConfigureAwait(false); + var victimId = await SeedMemberAsync(teamId, TeamRole.Owner).ConfigureAwait(false); + var stub = new StubOAuthClient(ProviderKind.GitLab, BuildToken("victim-token")); + var init = await InitAsync(userId, teamId, instanceId, stub, returnUrl: null).ConfigureAwait(false); + + var act = async () => await CompleteAsInitiatorAsync(victimId, init.State, "victim-code", stub).ConfigureAwait(false); + + var refused = await act.ShouldThrowAsync().ConfigureAwait(false); + refused.Reason.ShouldContain("not started by you"); + stub.LastExchange.ShouldBeNull("the victim's code must never reach the token endpoint"); + + using (var verify = _fixture.BeginScope()) + { + var db = verify.Resolve(); + (await db.Credential.AsNoTracking().AnyAsync(c => c.ProviderInstanceId == instanceId).ConfigureAwait(false)).ShouldBeFalse(); + (await db.OAuthPendingState.AsNoTracking().AnyAsync(s => s.State == init.State).ConfigureAwait(false)).ShouldBeTrue("a refused finish consumes nothing"); + } + + (await CompleteAsInitiatorAsync(userId, init.State, "own-code", stub).ConfigureAwait(false)).CredentialId.ShouldNotBeNull("the starter still finishes it"); + } + + [Fact] + public async Task A_handed_off_connect_is_not_finished_without_a_session() + { + var (userId, teamId, instanceId) = await SeedOAuthCapableInstanceAsync(ProviderKind.GitLab).ConfigureAwait(false); + var stub = new StubOAuthClient(ProviderKind.GitLab, BuildToken("at")); + var init = await InitAsync(userId, teamId, instanceId, stub, returnUrl: null).ConfigureAwait(false); + + using var anonymous = _fixture.BeginScope(b => + { + b.RegisterInstance(new Infrastructure.TestCurrentUser(null)).As().SingleInstance(); + b.RegisterInstance(new SingleProviderRegistry(stub)).SingleInstance(); + }); + + var act = async () => await anonymous.Resolve().Send(new CompleteCredentialOAuthAsInitiatorCommand { State = init.State, Code = "code" }).ConfigureAwait(false); + + await act.ShouldThrowAsync().ConfigureAwait(false); + stub.LastExchange.ShouldBeNull(); + } + + [Fact] + public async Task Either_proof_spends_the_state_for_both() + { + // One connect, one credential: finishing it through the SPA refuses the cookie, and the cookie refuses the SPA. + var (userId, teamId, instanceId) = await SeedOAuthCapableInstanceAsync(ProviderKind.GitLab).ConfigureAwait(false); + var stub = new StubOAuthClient(ProviderKind.GitLab, BuildToken("at")); + + var viaSpa = await InitAsync(userId, teamId, instanceId, stub, returnUrl: null).ConfigureAwait(false); + await CompleteAsInitiatorAsync(userId, viaSpa.State, "c1", stub).ConfigureAwait(false); + await Should.ThrowAsync(async () => await CallbackAsync(viaSpa.State, "c2", viaSpa.BrowserNonce, stub).ConfigureAwait(false)).ConfigureAwait(false); + + var viaCookie = await InitAsync(userId, teamId, instanceId, stub, returnUrl: null).ConfigureAwait(false); + (await CallbackAsync(viaCookie.State, "c3", viaCookie.BrowserNonce, stub).ConfigureAwait(false)).CredentialId.ShouldNotBeNull(); + await Should.ThrowAsync(async () => await CompleteAsInitiatorAsync(userId, viaCookie.State, "c4", stub).ConfigureAwait(false)).ConfigureAwait(false); + } + + [Theory] + [InlineData("http://127.0.0.1:5180/settings/credentials", "http://127.0.0.1:5180/oauth-callback")] + [InlineData("/settings/credentials", "/oauth-callback")] + public async Task A_handed_off_connect_lands_on_the_completion_page_of_the_origin_it_named(string returnUrl, string completionPage) + { + var (userId, teamId, instanceId) = await SeedOAuthCapableInstanceAsync(ProviderKind.GitLab).ConfigureAwait(false); + var stub = new StubOAuthClient(ProviderKind.GitLab, BuildToken("at")); + var init = await InitAsync(userId, teamId, instanceId, stub, returnUrl).ConfigureAwait(false); + + (await CallbackAsync(init.State, "c", browserNonce: null, stub).ConfigureAwait(false)).ReturnUrl.ShouldBe(completionPage); + } + + [Fact] + public async Task An_expired_connect_is_refused_rather_than_handed_off() + { + var (userId, teamId, instanceId) = await SeedOAuthCapableInstanceAsync(ProviderKind.GitLab).ConfigureAwait(false); + var stub = new StubOAuthClient(ProviderKind.GitLab, BuildToken("at")); + var state = $"expired-{Guid.NewGuid():N}"; + + await InsertStateAsAnOlderBuildAsync(state, instanceId, teamId, userId, OAuthBrowserNonce.Hash(OAuthBrowserNonce.Mint()), DateTimeOffset.UtcNow.AddMinutes(-1)).ConfigureAwait(false); + + var refused = await Should.ThrowAsync(async () => await CallbackAsync(state, "c", browserNonce: null, stub).ConfigureAwait(false)).ConfigureAwait(false); + refused.Reason.ShouldContain("expired"); + + // Not left for the deployment-wide expiry sweep, whose tests share this database. + using var cleanup = _fixture.BeginScope(); + await cleanup.Resolve().OAuthPendingState.Where(s => s.State == state).ExecuteDeleteAsync().ConfigureAwait(false); + } + + // ── Mixed versions: rows either build writes are finished by the other ────────────────────────────────── + + /// + /// A rolling deploy puts the start of a connect and its callback on different builds. A row written by the previous + /// build (with the nonce hash, #2104) or by one before migration 0246 (without it) — inserted here with exactly the + /// columns that build writes — is handed off by this one and finished by its starter. This build adds no column, so + /// nothing a previous build leaves out can make a row unreadable here. + /// + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task A_connect_an_older_build_started_is_finished_here_by_its_starter(bool olderBuildBindsTheBrowser) + { + var (userId, teamId, instanceId) = await SeedOAuthCapableInstanceAsync(ProviderKind.GitLab).ConfigureAwait(false); + var stub = new StubOAuthClient(ProviderKind.GitLab, BuildToken("at")); + var state = $"older-build-{Guid.NewGuid():N}"; + + await InsertStateAsAnOlderBuildAsync(state, instanceId, teamId, userId, olderBuildBindsTheBrowser ? OAuthBrowserNonce.Hash(OAuthBrowserNonce.Mint()) : null, DateTimeOffset.UtcNow.AddMinutes(5)).ConfigureAwait(false); + + (await CallbackAsync(state, "c", browserNonce: null, stub).ConfigureAwait(false)).CredentialId.ShouldBeNull("handed off, not refused"); + (await CompleteAsInitiatorAsync(userId, state, "c", stub).ConfigureAwait(false)).CredentialId.ShouldNotBeNull(); + } + + [Fact] + public async Task A_connect_this_build_started_matches_the_previous_builds_callback_lookup() + { + // The previous build's callback consumes `state = $1 AND browser_nonce_hash = $2` (OAuthStateStore at #2104). + // Run verbatim against a row this build wrote: an older pod that receives the callback still finishes it. + var (userId, teamId, instanceId) = await SeedOAuthCapableInstanceAsync(ProviderKind.GitLab).ConfigureAwait(false); + var stub = new StubOAuthClient(ProviderKind.GitLab, BuildToken("at")); + var init = await InitAsync(userId, teamId, instanceId, stub, returnUrl: "/oauth-callback").ConfigureAwait(false); + var hash = OAuthBrowserNonce.Hash(init.BrowserNonce); + + using var verify = _fixture.BeginScope(); + var matches = await verify.Resolve().Database.SqlQuery($"SELECT count(*)::int AS \"Value\" FROM oauth_pending_state WHERE state = {init.State} AND browser_nonce_hash = {hash}").SingleAsync().ConfigureAwait(false); + + matches.ShouldBe(1); + } + // ── Helpers ──────────────────────────────────────────────────────────────────── private async Task<(Guid UserId, Guid TeamId, Guid InstanceId)> SeedOAuthCapableInstanceAsync(ProviderKind kind) @@ -391,6 +553,55 @@ private async Task InitAsync(Guid userId, Guid teamId return await scope.Resolve().Send(new InitCredentialOAuthCommand { ProviderInstanceId = instanceId, DisplayName = "connect", ReturnUrl = returnUrl }).ConfigureAwait(false); } + private async Task CallbackAsync(string state, string code, string? browserNonce, StubOAuthClient stub) + { + using var scope = _fixture.BeginScope(b => b.RegisterInstance(new SingleProviderRegistry(stub)).SingleInstance()); + + return await scope.Resolve().Send(new CompleteCredentialOAuthCommand { Code = code, State = state, BrowserNonce = browserNonce }).ConfigureAwait(false); + } + + /// The SPA's completion page posting the handed-off code as — signed in, with no team header in play. + private async Task CompleteAsInitiatorAsync(Guid callerId, string state, string code, StubOAuthClient stub) + { + using var scope = _fixture.BeginScope(b => + { + b.RegisterInstance(new Infrastructure.TestCurrentUser(callerId, "caller")).As().SingleInstance(); + b.RegisterInstance(new SingleProviderRegistry(stub)).SingleInstance(); + }); + + return await scope.Resolve().Send(new CompleteCredentialOAuthAsInitiatorCommand { State = state, Code = code }).ConfigureAwait(false); + } + + private async Task SeedMemberAsync(Guid teamId, TeamRole role) + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + var user = new User { Id = Guid.NewGuid(), Email = $"m-{Guid.NewGuid():N}@x", Name = $"member-{Guid.NewGuid():N}" }; + + db.User.Add(user); + db.TeamMembership.Add(new TeamMembership { Id = Guid.NewGuid(), TeamId = teamId, UserId = user.Id, Role = role }); + await db.SaveChangesAsync().ConfigureAwait(false); + + return user.Id; + } + + /// + /// A pending state as an older build writes it: the columns of 0005 plus, from #2104 on, browser_nonce_hash — and + /// nothing this build might add (requested_scopes is left NULL, as a connect with default scopes stores it). Raw SQL + /// so a new required column could not be filled in by the entity on its behalf. + /// + private async Task InsertStateAsAnOlderBuildAsync(string state, Guid instanceId, Guid teamId, Guid initiatorId, string? browserNonceHash, DateTimeOffset expires) + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + var now = DateTimeOffset.UtcNow; + + if (browserNonceHash == null) + await db.Database.ExecuteSqlInterpolatedAsync($"INSERT INTO oauth_pending_state (state, provider_instance_id, team_id, initiator_user_id, code_verifier, intended_display_name, intended_owner_user_id, return_url, expires_date, created_date, created_by, last_modified_date, last_modified_by) VALUES ({state}, {instanceId}, {teamId}, {initiatorId}, {"verifier"}, {"older build"}, {initiatorId}, {"/oauth-callback"}, {expires}, {now}, {initiatorId}, {now}, {initiatorId})").ConfigureAwait(false); + else + await db.Database.ExecuteSqlInterpolatedAsync($"INSERT INTO oauth_pending_state (state, provider_instance_id, team_id, initiator_user_id, code_verifier, intended_display_name, intended_owner_user_id, return_url, expires_date, created_date, created_by, last_modified_date, last_modified_by, browser_nonce_hash) VALUES ({state}, {instanceId}, {teamId}, {initiatorId}, {"verifier"}, {"older build"}, {initiatorId}, {"/oauth-callback"}, {expires}, {now}, {initiatorId}, {now}, {initiatorId}, {browserNonceHash})").ConfigureAwait(false); + } + private static void RegisterTestUserAndTeam(ContainerBuilder b, Guid userId, Guid teamId) { b.RegisterInstance(new Infrastructure.TestCurrentUser(userId, "tester", new[] { Roles.Admin })).As().SingleInstance(); @@ -442,6 +653,6 @@ private async Task ConnectViaOAuthAsync(Guid userId, Guid teamId, Guid ins BrowserNonce = initResult.BrowserNonce }).ConfigureAwait(false); - return result.CredentialId; + return result.CredentialId.ShouldNotBeNull(); } } diff --git a/backend/tests/CodeSpace.IntegrationTests/OAuth/OAuthStateCleanupTests.cs b/backend/tests/CodeSpace.IntegrationTests/OAuth/OAuthStateCleanupTests.cs index 9ade73adc..b3b948b89 100644 --- a/backend/tests/CodeSpace.IntegrationTests/OAuth/OAuthStateCleanupTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/OAuth/OAuthStateCleanupTests.cs @@ -35,7 +35,9 @@ public async Task DeleteExpired_removes_only_rows_past_their_expires_date() var cleanup = scope.Resolve(); var deleted = await cleanup.DeleteExpiredAsync(CancellationToken.None).ConfigureAwait(false); - deleted.ShouldBe(1); + // A floor, not a tally: the sweep is deployment-wide, and rows other tests in this database left behind can + // expire too. What this test owns is asserted below by name. + deleted.ShouldBeGreaterThanOrEqualTo(1); var db = scope.Resolve(); var survivors = await db.OAuthPendingState.AsNoTracking() diff --git a/backend/tests/CodeSpace.IntegrationTests/OAuth/ProviderReconnectFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/OAuth/ProviderReconnectFlowTests.cs index b975c2fe1..5cd642555 100644 --- a/backend/tests/CodeSpace.IntegrationTests/OAuth/ProviderReconnectFlowTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/OAuth/ProviderReconnectFlowTests.cs @@ -274,7 +274,7 @@ private async Task ConnectOAuthAsync(World world, Guid userId, OAuthTokenR using var callback = Scope(world, userId: null); - return (await callback.Resolve().Send(new CompleteCredentialOAuthCommand { State = init.State, Code = "code-from-provider", BrowserNonce = init.BrowserNonce })).CredentialId; + return (await callback.Resolve().Send(new CompleteCredentialOAuthCommand { State = init.State, Code = "code-from-provider", BrowserNonce = init.BrowserNonce })).CredentialId.ShouldNotBeNull(); } /// The Providers modal's "Use a token". diff --git a/backend/tests/CodeSpace.UnitTests/Architecture/RequestAuthorizationInventoryTests.cs b/backend/tests/CodeSpace.UnitTests/Architecture/RequestAuthorizationInventoryTests.cs index a7c09b8bc..9c42f0368 100644 --- a/backend/tests/CodeSpace.UnitTests/Architecture/RequestAuthorizationInventoryTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Architecture/RequestAuthorizationInventoryTests.cs @@ -37,7 +37,7 @@ public class RequestAuthorizationInventoryTests ["ReceiveConnectionWebhookCommand"] = "the same inbound, from a group/organization hook; proof is the HMAC verified against THAT hook's own secret, never a repository's.", ["ResumeWorkflowCallbackCommand"] = "proof is the single-use high-entropy callback token in the URL.", ["WebhookExistsQuery"] = "asked before a delivery's body is read; it answers only whether the hook id names a hook, which the delivery's own 404 already tells the caller.", - ["CompleteCredentialOAuthCommand"] = "the provider redirect carries no JWT; proof is the one-time OAuthPendingState row together with the nonce cookie only the starting browser holds.", + ["CompleteCredentialOAuthCommand"] = "the provider redirect carries no JWT; proof is the one-time OAuthPendingState row together with the nonce cookie only the starting browser holds. Without it nothing is consumed: the callback only hands the code to the SPA's completion page, where CompleteCredentialOAuthAsInitiatorCommand requires the signed-in starter.", ["AcceptInvitationCommand"] = "the invitee has no account yet; proof is the single-use invitation token in the route.", ["PreviewInvitationQuery"] = "same token, read-only, and it answers nothing at all unless the token checks out.", ["ResetPasswordCommand"] = "someone who cannot sign in is exactly who needs this; proof is the single-use reset token in the route.", diff --git a/backend/tests/CodeSpace.UnitTests/OAuth/OAuthReturnUrlTests.cs b/backend/tests/CodeSpace.UnitTests/OAuth/OAuthReturnUrlTests.cs index 647cbda56..d7ff37576 100644 --- a/backend/tests/CodeSpace.UnitTests/OAuth/OAuthReturnUrlTests.cs +++ b/backend/tests/CodeSpace.UnitTests/OAuth/OAuthReturnUrlTests.cs @@ -6,12 +6,12 @@ namespace CodeSpace.UnitTests.OAuth; /// /// 🟢 Unit: where a finished OAuth connect may send the browser. The address comes from whoever starts the connect, /// and the browser that follows it has just left a consent screen that made the whole trip look legitimate — so only -/// a path on this site, or an address on its public origin, is ever followed. +/// a path on this site, or an address on an origin this deployment serves its web app from, is ever followed. /// [Trait("Category", "Unit")] public class OAuthReturnUrlTests { - private const string PublicOrigin = "https://codespace.example"; + private static readonly string[] Origins = { "https://codespace.example", "https://alias.example:8443" }; [Theory] [InlineData("/")] @@ -20,9 +20,10 @@ public class OAuthReturnUrlTests [InlineData("https://codespace.example/oauth-callback")] [InlineData("HTTPS://CODESPACE.EXAMPLE/oauth-callback")] [InlineData("https://codespace.example:443/x")] - public void A_path_on_this_site_or_an_address_on_its_public_origin_is_allowed(string returnUrl) + [InlineData("https://alias.example:8443/oauth-callback")] + public void A_path_on_this_site_or_an_address_on_any_of_its_origins_is_allowed(string returnUrl) { - OAuthReturnUrl.IsAllowed(returnUrl, PublicOrigin).ShouldBeTrue(); + OAuthReturnUrl.IsAllowed(returnUrl, Origins).ShouldBeTrue(); } [Theory] @@ -33,21 +34,81 @@ public void A_path_on_this_site_or_an_address_on_its_public_origin_is_allowed(st [InlineData("javascript:alert(1)")] [InlineData("http://codespace.example/x")] // right host, wrong scheme [InlineData("https://codespace.example:8443/x")] // right host, wrong port + [InlineData("https://alias.example/x")] // the alias, on a port it does not answer on [InlineData("https://codespace.example.attacker.example/x")] [InlineData("https://user@codespace.example/x")] // user-info: nobody's return address carries one [InlineData("settings")] [InlineData("")] public void Anything_else_is_refused(string returnUrl) { - OAuthReturnUrl.IsAllowed(returnUrl, PublicOrigin).ShouldBeFalse(); + OAuthReturnUrl.IsAllowed(returnUrl, Origins).ShouldBeFalse(); + } + + [Theory] + [InlineData("/oauth-callback", true)] + [InlineData("https://codespace.example/oauth-callback", false)] + public void A_path_needs_no_configured_origin_and_an_absolute_address_needs_one(string returnUrl, bool allowed) + { + // A deployment that configured no origin at all still connects with a path; it cannot be sent anywhere absolute. + OAuthReturnUrl.IsAllowed(returnUrl, Array.Empty()).ShouldBe(allowed); } [Theory] [InlineData(null, "/")] [InlineData("https://attacker.example/landing", "/")] [InlineData("/oauth-callback", "/oauth-callback")] + [InlineData("https://alias.example:8443/oauth-callback", "https://alias.example:8443/oauth-callback")] public void A_refused_or_missing_address_falls_back_to_the_root(string? returnUrl, string landing) { - OAuthReturnUrl.AllowedOrFallback(returnUrl, PublicOrigin).ShouldBe(landing); + OAuthReturnUrl.AllowedOrFallback(returnUrl, Origins).ShouldBe(landing); + } + + /// + /// A connect handed to the SPA to finish lands on the SPA's own completion route — on the origin the return address + /// named when that origin passes, or on this site when it named a path, nothing, or somewhere that no longer passes. + /// The code travels with it, so it never lands on an origin outside the set. + /// + [Theory] + [InlineData("https://alias.example:8443/settings?tab=x", "https://alias.example:8443/oauth-callback")] + [InlineData("HTTPS://CODESPACE.EXAMPLE/oauth-callback", "https://codespace.example/oauth-callback")] + [InlineData("/settings/credentials", "/oauth-callback")] + [InlineData(null, "/oauth-callback")] + [InlineData("https://attacker.example/oauth-callback", "/oauth-callback")] + public void A_handed_off_connect_finishes_on_the_completion_page_of_an_allowed_origin(string? returnUrl, string completionPage) + { + OAuthReturnUrl.CompletionPageFor(returnUrl, Origins).ShouldBe(completionPage); + } + + [Fact] + public void A_hand_off_carries_the_code_and_state_escaped_in_the_fragment_only() + { + var handOff = OAuthReturnUrl.HandOff("https://spa.example/oauth-callback", "a/b+c d", "st&ate"); + + handOff.ShouldBe("https://spa.example/oauth-callback#oauthCode=a%2Fb%2Bc%20d&oauthState=st%26ate"); + } + + /// + /// Drift detector across the language seam: the callback writes the hand-off, the SPA's completion page reads it, and + /// no compiler sees both. Renaming a key, moving the route, or reading the query instead of the fragment on either + /// side would hand every cross-site connect to a page that finds nothing — so the page is read here as it ships. + /// + [Fact] + public void The_completion_page_reads_the_hand_off_where_the_callback_writes_it() + { + var page = File.ReadAllText(Path.Combine(FindRepoRoot(), "frontend", "src", "routes", "oauth-callback.tsx")); + + page.ShouldContain($"createFileRoute(\"{OAuthReturnUrl.CompletionPage}\")"); + page.ShouldContain($"get(\"{OAuthReturnUrl.HandOffCodeKey}\")"); + page.ShouldContain($"get(\"{OAuthReturnUrl.HandOffStateKey}\")"); + page.ShouldContain("window.location.hash", customMessage: "the code arrives in the fragment; a page reading the query would find nothing"); + } + + private static string FindRepoRoot() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + + while (dir is not null && !Directory.Exists(Path.Combine(dir.FullName, "frontend"))) dir = dir.Parent; + + return dir?.FullName ?? throw new InvalidOperationException("repo root not found"); } } diff --git a/backend/tests/CodeSpace.UnitTests/Settings/OAuthReturnOriginsSettingTests.cs b/backend/tests/CodeSpace.UnitTests/Settings/OAuthReturnOriginsSettingTests.cs new file mode 100644 index 000000000..70a59d8e2 --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Settings/OAuthReturnOriginsSettingTests.cs @@ -0,0 +1,92 @@ +using CodeSpace.Core.Settings.OAuth; +using Microsoft.Extensions.Configuration; +using Shouldly; + +namespace CodeSpace.UnitTests.Settings; + +/// +/// 🟢 Unit: the origins an OAuth connect may return the browser to, as configuration states them — and never a reason to +/// refuse to serve. PublicBaseUrlSetting throws outside Development when its key is unset, and an OAuth service +/// that injected it answered 500 to every connect, path return or not, on the shipped docker-compose API. This setting +/// reads the same key raw, so a missing value only narrows the set. +/// +[Trait("Category", "Unit")] +public class OAuthReturnOriginsSettingTests +{ + [Fact] + public void Outside_development_with_nothing_configured_it_is_empty_rather_than_a_failure() + { + var setting = Should.NotThrow(() => Build(Production())); + + setting.Value.ShouldBeEmpty(); + } + + [Fact] + public void The_public_base_url_is_read_raw_and_trimmed() + { + Build(Production(("App:PublicBaseUrl", " https://codespace.example/ "))).Value.ShouldBe(new[] { "https://codespace.example" }); + } + + [Theory] + [InlineData("Cors:AllowedOrigins:0", "https://spa.example")] + [InlineData("Cors:AllowedOrigins", "https://spa.example, https://alias.example")] + public void Every_cors_origin_is_a_return_origin_in_either_configuration_shape(string key, string value) + { + // A direct-mode SPA calls the API cross-origin, so its origin is on this list or it could not call at all. + Build(Production((key, value))).Value.ShouldContain("https://spa.example"); + } + + [Fact] + public void The_callback_urls_own_origin_is_a_return_origin_and_its_path_is_not() + { + // The provider already sends the browser to this host; in a same-origin deployment it is where the SPA answers. + Build(Production(("OAuth:CallbackUrl", "https://codespace.example/api/credentials/oauth/callback"))).Value.ShouldBe(new[] { "https://codespace.example" }); + } + + [Theory] + [InlineData("not a url")] + [InlineData("/api/credentials/oauth/callback")] + [InlineData("ftp://files.example/callback")] + public void A_callback_url_that_names_no_web_origin_adds_nothing_and_breaks_nothing(string callbackUrl) + { + Should.NotThrow(() => Build(Production(("OAuth:CallbackUrl", callbackUrl)))).Value.ShouldBeEmpty(); + } + + [Theory] + [InlineData(null)] + [InlineData("Development")] + [InlineData("development")] + public void Development_adds_the_dev_spa_origins_the_cors_policy_adds(string? environment) + { + var value = Build(new Dictionary { ["ASPNETCORE_ENVIRONMENT"] = environment }).Value; + + value.ShouldBe(new[] { "http://localhost:5180", "http://127.0.0.1:5180" }, ignoreOrder: true); + } + + [Theory] + [InlineData("Production")] + [InlineData("Staging")] + public void No_other_environment_gets_the_dev_origins(string environment) + { + Build(new Dictionary { ["ASPNETCORE_ENVIRONMENT"] = environment }).Value.ShouldBeEmpty(); + } + + [Fact] + public void The_sources_are_merged_without_duplicates() + { + var value = Build(Production(("App:PublicBaseUrl", "https://codespace.example"), ("Cors:AllowedOrigins", "HTTPS://CODESPACE.EXAMPLE,https://alias.example"), ("OAuth:CallbackUrl", "https://codespace.example/api/credentials/oauth/callback"))).Value; + + value.ShouldBe(new[] { "https://codespace.example", "https://alias.example" }); + } + + private static Dictionary Production(params (string Key, string Value)[] entries) + { + var values = new Dictionary { ["ASPNETCORE_ENVIRONMENT"] = "Production" }; + + foreach (var (key, value) in entries) values[key] = value; + + return values; + } + + private static OAuthReturnOriginsSetting Build(Dictionary values) => new(new ConfigurationBuilder().AddInMemoryCollection(values).Build()); +} diff --git a/frontend/.env.example b/frontend/.env.example index 1bd3bb6c7..8d02c0b2f 100644 --- a/frontend/.env.example +++ b/frontend/.env.example @@ -12,6 +12,14 @@ # no CORS preflight, OAuth callback works at /api/credentials/oauth/callback # under the SPA's own port. # +# An OAuth connect comes back to the origin the page was opened on when the +# API lists it: App:PublicBaseUrl, Cors:AllowedOrigins, OAuth:CallbackUrl's own +# origin, and in Development both http://localhost:5180 and the +# http://127.0.0.1:5180 Vite prints. On an origin the API does not list (a LAN +# IP, a tunnel host), the connect still starts, but its popup comes back to the +# host OAuth:CallbackUrl names and stays open there with the outcome; list the +# origin in Cors:AllowedOrigins, or connect from that host. +# # DIRECT MODE (this var set to a full origin): every API call goes to that # origin directly, skipping the Vite proxy. Useful when you want to: # - hit a backend running on a different host (LAN dev, remote tunnel) @@ -20,11 +28,16 @@ # # Requirements when you set this: # 1. The backend's `Cors:AllowedOrigins` (appsettings) must list this SPA's -# origin — http://localhost:5180 is added automatically in Development, -# anything else needs an explicit entry. -# 2. The backend's `OAuth:CallbackUrl` must point at THIS SPA's origin too -# (e.g. http://localhost:5180/api/credentials/oauth/callback), because -# the OAuth popup messages back into the SPA window. +# origin — http://localhost:5180 and http://127.0.0.1:5180 are added +# automatically in Development, anything else needs an explicit entry. +# An OAuth connect may return to any origin on that list, so this entry +# is also what lets the OAuth popup come back to this SPA. +# 2. The backend's `OAuth:CallbackUrl` may point at the API itself (e.g. +# http://localhost:5099/api/credentials/oauth/callback). When the browser +# cannot keep the connect's binding cookie — the API on another site, or +# a connect started on another host than the callback URL names — the +# callback hands the connect to this SPA's /oauth-callback page, which +# finishes it as the signed-in user who started it. # 3. For the HttpOnly session cookie, the API must be on the SAME SITE as # the SPA (same host name; ports may differ): http://localhost:5180 → # http://localhost:5099 works, http://127.0.0.1:5180 → localhost does diff --git a/frontend/src/api/oauth.test.ts b/frontend/src/api/oauth.test.ts index ec2a61ce9..091137dc8 100644 --- a/frontend/src/api/oauth.test.ts +++ b/frontend/src/api/oauth.test.ts @@ -21,3 +21,25 @@ describe("oauthApi.initOAuth", () => { expect(init.method).toBe("POST"); }); }); + +/** + * A connect whose callback could not tell which browser started it is handed to the SPA's completion page, which + * finishes it as the signed-in user. The code is single-use and travels in the body, never in a URL. + */ +describe("oauthApi.completeOAuth", () => { + afterEach(() => { vi.unstubAllGlobals(); }); + + it("posts the handed-off code and state with the session", async () => { + const fetchMock = vi.fn().mockResolvedValue(new Response(JSON.stringify({ credentialId: "cred-1" }), { status: 200, headers: { "Content-Type": "application/json" } })); + vi.stubGlobal("fetch", fetchMock); + + const result = await oauthApi.completeOAuth({ code: "the-code", state: "the-state" }); + + const [url, init] = fetchMock.mock.calls[0] as [string, RequestInit]; + expect(url).toMatch(/\/api\/credentials\/oauth\/complete$/); + expect(init.method).toBe("POST"); + expect(init.credentials).toBe("include"); + expect(JSON.parse(init.body as string)).toEqual({ code: "the-code", state: "the-state" }); + expect(result).toEqual({ credentialId: "cred-1" }); + }); +}); diff --git a/frontend/src/api/oauth.ts b/frontend/src/api/oauth.ts index 5729eb0ea..676792bda 100644 --- a/frontend/src/api/oauth.ts +++ b/frontend/src/api/oauth.ts @@ -1,5 +1,5 @@ import { ApiError, fetchJson } from "./request"; -import type { CredentialCapabilitiesResponse, CredentialSummary, CredentialUsage, DeleteProviderInstanceResponse, InitOAuthResponse, ProviderDefaults, ProviderInstanceSummary, ProviderInstanceUsage, ProviderKind, RevokeCredentialResponse } from "./types"; +import type { CompleteOAuthResponse, CredentialCapabilitiesResponse, CredentialSummary, CredentialUsage, DeleteProviderInstanceResponse, InitOAuthResponse, ProviderDefaults, ProviderInstanceSummary, ProviderInstanceUsage, ProviderKind, RevokeCredentialResponse } from "./types"; /** * Thin typed wrappers around the new OAuth + credential endpoints. They handle: @@ -85,6 +85,13 @@ export const oauthApi = { credentials: "include", }), + // Finishes a connect the callback handed to /oauth-callback because this browser held no binding cookie (the API on + // another site never sets one). The API accepts it only from the signed-in user who started the connect. + completeOAuth: (input: { code: string; state: string }) => fetchJson("/api/credentials/oauth/complete", { + method: "POST", + body: JSON.stringify(input), + }), + // Add a GitLab Group Access Token as a TEAM-SERVICE credential (owned by the team, not a person). addGroupAccessToken: (input: { providerInstanceId: string; displayName: string; token: string }) => fetchJson<{ id: string }>("/api/credentials/group-access-token", { diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 5bbe1e533..b591ba952 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -51,6 +51,10 @@ export interface InitOAuthResponse { state: string; } +export interface CompleteOAuthResponse { + credentialId: string; +} + export interface RevokeCredentialResponse { credentialId: string; providerAcknowledged: boolean; diff --git a/frontend/src/hooks/use-oauth-flow.test.ts b/frontend/src/hooks/use-oauth-flow.test.ts new file mode 100644 index 000000000..83b790df6 --- /dev/null +++ b/frontend/src/hooks/use-oauth-flow.test.ts @@ -0,0 +1,104 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { ApiError } from "@/api/oauth"; + +import { initAtFirstAcceptedReturnUrl, oauthReturnUrlsFor } from "./use-oauth-flow"; + +/** + * Where a connect brings its popup back to. A connect whose callback cannot find the browser's binding cookie — started + * on another host than the callback names — is handed to the completion page on the origin it named, and only the + * page's own origin holds the session and the opener that finish it. Vite prints http://127.0.0.1:5180 while + * OAuth:CallbackUrl names localhost:5180, so a path there brought the popup back to localhost: signed out, its message + * to the 127.0.0.1 opener dropped, the connect reported as cancelled. A path stays the fallback with the API on the + * page's own origin, for an origin the API does not list; in direct mode it would land on the API's host instead. + */ +describe("oauthReturnUrlsFor", () => { + it.each([ + ["the dev proxy, on the address Vite prints", "http://127.0.0.1:5180", ""], + ["the dev proxy, on localhost", "http://localhost:5180", ""], + ["one host fronting both", "https://codespace.example", "https://codespace.example"], + ["the API named with a path on this origin", "https://codespace.example", "https://codespace.example/api-root"], + ])("asks for the page's own origin, then a path, when the API answers on the page's origin — %s", (_, pageOrigin, apiBase) => { + expect(oauthReturnUrlsFor(pageOrigin, apiBase)).toEqual([`${pageOrigin}/oauth-callback`, "/oauth-callback"]); + }); + + it.each([ + ["another port on the same host", "http://localhost:5180", "http://localhost:5099"], + ["another site", "https://spa.example", "https://api.example/v1"], + ])("asks only for the page's own origin when the API answers elsewhere — %s", (_, pageOrigin, apiBase) => { + expect(oauthReturnUrlsFor(pageOrigin, apiBase)).toEqual([`${pageOrigin}/oauth-callback`]); + }); +}); + +/** + * Starting a connect at the first return address the API accepts. Both API builds refuse a return address they do not + * accept with 400 invalid_request before starting anything, so asking again with the next address leaves nothing behind; + * any other answer is the outcome, since the return address is the only thing that differs between tries. + */ +describe("initAtFirstAcceptedReturnUrl", () => { + const request = { providerInstanceId: "pi-1", displayName: "Maya's GitLab" }; + const started = { authorizeUrl: "https://gitlab.example/oauth/authorize?state=s", state: "s" }; + + afterEach(() => { vi.unstubAllGlobals(); }); + + it("starts at the page's own origin when the API accepts it", async () => { + const fetchMock = stubInit([200, started]); + + await expect(initAtFirstAcceptedReturnUrl(request, ["http://127.0.0.1:5180/oauth-callback", "/oauth-callback"])).resolves.toEqual(started); + + expect(returnUrlsAsked(fetchMock)).toEqual(["http://127.0.0.1:5180/oauth-callback"]); + }); + + it.each([ + ["this build", "returnUrl must be a path on this site, or an address on an origin this deployment serves its web app from — App:PublicBaseUrl, or one listed in Cors:AllowedOrigins. (Parameter 'returnUrl')"], + ["a build from before it read Cors:AllowedOrigins", "returnUrl must be a path on this site or an address on its public origin. (Parameter 'returnUrl')"], + ])("falls back to a path when the API does not list the page's origin — %s", async (_, message) => { + const fetchMock = stubInit([400, { code: "invalid_request", message }], [200, started]); + + await expect(initAtFirstAcceptedReturnUrl(request, ["https://alias.example/oauth-callback", "/oauth-callback"])).resolves.toEqual(started); + + expect(returnUrlsAsked(fetchMock)).toEqual(["https://alias.example/oauth-callback", "/oauth-callback"]); + }); + + it.each([ + ["a provider that is not OAuth-configured", 400, "oauth_callback_invalid"], + ["a caller without the permission", 403, "forbidden"], + ])("does not ask again for any other refusal — %s", async (_, status, code) => { + const fetchMock = stubInit([status, { code, message: "refused" }]); + + await expect(initAtFirstAcceptedReturnUrl(request, ["https://spa.example/oauth-callback", "/oauth-callback"])).rejects.toMatchObject({ status, code }); + + expect(returnUrlsAsked(fetchMock)).toHaveLength(1); + }); + + it("reports the last refusal when no address is accepted", async () => { + stubInit([400, { code: "invalid_request", message: "first" }], [400, { code: "invalid_request", message: "last" }]); + + const refused = await initAtFirstAcceptedReturnUrl(request, ["https://alias.example/oauth-callback", "/oauth-callback"]).catch((err: unknown) => err); + + expect(refused).toBeInstanceOf(ApiError); + expect((refused as ApiError).message).toBe("last"); + }); + + /** Answers POST /api/credentials/oauth/init with each [status, body] in turn. */ + function stubInit(...answers: [number, unknown][]) { + const queue = [...answers]; + const fetchMock = vi.fn(async (_input: RequestInfo | URL, _init?: RequestInit) => { + const [status, body] = queue.shift() ?? [500, { code: "unexpected", message: "no more answers" }]; + + return new Response(JSON.stringify(body), { status, headers: { "Content-Type": "application/json" } }); + }); + + vi.stubGlobal("fetch", fetchMock); + + return fetchMock; + } + + function returnUrlsAsked(fetchMock: ReturnType) { + return fetchMock.mock.calls.map(([input, init]) => { + expect(String(input)).toMatch(/\/api\/credentials\/oauth\/init$/); + + return JSON.parse((init as RequestInit).body as string).returnUrl; + }); + } +}); diff --git a/frontend/src/hooks/use-oauth-flow.ts b/frontend/src/hooks/use-oauth-flow.ts index f64ce2cab..0f156f311 100644 --- a/frontend/src/hooks/use-oauth-flow.ts +++ b/frontend/src/hooks/use-oauth-flow.ts @@ -2,15 +2,19 @@ import { useQueryClient } from "@tanstack/react-query"; import { useCallback } from "react"; import { ApiError, oauthApi, type InitOAuthRequest } from "@/api/oauth"; +import type { InitOAuthResponse } from "@/api/types"; /** * Runs an OAuth Authorization Code + PKCE flow in a popup. Resolves with the freshly * persisted credential id on success; rejects with a typed error otherwise. * * Mechanism: - * 1. Call POST /api/credentials/oauth/init → { authorizeUrl } + * 1. Call POST /api/credentials/oauth/init → { authorizeUrl } — at the first return address the API accepts * 2. window.open(authorizeUrl) — popup with the provider's consent page - * 3. After consent, provider hits backend /callback, backend hits frontend /oauth-callback + * 3. After consent, provider hits backend /callback, backend hits frontend /oauth-callback — with the finished + * credential when the popup carried the binding cookie, or with the code and state to finish signed in when it + * could not (the API on another site, or a connect started on another host than the callback's), which + * /oauth-callback then does * 4. /oauth-callback posts { type: 'codespace:oauth-result' } back via window.opener * 5. This hook's message listener receives it, closes the popup, resolves * @@ -21,6 +25,7 @@ import { ApiError, oauthApi, type InitOAuthRequest } from "@/api/oauth"; */ const OAUTH_MESSAGE_TYPE = "codespace:oauth-result"; +const CALLBACK_ROUTE = "/oauth-callback"; const POPUP_FEATURES = "width=600,height=720,resizable=yes,scrollbars=yes,status=yes"; const POPUP_POLL_INTERVAL_MS = 500; @@ -49,17 +54,60 @@ export class OAuthFlowError extends Error { } interface RunInput extends Omit { - /** Optional override; defaults to the in-app `/oauth-callback` route on the current origin. */ + /** Optional override; defaults to the in-app `/oauth-callback` route on this page's origin (see `oauthReturnUrlsFor`). */ returnUrl?: string; } +/** The answer both API builds give, before starting anything, to an argument they refuse — a return address they do not accept among them. */ +const REFUSED_ARGUMENT = "invalid_request"; + +/** + * Where a connect may bring the popup back to, in the order to ask. This page's own origin first: a connect whose + * callback cannot find the binding cookie — the API on another site, or a connect started on another host than + * OAuth:CallbackUrl names — is handed to the completion page on the origin the connect named, and only this origin + * holds the session and the opener that finish it. Vite prints http://127.0.0.1:5180 while the callback names + * localhost:5180; a path there brought the popup back to localhost, signed out and unheard by this page. + * + * A path is the fallback when the API answers on this page's own origin (the dev proxy, or one host fronting both): it + * needs no origin configured on the API, so a page on an alias the API does not list — or an API pod from before it + * read Cors:AllowedOrigins, which matched App:PublicBaseUrl alone — can still start a connect. It finishes on the + * callback's own host, which is this page's only when this page is on that host. In direct mode (VITE_API_URL on another + * origin) the callback runs on the API's host and a path would land the popup there; this page's own origin, which the + * API lists in Cors:AllowedOrigins for the page to reach it at all, is the one address. + */ +export function oauthReturnUrlsFor(pageOrigin: string, apiBase: string): string[] { + const apiOrigin = new URL(apiBase || "/", pageOrigin).origin; + const ownOrigin = `${pageOrigin}${CALLBACK_ROUTE}`; + + return apiOrigin === pageOrigin ? [ownOrigin, CALLBACK_ROUTE] : [ownOrigin]; +} + +/** + * Starts the connect at the first return address the API accepts. Only a refused argument moves on to the next: both API + * builds refuse a return address that way before starting anything, and the return address is the one argument that + * differs between tries, so any other answer — or the last refusal — is the outcome. + */ +export async function initAtFirstAcceptedReturnUrl(request: Omit, [returnUrl, ...fallbacks]: string[]): Promise { + try { + return await oauthApi.initOAuth({ ...request, returnUrl }); + } catch (err) { + if (fallbacks.length === 0 || !isRefusedArgument(err)) throw err; + + return initAtFirstAcceptedReturnUrl(request, fallbacks); + } +} + +function isRefusedArgument(err: unknown): boolean { + return err instanceof ApiError && err.status === 400 && err.code === REFUSED_ARGUMENT; +} + export function useOAuthFlow() { const queryClient = useQueryClient(); - return useCallback(async (input: RunInput): Promise<{ credentialId: string }> => { - const returnUrl = input.returnUrl ?? `${window.location.origin}/oauth-callback`; + return useCallback(async ({ returnUrl, ...request }: RunInput): Promise<{ credentialId: string }> => { + const returnUrls = returnUrl ? [returnUrl] : oauthReturnUrlsFor(window.location.origin, import.meta.env.VITE_API_URL ?? ""); - const init = await initOrThrow({ ...input, returnUrl }); + const init = await initOrThrow(request, returnUrls); const popup = window.open(init.authorizeUrl, "codespace-oauth", POPUP_FEATURES); if (!popup) throw new OAuthFlowError("popup_blocked", "Popup blocked by the browser. Allow popups for this site and try again."); @@ -77,9 +125,9 @@ export function useOAuthFlow() { }, [queryClient]); } -async function initOrThrow(input: InitOAuthRequest) { +async function initOrThrow(request: Omit, returnUrls: string[]) { try { - return await oauthApi.initOAuth(input); + return await initAtFirstAcceptedReturnUrl(request, returnUrls); } catch (err) { if (err instanceof ApiError) throw new OAuthFlowError("init_failed", err.message); throw new OAuthFlowError("init_failed", err instanceof Error ? err.message : "Failed to start OAuth flow"); diff --git a/frontend/src/routes/oauth-callback.test.tsx b/frontend/src/routes/oauth-callback.test.tsx new file mode 100644 index 000000000..1df3722f8 --- /dev/null +++ b/frontend/src/routes/oauth-callback.test.tsx @@ -0,0 +1,170 @@ +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { RouterProvider, createMemoryHistory, createRouter } from "@tanstack/react-router"; +import { render, screen, waitFor } from "@testing-library/react"; +import { StrictMode } from "react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { routeTree } from "@/routeTree.gen"; + +/** + * The page the OAuth popup lands on. Most connects arrive already finished — the API's callback found the binding + * cookie of the browser that started them. But a browser keeps no cookie from an API on another site, so there the + * callback hands the code and state to this page instead, in the fragment, and this page finishes the connect as the + * signed-in user. The API accepts that only from the person who started the connect; what is pinned here is that the + * page spends the single-use code exactly once, keeps it out of the address bar, and tells its opener how it ended — + * or, when no page on this origin opened it, stays open so the person can read how it ended. + */ +describe("/oauth-callback", () => { + /** The SPA page that opened the popup, on this origin — the one window that hears how the connect ended. */ + const opener = { closed: false, postMessage: vi.fn(), location: { get origin() { return window.location.origin; } } }; + + beforeEach(() => { + openedBy(opener); + vi.spyOn(window, "close").mockImplementation(() => {}); + }); + + afterEach(() => { + opener.postMessage.mockReset(); + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + localStorage.clear(); + window.history.replaceState(null, "", "/"); + }); + + it("finishes a handed-off connect once, as the signed-in user, and tells the opener once", async () => { + const fetchMock = stubComplete([200, { credentialId: "cred-42" }]); + + await openAt("/oauth-callback#oauthCode=the%2Fcode&oauthState=the-state"); + + await waitFor(() => expect(opener.postMessage).toHaveBeenCalledWith({ type: "codespace:oauth-result", status: "success", credentialId: "cred-42" }, window.location.origin)); + // StrictMode runs the settling effect twice. A second run finds the fragment already cleared and, unguarded, settles + // as a failure first — the opener acts on the first message it hears and reports a connect that succeeded as failed. + expect(opener.postMessage).toHaveBeenCalledTimes(1); + expect(completeCalls(fetchMock)).toEqual([{ code: "the/code", state: "the-state" }]); + expect(screen.getByText("Connected")).toBeTruthy(); + await waitFor(() => expect(window.close).toHaveBeenCalled()); + }); + + it("clears the code from the address bar before anything else can read it", async () => { + stubComplete([200, { credentialId: "cred-42" }]); + + await openAt("/oauth-callback#oauthCode=the-code&oauthState=the-state"); + + await waitFor(() => expect(opener.postMessage).toHaveBeenCalled()); + expect(window.location.hash).toBe(""); + expect(window.location.href).not.toContain("the-code"); + }); + + it("reports a refusal to the opener — someone other than the starter finishes nothing", async () => { + stubComplete([400, { code: "oauth_callback_invalid", message: "state token missing, expired, already consumed, or not started by you" }]); + + await openAt("/oauth-callback#oauthCode=c&oauthState=s"); + + await waitFor(() => expect(opener.postMessage).toHaveBeenCalledWith({ type: "codespace:oauth-result", status: "error", error: "oauth_callback_invalid", description: "state token missing, expired, already consumed, or not started by you" }, window.location.origin)); + expect(screen.getByText("Authorization failed")).toBeTruthy(); + }); + + it("asks a signed-out browser to sign in rather than sending it away", async () => { + stubComplete([401, { code: "unauthorized", message: "Authentication required." }]); + + await openAt("/oauth-callback#oauthCode=c&oauthState=s"); + + await waitFor(() => expect(opener.postMessage).toHaveBeenCalledWith(expect.objectContaining({ status: "error", error: "unauthorized", description: expect.stringMatching(/sign in/i) }), window.location.origin)); + expect(window.location.pathname).toBe("/oauth-callback"); + }); + + it.each([ + ["no page opened it", null], + ["the page that opened it is on another origin", crossOriginOpener()], + ])("stays open with how it ended when %s — nobody else would ever say", async (_, popupOpener) => { + // A connect started on another host than the one it came back to (an alias the API does not list) lands here on + // an origin its opener is not on: a message to that opener is dropped, and closing would hide the only account of + // the outcome — "sign in" here, or a credential that was in fact created. + openedBy(popupOpener); + stubComplete([401, { code: "unauthorized", message: "Authentication required." }]); + + await openAt("/oauth-callback#oauthCode=c&oauthState=s"); + + await waitFor(() => expect(screen.getByText(/sign in to codespace in this browser/i)).toBeTruthy()); + await new Promise((resolve) => setTimeout(resolve, 400)); + expect(window.close).not.toHaveBeenCalled(); + }); + + it("asks again when an API pod from before this page answers that it has no such endpoint", async () => { + // Mid rolling deploy the POST can reach a pod that predates it: a 404 that consumed nothing. + const fetchMock = stubComplete([404, { code: "not_found", message: "This server has no /api/credentials/oauth/complete." }], [200, { credentialId: "cred-7" }]); + + await openAt("/oauth-callback#oauthCode=c&oauthState=s"); + + await waitFor(() => expect(opener.postMessage).toHaveBeenCalledWith(expect.objectContaining({ status: "success", credentialId: "cred-7" }), window.location.origin)); + expect(completeCalls(fetchMock)).toHaveLength(2); + }); + + it.each([ + ["?oauthCredentialId=cred-9", { status: "success", credentialId: "cred-9" }], + ["?oauthError=access_denied&oauthErrorDescription=The%20user%20denied", { status: "error", error: "access_denied", description: "The user denied" }], + ])("passes on a connect the API already settled (%s) without calling it", async (search, outcome) => { + const fetchMock = stubComplete(); + + await openAt(`/oauth-callback${search}`); + + await waitFor(() => expect(opener.postMessage).toHaveBeenCalledWith({ type: "codespace:oauth-result", ...outcome }, window.location.origin)); + expect(completeCalls(fetchMock)).toHaveLength(0); + }); + + /** + * Answers POST /api/credentials/oauth/complete with each [status, body] in turn; every other URL is a 404. Each answer + * arrives a macrotask later, as a network answer does: an answer in the same tick lets React batch a doubled effect's + * outcome away, and a test of "spends the code once" could then never fail. + */ + function stubComplete(...answers: [number, unknown][]) { + const queue = [...answers]; + const fetchMock = vi.fn(async (input: RequestInfo | URL, _init?: RequestInit) => { + const url = typeof input === "string" ? input : input.toString(); + const [status, body] = url.includes("/api/credentials/oauth/complete") && queue.length > 0 ? queue.shift()! : [404, undefined]; + + await new Promise((resolve) => setTimeout(resolve, 20)); + + return new Response(body === undefined ? "" : JSON.stringify(body), { status, headers: { "Content-Type": "application/json" } }); + }); + + vi.stubGlobal("fetch", fetchMock); + + return fetchMock; + } + + function completeCalls(fetchMock: ReturnType) { + return fetchMock.mock.calls + .filter(([input]) => String(input).includes("/api/credentials/oauth/complete")) + .map(([, init]) => JSON.parse((init as RequestInit).body as string)); + } + + function openedBy(popupOpener: unknown) { + Object.defineProperty(window, "opener", { value: popupOpener, configurable: true }); + } + + /** A window on another origin: a browser lets this page see that it exists, and refuses to tell it where it is. */ + function crossOriginOpener() { + return { closed: false, postMessage: vi.fn(), location: { get origin(): string { throw new DOMException("Blocked a frame from accessing a cross-origin frame.", "SecurityError"); } } }; + } + + /** The popup as a browser opens it — the real address bar, the real route tree, and StrictMode's doubled effects. */ + async function openAt(url: string) { + localStorage.setItem("codespace.session", "cookie"); + window.history.replaceState(null, "", url); + + const router = createRouter({ routeTree, history: createMemoryHistory({ initialEntries: [url] }) }); + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + + render( + + + {/* eslint-disable-next-line @typescript-eslint/no-explicit-any */} + + + , + ); + + await waitFor(() => expect(router.state.status).toBe("idle"), { timeout: 15_000 }); + } +}); diff --git a/frontend/src/routes/oauth-callback.tsx b/frontend/src/routes/oauth-callback.tsx index f4cb1b3bf..52d9e26ac 100644 --- a/frontend/src/routes/oauth-callback.tsx +++ b/frontend/src/routes/oauth-callback.tsx @@ -1,62 +1,174 @@ import { createFileRoute } from "@tanstack/react-router"; -import { useEffect } from "react"; +import { useEffect, useRef, useState } from "react"; + +import { ApiError, oauthApi } from "@/api/oauth"; /** * Renders inside the OAuth popup window after the backend's /api/credentials/oauth/callback - * has exchanged the code and redirected here with either: - * • `?oauthCredentialId=` on success - * • `?oauthError=&oauthErrorDescription=` on failure + * redirected here with one of: + * • `?oauthCredentialId=` — the callback finished the connect (this browser held its binding cookie) + * • `?oauthError=&oauthErrorDescription=` — the provider refused + * • `#oauthCode=&oauthState=` — the callback could not tell this browser started the connect and + * handed it here instead: a browser keeps no cookie from an API on another site, so with the SPA and the API on + * different sites every connect arrives this way. This page finishes it as the signed-in user, and the API accepts + * that only from the person who started the connect. The fragment, unlike a query, reaches no server log. * * Responsibilities (in this order, fast): - * 1. Post the outcome to window.opener so the originating modal can react. - * 2. Close the popup so the user lands back on the original page. - * 3. As a fallback (if window.close is blocked), show a short instruction. + * 1. Settle the outcome — finishing a handed-off connect first, exactly once. + * 2. Post the outcome to window.opener so the originating modal can react. + * 3. Close the popup so the user lands back on the original page — only once that page heard: with no opener on this + * origin, the popup stays open and is the one place the outcome is shown. + * 4. As a fallback (if window.close is blocked), show a short instruction. * * No router navigation, no analytics — this page must be cheap and predictable. */ const OAUTH_MESSAGE_TYPE = "codespace:oauth-result"; +/** A pod from before the completion endpoint answers 404 for it and consumes nothing; mid rolling deploy, the next try may reach one that has it. */ +const COMPLETE_ATTEMPTS = 3; +const COMPLETE_RETRY_DELAY_MS = 200; + +type Outcome = + | { status: "success"; credentialId: string } + | { status: "error"; error: string; description?: string }; + +interface HandOff { + code: string; + state: string; +} + export const Route = createFileRoute("/oauth-callback")({ component: OAuthCallback, }); function OAuthCallback() { + const [outcome, setOutcome] = useState(null); + const settling = useRef(false); + useEffect(() => { - const params = new URLSearchParams(window.location.search); - const credentialId = params.get("oauthCredentialId"); - const error = params.get("oauthError"); - const errorDescription = params.get("oauthErrorDescription"); + // StrictMode runs effects twice in development, and a handed-off code is single-use: spend it once. + if (settling.current) return; + settling.current = true; - const payload = credentialId - ? { type: OAUTH_MESSAGE_TYPE, status: "success" as const, credentialId } - : { type: OAUTH_MESSAGE_TYPE, status: "error" as const, error: error ?? "unknown", description: errorDescription ?? undefined }; + void settle().then(setOutcome); + }, []); + + useEffect(() => { + if (!outcome) return; - // The popup was opened by the SPA; opener is the same origin. Restrict the message - // to that origin so a malicious co-tenant page can't intercept by claiming opener. - if (window.opener && !window.opener.closed) window.opener.postMessage(payload, window.location.origin); + // Nobody on this origin will hear how it ended — so this window stays open and says it, rather than leaving the + // opener to report a cancel for a connect that asked the person to sign in, or that created a credential. + if (!tellOpener(outcome)) return; // Slight delay so the receiver has a chance to attach before we close — also gives // the user a brief glimpse of the "Connected" state in case close is blocked. const timer = window.setTimeout(() => { try { window.close(); } catch { /* popup blockers may refuse */ } }, 250); return () => window.clearTimeout(timer); - }, []); + }, [outcome]); + + return ; +} + +async function settle(): Promise { + const handOff = takeHandOff(); + + if (handOff) return finish(handOff); const params = new URLSearchParams(window.location.search); - const hasError = params.has("oauthError"); + const credentialId = params.get("oauthCredentialId"); + + if (credentialId) return { status: "success", credentialId }; + + return { status: "error", error: params.get("oauthError") ?? "unknown", description: params.get("oauthErrorDescription") ?? undefined }; +} + +/** The code and state the callback handed over — read once and cleared from the address bar, history and any later Referer. */ +function takeHandOff(): HandOff | null { + const fragment = new URLSearchParams(window.location.hash.slice(1)); + const code = fragment.get("oauthCode"); + const state = fragment.get("oauthState"); + + if (!code || !state) return null; + + window.history.replaceState(window.history.state, "", window.location.pathname + window.location.search); + + return { code, state }; +} + +async function finish(handOff: HandOff): Promise { + try { + const { credentialId } = await completeOnAPodThatCan(handOff); + + return { status: "success", credentialId }; + } catch (err) { + return failureOf(err); + } +} + +async function completeOnAPodThatCan(handOff: HandOff) { + for (let attempt = 1; ; attempt++) { + try { + return await oauthApi.completeOAuth(handOff); + } catch (err) { + if (!(err instanceof ApiError) || err.status !== 404 || attempt >= COMPLETE_ATTEMPTS) throw err; + + await new Promise((resolve) => window.setTimeout(resolve, COMPLETE_RETRY_DELAY_MS)); + } + } +} + +/** + * Posts the outcome to the SPA page that opened this popup; false when no such page is there to hear it. The message is + * restricted to this origin so a co-tenant page can't intercept it by claiming to be the opener, so an opener on any + * other origin — the connect came back to another host than it started on — would drop it unheard. + */ +function tellOpener(outcome: Outcome): boolean { + const opener = sameOriginOpener(); + + opener?.postMessage({ type: OAUTH_MESSAGE_TYPE, ...outcome }, window.location.origin); + + return opener != null; +} + +/** The window that opened this popup when it is still open and on this origin; a browser refuses to read another origin's location. */ +function sameOriginOpener(): Window | null { + const opener = window.opener as Window | null; + + if (!opener || opener.closed) return null; + + try { + return opener.location.origin === window.location.origin ? opener : null; + } catch { + return null; + } +} + +function failureOf(err: unknown): Outcome { + if (err instanceof ApiError && err.status === 401) return { status: "error", error: err.code, description: "Sign in to CodeSpace in this browser, then connect again." }; + + if (err instanceof ApiError) return { status: "error", error: err.code, description: err.message }; + + return { status: "error", error: "complete_failed", description: err instanceof Error ? err.message : undefined }; +} + +/** The warm palette the signed-out pages use (styles/auth.css): this popup sits outside the app shell and its stylesheet. */ +const PALETTE = { ground: "#FAF9F5", ink: "#1F1E1D", muted: "#75736E", pending: "#F5F4EE", good: "#3E8E63", goodSoft: "#E3F1E9", danger: "#C0623D", dangerSoft: "#F4E3D8" }; + +function OAuthCallbackView({ outcome }: { outcome: Outcome | null }) { + const failed = outcome?.status === "error"; + const badge = outcome == null ? { ground: PALETTE.pending, ink: PALETTE.muted, mark: "…" } : failed ? { ground: PALETTE.dangerSoft, ink: PALETTE.danger, mark: "!" } : { ground: PALETTE.goodSoft, ink: PALETTE.good, mark: "OK" }; + const title = outcome == null ? "Finishing connection…" : failed ? "Authorization failed" : "Connected"; + const detail = failed && outcome.description ? outcome.description : "You can close this window."; return ( -
-
- {hasError ? "!" : "OK"} -
-
- {hasError ? "Authorization failed" : "Connected"} -
-
- You can close this window. +
+
+ {badge.mark}
+
{title}
+ {outcome != null &&
{detail}
}
); }