Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,29 @@ private static async Task<IResult> 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<ConsentTicket>(denyTicketId);
if (deniedTicket is { DeniedAt: not null } && deniedTicket.Subject == user.Id)
{
return Results.Forbid(
new AuthenticationProperties(new Dictionary<string, string?>
{
[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;

Expand Down
60 changes: 47 additions & 13 deletions src/dotnet/Modgud.Api/Features/Auth/OAuth/ConsentEndpoints.cs
Original file line number Diff line number Diff line change
Expand Up @@ -163,22 +163,41 @@ private static async Task<IResult> 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
{
await session.SaveChangesAsync();
}
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,
});
}

Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -316,4 +344,10 @@ public class ConsentDecision
public class ConsentResult
{
public required string RedirectUrl { get; init; }

/// <summary>True when <see cref="RedirectUrl"/> 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 <c>error=access_denied</c> callback.</summary>
public bool ReturnsToClient { get; init; }
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@ public record RegisterDto
/// non-empty the server quietly rejects (still 200 OK to keep the
/// bot in the dark).</summary>
public string? Honeypot { get; init; }

/// <summary>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 <c>?redirect=</c> so the verify page can forward it
/// to /login; ignored otherwise.</summary>
public string? ReturnUrl { get; init; }
}

/// <summary>Generic response — the same shape regardless of whether the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@ namespace Modgud.Authentication.Api.Account;

public static class MagicLinkEndpoints
{
public record MagicLinkRequestDto(string Email);
/// <summary><paramref name="ReturnUrl"/> 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 <c>?redirect=</c>.</summary>
public record MagicLinkRequestDto(string Email, string? ReturnUrl = null);
public record MagicLinkLoginDto(Guid UserId, string Token);

public static WebApplication MapMagicLinkEndpoints(this WebApplication application, string path)
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,12 @@ namespace Modgud.Authentication.Api.Account;

public static class PasswordResetEndpoints
{
public record ForgotPasswordRequest(string UserName);
/// <summary><paramref name="ReturnUrl"/> 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 <c>?redirect=</c>.</summary>
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)
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down Expand Up @@ -184,6 +184,15 @@ public record ExternalLoginDto(
string? IconName,
string? ButtonColorHex);

/// <summary>
/// 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 '/'.
/// </summary>
private static string SanitizeReturnUrl(string? returnUrl) =>
LoginRedirectGuard.IsSameOriginPath(returnUrl) ? returnUrl! : "/";

/// <summary>
/// Returns true when the request looks like a same-site navigation:
/// either the Origin or the Referer header matches the request host.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
SamlContextBuilder contextBuilder,
SamlSpCertificateService spCertService,
ExternalLoginProcessor processor,
SignInManager<ApplicationUser> signInManager,

Check warning on line 36 in src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlLoginFlow.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

Parameter 'signInManager' is unread.

Check warning on line 36 in src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlLoginFlow.cs

View workflow job for this annotation

GitHub Actions / Backend Build & Test

Parameter 'signInManager' is unread.
ISessionService sessionService,
ISecurityAuditLog securityAudit,
ILogger<SamlLoginFlow> logger)
Expand Down Expand Up @@ -448,8 +448,8 @@
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;
}
}
33 changes: 33 additions & 0 deletions src/dotnet/Modgud.Authentication/Api/LoginRedirectGuard.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
using System.Linq;

namespace Modgud.Authentication.Api;

/// <summary>
/// Open-redirect guard shared by every server-side post-login continuation
/// (external-IdP <c>returnUrl</c>, 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 <c>Location</c> header or appended to
/// an e-mailed URL, so it must not be able to leave our origin.
/// </summary>
public static class LoginRedirectGuard
{
/// <summary>
/// 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 <c>/\t/evil.com</c> — which passes a naive '//' check —
/// collapses to <c>//evil.com</c> (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.
/// </summary>
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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ public async Task<RegisterResponseDto> 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
{
Expand Down Expand Up @@ -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 ?? "";
Expand Down
11 changes: 11 additions & 0 deletions src/dotnet/Modgud.Domain/OAuth/Consent/ConsentTicket.cs
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,15 @@ public class ConsentTicket
/// ticket are rejected — single-use enforced.
/// </summary>
public DateTimeOffset? ConsumedAt { get; set; }

/// <summary>
/// Set (alongside <see cref="ConsumedAt"/>) when the user DENIES the
/// request. The consent endpoint then re-enters <c>/connect/authorize</c>
/// with <c>?deny_ticket={Id}</c>; the authorize endpoint verifies this
/// marker (and <see cref="Subject"/>) before letting OpenIddict emit the
/// RFC 6749 <c>access_denied</c> error to the client — honoring the
/// client's <c>response_mode</c> + RFC 9207 <c>iss</c>, symmetric with the
/// approve path's re-entry.
/// </summary>
public DateTimeOffset? DeniedAt { get; set; }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
using Modgud.Authentication.Api;

namespace Modgud.Tests.Unit.Authentication;

/// <summary>
/// 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 <c>Location</c> header, so every accept/reject
/// case — especially the control-char smuggling that a naive '//' check misses
/// — is pinned here.
/// </summary>
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));
}
}
Loading
Loading