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.
This commit is contained in:
Joseph Doherty
@@ -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`.
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Returns <see langword="true"/> when <paramref name="url"/> is a relative (local) URL
|
||||
/// safe to redirect to after login. Rejects absolute URLs (with a scheme, e.g.
|
||||
/// <c>https://evil.com</c>), protocol-relative URLs (<c>//evil.com</c>), and
|
||||
/// anything that is not a well-formed relative URI — preventing open-redirect attacks
|
||||
/// via a crafted <c>returnUrl</c> query parameter.
|
||||
/// </summary>
|
||||
/// <param name="url">The candidate redirect target.</param>
|
||||
private static bool IsLocalUrl(string url) =>
|
||||
Uri.IsWellFormedUriString(url, UriKind.Relative) && !url.StartsWith("//");
|
||||
|
||||
/// <summary>
|
||||
/// Case-insensitive set-union of two role lists, preserving the de-duplication semantics the
|
||||
/// legacy <c>RoleMapper.Merge</c> applied. Used to fold any pre-resolved roles (the DevStub
|
||||
|
||||
@@ -5,8 +5,9 @@ namespace ZB.MOM.WW.OtOpcUa.Security.Ldap;
|
||||
|
||||
/// <summary>
|
||||
/// LDAP + role-mapping configuration for the Admin UI. Bound from <c>appsettings.json</c>
|
||||
/// <c>Security:Ldap</c> section. Defaults point at the local GLAuth dev instance (see
|
||||
/// <c>C:\publish\glauth\auth.md</c>).
|
||||
/// <c>Security:Ldap</c> section. Defaults point at the shared GLAuth dev instance on the
|
||||
/// Linux Docker host (<c>10.100.0.35:3893</c>) — see <c>docs/security.md</c> §"LDAP bind
|
||||
/// flow" and <c>CLAUDE.md</c> §"LDAP Authentication" for setup details.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Carries both the wire fields the shared <c>ZB.MOM.WW.Auth.Ldap</c> directory client needs
|
||||
|
||||
@@ -453,6 +453,40 @@ public sealed class AuthEndpointsIntegrationTests : IAsyncLifetime
|
||||
resp.Headers.Location.OriginalString.ShouldContain("ReturnUrl");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Security-001 regression: a successful login with an absolute <c>returnUrl</c> must NOT
|
||||
/// redirect to the external URL. An attacker can craft
|
||||
/// <c>/login?returnUrl=https://evil.com</c>; the <see cref="LoginCard"/> component echoes
|
||||
/// the value into the hidden form field verbatim. The <c>/auth/login</c> endpoint must
|
||||
/// validate the URL is local before redirecting; it must fall back to <c>"/"</c> (the app
|
||||
/// root) rather than forwarding to an external host.
|
||||
/// </summary>
|
||||
[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<string, string>
|
||||
{
|
||||
["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");
|
||||
}
|
||||
|
||||
/// <summary>Anonymous XHR GET of a protected route returns 401 (caller signaled non-browser
|
||||
/// via the <c>X-Requested-With</c> header — the ASP.NET cookie handler's IsAjaxRequest
|
||||
/// heuristic). The framework still writes a <c>Location</c> header alongside the 401;
|
||||
|
||||
Reference in New Issue
Block a user