fix(sri): only penalize off-origin proto-relative URLs on HTTP - #561
Draft
caugner wants to merge 25 commits into
Draft
fix(sri): only penalize off-origin proto-relative URLs on HTTP#561caugner wants to merge 25 commits into
caugner wants to merge 25 commits into
Conversation
Treat protocol-relative script URLs (`src="//cdn.example.com/…"`) by the risk they actually carry instead of a blanket -50. On-origin URLs (same second-level domain) now score as a secure origin, since any attack on them also applies to the document. Off-origin URLs are penalized only when the site can be served over HTTP (no HTTP server, or HTTP redirects to HTTPS distinguishes the document scheme); when HTTPS is enforced they score like an `https://` external script.
The `subresource-integrity` analyzer no longer penalizes protocol-relative URLs unconditionally, only when they can resolve to HTTP. Update the grade descriptions to match, so users are not told a `src="//..."` URL is penalized when it scored 0 or -5.
The `!requests.responses.http` branch of `httpEnforcesHttps` was untested; add a case asserting an off-origin protocol-relative script scores as loaded securely (-5) when there is no HTTP server to serve it over HTTP.
`parse(requests.site.hostname).domain` is loop-invariant and was recomputed per script in two branches. Compute it once as `siteDomain`.
`httpEnforcesHttps` only checked the final URL of the HTTP redirect chain, so a chain with an intermediate HTTP hop (`http` -> `http` -> `https`) that could be downgraded was treated as enforcing HTTPS. Require every hop after the initial HTTP request to be `https:` instead, matching how `redirection.js` rejects `RedirectionNotToHttpsOnInitialRedirection`.
The enforcement-independence of on-origin protocol-relative scripts was asserted inside `checks for same origin`, where the behavior was not discoverable. Move it to a dedicated, named test and trim the comments to what the test ID does not already convey.
…th-https Rebuild the protocol-relative URL handling (issue #464) on top of the exact-origin SRI model from #559. - Drop all `tldts` usage; the origin dimension now compares the resolved script `origin` against the page's `baseOrigin` (scheme + host + port). - Collapse the per-branch origin computation to a single `new URL(scriptSrc, baseUrl).origin === baseOrigin`, guarded on `baseUrl` so a missing session resolves to a foreign origin. Same-host protocol-relative URLs are therefore same-origin. - Keep the scheme dimension (`httpEnforcesHttps` and the `relativeProtocol && httpEnforcesHttps` clause) unchanged. - Update tests for exact-origin semantics: `//www.mozilla.org` against apex `mozilla.org` is now off-origin (scored by scheme), and the on-origin path uses a new same-host `//mozilla.org` fixture.
…e URL Compute the scheme dimension from `new URL(scriptSrc, baseUrl).protocol` for full and path-relative URLs, replacing the separate `new URL(scriptSrc).protocol` extraction and the `relativeOrigin` branch. This folds the `relativeOrigin` classification away, leaving only the `relativeProtocol` flag, whose scheme still depends on HTTP reachability (`httpEnforcesHttps`) rather than the resolved scheme — a protocol-relative URL resolves to the page's own (HTTPS) scheme here, which would mask its downgrade risk on an HTTP visit. Path-relative URLs now take their scheme from `baseUrl` (the served origin) instead of `session.url`; these are identical in practice, and `baseUrl` is the scheme the resource actually loads over.
The guard is `!secureOrigin` (off-origin) under the exact-origin model, not "not a relative URI" as the pre-existing comment claimed.
The variable tests whether the script resolves to the page's own origin, a sameness check, not a security one. `sameOrigin` also reads clearly against the security-focused `secureScheme` it pairs with.
Reword the "even if" phrasing to state plainly that SRI on same-origin scripts is rewarded despite not being required there.
… test Restore the protocol-relative sub-case to "checks for same origin" using a genuinely same-host fixture (//mozilla.org), rather than moving it out. The standalone on-origin test now only asserts the non-redundant property — that the verdict is unchanged without HTTP→HTTPS enforcement.
…tive Point test_content_sri_sameorigin3.html at //mozilla.org so it exercises a genuine same-origin protocol-relative script, keeping the original "checks for same origin" sub-case in place instead of adding a parallel fixture. Drop the separate off-origin protocol-relative test — that behavior is already covered by the fb.me no-protocol matrix and the different-subdomain test.
An on-origin script's verdict never consults `secureScheme` (the same-origin branch ignores it without integrity), so the no-enforcement case is identical to the sameorigin3 case in "checks for same origin".
Judge a protocol-relative sub-resource by whether a normal visitor is served the document over HTTPS, using the final scheme of the HTTP redirect chain rather than requiring every hop after the initial request to be HTTPS. An intermediate `http` hop (`http` → `http` → `https`) still lands the visitor on HTTPS, so the script loads over HTTPS and carries no extra SRI risk. That downgradeable hop is a redirect flaw already penalized by the redirection test (`RedirectionNotToHttpsOnInitialRedirection`), so docking it here too would penalize one flaw twice.
Explain the exact-origin rule once at `baseOrigin`, and the protocol-relative scheme handling once, referencing `httpEnforcesHttps` from the later comments instead of restating it.
Collapse the two `new URL(scriptSrc, baseUrl)` constructions into a single `scriptUrl`, and derive `sameOrigin` and `secureScheme` from it. This also drops the resolved scheme computation for protocol-relative URLs, whose security is judged by `httpEnforcesHttps` rather than the resolved scheme.
Resolving every `src` against the base URL means a malformed value such as `src="//"` throws from `new URL`, which aborts the entire scan since `analyzeScan` runs tests without a per-test `try`/`catch`. Guard the resolution and skip such a `src`, since it is not a loadable sub-resource.
Match the spelling used in the grade descriptions and PR copy.
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
2 participants
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.
Description
Update the
subresource-integrityanalyzer to score protocol-relative script URLs (src="//cdn.example.com/…") according to the actual risk, instead of applying a blanket −50://www.mozilla.orgonmozilla.org) is a distinct origin and is handled by the off-origin case below, not treated as same-origin.https://(−5 without SRI) when a normal visitor ends up on HTTPS (no HTTP server, or the HTTP request ultimately redirects to HTTPS), and ashttp://(−50 / −20 with SRI) otherwise.This also reworks the −20 / −50 grade descriptions, which previously stated protocol-relative URLs are penalized unconditionally:
Motivation
Only penalize protocol-relative URLs in the one case that carries additional risk, per the security team's analysis in #464.
Given a document that loads a protocol-relative sub-resource, there are four cases:
Only case 2 warrants a penalty. The observatory distinguishes the document scheme from the redirect chain, so it penalizes a protocol-relative URL only when it is off-origin and a normal visitor is served the document over HTTP.
The document counts as served over HTTPS whenever a visitor ends up there — including through a downgradeable intermediate hop (
http → http → https), since the visitor still lands on HTTPS and the script loads over HTTPS. That insecure hop is a redirect flaw already penalized by the redirection test (RedirectionNotToHttpsOnInitialRedirection); scoring it here as well would dock one flaw twice.Additional details
Example that previously scored −50 despite being served only over HTTPS: https://developer.mozilla.org/en-US/observatory/analyze?host=runephilosof-abtion.github.io%2Fobservatory-test%2F
Since every
srcis now resolved against the served base URL, asrcthat cannot be resolved into a URL (e.g.src="//") is skipped rather than throwing and aborting the whole scan.Related issues and pull requests
Fixes #464.