DC-150 Update: resolvedReturnUrl validates url as relative path before passing to router.replace()

Author: noxmwalsh

src/SEBT.Portal.Api/Controllers/Auth/OidcController.cs

@@ -88,6 +88,7 @@ public async Task<IActionResult> GetConfig(
     /// Completes OIDC login when the Next.js server has already exchanged the code and validated the id_token.
     /// Accepts a short-lived callbackToken (JWT signed with Oidc:CompleteLoginSigningKey) containing IdP claims;
     /// copies non-common IdP claims into the portal JWT and returns it.
+    /// Step-up may echo <c>returnUrl</c> only when it is a safe relative path (see <see cref="TrySanitizeStepUpReturnUrl"/>).
     /// </summary>
     [HttpPost("complete-login")]
     [ProducesResponseType(StatusCodes.Status200OK)]
@@ -184,9 +185,48 @@ public async Task<IActionResult> CompleteLogin(
         }
 
         var token = jwtService.GenerateToken(user, additionalClaims);
-        return body.IsStepUp && !string.IsNullOrEmpty(body.ReturnUrl)
-            ? Ok(new { token, returnUrl = body.ReturnUrl })
-            : Ok(new { token });
+        if (!body.IsStepUp)
+            return Ok(new { token });
+
+        var safeReturnUrl = TrySanitizeStepUpReturnUrl(body.ReturnUrl);
+        if (safeReturnUrl != null)
+            return Ok(new { token, returnUrl = safeReturnUrl });
+
+        if (!string.IsNullOrWhiteSpace(body.ReturnUrl))
+            logger.LogWarning("Step-up complete-login: returnUrl rejected (must be a safe relative path).");
+
+        return Ok(new { token });
+    }
+
+    private const int MaxStepUpReturnUrlLength = 4096;
+
+    /// <summary>
+    /// Step-up post-login navigation: only same-document relative paths (for example <c>/profile/address</c>).
+    /// Rejects absolute URLs and scheme-relative paths so the API never echoes an open redirect.
+    /// </summary>
+    private static string? TrySanitizeStepUpReturnUrl(string? returnUrl)
+    {
+        if (string.IsNullOrWhiteSpace(returnUrl))
+            return null;
+        var t = returnUrl.Trim();
+        if (t.Length > MaxStepUpReturnUrlLength)
+            return null;
+        if (!t.StartsWith("/", StringComparison.Ordinal))
+            return null;
+        if (t.StartsWith("//", StringComparison.Ordinal))
+            return null;
+        var pathPart = t;
+        var qIdx = t.IndexOf('?', StringComparison.Ordinal);
+        if (qIdx >= 0)
+            pathPart = t[..qIdx];
+        if (pathPart.Contains("://", StringComparison.Ordinal))
+            return null;
+        if (t.Contains("\\", StringComparison.Ordinal))
+            return null;
+        if (t.Contains("\r", StringComparison.Ordinal) || t.Contains("\n", StringComparison.Ordinal)
+            || t.Contains("\0", StringComparison.Ordinal))
+            return null;
+        return t;
     }
 
     /// <summary>

src/SEBT.Portal.Api/Models/CompleteLoginRequest.cs

@@ -6,6 +6,7 @@ namespace SEBT.Portal.Api.Models;
 /// Request body for completing OIDC login after the Next.js server has exchanged the code and validated the id_token.
 /// CallbackToken is a short-lived JWT signed by the Next.js app (OIDC_COMPLETE_LOGIN_SIGNING_KEY) containing the IdP claims.
 /// When IsStepUp is true, updates the user's IAL level and ID proofing status instead of creating a new session.
