fix(security) - parse meta-refresh directives and commit Head() meta … - #58
Merged
Conversation
…entries transactionally
…ive refresh directives, and unify client/SSR meta ordering
…hare one canonical URL policy across client and SSR
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.
Description
Four defects in
<meta http-equiv="refresh">handling, found after #57.Detection substring-matched a grammar. Dangerous destinations were found by asking whether the lower-cased
contentcontainedurl=javascript:(plus three sibling schemes). That recognises exactly one spelling of a directive the browser's refresh parser accepts in many — every one of these was a live redirect the check never saw:Pattern-matching a grammar is a losing position: each new spelling needs a new pattern, and the attacker picks the spelling. The destination is now extracted structurally and handed to
sanitizeUrl()— the same protocol authority every other URL sink uses — so adding a scheme to that allowlist fixes this sink automatically.Reactive attributes were validated independently.
Head()created one effect per reactive attribute, so every write was judged alone. That lets the combination become dangerous while no individual write ever looks wrong:With
equiv()initially"x-custom"the entry is not a refresh, so the static content is accepted on that basis.setEquiv("refresh")then re-runs only thehttp-equiveffect — the content is never revalidated, and the element is a live redirect nothing ever approved. Security decisions are properties of the whole entry, so the whole entry is now the unit of work.Duplicate case-insensitive names split verdict from commit. HTML attribute names are case-insensitive; JavaScript object keys are not, so
{ "http-equiv": "x-custom", "HTTP-EQUIV": "refresh", … }is legal. A first-match lookup validatedx-customwhile the DOM loop wrote both and the laterHTTP-EQUIVbecame effective.Client and SSR each carried the rule.
head.tsandssr.tsheld separate copies of the same four checks, so a fix to either would have silently diverged from the other.What changed
src/utils/metaRefresh.tsis the single policy, called byHead(),renderToDocument, and router SSR:Anything it cannot read unambiguously is dropped — unterminated quotes, competing
url=assignments, non-numeric delays, trailing junk, empty destinations, non-urlkeys. This is deliberately stricter than a browser and makes no claim of WHATWG parity: real engines recover aggressively from malformed input and differ at the edges, and reproducing that would mean matching recovery behaviour I cannot verify across the whole support floor. The cost is a few odd-but-harmless directives dropped; the benefit is that nothing unreadable is ever emitted on the strength of "no forbidden substring was found".The verdict is a discriminated union (
not-refresh/delay-only/allowed/forbidden) because callers act differently on each — a boolean forced "ignore me, I'm a description tag" and "drop this element" into one answer.Duplicate casings are rejected, not resolved by precedence. Last-write-wins would also be sound if the validated value were provably the committed one, but rejection removes the class of bug rather than re-parameterising it.
Head()now runs one effect per entry: resolve every attribute → fold duplicate casings → validate the assembled snapshot → reconcile. A snapshot that fails validation detaches the element rather than blanking an attribute, since a partially-cleared element is still live markup and a detached one must never count as active.Related Issue
Closes #
Type of Change
Breaking: a dangerous or ambiguous refresh entry is now removed entirely, where previously its
contentattribute was blanked whilehttp-equiv="refresh"stayed in the head. Applications relying on odd-but-harmless refresh spellings the strict parser rejects will see those entries dropped.Checklist
The browser test synchronises on behaviour, not a delay. A benign control performs a real same-origin refresh and the test waits on
waitForURL()— proving the probe can observe a refresh at all, and bounding how long the engine takes to honour one. The negative cases are then backed by a route interceptor counting requests to a protected same-origin path plus an unchangedpage.url(). No external host is contacted.Two judgement calls worth review:
head.coverage2asserted the old mitigation (blankcontent, keep the element). That expectation is superseded by removal, so it now asserts detachment and restoration once the value is safe again.HeadPropswas deliberately not widened. Reactive getters are typed to returnstring, so "returnnullto remove an attribute" is not expressible today. Reconciliation does remove attributes that disappear from the resolved snapshot; I did not broaden the public type just to write that test.