From d23e585cdbc72b52a3aa8660a26746760f8b91be Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 10:22:59 -0400 Subject: [PATCH] review(Security): fix login open-redirect (High) + stale LDAP doc Code review at HEAD 7286d320. Security-001 (High): guard returnUrl with a local-URL check before redirect (open-redirect/phishing vector) + regression test. Security-002: update stale LdapOptions dev-LDAP doc reference. --- code-reviews/Security/findings.md | 64 +++++++++++++++++++ .../Endpoints/AuthEndpoints.cs | 18 +++++- .../Ldap/LdapOptions.cs | 5 +- .../AuthEndpointsIntegrationTests.cs | 34 ++++++++++ 4 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 code-reviews/Security/findings.md diff --git a/code-reviews/Security/findings.md b/code-reviews/Security/findings.md new file mode 100644 index 00000000..62a8b8e6 --- /dev/null +++ b/code-reviews/Security/findings.md @@ -0,0 +1,64 @@ +# Code Review — Security + +| Field | Value | +|---|---| +| Module | `src/Server/ZB.MOM.WW.OtOpcUa.Security` | +| Reviewer | Claude Code | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | +| Status | Reviewed | +| Open findings | 0 | + +## Checklist coverage + +A comprehensive review completes every category, recording "No issues found" where +a category produced nothing rather than leaving it blank. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Security-001 (open redirect on successful login) | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No issues found | +| 4 | Error handling & resilience | No issues found | +| 5 | Security | Security-001 (open redirect), Security-002 (stale doc) | +| 6 | Performance & resource management | No issues found | +| 7 | Design-document adherence | No issues found | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | Security-001 was untested (regression test added) | +| 10 | Documentation & comments | Security-002 (stale LdapOptions comment) | + +## Findings + +### Security-001 + +| Field | Value | +|---|---| +| Severity | High | +| Category | Security | +| Location | `src/Server/ZB.MOM.WW.OtOpcUa.Security/Endpoints/AuthEndpoints.cs:135` | +| Status | Resolved | + +**Description:** The `/auth/login` endpoint (form POST path) redirects to `returnUrl` on successful authentication without validating that the URL is local to the application. An attacker can craft a phishing link such as `https://admin.example.com/login?returnUrl=https%3A%2F%2Fevil.com` — the shared `LoginCard` component echoes the value verbatim into the hidden `returnUrl` form field (the `LoginCard.razor` security comment explicitly states: "the consuming app's POST handler MUST validate it is a local/relative URL before redirecting to prevent open-redirect"). After a successful bind the endpoint issues `Results.Redirect("https://evil.com")`, silently forwarding the authenticated user to an attacker-controlled site. + +The `RedirectToLogin.razor` Blazor component's own `returnUrl` generation is safe (`Nav.ToBaseRelativePath` always produces a relative path), but nothing prevents an attacker from directly constructing the URL. The normal cookie-auth challenge path also produces only relative paths. The gap is the absence of server-side local-URL validation on the consumed form field before redirecting. + +**Recommendation:** Replace `Results.Redirect(returnUrl)` with a local-URL guard: use `Uri.IsWellFormedUriString(returnUrl, UriKind.Relative)` to validate the URL is relative before redirecting; fall back to `"/"` for absolute or malformed values. `Results.LocalRedirect` could alternatively be used but throws on invalid input (which the endpoint would need to catch). + +**Resolution:** Fixed 2026-06-19. Added `IsLocalUrl` helper that rejects absolute and protocol-relative URLs; success-path now falls back to `"/"` on invalid input. Regression test `Login_with_absolute_returnUrl_does_not_open_redirect` added to `AuthEndpointsIntegrationTests`. + +--- + +### Security-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Server/ZB.MOM.WW.OtOpcUa.Security/Ldap/LdapOptions.cs:9` | +| Status | Resolved | + +**Description:** The XML doc comment on `LdapOptions` references `C:\publish\glauth\auth.md` as the source for dev LDAP defaults. Per `CLAUDE.md` (§ LDAP Authentication) the per-VM NSSM GLAuth at `C:\publish\glauth\` is obsolete — the shared GLAuth now runs on the Linux Docker host at `10.100.0.35:3893`. A developer following the stale reference would look for a file that no longer exists on the machine. + +**Recommendation:** Update the doc comment to remove the stale local-path reference and direct readers to `docs/security.md` and `CLAUDE.md` §"LDAP Authentication" for dev LDAP setup. + +**Resolution:** Fixed 2026-06-19. Stale `C:\publish\glauth\auth.md` reference replaced with pointer to `docs/security.md`. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Security/Endpoints/AuthEndpoints.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Security/Endpoints/AuthEndpoints.cs index 48495d78..9bb01e02 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Security/Endpoints/AuthEndpoints.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Security/Endpoints/AuthEndpoints.cs @@ -132,9 +132,25 @@ public static class AuthEndpoints await http.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, principal); if (!isForm) return Results.NoContent(); - return Results.Redirect(string.IsNullOrWhiteSpace(returnUrl) ? "/" : returnUrl); + // Security-001: validate returnUrl is a relative (local) URL before redirecting. + // An attacker can supply an absolute URL (e.g. https://evil.com) via a crafted link; + // LoginCard echoes the value verbatim into the hidden form field. Fall back to the + // app root rather than following the external URL (open-redirect prevention). + var destination = !string.IsNullOrWhiteSpace(returnUrl) && IsLocalUrl(returnUrl) ? returnUrl : "/"; + return Results.Redirect(destination); } + /// + /// Returns when is a relative (local) URL + /// safe to redirect to after login. Rejects absolute URLs (with a scheme, e.g. + /// https://evil.com), protocol-relative URLs (//evil.com), and + /// anything that is not a well-formed relative URI — preventing open-redirect attacks + /// via a crafted returnUrl query parameter. + /// + /// The candidate redirect target. + private static bool IsLocalUrl(string url) => + Uri.IsWellFormedUriString(url, UriKind.Relative) && !url.StartsWith("//"); + /// /// Case-insensitive set-union of two role lists, preserving the de-duplication semantics the /// legacy RoleMapper.Merge applied. Used to fold any pre-resolved roles (the DevStub diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Security/Ldap/LdapOptions.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Security/Ldap/LdapOptions.cs index 50afcb4d..f750df7a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Security/Ldap/LdapOptions.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Security/Ldap/LdapOptions.cs @@ -5,8 +5,9 @@ namespace ZB.MOM.WW.OtOpcUa.Security.Ldap; /// /// LDAP + role-mapping configuration for the Admin UI. Bound from appsettings.json -/// Security:Ldap section. Defaults point at the local GLAuth dev instance (see -/// C:\publish\glauth\auth.md). +/// Security:Ldap section. Defaults point at the shared GLAuth dev instance on the +/// Linux Docker host (10.100.0.35:3893) — see docs/security.md §"LDAP bind +/// flow" and CLAUDE.md §"LDAP Authentication" for setup details. /// /// /// Carries both the wire fields the shared ZB.MOM.WW.Auth.Ldap directory client needs diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Security.Tests/AuthEndpointsIntegrationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Security.Tests/AuthEndpointsIntegrationTests.cs index 5d38e025..9cf8ab11 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Security.Tests/AuthEndpointsIntegrationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Security.Tests/AuthEndpointsIntegrationTests.cs @@ -453,6 +453,40 @@ public sealed class AuthEndpointsIntegrationTests : IAsyncLifetime resp.Headers.Location.OriginalString.ShouldContain("ReturnUrl"); } + /// + /// Security-001 regression: a successful login with an absolute returnUrl must NOT + /// redirect to the external URL. An attacker can craft + /// /login?returnUrl=https://evil.com; the component echoes + /// the value into the hidden form field verbatim. The /auth/login endpoint must + /// validate the URL is local before redirecting; it must fall back to "/" (the app + /// root) rather than forwarding to an external host. + /// + [Fact] + public async Task Login_with_absolute_returnUrl_does_not_open_redirect() + { + var client = NewClientNoRedirect(); + + // POST to /auth/login with valid credentials AND an absolute returnUrl. + var formContent = new FormUrlEncodedContent(new Dictionary + { + ["username"] = "alice", + ["password"] = "valid-password", + ["returnUrl"] = "https://evil.com/steal-session", + }); + var resp = await client.PostAsync("/auth/login", formContent, Ct); + + // The endpoint must redirect (302), but NOT to the external URL. + resp.StatusCode.ShouldBe(HttpStatusCode.Found, + "a successful form-POST login must always redirect"); + + var location = resp.Headers.Location?.OriginalString ?? string.Empty; + location.Contains("evil.com").ShouldBeFalse( + "a successful login must not redirect to an externally-supplied absolute URL"); + + // The fallback destination must be the app root — not an error page. + location.ShouldBe("/", "the safe fallback for an invalid returnUrl is the app root"); + } + /// Anonymous XHR GET of a protected route returns 401 (caller signaled non-browser /// via the X-Requested-With header — the ASP.NET cookie handler's IsAjaxRequest /// heuristic). The framework still writes a Location header alongside the 401;