security: tighten app-wide Referrer-Policy to same-origin (#383) - #384
Merged
Conversation
Remote resource hosts were told the origin of every PoracleWeb instance a user browsed: uicons on raw.githubusercontent.com, Discord avatars on cdn.discordapp.com, and the Google Fonts stylesheets. #242 fixed this per-element for the gym picker; annotating every tag in the app does not scale. The security-headers middleware sent strict-origin-when-cross-origin -- the browser default, which sends the origin cross-origin. It now sends same-origin: full referrer within the site, nothing to third parties. One line, every case. no-referrer was rejected. AuthController reads the Referer header on DiscordLogin, the OIDC login path, and OIDC RP-initiated logout to recover which frontend origin the user came from, validate it against the configured CORS origins, and redirect back there after the provider callback. Blanking the same-origin referrer degrades all three to this host's own origin and bounces users to the wrong place after login. Header values moved out of the inline lambda in Program.cs into a SecurityHeaders class so they can be asserted without booting the app. The CSP is carried over byte-identical; a test pins it against the original literal. Tests cover the policy value, a guard that it never becomes no-referrer or any of the origin-leaking values, and the AuthController origin recovery it depends on -- which had no coverage at all before. Closes #383
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.
Closes #383. Follow-up to #242 / #382, which fixed this per-element for the gym picker only.
The change
Program.cssentReferrer-Policy: strict-origin-when-cross-origin— the browser default, which discloses the origin on every cross-origin request. It now sendssame-origin: full referrer within the site, nothing at all to third parties.That covers, in one line, every remote resource the SPA loads:
raw.githubusercontent.com(icon.service.ts:5, operator-overridable so possibly a self-hosted mirror) across the Pokémon, raid, egg, lure, invasion, gym and quick-pick lists and dialogscdn.discordapp.com(discord-avatar.component.ts,admin-users.component.html)index.htmlFor a private or invite-only instance the hostname is the part worth withholding.
Why not
no-referrerAuthControllerreadsRequest.Headers.Refererin three places —DiscordLogin(:85), the OIDC login path (:263), and OIDC RP-initiated logout (:312) — to recover which frontend origin the user came from, validate it against the configured CORS origins, and stash it inoauth_originso the callback can redirect back there.no-referrerblanks that on same-origin requests too. All three would silently degrade toselfOrigin. Harmless when the SPA is served by this host; wrong when it isn't, which is exactly the split-origin dev setup.same-originkeeps the same-origin referrer intact and still sends nothing cross-origin, so it gets the privacy win without touching auth.There's a
<remarks>block onSecurityHeaders.ReferrerPolicysaying this, plus a test that fails if someone "tightens" it tono-referrerlater. That trap is the whole reason this PR carries tests for a one-value change.Refactor, and why
The header values moved out of the inline lambda in
Program.csintoConfiguration/SecurityHeaders.cs. Values in a top-level statement lambda can't be asserted without booting the app, and booting it drags in MySQL and PoracleNG —Microsoft.AspNetCore.Mvc.Testingis referenced in the test project but unused, and there's noWebApplicationFactoryharness to extend. A staticApply(IHeaderDictionary)is testable directly.The CSP came across byte-identical, split over three concatenated lines for readability.
Apply_SetsContentSecurityPolicy_UnchangedFromTheInlineVersionpins it against the original single-line literal so the reformatting can't have introduced a typo.Tests
SecurityHeadersTests(11 cases):same-originno-referrer, with the AuthController reasoning in the doc commentstrict-origin-when-cross-origin,no-referrer-when-downgrade,origin,origin-when-cross-origin,unsafe-url)Referrer-Policyis overwritten, not appended toAuthControllerLoginOriginTests(5 cases) — new coverage for the origin recovery this change depends on, which had none. Runs against the realAuthControllerand asserts theoauth_origincookie: allowed CORS referer honored, same-origin referer honored when no CORS origins are configured, disallowed referer rejected, non-absolute referer rejected, absent referer falling back to self. That last one is theno-referrerscenario pinned as a test rather than an argument.Full suite: 1457 passed, 0 failed (was 1452 before, +5 net after the additions — the 11 header cases replace nothing).
On the acceptance list in #383
Two items I did not tick, stated plainly rather than left implied:
same-originstill sends a same-origin referrer, so those code paths see exactly what they saw before. Worth a smoke test on dev before release.The per-element
referrerpolicyattributes from #242 are left in place. They're redundant under this header but harmless, and they hold if the header is ever relaxed.