sitetile: escape link and image destinations before restoring them, and only emit safe schemes - #29
Conversation
|
Round-1 review: fix first — P1-1, P1-2, P2-1. The escaping change itself is sound (60k grammar-directed differential fuzz: 2,536 unsafe outputs closed, 0 introduced; it also closes an entity-encoded scheme case the URL check alone would miss).
Fix round in progress. |
|
Round-2 fix pushed (1103cc3): every |
|
Round-2 review (opus): fix first — R2-P1-1, R2-P2-1. The round-1 fix holds (extended sweep over
Fix round in progress. |
|
Round-3 fix pushed (1a779f4): the collection GitHub anchor is gated on its own destination (only mis-bound gate of five); malformed numeric entities degrade to the original text instead of throwing; |
|
Round-3 review (opus): fix first — R3-P2-1, R3-P2-2. Round-2 items are closed on rounds 1–2's own harnesses (0 introduced; residual 0 after classifying
Fix round in progress. |
|
Round-4 fix pushed (e17cbff): form action, header cart href, favicon/apple-touch-icon and fonts stylesheet hrefs, and a previously unaudited hero |
|
Round-4 review (opus): fix first — R4-P1-1, R4-P2-1. Round 3's items are closed and reproduced (identity swap FAIL 16; form-gate-only removal → exactly 3 red; hero
Fix round in progress. |
|
Round-5 fix pushed (5b43135): |
inlineHtml() stashed a link/image destination BEFORE escaping the surrounding text, then restored it RAW after the escaping pass. The stash regex only requires the `](...)` shape, not a matching `[`, so two unrelated bracket-fragments elsewhere in ordinary Markdown prose could each stash their own bracketed content as a "destination" and, once restored raw, splice a live, syntactically complete element into the rendered page -- with no real link ever forming. Reachable by anyone who can write page Markdown. Fix: restore each destination through the same escaper (escHtml) that already runs over the rest of the string, regardless of whether it ends up inside a tag or bare in text. A destination that does form a link or image is additionally validated against a scheme allowlist (http, https, mailto, and scheme-less/relative/fragment) before it is allowed into an href or src at all -- escaping alone stops a raw element from forming but does nothing about a javascript: or data: scheme. A disallowed destination renders as plain text instead. Covers both consumers of the same stash (link href and image src/alt share one mechanism in this function); no other stash/restore of raw destination text exists elsewhere in the file. Adds targeted tests plus keeps all existing sitetile tests green.
…ations Round 2 of the destination-escaping fix (round-1 review findings P1-1, P1-2, P2-1, P2-2): - P1-1: route every href/src emitter through the shared safe-destination check, not just inlineHtml's own link/image regexes. Covers ctaHtml, ctaButtonsHtml, linkButtonsHtml (author cta=/button= params and body link-buttons — reachable from live Cta.astro/Hero.astro), plus every other href/src emitter found by grepping the file: grid/gallery/carousel cells, the grid overlay/action pattern, people name+link row, collection head link and item GitHub/learn links, tagcloud tags, sidebar nav items and the sidebar logo image, and imgTag itself (heroParts' multi-image extraction and firstImage-derived figures called it directly with no check at all). A disallowed destination now degrades to plain text/a non-live element everywhere, never a live link or image. - P1-2: widen the href allowlist to http(s), mailto, tel, sms, ftp(s), and scheme-less/relative/fragment destinations. Images additionally allow data: for a fixed set of raster MIME types only (png/jpeg/jpg/gif/webp/ avif) — never svg+xml, and never data: on a plain href. A disallowed destination no longer disappears silently: it renders as plain text, and a new opt-in diagnostics queue (takeDropWarnings) records the page and scheme for whoever calls it — additive, no existing export's shape or behavior changes for callers that don't use it. - P2-1: decode HTML entities in a destination exactly once before either validating its scheme or escaping it, instead of escaping an already- escaped destination a second time. Fixes both the double-escape on ordinary entity-bearing destinations (e.g. an imported `&` in a query string) and strengthens the scheme check itself against an entity-obfuscated scheme. - P2-2: `![[wikilink]]` embeds now go through the same scheme check as the sibling markdown-image path. Tests: full-string assertions for each of the above, the widened allowlist, the entity-decode-once behavior, the wikilink guard, and the drop-warning queue. 109/109 pass (was 93/93 before this commit). Differential fuzz (round-1's own grammar-directed harness and ruler, 60,000 inputs, same seed): 0 unsafe introduced, all 2,597 pre-existing unsafe outputs still closed (including the 61 that were only closed on the non-wikilink paths before this commit), all 7,399 double-escape regressions from round 1 resolved, and the widened allowlist recovers destinations round 1 had dropped where the fuzz grammar happens to construct one of the newly-allowed schemes.
…, and cover Astro consumers Round 2 review follow-up: - collection's GitHub item anchor validated the wrong field (it.learn instead of it.href), letting a disallowed href go live whenever the other field was safe; it now gates on isSafeHref(it.href) and records the drop. - decodeEntitiesOnce no longer throws RangeError on an out-of-range or malformed numeric entity; it degrades to the raw text and the whole decoder falls back to the un-decoded string on any exception. - linkKind normalises backslashes to slashes before classifying a destination, so a protocol-relative reference spelled with backslashes classifies the same way regardless of which slash/backslash mix is used. - exported safeHref/safeSrc so the Astro rendering layer can share the same allowlist policy; wired them into every coral section component and layout chrome binding that builds href=/src= from author-controlled destinations (frontmatter and page content), which previously had no scheme check of their own. Verified with the existing site-core.test.js suite (7 new cases), the round 2 exhaustive sweep and grammar-directed fuzz harnesses re-run against this change (0 introduced, 0 residual unsafe emissions outside policy/by-design), and a full astro build + smoke suite (121/121).
…sert it in the smoke build Form action, header cart href, favicon/apple-touch-icon and fonts stylesheet hrefs, and the hero background url() are now validated where the value is read from page or site data, so a template cannot forget the check. A scheme-check fixture site exercises every coral with a disallowed/safe pair and the smoke build scans the built HTML and JS for disallowed schemes in href, src, action, srcset and inline assignments, failing in both directions (a scratch build with the check replaced by the identity fails 16 assertions). Repeated safe-destination calls in templates are deduplicated so the drop diagnostics are not amplified; control characters in numeric entities are rejected; site-core's own emitters decode a destination once before escaping, matching the Astro side.
…e scan's blind spot, and quote the hero bg() CSS string Round 4 review found that postUrl()/blogBase() returned an author-controlled permalink/blog-url-pattern/blog-path verbatim, reaching a live href on the blog index, both term archives, every sibling post's prev/next nav, "Keep reading"/ "Recent posts", rss.xml, sitemap.xml, and the archive island's client corpus -- and that the smoke's disallowedSchemeHits() scan was scoped to one page, so it could not see any of it. - gate postUrl()/blogBase() once, at their return, on the same allowlist every other destination in this renderer already uses; a disallowed value degrades to the historic default rather than the raw scheme - extend the same gate to categoryBase()/tagBase() (blog-category-base/ blog-tag-base) and authorBase() (blog-author-base, the same shape, found while auditing every pageHref() in the term-archive family) -- every route file that read one of these fields directly now goes through the gated helper instead - gate a post's featured image (parsePost's own `image` field) and the site-wide share-image/og-image meta tags the same way - run disallowedSchemeHits() over every built page plus rss.xml/sitemap.xml/ search-index.json/reef-posts.json, not just one fixture page; widen the scan itself to catch a second matching attribute per tag, srcset candidates, style="...url(...)", <meta content=...>, and entity/whitespace-obfuscated schemes - add an isolated second astro build (a throwaway sibling of astro/, never committed) carrying a hostile permalink/blog-url-pattern/blog-path/category- base/tag-base fixture, since these are site-level fields a single-page fixture can't exercise without poisoning ~150 unrelated assertions - extend the scheme-check fixture to the remaining coral types (cta, social, timeline, faq, prose, embed), reaching coverage of all known section types - quote and CSS-string-escape the hero bg= destination before it reaches url(...); an otherwise-allowed relative path could previously close the CSS declaration via a stray ) or ; and open new ones in the same inline style - bring imgTag's src emission in line with every other already-gated emitter (decode once, then escape) so the reference renderer and the Astro layer agree on an entity-bearing image destination Two-arm proof (throwaway scratch copies, never written back): a full identity swap of safeHref/safeSrc turns 28 assertions red; each single-gate removal (postUrl's permalink gate, the hero bg quoting, categoryBase/tagBase, and Form.astro's action gate) turns red only the assertions that trace to it. site-core.test.js (117), all sitetile unit suites (32/32) and the astro smoke (160/160) are green; the round 1-3 fuzz/sweep harnesses rerun unchanged with 0 introduced and 0 residual.
… #496 x link-dest rebase
5b43135 to
9ade1d1
Compare
|
Rebased onto main after #28 merged (head 9ade1d1): three conflicts in |
|
Merged after round 5 (verdict: mergeable). Remaining follow-ups, all P3 and all about the smoke ruler rather than the gates themselves — every sink is gated and unit-tested:
Separate correctness issue filed: a safe absolute |
1 participant
Problem
inlineHtml()stashed link and image destinations before escaping the surrounding text and restored them verbatim afterwards. The stash matcher only looked for a](…)shape, so bracketed fragments in ordinary prose could be restored unescaped into the output. Reported privately; no payload in this PR by design.Fix
isSafeHref()parses the destination withnew URL(dest, base)and allows onlyhttp:,https:,mailto:, and scheme-less / relative / fragment destinations; anything else renders as plain text instead of a link or image.hrefand imagesrc. No other stash/restore path exists in the file.Tests
node packages/sitetile/site-core.test.js: 93/93 (5 new). A 20,000-input differential fuzz over[ ] ( ) < > " ' / \scriptfound no output containing<scriptor anon*=` attribute, and every changed output is strictly more escaped than before with no content lost.Downstream
reef vendors this file byte-for-byte; the vendor copy will be updated after this merges.