diff --git a/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs b/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs index 887af59e..4b92852c 100644 --- a/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs +++ b/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs @@ -163,6 +163,29 @@ private static async Task AuthorizeAsync( new[] { OpenIddictServerAspNetCoreDefaults.AuthenticationScheme }); } + // Consent-deny re-entry: /connect/consent redirects a DENIED decision + // back here with ?deny_ticket={id} so OpenIddict emits the RFC 6749 + // access_denied error to the client's redirect_uri honoring its + // response_mode + RFC 9207 iss — symmetric with the approve path, which + // re-enters authorize to complete the grant. We require a genuine denied, + // subject-bound ticket before acting; a forged/mismatched deny_ticket + // falls through to the normal flow (re-prompts consent), leaking nothing. + if ((string?)request.GetParameter("deny_ticket") is { Length: > 0 } denyTicketRaw + && Guid.TryParseExact(denyTicketRaw, "N", out var denyTicketId)) + { + var deniedTicket = await session.LoadAsync(denyTicketId); + if (deniedTicket is { DeniedAt: not null } && deniedTicket.Subject == user.Id) + { + return Results.Forbid( + new AuthenticationProperties(new Dictionary + { + [OpenIddictServerAspNetCoreConstants.Properties.Error] = Errors.AccessDenied, + [OpenIddictServerAspNetCoreConstants.Properties.ErrorDescription] = "The user denied the authorization request.", + }), + new[] { OpenIddictServerAspNetCoreDefaults.AuthenticationScheme }); + } + } + var subject = user.Id.ToString(); var clientPk = await applicationManager.GetIdAsync(application) ?? string.Empty; diff --git a/src/dotnet/Modgud.Api/Features/Auth/OAuth/ConsentEndpoints.cs b/src/dotnet/Modgud.Api/Features/Auth/OAuth/ConsentEndpoints.cs index 05e8ac34..d4603479 100644 --- a/src/dotnet/Modgud.Api/Features/Auth/OAuth/ConsentEndpoints.cs +++ b/src/dotnet/Modgud.Api/Features/Auth/OAuth/ConsentEndpoints.cs @@ -163,6 +163,10 @@ private static async Task SubmitConsentAsync( // means the loser doesn't even mint a duplicate Permanent authorization // row (the previously-documented benign residual is now gone too). record!.ConsumedAt = DateTimeOffset.UtcNow; + // Mark the deny atomically with the claim so the authorize re-entry + // below can prove this was a genuine denial (not just any consumed + // ticket) before OpenIddict emits an error to the client. + if (!decision.Approved) record.DeniedAt = DateTimeOffset.UtcNow; session.Store(record); try { @@ -170,15 +174,30 @@ private static async Task SubmitConsentAsync( } catch (JasperFx.ConcurrencyException) { - return Results.Conflict(new { message = "Consent ticket has already been used." }); + return Results.Conflict(new + { + message = "Consent ticket has already been used.", + retryUrl = "/connect/authorize" + record.AuthorizeRequestQuery, + }); } if (!decision.Approved) { + // Return control to the CLIENT (RFC 6749 §4.1.2.1) by re-entering + // /connect/authorize with a deny marker — symmetric with the + // approve path, which likewise re-enters authorize to complete the + // grant. OpenIddict then emits the access_denied error to the + // client's registered redirect_uri, honoring the client's + // response_mode (query/fragment/form_post) and RFC 9207 iss — none + // of which a hand-built redirect here would get right. It's a 302 + // to a same-origin /connect/authorize URL (not the client URI), so + // there is no window.location.assign(javascript:) sink either. + // AuthorizeAsync validates DeniedAt + subject-binding before acting. return Results.Ok(new ConsentResult { - RedirectUrl = $"/consent/denied?error={Uri.EscapeDataString(Errors.AccessDenied)}" + - $"&error_description={Uri.EscapeDataString("The user denied the authorization request.")}", + RedirectUrl = "/connect/authorize" + record.AuthorizeRequestQuery + + "&deny_ticket=" + record.Id.ToString("N"), + ReturnsToClient = true, }); } @@ -248,26 +267,35 @@ await authorizationManager.CreateAsync( return (null, Results.NotFound(new { message = "Consent ticket not found or expired." })); } - if (record.ConsumedAt is not null) - { - return (null, Results.Conflict(new { message = "Consent ticket has already been used." })); - } - - if (record.ExpiresAt < DateTimeOffset.UtcNow) - { - return (null, Results.BadRequest(new { message = "Consent ticket has expired." })); - } - // OAUTH-03 fix: subject binding. An attacker forcing a victim to POST // a consent decision can only act on tickets the victim's own session // created — and tickets are only created by /authorize, which is // session-scoped. Cross-user tampering is impossible by construction. + // Checked BEFORE the consumed/expired branches so the retryUrl below — + // which carries the locked-in authorize query (state, PKCE challenge) + // — is only ever disclosed to the ticket's own subject. var user = await userManager.GetUserAsync(currentUserPrincipal); if (user is null || user.Id != record.Subject) { return (null, Results.Forbid()); } + // A dead ticket is not a dead end: re-entering /connect/authorize with + // the locked-in query is safe — it mints a fresh ticket, or completes + // silently via the remembered authorization — so hand the SPA a retry + // URL instead of stranding the user. + var retryUrl = "/connect/authorize" + record.AuthorizeRequestQuery; + + if (record.ConsumedAt is not null) + { + return (null, Results.Conflict(new { message = "Consent ticket has already been used.", retryUrl })); + } + + if (record.ExpiresAt < DateTimeOffset.UtcNow) + { + return (null, Results.BadRequest(new { message = "Consent ticket has expired.", retryUrl })); + } + return (record, null); } } @@ -316,4 +344,10 @@ public class ConsentDecision public class ConsentResult { public required string RedirectUrl { get; init; } + + /// True when points back at the CLIENT + /// app (RFC 6749 §4.1.2.1 error redirect after a deny) rather than an + /// IdP-local page — the SPA must full-page-navigate there so the client + /// receives its error=access_denied callback. + public bool ReturnsToClient { get; init; } } diff --git a/src/dotnet/Modgud.Application/DTOs/SelfRegistration/SelfRegistrationDtos.cs b/src/dotnet/Modgud.Application/DTOs/SelfRegistration/SelfRegistrationDtos.cs index e57918bf..5155843c 100644 --- a/src/dotnet/Modgud.Application/DTOs/SelfRegistration/SelfRegistrationDtos.cs +++ b/src/dotnet/Modgud.Application/DTOs/SelfRegistration/SelfRegistrationDtos.cs @@ -46,6 +46,13 @@ public record RegisterDto /// non-empty the server quietly rejects (still 200 OK to keep the /// bot in the dark). public string? Honeypot { get; init; } + + /// Optional pending post-login continuation (e.g. a client app's + /// /connect/authorize flow the user detoured away from via "Register"). + /// Validated server-side as a same-origin path and appended to the emailed + /// verification link as ?redirect= so the verify page can forward it + /// to /login; ignored otherwise. + public string? ReturnUrl { get; init; } } /// Generic response — the same shape regardless of whether the diff --git a/src/dotnet/Modgud.Authentication/Api/Account/MagicLinkEndpoints.cs b/src/dotnet/Modgud.Authentication/Api/Account/MagicLinkEndpoints.cs index 30d204a0..ef5eb3d1 100644 --- a/src/dotnet/Modgud.Authentication/Api/Account/MagicLinkEndpoints.cs +++ b/src/dotnet/Modgud.Authentication/Api/Account/MagicLinkEndpoints.cs @@ -17,7 +17,11 @@ namespace Modgud.Authentication.Api.Account; public static class MagicLinkEndpoints { - public record MagicLinkRequestDto(string Email); + /// threads a pending post-login + /// continuation (e.g. a client app's /connect/authorize flow) through the + /// e-mail round trip; it is validated as a same-origin path before being + /// appended to the emailed URL as ?redirect=. + public record MagicLinkRequestDto(string Email, string? ReturnUrl = null); public record MagicLinkLoginDto(Guid UserId, string Token); public static WebApplication MapMagicLinkEndpoints(this WebApplication application, string path) @@ -129,6 +133,13 @@ public static WebApplication MapMagicLinkEndpoints(this WebApplication applicati var encodedToken = Uri.EscapeDataString(token); var magicUrl = $"{appUrl}/magic-login?userId={user.Id}&token={encodedToken}"; + // Thread the pending continuation through the e-mail round trip so + // a magic-link login can complete a client app's OIDC authorize + // flow. Open-redirect guard mirrors the SPA's: only a + // single-'/'-rooted same-origin path may ride along. + if (Api.LoginRedirectGuard.IsSameOriginPath(request.ReturnUrl) && request.ReturnUrl != "/") + magicUrl += $"&redirect={Uri.EscapeDataString(request.ReturnUrl!)}"; + await emailService.SendTemplatedEmailAsync( user.Email!, EmailTemplate.MagicLink, diff --git a/src/dotnet/Modgud.Authentication/Api/Account/PasswordResetEndpoints.cs b/src/dotnet/Modgud.Authentication/Api/Account/PasswordResetEndpoints.cs index 3a20a9af..d694f256 100644 --- a/src/dotnet/Modgud.Authentication/Api/Account/PasswordResetEndpoints.cs +++ b/src/dotnet/Modgud.Authentication/Api/Account/PasswordResetEndpoints.cs @@ -11,7 +11,12 @@ namespace Modgud.Authentication.Api.Account; public static class PasswordResetEndpoints { - public record ForgotPasswordRequest(string UserName); + /// threads a pending post-login + /// continuation (e.g. a client app's /connect/authorize flow the user + /// detoured away from via "Forgot password?") through the e-mail round + /// trip; validated as a same-origin path before it is appended to the + /// emailed reset URL as ?redirect=. + public record ForgotPasswordRequest(string UserName, string? ReturnUrl = null); public record ResetPasswordRequest(string UserId, string Token, string NewPassword); public static WebApplication MapPasswordResetEndpoints(this WebApplication application, string path) @@ -58,6 +63,13 @@ public static WebApplication MapPasswordResetEndpoints(this WebApplication appli var appUrl = RealmPublicUrl.RealmPublicBaseUrl(realm, env); var resetUrl = $"{appUrl}/reset-password?userId={userId}&token={encodedToken}"; + // Thread the pending continuation through the e-mail round trip + // so a password reset can resume a client app's OIDC authorize + // flow. Same-origin guard mirrors the SPA's; the reset page + // forwards ?redirect= to /login after a successful reset. + if (LoginRedirectGuard.IsSameOriginPath(request.ReturnUrl) && request.ReturnUrl != "/") + resetUrl += $"&redirect={Uri.EscapeDataString(request.ReturnUrl!)}"; + await emailService.SendTemplatedEmailAsync( user.Email, EmailTemplate.PasswordReset, diff --git a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ExternalAuthEndpoints.cs b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ExternalAuthEndpoints.cs index be4cab37..5b05751e 100644 --- a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ExternalAuthEndpoints.cs +++ b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ExternalAuthEndpoints.cs @@ -73,7 +73,7 @@ public static void MapExternalAuthEndpoints(this IEndpointRouteBuilder endpoints Items = { ["loginProviderId"] = loginProviderId.ToString("N"), - ["returnUrl"] = string.IsNullOrWhiteSpace(returnUrl) ? "/" : returnUrl!, + ["returnUrl"] = SanitizeReturnUrl(returnUrl), }, }; return Results.Challenge(props, [schemeName]); @@ -184,6 +184,15 @@ public record ExternalLoginDto( string? IconName, string? ButtonColorHex); + /// + /// Open-redirect guard for the post-login continuation: the finish + /// endpoint redirects to the stored value verbatim, so only a same-origin + /// absolute path ('/…', but not protocol-relative '//…' or + /// backslash-smuggling '/\…') survives; anything else falls back to '/'. + /// + private static string SanitizeReturnUrl(string? returnUrl) => + LoginRedirectGuard.IsSameOriginPath(returnUrl) ? returnUrl! : "/"; + /// /// Returns true when the request looks like a same-site navigation: /// either the Origin or the Referer header matches the request host. diff --git a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlLoginFlow.cs b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlLoginFlow.cs index 04275fee..8f473471 100644 --- a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlLoginFlow.cs +++ b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlLoginFlow.cs @@ -448,8 +448,8 @@ private static ClaimsPrincipal BuildExternalPrincipal( if (string.IsNullOrWhiteSpace(raw)) return null; if (Uri.TryCreate(raw, UriKind.Absolute, out _)) return null; - if (!raw.StartsWith('/')) return null; - - return raw; + // Same-origin absolute path only (rejects //…, /\…, and control-char + // smuggling like /\t/evil.com that a browser collapses to //evil.com). + return Modgud.Authentication.Api.LoginRedirectGuard.IsSameOriginPath(raw) ? raw : null; } } diff --git a/src/dotnet/Modgud.Authentication/Api/LoginRedirectGuard.cs b/src/dotnet/Modgud.Authentication/Api/LoginRedirectGuard.cs new file mode 100644 index 00000000..58cc0853 --- /dev/null +++ b/src/dotnet/Modgud.Authentication/Api/LoginRedirectGuard.cs @@ -0,0 +1,33 @@ +using System.Linq; + +namespace Modgud.Authentication.Api; + +/// +/// Open-redirect guard shared by every server-side post-login continuation +/// (external-IdP returnUrl, SAML RelayState, magic-link ReturnUrl). A +/// continuation is only honored when it is a same-origin absolute path — the +/// value is later emitted verbatim into a Location header or appended to +/// an e-mailed URL, so it must not be able to leave our origin. +/// +public static class LoginRedirectGuard +{ + /// + /// True only for a same-origin absolute path: it must start with a single + /// '/', and must not be protocol-relative ('//…'), backslash-smuggled + /// ('/\…'), or contain any ASCII control character. The control-char check + /// is load-bearing: a browser strips TAB/CR/LF while resolving a redirect, + /// so a value like /\t/evil.com — which passes a naive '//' check — + /// collapses to //evil.com (protocol-relative → external host) in the + /// browser's URL parser. A well-formed, single-decoded path never carries a + /// raw control character, so rejecting them costs nothing legitimate. + /// + public static bool IsSameOriginPath(string? value) + { + if (string.IsNullOrEmpty(value)) return false; + if (value[0] != '/') return false; // must be absolute path + if (value.StartsWith("//", StringComparison.Ordinal)) return false; // protocol-relative + if (value.StartsWith("/\\", StringComparison.Ordinal)) return false; // backslash-smuggling + if (value.Any(char.IsControl)) return false; // TAB/CR/LF etc. strip to '//' + return true; + } +} diff --git a/src/dotnet/Modgud.Authentication/SelfRegistration/SelfRegistrationService.cs b/src/dotnet/Modgud.Authentication/SelfRegistration/SelfRegistrationService.cs index e7783c8a..b7fbeeaa 100644 --- a/src/dotnet/Modgud.Authentication/SelfRegistration/SelfRegistrationService.cs +++ b/src/dotnet/Modgud.Authentication/SelfRegistration/SelfRegistrationService.cs @@ -220,7 +220,7 @@ public async Task RegisterAsync( // flips it. We log so the admin sees why no email was sent. if (settings.RequireEmailVerification) { - await SendVerificationEmailAsync(appUser, realm, token, ct); + await SendVerificationEmailAsync(appUser, realm, token, dto.ReturnUrl, ct); } else { @@ -311,11 +311,19 @@ private async Task SendVerificationEmailAsync( ApplicationUser user, Realm realm, string plaintextToken, + string? returnUrl, CancellationToken ct) { var appUrl = RealmPublicUrl.RealmPublicBaseUrl(realm, env); var url = $"{appUrl}/verify-email?token={Uri.EscapeDataString(plaintextToken)}"; + // Thread the pending continuation through the e-mail round trip so a + // self-registration can resume a client app's OIDC authorize flow. + // Same-origin guard mirrors the SPA's; the verify page forwards + // ?redirect= to /login once the account is confirmed. + if (Modgud.Authentication.Api.LoginRedirectGuard.IsSameOriginPath(returnUrl) && returnUrl != "/") + url += $"&redirect={Uri.EscapeDataString(returnUrl!)}"; + var displayName = !string.IsNullOrWhiteSpace(user.Firstname) ? $"{user.Firstname} {user.Lastname}".Trim() : user.UserName ?? user.Email ?? ""; diff --git a/src/dotnet/Modgud.Domain/OAuth/Consent/ConsentTicket.cs b/src/dotnet/Modgud.Domain/OAuth/Consent/ConsentTicket.cs index 79e83cb8..b71ba250 100644 --- a/src/dotnet/Modgud.Domain/OAuth/Consent/ConsentTicket.cs +++ b/src/dotnet/Modgud.Domain/OAuth/Consent/ConsentTicket.cs @@ -75,4 +75,15 @@ public class ConsentTicket /// ticket are rejected — single-use enforced. /// public DateTimeOffset? ConsumedAt { get; set; } + + /// + /// Set (alongside ) when the user DENIES the + /// request. The consent endpoint then re-enters /connect/authorize + /// with ?deny_ticket={Id}; the authorize endpoint verifies this + /// marker (and ) before letting OpenIddict emit the + /// RFC 6749 access_denied error to the client — honoring the + /// client's response_mode + RFC 9207 iss, symmetric with the + /// approve path's re-entry. + /// + public DateTimeOffset? DeniedAt { get; set; } } diff --git a/src/dotnet/Modgud.Tests.Unit/Authentication/LoginRedirectGuardTests.cs b/src/dotnet/Modgud.Tests.Unit/Authentication/LoginRedirectGuardTests.cs new file mode 100644 index 00000000..8c31fd17 --- /dev/null +++ b/src/dotnet/Modgud.Tests.Unit/Authentication/LoginRedirectGuardTests.cs @@ -0,0 +1,40 @@ +using Modgud.Authentication.Api; + +namespace Modgud.Tests.Unit.Authentication; + +/// +/// Pinning tests for the shared open-redirect guard behind every server-side +/// post-login continuation (external-IdP returnUrl, SAML RelayState, magic-link +/// ReturnUrl). This guard is the only barrier between an attacker-controlled +/// continuation and a verbatim Location header, so every accept/reject +/// case — especially the control-char smuggling that a naive '//' check misses +/// — is pinned here. +/// +public class LoginRedirectGuardTests +{ + [Theory] + [InlineData("/")] + [InlineData("/dashboard")] + [InlineData("/connect/authorize?response_type=code&client_id=x&scope=openid%20profile")] + [InlineData("/path/with spaces")] // literal space is not a control char; browser %-encodes it, no '//' collapse + public void Accepts_same_origin_absolute_paths(string value) + { + Assert.True(LoginRedirectGuard.IsSameOriginPath(value)); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("dashboard")] // not rooted + [InlineData("https://evil.com")] // absolute external + [InlineData("//evil.com")] // protocol-relative + [InlineData("/\\evil.com")] // backslash-smuggling + [InlineData("/\t/evil.com")] // TAB → browser strips → //evil.com + [InlineData("/\n/evil.com")] // LF + [InlineData("/\r/evil.com")] // CR + [InlineData("/foo\tbar")] // embedded TAB anywhere + public void Rejects_non_same_origin_or_control_char_values(string? value) + { + Assert.False(LoginRedirectGuard.IsSameOriginPath(value)); + } +} diff --git a/src/frontend-vue/e2e/07-oidc-continuation.spec.ts b/src/frontend-vue/e2e/07-oidc-continuation.spec.ts new file mode 100644 index 00000000..d9f8504a --- /dev/null +++ b/src/frontend-vue/e2e/07-oidc-continuation.spec.ts @@ -0,0 +1,266 @@ +import { test, expect, request as pwRequest, type APIRequestContext } from '@playwright/test' +import { createHash, randomBytes } from 'node:crypto' +import { uniqueSuffix } from './helpers' +import { clearMailpit, waitForMail } from './mailpit' + +/** + * OIDC continuation — a client app's /connect/authorize flow must survive + * every path through the login SPA and hand control BACK to the client. + * + * Regression coverage for the 2026-07-10 login-redirect bug report: the + * pending authorize continuation rides `?redirect=` (cookie handler's + * ReturnUrlParameter) and used to be dropped by mid-flow detours, the + * magic-link e-mail round trip, and the consent deny/dead-ticket paths — + * stranding users on the IdP dashboard while the client waited forever. + * + * Two journeys, both entered like a real client app (unauthenticated + * /connect/authorize with PKCE): + * + * 1. challenge → login page (detour links forward ?redirect=) → password + * login resumes authorize → consent → DENY returns the RFC 6749 + * `error=access_denied` (+ state) to the client's redirect_uri. + * 2. challenge → magic-link request (continuation rides the emailed URL + * as ?redirect=) → opening the mail's link signs in AND resumes + * authorize → consent → ALLOW lands ?code= on the client. + */ + +const ADMIN_USER = process.env.E2E_ADMIN_USER ?? 'admin' +const ADMIN_PASSWORD = process.env.E2E_ADMIN_PASSWORD ?? 'ABC12abc!' +const TEST_PASSWORD = 'TestPass1234!' + +// Unique-per-run names so re-runs against a reused container never collide. +const SUFFIX = uniqueSuffix() +const CLIENT_ID = `pw-cont-${SUFFIX}` +// Loopback redirect — nothing is listening; the navigation never commits, so +// tests capture the *issued* request (fires before the connection refuses). +const REDIRECT_URI = 'http://localhost/pw-cont-cb' + +let baseURL: string +test.beforeAll(({ baseURL: b }) => { baseURL = b! }) + +/** Admin-authenticated API context — used only for provisioning. */ +async function adminContext(): Promise { + const ctx = await pwRequest.newContext({ baseURL }) + const res = await ctx.post('/api/account/login', { + data: { UserName: ADMIN_USER, Password: ADMIN_PASSWORD, RememberMe: false }, + }) + if (!res.ok()) throw new Error(`admin login failed: ${res.status()} ${await res.text()}`) + return ctx +} + +/** Explicit-consent public PKCE client — the shape a BFF/SPA relying party uses. */ +async function createExplicitClient(admin: APIRequestContext): Promise { + const res = await admin.post('/api/admin/oauth/clients', { + data: { + ClientId: CLIENT_ID, + DisplayName: `Playwright Continuation ${SUFFIX}`, + ClientType: 'public', + ConsentType: 'explicit', + RequireClientSecret: false, + RedirectUris: [REDIRECT_URI], + Scopes: ['openid'], + AllowedGrantTypes: ['authorization_code'], + }, + }) + if (!res.ok()) throw new Error(`client create failed: ${res.status()} ${await res.text()}`) +} + +/** Test user with a confirmed email (magic-link self-service requires it). + * A fresh per-call suffix so multiple tests in this file don't collide. */ +async function createUser(admin: APIRequestContext): Promise<{ id: string; email: string }> { + const userName = `pw-cont-user-${uniqueSuffix()}` + const email = `${userName}@modgud.test` + const createRes = await admin.post('/api/user', { + data: { Firstname: 'PW', Lastname: 'Continuation', Acronym: userName, Email: email, EmailConfirmed: true }, + }) + if (!createRes.ok()) throw new Error(`create user: ${createRes.status()} ${await createRes.text()}`) + const created = await createRes.json() + const passRes = await admin.put(`/api/user/${created.Id}/password`, { data: { Password: TEST_PASSWORD } }) + if (!passRes.ok()) throw new Error(`set-password: ${passRes.status()} ${await passRes.text()}`) + return { id: created.Id as string, email } +} + +function pkce(): { verifier: string; challenge: string } { + const base64Url = (buf: Buffer) => + buf.toString('base64').replace(/=/g, '').replace(/\+/g, '-').replace(/\//g, '_') + const verifier = base64Url(randomBytes(32)) + return { verifier, challenge: base64Url(createHash('sha256').update(verifier, 'ascii').digest()) } +} + +function authorizeUrl(state: string, challenge: string): string { + const params = new URLSearchParams({ + response_type: 'code', + client_id: CLIENT_ID, + redirect_uri: REDIRECT_URI, + scope: 'openid', + state, + code_challenge: challenge, + code_challenge_method: 'S256', + }) + return `/connect/authorize?${params.toString()}` +} + +test.describe.configure({ mode: 'serial' }) + +test.describe('OIDC continuation — authorize survives the login SPA', () => { + test.beforeAll(async () => { + const admin = await adminContext() + try { + await createExplicitClient(admin) + } finally { + await admin.dispose() + } + }) + + test('challenge → detour links forward the continuation → password login resumes authorize → deny returns error=access_denied to the client', async ({ page }, testInfo) => { + const { challenge } = pkce() + const state = randomBytes(16).toString('hex') + + // ── §1 An unauthenticated authorize is challenged to /login?redirect= ── + await page.goto(authorizeUrl(state, challenge)) + await page.waitForURL(/\/login\?redirect=/, { timeout: 10_000 }) + await page.screenshot({ path: testInfo.outputPath('01-challenged-login.png'), fullPage: true }) + + // ── §2 Mid-flow detours must forward ?redirect= (href-level, language- + // independent). vue-router leaves RFC 3986-legal '/' and '?' + // unencoded in query values, so match both encodings. ── + const continuationHref = /redirect=(%2F|\/)connect(%2F|\/)authorize/ + const forgotLink = page.locator('a[href^="/forgot-password"]') + await expect(forgotLink).toHaveAttribute('href', continuationHref) + await forgotLink.click() + await page.waitForURL(/\/forgot-password\?redirect=/, { timeout: 10_000 }) + + const backLink = page.locator('a[href^="/login"]').first() + await expect(backLink).toHaveAttribute('href', continuationHref) + await backLink.click() + await page.waitForURL(/\/login\?redirect=/, { timeout: 10_000 }) + + // ── §3 Password login resumes the authorize flow → consent ticket ── + await page.getByRole('textbox', { name: /benutzername|username/i }).fill(ADMIN_USER) + await page.getByRole('textbox', { name: /passwort|password/i }).fill(ADMIN_PASSWORD) + await page.getByRole('button', { name: /anmelden|sign in|login/i }).first().click() + + // A fresh admin may hit the secure-setup (2FA nudge) interstitial first. + // Postponing finishes through the same finishLogin(), so the continuation + // must survive that path too. + const postpone = page.getByRole('button', { name: /Später|Postpone|Later|Skip/i }).first() + await Promise.race([ + page.waitForURL(/\/consent\?ticket=/, { timeout: 15_000 }), + postpone.waitFor({ timeout: 15_000 }), + ]) + if (!/\/consent\?ticket=/.test(page.url())) { + await postpone.click() + } + await page.waitForURL(/\/consent\?ticket=/, { timeout: 15_000 }) + await page.screenshot({ path: testInfo.outputPath('02-consent.png'), fullPage: true }) + + // ── §4 Deny → RFC 6749 §4.1.2.1 error redirect BACK to the client ── + const redirectReq = page.waitForRequest( + (req) => req.url().startsWith(REDIRECT_URI), { timeout: 10_000 }) + await page.getByRole('button', { name: /^Deny$|^Ablehnen$/i }).click() + const callback = new URL((await redirectReq).url()) + expect(callback.searchParams.get('error')).toBe('access_denied') + expect(callback.searchParams.get('state')).toBe(state) + }) + + test('magic-link login preserves the authorize continuation through the e-mail round trip', async ({ page }, testInfo) => { + const admin = await adminContext() + let user: { id: string; email: string } + try { + user = await createUser(admin) + } finally { + await admin.dispose() + } + + const { challenge } = pkce() + const state = randomBytes(16).toString('hex') + + await clearMailpit() + const before = new Date(Date.now() - 60_000) + + // ── §1 Enter like a client app: authorize → challenged to /login?redirect= ── + await page.goto(authorizeUrl(state, challenge)) + await page.waitForURL(/\/login\?redirect=/, { timeout: 10_000 }) + + // ── §2 Request the magic link from THIS login page — the pending + // continuation must ride into the request ── + await page.getByRole('button', { name: /login link via email|anmelde-link per e-mail/i }).click() + await page.getByPlaceholder(/email@/i).fill(user.email) + await page.getByRole('button', { name: /send link|link senden/i }).click() + await expect(page.getByText(/login link was sent|anmelde-link.*gesendet|check your inbox|posteingang/i)) + .toBeVisible({ timeout: 10_000 }) + + // ── §3 The emailed URL carries the continuation as ?redirect= ── + const mail = await waitForMail(user.email, before, 30_000) + const href = mail.HTML.match(/href="([^"]*magic-login[^"]*)"/)?.[1] + expect(href, 'magic-login link present in the mail body').toBeTruthy() + const linkUrl = new URL(href!.replace(/&/g, '&')) + expect(linkUrl.searchParams.get('redirect'), 'emailed link carries the authorize continuation') + .toContain('/connect/authorize') + + // ── §4 Opening the link signs in AND resumes the authorize flow ── + await page.goto(`${linkUrl.pathname}${linkUrl.search}`) + await page.waitForURL(/\/consent\?ticket=/, { timeout: 15_000 }) + await page.screenshot({ path: testInfo.outputPath('01-consent-after-magic.png'), fullPage: true }) + + // ── §5 Allow → the client receives its ?code= callback ── + const redirectReq = page.waitForRequest( + (req) => req.url().startsWith(REDIRECT_URI), { timeout: 10_000 }) + await page.getByRole('button', { name: /^Allow$|^Erlauben$/i }).click() + const callback = new URL((await redirectReq).url()) + expect(callback.searchParams.get('code'), 'authorization code delivered to the client').toBeTruthy() + expect(callback.searchParams.get('state')).toBe(state) + }) + + test('password-reset threads the authorize continuation through the e-mail round trip', async ({ page }, testInfo) => { + const admin = await adminContext() + let user: { id: string; email: string } + try { + user = await createUser(admin) + } finally { + await admin.dispose() + } + + const { challenge } = pkce() + const state = randomBytes(16).toString('hex') + const authorize = authorizeUrl(state, challenge) + + await clearMailpit() + const before = new Date(Date.now() - 60_000) + + // ── §1 Reach the forgot-password page carrying the pending continuation. + // "Forgot password?" on /login already forwards ?redirect=, so a + // real user arrives here with it in the URL. ── + await page.goto(`/forgot-password?redirect=${encodeURIComponent(authorize)}`) + await page.getByRole('textbox').first().fill(user.email) + await page.getByRole('button', { name: /send link|link senden/i }).click() + await expect(page.getByText(/reset link has been sent|link.*gesendet|e-?mail.*(sent|gesendet)/i)) + .toBeVisible({ timeout: 10_000 }) + await page.screenshot({ path: testInfo.outputPath('01-reset-requested.png'), fullPage: true }) + + // ── §2 The emailed reset link carries the continuation as ?redirect=. ── + const mail = await waitForMail(user.email, before, 30_000) + const href = mail.HTML.match(/href="([^"]*reset-password[^"]*)"/)?.[1] + expect(href, 'reset-password link present in the mail body').toBeTruthy() + const linkUrl = new URL(href!.replace(/&/g, '&')) + expect(linkUrl.searchParams.get('redirect'), 'emailed reset link carries the authorize continuation') + .toContain('/connect/authorize') + + // ── §3 The reset page forwards the continuation to /login on success, so + // a completed reset resumes the client flow rather than stranding. + // Copy-independent selectors (input[type=password] / submit) so the + // assertion doesn't hinge on the active UI language. ── + await page.goto(`${linkUrl.pathname}${linkUrl.search}`) + const pwInputs = page.locator('input[type="password"]') + await pwInputs.nth(0).fill('ResetPass1234!') + await pwInputs.nth(1).fill('ResetPass1234!') + await page.locator('button[type="submit"]').click() + // Success card replaces the form with a single "go to login" button. + const toLogin = page.getByRole('button', { name: /login|anmeld/i }) + await expect(toLogin).toBeVisible({ timeout: 10_000 }) + await toLogin.click() + await page.waitForURL(/\/login\?redirect=/, { timeout: 10_000 }) + expect(decodeURIComponent(new URL(page.url()).searchParams.get('redirect') ?? '')) + .toContain('/connect/authorize') + }) +}) diff --git a/src/frontend-vue/src/composables/useLoginRedirect.ts b/src/frontend-vue/src/composables/useLoginRedirect.ts new file mode 100644 index 00000000..7f9eb362 --- /dev/null +++ b/src/frontend-vue/src/composables/useLoginRedirect.ts @@ -0,0 +1,78 @@ +import { computed } from 'vue' +import { useRoute, useRouter } from 'vue-router' +import { useHttpClient } from '@/composables/useHttpClient' + +// Paths outside the SPA (served directly by the backend) — Vue Router can't navigate +// there, so we use a full-page load after successful login. +// `/connect/*` are the OpenIddict OAuth/OIDC endpoints; an inbound third-party +// client kicks the flow off there, so completing login has to send the browser +// back to /connect/authorize verbatim, not push the path through Vue Router +// (which would silently drop it as an unknown route). +const NON_SPA_PREFIXES = ['/docs/', '/docs', '/connect/', '/connect'] + +// Open-redirect guard: a redirect target has to be a same-origin path, +// i.e. a string starting with a single '/'. Anything starting with '//', +// 'http:', 'https:', a scheme, or a backslash could send the user to an +// attacker-controlled host after a successful login. +export function isSameOriginPath(value: unknown): value is string { + if (typeof value !== 'string') return false // repeated ?redirect= → string[] + if (!value.startsWith('/')) return false // must be absolute path + if (value.startsWith('//')) return false // protocol-relative URL + if (value.startsWith('/\\')) return false // backslash-smuggling + // Control chars (TAB/CR/LF) are stripped by the browser's URL parser, so + // '/\t/evil.com' would collapse to '//evil.com' — reject them. + // eslint-disable-next-line no-control-regex + if (/[\u0000-\u001f\u007f]/.test(value)) return false + return true +} + +/** + * Shared post-login continuation. Every flow that establishes the session + * cookie (password, TOTP, email-OTP, passkey, magic-link, external IdP, …) + * must finish through here so a pending `?redirect=` — typically the + * /connect/authorize continuation of a client app's OIDC flow — is honored + * instead of stranding the user on the dashboard. + */ +export function useLoginRedirect() { + const route = useRoute() + const router = useRouter() + const gdprHttp = useHttpClient('/api/auth') + + // Redirect target after login. Set as ?redirect= by the router guard, the + // cookie handler's login challenge (ReturnUrlParameter = "redirect"), or a + // magic-link email URL. vue-router already URL-decodes query values, so the + // raw value is used as-is; failing the same-origin guard falls back to the + // dashboard. + const redirectTarget = computed(() => { + // route.query.redirect is string | string[] | undefined (a repeated param + // yields an array); isSameOriginPath rejects any non-string, so a crafted + // ?redirect=/a&redirect=/b falls back to '/' instead of throwing. + const r = route.query.redirect + return isSameOriginPath(r) ? r : '/' + }) + + async function finishLogin() { + const target = redirectTarget.value + + // Self-service grace interstitial: a user who scheduled their own deletion + // stays able to log in precisely so they can cancel. Divert them to the + // interstitial (which continues to `target` on cancel/continue) before the + // normal redirect. Admin recycle-bin users can't log in, so never land here. + try { + const status = await gdprHttp.addPath('deletion-status') + .get<{ IsPending: boolean; Initiator?: string | null }>() + if (status?.IsPending && status.Initiator === 'SelfService') { + router.push({ path: '/deletion-pending', query: { redirect: target } }) + return + } + } catch { /* status unavailable — never block the login on it */ } + + if (NON_SPA_PREFIXES.some((p) => target === p || target.startsWith(p + '/') || target.startsWith(p + '?'))) { + window.location.assign(target) + } else { + router.push(target) + } + } + + return { redirectTarget, finishLogin } +} diff --git a/src/frontend-vue/src/models/consent.ts b/src/frontend-vue/src/models/consent.ts index eb0f87b4..300d420e 100644 --- a/src/frontend-vue/src/models/consent.ts +++ b/src/frontend-vue/src/models/consent.ts @@ -38,8 +38,14 @@ export interface ConsentDecision { } export interface ConsentResult { - /** Non-SPA URL. Approve → /connect/authorize?; - * Deny → /consent/denied?error=… (an IdP-side error landing page). - * Use window.location.assign — Vue Router cannot navigate to either. */ + /** Non-SPA, same-origin /connect/authorize URL. Approve → re-enters with + * the original query to complete the grant; Deny → re-enters with a + * deny marker (?deny_ticket=…) so OpenIddict emits the RFC 6749 + * error=access_denied to the client honoring its response_mode + iss. + * Use window.location.assign — Vue Router cannot navigate to it. */ RedirectUrl: string + /** True on a deny that hands control back to the client via the authorize + * re-entry (vs. the defensive inline-denied fallback). Both cases + * full-page-navigate to a same-origin RedirectUrl. */ + ReturnsToClient?: boolean } diff --git a/src/frontend-vue/src/stores/auth.store.ts b/src/frontend-vue/src/stores/auth.store.ts index 403d6a93..f8d33005 100644 --- a/src/frontend-vue/src/stores/auth.store.ts +++ b/src/frontend-vue/src/stores/auth.store.ts @@ -152,10 +152,13 @@ export const useAuthStore = defineStore('auth', () => { } /** - * Request a magic link email for passwordless login. + * Request a magic link email for passwordless login. `returnUrl` threads a + * pending post-login continuation (e.g. a /connect/authorize OIDC flow) + * through the e-mail round trip — the backend appends it to the emailed + * /magic-login URL as ?redirect= after validating it server-side. */ - async function requestMagicLink(email: string): Promise { - await magicLinkHttp.addPath('request').post({ Email: email }) + async function requestMagicLink(email: string, returnUrl?: string): Promise { + await magicLinkHttp.addPath('request').post({ Email: email, ReturnUrl: returnUrl ?? null }) } /** diff --git a/src/frontend-vue/src/views/auth/ConsentView.vue b/src/frontend-vue/src/views/auth/ConsentView.vue index 0aaeda35..86a469c4 100644 --- a/src/frontend-vue/src/views/auth/ConsentView.vue +++ b/src/frontend-vue/src/views/auth/ConsentView.vue @@ -33,6 +33,25 @@ const phase = ref('loading') const error = ref('') const submitting = ref(false) +// A dead ticket (expired/consumed) is not a dead end: the backend hands us +// a retry URL (/connect/authorize + the locked-in query) that safely mints a +// fresh ticket — or completes silently via the remembered authorization. +const retryUrl = ref(null) + +function readRetryUrl(e: HttpClientError): string | null { + const b = e.body + if (b && typeof b === 'object' && 'retryUrl' in b && typeof b.retryUrl === 'string' + && b.retryUrl.startsWith('/connect/authorize')) { + return b.retryUrl + } + return null +} + +function retryAuthorize() { + // Server-side endpoint — Vue Router cannot navigate there. + if (retryUrl.value) window.location.assign(retryUrl.value) +} + const model = ref(null) // approval is keyed by scope-name; required scopes are pre-checked AND // the toggle is rendered disabled so the user cannot uncheck them. @@ -103,9 +122,11 @@ onMounted(async () => { break case 409: error.value = t('consent.alreadyUsed', {}, 'This consent request has already been completed. Please start over from the app.') + retryUrl.value = readRetryUrl(e) break case 400: error.value = t('consent.expired', {}, 'Consent request expired. Please start over from the app.') + retryUrl.value = readRetryUrl(e) break default: error.value = t('consent.loadError', {}, 'Failed to load the consent request.') @@ -136,18 +157,32 @@ async function submit(approved: boolean) { // server-side endpoint Vue Router cannot navigate to. Full-page // assign so the OIDC dance continues on the backend. window.location.assign(result.RedirectUrl) + } else if (result.ReturnsToClient) { + // Deny — the backend re-enters /connect/authorize with a deny marker; + // OpenIddict then emits the RFC 6749 access_denied error to the client's + // redirect_uri (honoring its response_mode + iss). RedirectUrl is a + // same-origin /connect/authorize URL, so full-page-assign into it. + window.location.assign(result.RedirectUrl) } else { - // On deny the backend returns /consent/denied?error=… which is - // an IdP-side landing URL, not an RP redirect. We ignore it - // and render the denial state inline — cleaner UX, no extra - // route needed. + // Defensive fallback: no client redirect available — render the denial + // state inline (not reached while the backend always re-enters authorize). phase.value = 'denied' } } catch (e) { - if (e instanceof HttpClientError && e.status === 409) { - error.value = t('consent.alreadyUsed', {}, 'This consent request has already been completed. Please start over from the app.') - } else if (e instanceof HttpClientError && e.status === 400) { - error.value = t('consent.expired', {}, 'Consent request expired. Please start over from the app.') + if (e instanceof HttpClientError && (e.status === 409 || e.status === 400)) { + error.value = e.status === 409 + ? t('consent.alreadyUsed', {}, 'This consent request has already been completed. Please start over from the app.') + : t('consent.expired', {}, 'Consent request expired. Please start over from the app.') + retryUrl.value = readRetryUrl(e) + phase.value = 'error' + } else if (e instanceof HttpClientError && (e.status === 404 || e.status === 403)) { + // Ticket GC'd between the prompt and submit, or bound to a different + // user — not retryable in place, so surface the error card instead of + // leaving the stale Allow/Deny prompt showing a dead message. + error.value = e.status === 404 + ? t('consent.notFound', {}, 'Consent request not found or expired. Please start the sign-in flow again from the app.') + : t('consent.forbidden', {}, 'This consent ticket belongs to a different user. Please sign in with the correct account.') + phase.value = 'error' } else if (e instanceof HttpClientError) { error.value = t('consent.submitError', {}, 'Could not submit your decision. Please try again.') } else { @@ -195,7 +230,14 @@ function scopeDescription(name: string, fallback: string | null | undefined): st
{{ error }} - + + + {{ t('consent.retry', {}, 'Try again') }} + + {{ t('consent.toLogin', {}, 'Back to sign-in') }}
diff --git a/src/frontend-vue/src/views/auth/ForgotPasswordView.vue b/src/frontend-vue/src/views/auth/ForgotPasswordView.vue index 1aa70565..87a07616 100644 --- a/src/frontend-vue/src/views/auth/ForgotPasswordView.vue +++ b/src/frontend-vue/src/views/auth/ForgotPasswordView.vue @@ -1,6 +1,8 @@