+/// When set, <c>returnUrl</c> must be a safe relative path (starts with <c>/</c>, not <c>//</c>, no scheme). Otherwise it is ignored.
 /// </summary>
 public record CompleteLoginRequest(
     [property: JsonPropertyName("stateCode")] string? StateCode,

src/SEBT.Portal.Web/src/features/auth/api/oidc/schema.test.ts

@@ -0,0 +1,57 @@
+import { describe, expect, it } from 'vitest'
+
+import {
+  isSafeOidcStepUpReturnPath,
+  OIDC_RETURN_PATH_MAX_LENGTH,
+  OidcCompleteLoginResponseSchema
+} from './schema'
+
+describe('isSafeOidcStepUpReturnPath', () => {
+  it('accepts relative path with query', () => {
+    expect(isSafeOidcStepUpReturnPath('/profile/address')).toBe(true)
+    expect(isSafeOidcStepUpReturnPath('/dash?q=1')).toBe(true)
+  })
+
+  it('rejects absolute and scheme-relative URLs', () => {
+    expect(isSafeOidcStepUpReturnPath('https://evil.example/')).toBe(false)
+    expect(isSafeOidcStepUpReturnPath('//evil.example/path')).toBe(false)
+  })
+
+  it('allows http in query but not in path segment', () => {
+    expect(isSafeOidcStepUpReturnPath('/redirect?u=http://evil')).toBe(true)
+    expect(isSafeOidcStepUpReturnPath('/http://evil')).toBe(false)
+  })
+
+  it('rejects backslash and newlines in path', () => {
+    expect(isSafeOidcStepUpReturnPath('/path\\evil')).toBe(false)
+    expect(isSafeOidcStepUpReturnPath('/pa\nth')).toBe(false)
+  })
+
+  it('rejects overlong strings', () => {
+    expect(isSafeOidcStepUpReturnPath(`/${'a'.repeat(OIDC_RETURN_PATH_MAX_LENGTH)}`)).toBe(false)
+  })
+})
+
+describe('OidcCompleteLoginResponseSchema', () => {
+  it('parses token with safe returnUrl', () => {
+    const parsed = OidcCompleteLoginResponseSchema.safeParse({
+      token: 'jwt',
+      returnUrl: '/dashboard'
+    })
+    expect(parsed.success).toBe(true)
+    expect(parsed.data?.returnUrl).toBe('/dashboard')
+  })
+
+  it('rejects unsafe returnUrl', () => {
+    const parsed = OidcCompleteLoginResponseSchema.safeParse({
+      token: 'jwt',
+      returnUrl: 'https://evil.example/'
+    })
+    expect(parsed.success).toBe(false)
+  })
+
+  it('allows missing returnUrl', () => {
+    const parsed = OidcCompleteLoginResponseSchema.safeParse({ token: 'jwt' })
+    expect(parsed.success).toBe(true)
+  })
+})

src/SEBT.Portal.Web/src/features/auth/api/oidc/schema.ts

@@ -21,12 +21,32 @@ export const OidcCallbackTokenResponseSchema = z.object({
 
 export type OidcCallbackTokenResponse = z.infer<typeof OidcCallbackTokenResponseSchema>
 
-/** Step-up echoes `returnUrl` from PKCE: same-origin path or absolute URL; not always `z.string().url()`. */
+/** Max length for post-OIDC relative return path (path + query). */
+export const OIDC_RETURN_PATH_MAX_LENGTH = 4096
+
+/** Path segment (before `?`) for scheme checks; query may contain arbitrary values. */
+function oidcReturnPathSegment(v: string): string {
+  const q = v.indexOf('?')
+  return q < 0 ? v : v.slice(0, q)
+}
+
+/** Same rules as API `TrySanitizeStepUpReturnUrl`: relative path only, no open redirects. */
+export function isSafeOidcStepUpReturnPath(v: string): boolean {
+  const t = v.trim()
+  if (t.length === 0 || t.length > OIDC_RETURN_PATH_MAX_LENGTH) return false
+  if (!t.startsWith('/')) return false
+  if (t.startsWith('//')) return false
+  if (oidcReturnPathSegment(t).includes('://')) return false
+  if (t.includes('\\')) return false
+  if (/[\r\n\0]/.test(t)) return false
+  return true
+}
+
 const returnUrlAfterOidcSchema = z
   .string()
   .optional()
-  .refine((v) => v == null || v === '' || v.startsWith('/') || /^https?:\/\//i.test(v), {
-    message: 'returnUrl must be a path or http(s) URL'
+  .refine((v) => v == null || v === '' || isSafeOidcStepUpReturnPath(v), {
+    message: 'returnUrl must be a safe relative path'
   })
 
 export const OidcCompleteLoginResponseSchema = z.object({

test/SEBT.Portal.Tests/Unit/Controllers/OidcControllerTests.cs

@@ -190,6 +190,65 @@ public async Task CompleteLogin_WhenStepUpAndNoExistingUser_Returns400()
         await _userRepository.DidNotReceive().UpdateUserAsync(Arg.Any<User>(), Arg.Any<CancellationToken>());
     }
 
+    [Fact]
+    public async Task CompleteLogin_WhenStepUpAndSafeReturnUrl_Returns200WithReturnUrl()
+    {
+        const string signingKey = "complete-login-signing-key-at-least-32-characters-long";
+        _config["Oidc:CompleteLoginSigningKey"].Returns(signingKey);
+
+        var callbackToken = CreateValidCallbackToken(signingKey, email: "user@example.com");
+        var body = new CompleteLoginRequest(
+            CoStateKey,
+            callbackToken,
+            IsStepUp: true,
+            ReturnUrl: "/profile/address?q=1");
+
+        var user = new User { Id = 1, Email = "user@example.com" };
+        _userRepository.GetUserByEmailAsync("user@example.com", Arg.Any<CancellationToken>())
+            .Returns(user);
+        _userRepository.UpdateUserAsync(Arg.Any<User>(), Arg.Any<CancellationToken>())
+            .Returns(Task.CompletedTask);
+
+        const string portalJwt = "portal-jwt-returned-by-service";
+        _jwtService.GenerateToken(Arg.Any<User>(), Arg.Any<IReadOnlyDictionary<string, string>?>())
+            .Returns(portalJwt);
+
+        var result = await _controller.CompleteLogin(body, CancellationToken.None);
+
+        var okResult = Assert.IsType<OkObjectResult>(result);
+        var valueType = okResult.Value!.GetType();
+        Assert.Equal("/profile/address?q=1", valueType.GetProperty("returnUrl")!.GetValue(okResult.Value) as string);
+        Assert.Equal(portalJwt, valueType.GetProperty("token")!.GetValue(okResult.Value) as string);
+    }
+
+    [Fact]
+    public async Task CompleteLogin_WhenStepUpAndExternalReturnUrl_OmitsReturnUrlFromResponse()
+    {
+        const string signingKey = "complete-login-signing-key-at-least-32-characters-long";
+        _config["Oidc:CompleteLoginSigningKey"].Returns(signingKey);
+
+        var callbackToken = CreateValidCallbackToken(signingKey, email: "user@example.com");
+        var body = new CompleteLoginRequest(
+            CoStateKey,
+            callbackToken,
+            IsStepUp: true,
+            ReturnUrl: "https://evil.example/phish");
+
+        var user = new User { Id = 1, Email = "user@example.com" };
+        _userRepository.GetUserByEmailAsync("user@example.com", Arg.Any<CancellationToken>())
+            .Returns(user);
+        _userRepository.UpdateUserAsync(Arg.Any<User>(), Arg.Any<CancellationToken>())
+            .Returns(Task.CompletedTask);
+
+        _jwtService.GenerateToken(Arg.Any<User>(), Arg.Any<IReadOnlyDictionary<string, string>?>())
+            .Returns("portal-jwt");
+
+        var result = await _controller.CompleteLogin(body, CancellationToken.None);
+
+        var okResult = Assert.IsType<OkObjectResult>(result);
+        Assert.Null(okResult.Value!.GetType().GetProperty("returnUrl"));
+    }
+
     private static string CreateValidCallbackToken(string signingKey, string email)
     {
         return CreateCallbackTokenWithClaims(signingKey, new Claim("email", email));