fix(auth): prevent open redirect on OIDC login returnUrl

[sfmskywalker]

Summary

Closes the OIDC Blazor Server open redirect (returnUrl=/%09/evil.com) and the Round 2 regressions from treating scheme-less relative paths as local.

LocalReturnPath.Normalize now returns only a rooted local path (single leading /, not // or /\\) or /. Validation still runs on a decoded copy and the original string is returned when it is safe. Decoding that does not settle fails closed.

Legacy ElsaLogin and ElsaIdentityLoginMethod root base-relative values against NavigationManager.BaseUri so workflows/x under /studio/ becomes /studio/workflows/x. Drive-letter relatives such as c|/windows no longer reach Blazor as scheme-less targets.

AuthenticationController always calls Url.IsLocalUrl (no ?.). Controller tests construct a real UrlHelper with ControllerActionDescriptor. External Authentication callback/logout/logout-callback sinks LocalRedirect relatives to /.

Fixes #1106

Test plan

• Shared corpus rejects http:evil.com, encoded javascript:, tab-before-//, and c|\u2026 as /
• Relative workflows/x rows expect / at Normalize and server sinks
• ElsaLogin roots workflows/x against PathBase (/studio/workflows/x)
• Controller tests exercise Url.IsLocalUrl via a real UrlHelper
• Full Elsa.Studio.ExternalAuthentication.Tests suite: 474 passed, 0 failed (net10.0)

  

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 11f81123-136a-45bf-bc1c-70011b85e69b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

[cursor]

@greptileai

[greptile-apps]

Confidence Score: 5/5

[Critical risk] Fixes open redirect vulnerability in authentication return paths.

No outstanding findings block merging.

Summary

Return-path validation now preserves safe encoded URLs, roots legacy login destinations against the Studio base path, and keeps server broker redirects rooted. The three previously reported issues are fixed.

_{Reviews (4) · Last reviewed commit: "test(auth): construct UrlHelper with Con..."}

[sfmskywalker]

Elsa 3 Code Review: REQUEST_CHANGES + HIGH @ ea8535b

Code Review, Round 1/4

Scope: elsa-studio#1105 (release/3.9.0, base bd44366) — fix for the OIDC Blazor Server open redirect (#1106). One commit, 13 files.

Verdict: the open redirect is fixed. The original repro and every bypass in the corpus below now redirect to /. However, LocalReturnPath.Normalize returns the decoded string instead of the caller's string. That corrupts legitimate return URLs and, after OIDC sign-in, turns encoded non-ASCII paths into an HTTP 500 — a regression introduced by this PR. Two related return-path regressions are below. All three are small fixes; a proposed Normalize that keeps the security result and fixes the regressions is included and was run against the same corpus.

How this was verified

• Built the PR head and the base with the .NET 10 SDK.
• Hosted the real AuthenticationController on Kestrel: real model binding, real antiforgery on POST logout. A stand-in authentication handler does what the OIDC/cookie handlers do after the IdP round trip: Response.Redirect(properties.RedirectUri). Raw Location headers were read for GET /authentication/login?returnUrl=… and POST /authentication/logout. No end-to-end run against a real IdP.
• Elsa.Studio.ExternalAuthentication.Tests: 348/348 pass at the head.
• Revert check: with the decode/strip step removed from Normalize, 9 of the new/updated tests fail. The tests do guard the fix.

1. Original repro and bypass corpus

Payloads are as typed in the query string, so model binding decodes them once. Results are the Location header for login and logout; both behave the same.

Payload	Base bd44366	Head ea8535b
/%09/evil.com (original repro)	/<TAB>/evil.com (browser → //evil.com)	/
//evil.com, /\evil.com, \\evil.com	/	/
/%2F/evil.com, /%5C/evil.com, %2F%2Fevil.com, /%2F%2Fevil.com	/	/
Double encoded: /%2509/…, /%252F/…, /%255C/…, %252F%252F…	/%09/…, /%2F/…, /%5C/… passed through (same-origin path)	/
Triple encoded: /%25252F/…, %25252F%25252F…	passed through	/
Full-width/Unicode slashes ／／evil.com, %EF%BC%8F%EF%BC%8Fevil.com	/	/
/／/evil.com, /＼/evil.com, /∕/evil.com, /​/evil.com (ZWSP), //evil.com (BOM)	500 (non-ASCII header)	500 (unchanged, not a redirect)
//evil.com, /%09/evil.com, %20//evil.com	/	/
/ /evil.com, /%20/evil.com	/ /evil.com	/ /evil.com (same-origin path)
javascript:, JaVaScRiPt:, data:, mailto:, http:evil.com, https:/evil.com, http://, HTTPS://, %68ttps://	/	/
/javascript:alert(1)	same	same-origin path
/..//evil.com, /.//evil.com, /./ /evil.com, /%2E%2E//evil.com	same	passed through as same-origin paths. A browser resolves /..//evil.com to https://studio//evil.com, not to evil.com. Url.IsLocalUrl accepts these too.
/\/evil.com, \/evil.com, /<TAB>\evil.com, /%5C%5Cevil.com	mostly /; /<TAB>\evil.com passed through	/
Control characters %00, %0B, %0C, %7F, %0D%0A, %C2%85	500	/ (logout %00 → 400 from form parsing)
Malformed escapes /%/…, /%zz//…, /%E0%A4%A/…	passed through	passed through unchanged (no throw; same-origin)
9× encoded //evil.com without leading /	—	/
/ + 9× encoded //evil.com	—	returns /%2F%2Fevil.com: the decode loop hits its 8-pass cap and returns a partly decoded value. It is same-origin at every sink checked, but see item 2.
1,000,001-char deeply encoded input / 1M plain chars	—	37 ms / 98 ms; the loop terminates. Kestrel's request-line limit caps real inputs far lower. No DoS.

No bypass found.

2. Allowlist vs denylist; second layer at the sink

IsSafeLocalPath is a hybrid:

• It positively requires a leading /, which rules out every scheme.
• It then rejects //, any \, and any ://.
• The positive rule is the right one. It is the same rule Url.IsLocalUrl uses.

What makes it not obviously correct is the decode-then-return design (item 3, B1). Other problems:

• Partial decode at the cap: when the loop hits its 8-pass cap it returns a partly decoded value instead of failing closed.
• Strips instead of rejecting: it silently removes control characters, where rejecting them would be simpler and stricter.
• False positives from the :// check: it rejects legitimate /workflows?ref=https://docs. Verified: that now becomes /, while base kept it. This is the open CodeRabbit-style concern on the core port.
• Redundant IsAbsoluteUri check: Uri.TryCreate(…, UriKind.Relative, …) && !uri.IsAbsoluteUri can never fail its second half.
• Dead catch (UriFormatException): Uri.UnescapeDataString does not throw on malformed sequences in .NET. Verified with /%zz//evil.com.

Second layer at each sink:

• External Authentication Server controller: has one. It uses LocalRedirect(...) on its success, logout and logout-callback paths, and Redirect(ChooserUrl(...)) only to a constant /login?….
• CultureController: has one, Url.IsLocalUrl + LocalRedirect("~/"). The fallback keeps PathBase.
• OIDC AuthenticationController: has none. Challenge/SignOut hand RedirectUri to the handler, which redirects without validating. Normalize output always satisfies IsLocalUrl, so this is optional. A one-line Url.IsLocalUrl(path) ? path : "/" would make the controller self-evidently safe.
• Blazor NavigateTo sinks: have no framework equivalent of a second layer.

3. Correctness of legitimate paths

B1 (blocking): Normalize returns the fully decoded string, so legitimate encoded return URLs change meaning. ChallengeToLogin sends Uri.EscapeDataString("/" + relativePath), so the controller receives the already-encoded path and query, which Normalize then decodes again.

Return URL the controller receives	Base Location	Head Location
/workflows/instances?search=a%26b	…?search=a%26b	…?search=a&b (one parameter becomes two)
/workflows/instances?search=100%25	…?search=100%25	…?search=100% (malformed)
/workflows/definitions/abc%2Fdef/edit	unchanged	/workflows/definitions/abc/def/edit (different route)
/workflows/definitions?name=a%2Bb	unchanged	…?name=a+b (parsed as a space)
/caf%C3%A9, …?search=%E2%82%AC	unchanged (ASCII)	HTTP 500: the decoded é/€ is not a valid Location header value. With real OIDC this happens on the callback, after the user has signed in at the IdP.
/workflows?ref=https%3A%2F%2Fdocs	…?ref=https://docs	/
/workflows/definitions?x=1, …?x=1#frag	unchanged	unchanged ✓

Each hop that normalizes again decodes one more layer. Examples are ChooserUrl → LoginPanel, and the External Authentication login → callback.

Fix: validate a decoded copy, but return the caller's original string. The following was run against the full corpus above. Every attack row still yields /, every legitimate row above round-trips byte-for-byte, and / + 9× encoded now fails closed to /:

public static class LocalReturnPath
{
    private const int MaxDecodePasses = 8;

    public static string Normalize(string? candidate)
    {
        if (string.IsNullOrEmpty(candidate))
            return "/";

        var decoded = candidate;
        for (var pass = 0; pass < MaxDecodePasses; pass++)
        {
            var next = Uri.UnescapeDataString(decoded);
            if (next == decoded)
                return IsRootedLocalPath(candidate) && IsRootedLocalPath(decoded) ? candidate : "/";
            decoded = next;
        }

        return "/"; // Still changing after MaxDecodePasses: refuse rather than guess.
    }

    // A single leading '/', not followed by another '/', and no backslash or control character anywhere.
    private static bool IsRootedLocalPath(string path) =>
        path.StartsWith('/') &&
        !path.StartsWith("//", StringComparison.Ordinal) &&
        !path.Any(c => c == '\\' || char.IsControl(c));
}

Checking only the fully decoded form and the original is enough. Decoding never removes a literal /, \ or control character, so any intermediate layer that starts with // or contains one of them carries it into the final layer.

B2 (blocking): the legacy Elsa Login page loses its destination. With Authentication:Provider = ElsaLogin, ElsaIdentityAuthorizationService sends /login?returnUrl=workflows/definitions, a base-relative path from ToBaseRelativePath with no leading /. Login.razor.cs:59 now normalizes that to /, so after sign-in users land on the home page instead of the page they asked for. Base navigated to the base-relative path. This is the open Greptile P1.

• Fix both ends. Have the service send a rooted, escaped path (e.g. Uri.EscapeDataString(new Uri(navigationManager.Uri).PathAndQuery), which also keeps PathBase). On the page, root a base-relative value before normalizing, for links already in flight.
• Add a test for each end.

B3 (should fix): the legacy OIDC state now drops PathBase.

• Cause: NormalizeStateReturnUrl prefixes / to the base-relative path stored in state (workflows/x → /workflows/x).
• Base: NavigateTo("workflows/x") resolved against the base URI, so sub-path hosts (/studio/…) returned correctly.
• Head: the root-relative /workflows/x goes to the host root and drops PathBase.
• Fix: store new Uri(navigationManager.Uri).PathAndQuery, which is rooted and includes PathBase, as the core main version of this service already does. Keep the prefix only as a fallback for in-flight state.
• The :// clause in that helper is unnecessary once Normalize is fixed.
• I read this code; I didn't run it.

Other legitimate-path checks:

• Fragments: preserved.
• Culture redirects: the Server culture service sends a rooted PathAndQuery that includes PathBase, IsLocalUrl accepts it, and the new ~/ fallback replaces what used to be a 500 from LocalRedirect on a non-local value. No regression.
• Sub-path hosting for the OIDC controller, Elsa Identity and External Authentication: these already use root-relative paths, a pre-existing issue tracked in #1112. Not introduced here.

4. Coverage: every return-URL sink

Sink	Uses Normalize?
OIDC AuthenticationController.Login → Challenge(RedirectUri)	✓
OIDC AuthenticationController.Logout → SignOut(RedirectUri) → OidcSignOutEvents Response.Redirect(Properties.RedirectUri)	✓ (normalized upstream)
LoginPanel → LoginMethodComponentContext.ReturnPath → ElsaIdentityLoginMethod NavigateTo(Context.ReturnPath), and Server DirectOpenIdConnectLoginMethod	✓
ServerExternalAuthenticationLoginCoordinator (both NavigateTos)	✓
ExternalAuthenticationController Login (transaction + return_path), Callback success, ChooserUrl, Logout, LogoutCallback	✓ (+ LocalRedirect)
WASM BrowserExternalAuthenticationPkceService and ExternalAuthenticationWasmCallbackService.CompleteAsync → ExternalAuthenticationCallback NavigateTo	✓ (via ExternalAuthenticationReturnPath); CompleteLogout returns a constant /
Legacy Elsa.Studio.Login Login.razor.cs NavigateTo(returnUrl)	✓, but see B2
Legacy OpenIdConnectAuthorizationService.ReceiveAuthorizationCode (state)	✓, but see B3
CultureController.Set	IsLocalUrl + LocalRedirect ✓
WASM OIDC (RemoteAuthenticatorView)	Framework same-origin returnUrl check; not routed through Normalize. Acceptable.
UserTask.razor ?returnUrl=	Not auth, and not changed here. It navigates only to PathAndQuery of a same-host absolute URL, so it can't leave the origin.

No missed sink. One thing to note: the configuration guards that use Normalize(configuredPath) != configuredPath would start throwing for a configured callback path containing a % escape. That's harmless today, and B1's fix removes it.

5. Maintainability and tests (high bar)

• Normalize should be readable at a glance. The proposed version above is about 15 lines with one rule. Today's version is decode-loop + trim + strip + four rejections + Uri.TryCreate, plus dead and redundant branches.

• Wrappers: three delegating wrappers remain:

  • Elsa.Studio.ExternalAuthentication.Services.LocalReturnPath, a second type with the same name;
  • ExternalAuthenticationReturnPath;
  • LoginPanel.NormalizeReturnPath.

  Please call the shared type directly, at least in LoginPanel. Mark the public wrappers [Obsolete] if they have to stay for compatibility.

• Tests: the same corpus is copied into four files. LoginChooserTests.InvalidReturnPaths_AreNeverForwarded grows a list of Asserts instead of a table. Please:

  • Put one table-driven corpus in LocalReturnPathTests, covering the attack rows above plus the legitimate encoded rows from item 3 (a%26b, 100%25, abc%2Fdef, a%2Bb, caf%C3%A9, ?ref=https://…, fragment). The legitimate rows would have caught B1.
  • Have the wrapper and controller tests assert delegation with two or three rows each.
  • Add a controller row with the model-bound form "/\t/evil.com". The current controller rows pass the still-encoded "/%09/evil.com", which isn't what binding delivers. The Normalize fact does cover the bound form.
  • Add tests for Login.razor.cs (B2), OpenIdConnectAuthorizationService state handling (B3) and CultureController. None is covered today.

6. CI, Greptile, review threads

• CI: all checks are green at ea8535b (Build and test, GitGuardian, CLA). CodeRabbit skipped because this is not the default branch.
• Greptile: 4/5. Below the required 5/5. Two threads are open:
  • P1 "Login loses return destination" (Login.razor.cs:59) = B2;
  • P2 "Encoded query values change" (LocalReturnPath.cs:51) = B1. I'd treat it as blocking because of the 500.

Required before approval

1. B1: validate a decoded copy, return the original, and fail closed when decoding doesn't settle. Add the legitimate-encoding test rows.
2. B2: legacy Elsa Login keeps base-relative destinations; producer and consumer both fixed, with tests.
3. B3: keep PathBase in the legacy OIDC state.
4. Greptile 5/5 with both threads resolved, and CI green on the new head.

[cursor]

@greptileai

[sfmskywalker]

Elsa 3 Code Review: REQUEST_CHANGES + HIGH @ 7ea154f

Code Review, Round 2/4

Scope: elsa-studio#1105, new commit 7ea154f on top of ea8535b. Base release/3.9.0 @ bd44366.

Verdict: the Round 1 blockers are fixed:

• legitimate links now come back byte-for-byte;
• the 9×-encoded case fails closed;
• the legacy OIDC state keeps PathBase;
• the tests are shared and table-driven.

The new "relative paths are local" rule is the problem. A bare value is checked as /<input> but returned as <input>, and two sinks resolve bare values differently from the check:

1. Blazor's NavigationManager resolves Windows drive-letter forms to file:. c|/windows becomes file:///c:/windows.
2. ASP.NET LocalRedirect throws on any relative value, so the External Authentication Server callback and logout now return HTTP 500 for a relative return path.

No attacker-host redirect was found. Both are regressions from Round 1, which rejected every non-rooted value. The simplest fix is to keep Normalize rooted-only and root the one legacy caller that needs base-relative paths (sketch below).

How this was verified

• Builds and tests: built the head with the .NET 10 SDK. Elsa.Studio.ExternalAuthentication.Tests passes 491/491.
• Real controller: hosted the real AuthenticationController on Kestrel with UsePathBase("/elsa"), real model binding, real antiforgery on POST logout, and the real Url helper. A stand-in handler does what the OIDC/cookie handlers do after the IdP round trip, Response.Redirect(properties.RedirectUri). Raw Location headers were read. No end-to-end run against a real IdP.
• Simulated sinks: for every probe, Normalize's output was then resolved two ways:
  • Blazor Server: a real NavigationManager (ToAbsoluteUri, base https://studio.example/elsa/). Blazor Server's NavigateTo resolves the same way before navigating.
  • WASM: the WHATWG URL parser (Node new URL(value, document.baseURI)), which is what Blazor WASM's NavigateTo uses in the browser.
• The legacy ElsaLogin page and the login chooser call the same Normalize and then NavigateTo. They are covered by the NavigationManager and WHATWG columns.
• Corpus: 119 probes: the Round 2 relative-path hunt, the Round 1 corpus, and legitimate links.

1. The relative-path hunt

Each value is as received by Normalize, i.e. after one round of query decoding.

Probe	Normalize	Controller Location (login/logout)	Blazor Server resolution	WASM resolution
http:evil.com, https:evil.com, HTTP:evil.com, hTtPs:evil.com, https:/evil.com, http:\\evil.com, ftp:evil.com	/	/	studio	studio
javascript:alert(1) (all cases), javascript:alert(1)?x, data:text/html,…, vbscript:, mailto:, file:///…, blob:https://evil.com/x	/	/	studio	studio
javascript%3Aalert(1), %6Aavascript:, %6A%61vascript:, javascript%253A…, java%09script:, java<TAB>script:, java<LF>script:, <SOH>javascript:, <NUL>javascript:	/	/	studio	studio
\evil.com, \/evil.com, /\evil.com, <TAB>//evil.com, //evil.com, <LF>//evil.com, <NBSP>//, <U+3000>//, <U+2028>//	/	/	studio	studio
%2F%2Fevil.com, %5C%5Cevil.com, %2F%5Cevil.com, %20//evil.com, %09//evil.com, //user@evil.com	/	/	studio	studio
evil.com:443, evil.com:443/x, localhost:8080, C:/x, C:\x, x:y	/	/	studio	studio
@evil.com, user@evil.com, evil.com, evil.com/x	unchanged	/	https://studio.example/elsa/@evil.com etc.	same
.//evil.com, ..//evil.com, ../../../../..//evil.com, ./%2F/evil.com	unchanged	/	https://studio.example//evil.com (same origin)	same
<BOM>//evil.com, <ZWSP>//evil.com, <U+180E>//evil.com	unchanged	/	/elsa/%EF%BB%BF//evil.com (same origin)	same
Unicode lookalikes ｈttps:evil.com, https：evil.com, javascript：alert(1), ／／evil.com, ⁄⁄, ∕∕, ﹨﹨, ǃevil.com	unchanged	/	same-origin path (percent-encoded)	same
~//evil.com, #x:evil, ?x=javascript:alert(1), a?b:c	unchanged	/	same origin	same
~/x	unchanged	~/x (IsLocalUrl accepts ~/; Response.Redirect doesn't expand it, so the browser lands on /elsa/~/x, a harmless 404)	same origin	same
c|/windows, C|/x, z|/a/b, c|//evil.com/x	unchanged	/	file:///c:/windows, file:///C:/x, file:///z:/a/b, file:///c://evil.com/x	https://studio.example/elsa/c|/windows
c|evil.com, a|b	unchanged	/	UriFormatException (thrown inside NavigateTo)	same origin

Finding A (blocking): drive-letter forms leave the origin on Blazor Server. HasSchemeOrHost looks for a scheme:, but .NET's Uri also treats <letter>|/… as a DOS path. When the legacy ElsaLogin page or ElsaIdentityLoginMethod calls NavigateTo in a Blazor Server host, a crafted /login?returnUrl=c%7C/windows sends the browser to file:///c:/windows after sign-in. A value like c|evil.com instead throws UriFormatException from NavigateTo.

• Impact: browsers refuse https → file: navigation, so no attacker host is reachable. The real harm is a stuck or failed post-login page.
• Why it blocks: it breaks the helper's stated contract ("stays inside the Studio host"), and Round 1 rejected these inputs.
• Scope: WASM is unaffected, because the browser resolves the raw string relative to the page.
• Rooted forms are safe: /c|/windows resolves on-origin (https://studio.example/c%7C/windows).

Finding B (blocking): relative values crash the External Authentication Server sinks. ExternalAuthenticationController passes Normalize's output to LocalRedirect in three places:

• the callback success path (LocalRedirect(LocalReturnPath.Normalize(transaction.ReturnPath)));
• Logout (LocalRedirect(safeReturnPath));
• LogoutCallback.

LocalRedirect requires IsLocalUrl, which rejects relative paths. Verified: LocalRedirect(Normalize("workflows/x")) throws InvalidOperationException: The supplied URL is not local, which surfaces as HTTP 500. Any returnPath=workflows/x on /authentication/external/login/{key}, or on the logout form, therefore ends in a 500 after a successful sign-in or sign-out. Round 1 returned /. This is the open Greptile thread "Root broker return paths".

Your question about ElsaLogin and the second check: the ElsaLogin path never reaches a controller. Login.razor.cs calls NavigationManager.NavigateTo(LocalReturnPath.Normalize(returnUrl), true) directly. So the new Url.IsLocalUrl check in the OIDC controller does nothing for it, and the bare relative value goes straight to NavigateTo (Finding A). In the OIDC controller itself, the second check does send every relative value to / (the controller Location column above). That is safe, but it means relative values work at one sink, 500 at another, and resolve to file: at a third.

Finding C (tests): the controller tests never exercise the second check.

• The controller tests use new AuthenticationController(), whose Url is null. Verified.
• SafeRedirectUri uses Url?.IsLocalUrl(...) == false, so in tests the second check is skipped entirely.
• As a result the shared corpus asserts Login("workflows/x").RedirectUri == "workflows/x", while the real controller returns /. The tests describe behaviour production doesn't have.

Fix: give the controller a real UrlHelper in tests (controller.Url = new UrlHelper(new ActionContext(new DefaultHttpContext(), new RouteData(), new ActionDescriptor()))), drop the ?., and expect / for relative rows at the controller.

Proposed fix for A, B and C.

• Revert Normalize to rooted-only: drop IsLocalDestination and HasSchemeOrHost, keep the Round 2 decode-and-return-original loop. That deletes about 45 lines.
• Root the single legacy caller against the app's base path:

// Elsa.Studio.Login/Pages/Login/Login.razor.cs
var basePath = new Uri(NavigationManager.BaseUri).AbsolutePath;   // "/" or "/elsa/"
var rooted = returnUrl is { Length: > 0 } && returnUrl[0] != '/' ? basePath + returnUrl : returnUrl;
NavigationManager.NavigateTo(LocalReturnPath.Normalize(rooted), true);

This keeps ElsaLogin deep links and PathBase. It turns c|/windows into /elsa/c|/windows (on-origin), keeps LocalRedirect happy everywhere, and leaves the shared helper with one obvious rule: "a rooted path or /".

Optionally, also have ElsaIdentityAuthorizationService send Uri.EscapeDataString(new Uri(navigationManager.Uri).PathAndQuery). It currently sends an unescaped base-relative value, so a & in the original query gets truncated; that is pre-existing.

2. Round 1 items

• 9×-encoded //evil.com: bare and /-prefixed both give / ✓. A 9×-encoded javascript: also gives / ✓.
• Legitimate links, byte-identical in Location, NavigationManager and WASM:
  • ?search=a%26b, ?search=100%25, /abc%2Fdef/edit, ?name=a%2Bb, ?ref=https://docs, ?ref=https%3A%2F%2Fdocs, #frag ✓;
  • /caf%C3%A9 and ?search=%E2%82%AC give 302, no 500 ✓.
• PathBase:
  • /elsa/workflows/definitions?x=1 round-trips under UsePathBase("/elsa") ✓;
  • legacy OIDC CaptureReturnPath stores PathAndQuery, so /elsa/studio/… is kept ✓ (test LegacyOidcStateHelperKeepsPathBase);
  • the OIDC controller and Elsa Identity remain root-relative, a pre-existing issue tracked in #1112.
• Round 1 corpus: unchanged results. Every attack row gives /. The same-origin pass-throughs (/ /evil.com, /..//evil.com, /./ /evil.com, /%zz//…) are as in Round 1. Raw non-ASCII /／/evil.com is still a 500, as before the PR; it is not a redirect.

3. Tests and corpus

• Corpus: one LocalReturnPathCorpus (34 rows) is used by LocalReturnPathTests, DirectOpenIdConnectLoginTests (login + logout), ExternalAuthenticationWasmTests and LoginChooserTests. It's readable and table-driven ✓.
• Fails when reverted:
  • pre-fix logic: 33 failures;
  • "return decoded" (the Round 1 behaviour): 36 failures;
  • "reject relative": 15 failures.
• Missing rows for this round's risk: http:evil.com, https:evil.com, javascript%3Aalert(1), %6Aavascript:alert(1), <TAB>//evil.com, c|/windows, ~//evil.com. Today the only scheme-without-slashes row is javascript:alert(1). Under the proposed rooted-only rule these become simple / rows.
• Trim duplicates:
  • five https://attacker…/http://evil… variants;
  • three …%26… query rows;
  • Normalize_PreservesEncodedLinksByteForByte, which repeats five corpus rows.
• Leftovers: catch (UriFormatException) around Uri.UnescapeDataString is still dead code; it never throws for malformed input.
• Missing tests: none yet for Login.razor.cs (ElsaLogin) or for the External Authentication LocalRedirect sinks. Either would have caught A or B.

4. CI, Greptile, threads

• CI at 7ea154f: all green (Build and test, GitGuardian, CLA). CodeRabbit skipped because this is not the default branch.
• Greptile: 4/5 on 7ea154f. Still below the required 5/5.
• Threads:
  • Both Round 1 threads are resolved.
  • One new thread is open: Greptile "Root broker return paths" (LocalReturnPath.cs:61) = Finding B.

Required before approval

1. A and B: Normalize returns only rooted paths (or /). ElsaLogin's base-relative value is rooted against BaseUri at the legacy page. Add tests for Login.razor.cs and for the External Authentication LocalRedirect sinks.
2. C: controller tests use a real UrlHelper, with no ?. seam.
3. Add the missing scheme-without-slashes and drive-letter rows to the corpus; trim the duplicates.
4. Greptile 5/5 with the new thread resolved, and CI green on the new head.

[sfmskywalker]

@greptileai

[sfmskywalker]

Elsa 3 Code Review: APPROVE + HIGH @ 8bb6b25

Code Review, Round 3/4

Scope: elsa-studio#1105, new commits cd6f22e and 8bb6b25 on top of 7ea154f. Base release/3.9.0 @ bd44366.

Verdict: approve. The open redirect from #1106 is closed and Round 2's Findings A, B and C are fixed. 145 probes give zero off-site navigations at every sink. Legitimate links come back byte-for-byte, PathBase is kept, and the security-relevant tests fail when the fix is reverted. CI is green, Greptile is 5/5 on this head, and there are no open threads. Two small test gaps are listed below as optional follow-ups; neither is exploitable.

How this was verified

• Builds and tests: built the head with the .NET 10 SDK. Elsa.Studio.ExternalAuthentication.Tests passes 474/474.
• Real controller: hosted the real AuthenticationController on Kestrel under UsePathBase("/studio"), with real model binding, real antiforgery on POST logout and the real Url helper. A stand-in handler does what the OIDC/cookie handlers do after the IdP round trip, Response.Redirect(properties.RedirectUri). Raw Location headers were read.
• Per probe:
  • Normalize(value) and RootAgainstBase(value, "https://studio.example/studio/");
  • Blazor Server resolution of both, via a real NavigationManager.ToAbsoluteUri;
  • WASM resolution of both, via the WHATWG URL parser.
• Probes: 145 in total: the full Round 2 set (122, including the drive-letter variants) plus 23 new BaseUri-rooting probes.

1. Results

Zero off-site results, and no exception, across all 145 probes at every sink: controller login/logout Location, plus Blazor Server and WASM resolution of both Normalize and RootAgainstBase output.

The BaseUri rooting used by ElsaLogin, LoginPanel and ElsaIdentityLoginMethod:

Probe	RootAgainstBase under /studio/	Blazor Server / WASM
workflows/x, workflows/definitions?tab=active, workflows/instances?search=a%26b, caf%C3%A9, ?x=1, #frag	/studio/… (byte-for-byte)	https://studio.example/studio/…
/evil.com	/evil.com (already rooted, not re-rooted)	https://studio.example/evil.com
\evil.com, ..\..\evil.com, %5Cevil.com, /%2Fevil.com, <TAB>, %09/evil.com	/	studio root
%2F/evil.com, %2f%2fevil.com, %252F%252Fevil.com, %E2%80%8B//evil.com	/studio/%2F/evil.com etc.	same-origin path (%2F stays encoded)
../../evil.com, ../..//evil.com, %2E%2E/%2E%2E/%2Fevil.com	/studio/../../evil.com etc.	https://studio.example/evil.com, https://studio.example//evil.com (same origin)
..%2F..%2Fevil.com, ..%2F..%2F%2Fevil.com	/studio/..%2F..%2F…	same-origin path
c|/windows, C|/x, z|/a/b, c|//evil.com/x, c|evil.com, a|b	/studio/c|/windows etc.	https://studio.example/studio/c%7C/windows etc. (no file:, no throw)
http:evil.com, javascript:alert(1) and the other scheme rows	/studio/http:evil.com etc.	same-origin path

Your questions:

• Is Normalize applied after rooting? Yes. RootAgainstBase prefixes the base path, then calls Normalize on the combined string. Already-rooted input skips the prefix and is normalized directly.
• Can a rooted value go off-site? No. Once a value starts with /<base>/, no relative input escapes the origin; dot segments can climb at most to the host root.
• Is PathBase doubled? Not for a rooted value that already carries it: /studio/workflows/x stays /studio/workflows/x. A base-relative value that itself starts with the base segment (studio/workflows/x) becomes /studio/studio/workflows/x, but no Studio producer emits that form, because ToBaseRelativePath strips the base.

2. Round 2 findings

• A (drive letters → file:), fixed.
  • Normalize is rooted-only, so c|/windows → / at the controller and at every Normalize sink.
  • Through RootAgainstBase it becomes /studio/c|/windows, which resolves on-origin.
  • No probe produces file: or UriFormatException.
• B (LocalRedirect 500 on relative paths), fixed. Relative values now normalize to /. Three new tests pin this with LocalRedirectResult.Url == "/":
  • Callback_RelativeReturnPath_LocalRedirectsToRoot;
  • Logout_RelativeReturnPath_LocalRedirectsToRoot;
  • LogoutCallback_RelativeReturnPath_LocalRedirectsToRoot.
• C (null Url test seam), fixed.
  • Controller tests build a real UrlHelper over a ControllerActionDescriptor, and SafeRedirectUri uses !Url.IsLocalUrl(path) with no ?..
  • The corpus now expects / for relative rows at the controller, matching production. The Kestrel harness confirms workflows/x → Location: /.
  • Removing Normalize from the controller, leaving only IsLocalUrl, fails 6 corpus rows, so both layers are exercised.

3. Legitimate links

These came back byte-identical in Normalize, RootAgainstBase and both Location headers, with no 500:

• /workflows/definitions?x=1, …#frag
• ?search=a%26b, ?search=100%25, /abc%2Fdef/edit, ?name=a%2Bb
• ?ref=https://docs, ?ref=https%3A%2F%2Fdocs
• /caf%C3%A9, ?search=%E2%82%AC
• /studio/workflows/definitions?x=1

The legacy OIDC state still captures PathAndQuery, so PathBase is kept, and LegacyOidcStateHelperKeepsPathBase covers it. The Round 1 corpus results are unchanged.

4. Tests and readability

• LocalReturnPath: a single rule (IsRootedLocalPath) plus the decode loop, about 55 lines including RootAgainstBase. The dead catch is gone. Easy to read.
• Corpus: LocalReturnPathCorpus now has 27 explicit rows plus 3 encoded rows. Duplicates are removed. The Round 2 gap rows were added: http:evil.com, javascript%3A…, %6Aavascript:, <TAB>//, c|/windows, c|evil.com.
• Revert and mutation checks:
  • pre-fix Normalize: 17 failures;
  • Normalize returning the decoded string: 20;
  • RootAgainstBase not rooting: 1;
  • RootAgainstBase skipping Normalize: 1;
  • controller without Normalize: 6.

Optional follow-ups (not blocking, not exploitable):

1. A mutation that skips Normalize only on RootAgainstBase's relative branch survives the suite. Adding ResolveReturnUrl("\\evil.com", studioBase) == "/" to ElsaLogin_RootsBaseRelativePathsAgainstPathBase pins "normalize after rooting".
   • Not exploitable: a /studio/ prefix cannot be resolved off-site.
2. Reverting the rooting in LoginPanel or in ElsaIdentityLoginMethod fails no test. One bUnit assertion would pin the deep-link behaviour: LoginPanel with base https://studio.example/studio/ and ReturnPath="workflows/x" gives Context.ReturnPath == "/studio/workflows/x".
   • Not exploitable: without rooting, the value falls back to /.

5. CI, Greptile, threads

• CI at 8bb6b25: all green (Build and test, GitGuardian, CLA). CodeRabbit skipped because this is not the default branch.
• Greptile: 5/5 on 8bb6b25 ✓.
• Threads: all three are resolved, including "Root broker return paths". None open.