Repository navigation
One local-URL policy, and a ratchet so a fifth copy cannot appear - #2573
Merged
Merged
Conversation
#2302: two open-redirect findings, both verified live on main first. GitHubLoginEndpoints.SafeLocal rejected "//host" but ACCEPTED "/\host". Browsers normalise the backslash in special-scheme URLs, so a crafted target still became a protocol-relative external redirect. DevAuthController `Redirect(returnUrl ?? "/")` at TWO sites, with no validation whatsoever. Gated on EnableDevLogin, which self-hosted and distributed deployments can turn on — and the route is registered regardless. The interesting part is the count. This assembly held FOUR implementations of one rule: ReturnUrlPolicy.Sanitize correct, and already covered by a theory pinning //evil, /\evil, absolute and javascript: targets EaConsentController.SafeReturnUrl correct GitHubLoginEndpoints.SafeLocal WRONG DevAuthController absent So testing the policy harder would have found none of it. Both wrong sinks now delegate — and so does the correct duplicate, because being right was never the issue: correct duplicates are what make an incorrect one look normal. RedirectSinksUseOnePolicyGuard pins both halves: no redirect may take a request-supplied target without the shared policy, and no second implementation of the protocol-relative check may exist. 🚨 The guard scans the ENCLOSING REGION, not the argument. My first version matched only the inline expression and reported all four EaConsentController sites as open redirects — every one of them correct, sanitised into a local first. Shipping that would have taught the next reader to suppress it. Verified, and one attempt at verification was itself wrong: my first red-proof edited the sources with a bad escape, the rebuild FAILED, and `--no-build` then ran the previous binary and reported the guard "passing" over the defect. Redone by reverting DevAuthController with `git checkout origin/main --`, confirming `Build succeeded`, and only then running: reverted -> guard FAILS, naming both DevAuthController sites restored -> 382/382 pass, -warnaserror clean Refs #2302. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens redirect handling in Memex.Portal.Shared by consolidating request-supplied redirect targets behind ReturnUrlPolicy.Sanitize, and adds a test “ratchet” intended to prevent new ad-hoc local-URL checks at redirect sinks.
Changes:
- Delegate
GitHubLoginEndpoints.SafeLocalandEaConsentController.SafeReturnUrltoReturnUrlPolicy.Sanitize. - Sanitize
returnUrlat both redirect sites inDevAuthController. - Add
RedirectSinksUseOnePolicyGuardto enforce consistent use of the shared policy at redirect sinks.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/Memex.Portal.Shared.Test/RedirectSinksUseOnePolicyGuard.cs | Adds guard tests to ratchet redirect sinks toward ReturnUrlPolicy.Sanitize and discourage duplicate local-URL checks. |
| memex/Memex.Portal.Shared/Social/GitHubLoginEndpoints.cs | Replaces hand-rolled SafeLocal with delegation to ReturnUrlPolicy.Sanitize. |
| memex/Memex.Portal.Shared/Authentication/EaConsentController.cs | Replaces SafeReturnUrl implementation with delegation to ReturnUrlPolicy.Sanitize. |
| memex/Memex.Portal.Shared/Authentication/DevAuthController.cs | Sanitizes returnUrl before redirecting on login and logout. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Three review findings, all correct, and the first is a live open redirect the original pass of #2302 missed. 1. GitHubConnectEndpoints.SafeReturn tested only for a leading "/", so "//evil.example" and "/\evil.example" passed straight through to Redirect. That is weaker than the SafeLocal bug this PR already fixes — that one at least rejected "//". Now delegates. So the assembly had FIVE copies of one rule, not four, and THREE were wrong. Counts in the surrounding comments corrected. 2. EaConsentController's doc had two consecutive <summary> blocks: my edit added one and left the original, which still described a hand-written IsLocalUrl "spelled out here" that no longer exists. Stale block removed. 3. The ratchet was narrower than its own name claimed, twice: * it recognised only returnUrl/returnTo, so the returnPath sink — the live bug above — was never ratcheted at all; * it scanned for StartsWith("//"), while ReturnUrlPolicy itself uses index comparison (u[1] == '/'), the exact form EaConsentController's copy used. 🚨 And fixing those exposed a worse version of the same mistake, which is the part worth reading. My widened guard allowed any call named Safe* to count as sanitisation — so when I red-proofed it against the restored GitHubConnect bug, it PASSED. A helper called SafeReturn that accepts "//evil.example" was certified by the guard because of what it was called. The invariant is not "a sanitiser is called" but "no one re-implements the rule". The second test now finds every Safe…/Sanitize… helper in the local-URL naming family outside ReturnUrlPolicy and requires its body to call ReturnUrlPolicy.Sanitize. Delegation is checkable; the correctness of a hand-rolled copy is not. Narrowed to that naming family after it flagged UpdatePolicySettingsTab's markdown Sanitize — a false positive is how a guard gets suppressed rather than fixed. Verified: with GitHubConnectEndpoints reverted the delegation test FAILS naming exactly SafeReturn and nothing else; restored, 382/382 pass, -warnaserror clean. Refs #2302. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rbuergi
enabled auto-merge
August 28, 2026 07:53
Contributor
Test Results (shard 5)1 610 tests 1 609 ✅ 9m 8s ⏱️ Results for commit 842cd52. |
Contributor
Test Results (shard 2)1 088 tests 896 ✅ 5m 24s ⏱️ Results for commit 842cd52. ♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 0)1 317 tests 1 317 ✅ 6m 5s ⏱️ Results for commit 842cd52. |
Contributor
Test Results (shard 3)1 126 tests 1 126 ✅ 18m 10s ⏱️ Results for commit 842cd52. |
Contributor
Test Results (shard 4) 8 files 8 suites 11m 59s ⏱️ Results for commit 842cd52. |
Contributor
Test Results (shard 1)739 tests 735 ✅ 7m 12s ⏱️ Results for commit 842cd52. |
Contributor
Test Results 44 files 44 suites 57m 59s ⏱️ Results for commit 842cd52. ♻️ This comment has been updated with latest results. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two more #2302 findings, both verified live on
mainbefore touching anything.GitHubLoginEndpoints.SafeLocal//host, accepted/\hostDevAuthControllerRedirect(returnUrl ?? "/")at two sites, no validation at allBrowsers normalise the backslash in special-scheme URLs, so
/\evil.comstill became a protocol-relative external redirect. AndDevAuthControlleris gated onEnableDevLogin— which self-hosted and distributed deployments can enable, with the route registered regardless.The count is the actual finding
This assembly held four implementations of one rule:
ReturnUrlPolicy.Sanitize//evil,/\evil, absolute URLs andjavascript:EaConsentController.SafeReturnUrlGitHubLoginEndpoints.SafeLocalDevAuthControllerTesting the policy harder would have found none of this. The policy was never the problem. So both wrong sinks now delegate — and so does the correct duplicate, because being right was not the issue: correct duplicates are what make an incorrect one look normal.
RedirectSinksUseOnePolicyGuardpins both halves — no redirect may take a request-supplied target without the shared policy, and no second implementation of the protocol-relative check may exist.The guard needed fixing before it was worth having
🚨 My first version matched only the inline argument, and reported all four
EaConsentControllersites as open redirects. Every one is correct — the value is sanitised into a local first (one of them reassignsreturnUrlitself). It now scans the enclosing region. Shipping the strict version would have taught the next reader to suppress the guard, which is worse than not having it.Verification — including one attempt that was itself wrong
My first red-proof edited the sources with a bad escape. The rebuild failed,
--no-buildthen ran the previous binary, and the guard reported "passing" over the defect. A textbook false pass, caught only because the build line said0where it should have saidBuild succeeded.Redone properly — revert via
git checkout origin/main --, confirm the build, then run:What's New
Skipped — no user-visible effect for any legitimate redirect target.