Skip to content

fix: forwarded slots in vector output, href allowlist in the background, byte-cut file name, calc() radius, fixed-element box - #8

Merged
bmmmm merged 6 commits into
mainfrom
fix/review-2026-10-07
Oct 7, 2026
Merged

bmmmm merged 6 commits into
mainfrom
fix/review-2026-10-07

Conversation

@bmmmm

@bmmmm bmmmm commented Oct 7, 2026

Copy link
Copy Markdown
Owner

What and why

A review of the whole tree turned up six defects worth fixing now: one in the vector picture (text and boxes slotted through a forwarding slot vanished), one against the background's own threat model (href was trusted), and four edge cases. Each fix is the smallest change at the root cause, one commit each, each pinned by a test that failed before it.

Claims

  • Vector output paints text and boxes that reach a shadow tree through two slots (slot forwarding, as Lit and Shoelace components do): walkFlatTree yields the forwarding <slot> instead of flattening it, so the node's flatParent has a Box in scene.ts; an empty slot still walks its fallback children
  • The background refuses a save whose run or link-area href is not an absolute http:, https: or mailto: URL (isKeptHref in links.ts, applied in messages.ts), so a forged message cannot put a javascript: link into the saved <a href>
  • The download name is cut at 120 UTF-8 bytes, not 120 UTF-16 units: a CJK or emoji title no longer yields a 200–360-byte name (filename.ts)
  • A border-radius whose computed value stays calc()/min()/max()/clamp() (mixed units, e.g. 10px / calc(50% - 2px)) makes the box a border patch instead of a square shape — before, parseFloat read it as 0 (scene.ts #reasonOf)
  • A selected position: fixed or sticky element keeps its box on screen while the page scrolls, and Save takes the region where the element is at that moment (overlay.ts onScroll re-reads docRectOf); the same handler serves inner scroll containers and resize, so a selected element follows those too
  • A second shortcut press on an unsupported page restarts the 3 s badge notice instead of having the first timer clear it early (flag.ts)
  • A text line of 200 000 runs renders; lineSeparator no longer spreads every run into Math.max (build.ts)
  • xmlAttr writes tab, newline and carriage return as &#9; &#10; &#13;, so an attribute value survives XML attribute-value normalization unchanged (xml.ts)

Verification

$ pnpm check
$ tsc --noEmit && tsc --noEmit -p tests && biome check .
Checked 147 files in 66ms. No fixes applied.

$ pnpm test
ℹ tests 465
ℹ pass 465
ℹ fail 0
  546 passed (1.8m)

$ pnpm build && pnpm lint:ext
errors          0
notices         0
warnings        2          # DANGEROUS_EVAL in the vendored OCR worker, as before

$ pnpm test:glue:chromium
ℹ tests 25
ℹ pass 25
ℹ fail 0

$ pnpm test:glue
ℹ tests 61
ℹ pass 58
ℹ fail 3

The three Firefox glue failures (item 12: metadata, full-viewport selection at DPR 2, vector output, same pages) see a 1265 px viewport: this machine shows classic scrollbars (a mouse is attached), which takes 15 px off the window. The same three fail on main for the same reason; no change here touches the viewport.

Tests

  • Added / changed:
    • tests/fixtures/vector-boxes.html: <snap-outer> → <snap-inner> forwarding, green "forwarded text" (fills) and a red "forwarded box"; tests/fixtures/shadow.html: a slot with fallback text; tests/layout/vector.spec.ts: the same forwarding through two closed trees (paint of the run), and boxes: a radius the engine keeps as calc() (asserts the premise that the computed value keeps the parenthesis, then unsupported: {border: 1})
    • tests/unit/messages.test.ts: rows run href javascript: and link href relative; tests/unit/links.test.ts: isKeptHref
    • tests/unit/filename.test.ts: byte length for "日本語"×60 and "ä"×300
    • tests/fixtures/overlay.html + tests/layout/overlay.spec.ts: a fixed footer selected, page scrolled by 500 px, box still on it, saved rect moved by 500 px (the test waits for the scroll event itself: Chromium fires it a frame after scrollTo)
    • tests/unit/flag.test.ts: two notices, the first timer must not clear the second; tests/unit/svg-build.test.ts: 200 000 runs on one line; tests/unit/xml.test.ts: the three references
  • Fails on main without this change: yes — each test was run red first (fill #000 and 2.6 % pixel diff for the forwarding fixture, paint null in the closed-tree case, unsupported {} for the calc radius, box at client y 170 / saved y 670 for the fixed footer, 207 bytes for the emoji title, RangeError: Maximum call stack size exceeded for the 200k runs). Mutants per condition operand were run too: assigned.length >= 0 (fallback line goes red), isHref without the null branch and isKeptHref returning true, the 2-byte count set to 1, the radius check inverted (every px-radius fixture becomes a patch), the rect refresh replaced by a self-assignment.

Public contracts

  • Saved SVG structure unchanged: raster <image> tiles under an invisible text layer — one <text> with textLength per run, fill-opacity="0", OCR words in <g id="ocr">, linked runs wrapped in <a href>
  • Capture record unchanged: the JSON in <snapii:capture> keeps schema: 1 and every field of captureRecord() in src/shared/svg/metadata.ts (no field removed, renamed or retyped); the Dublin Core fields stay as they are
  • Manifest permissions unchanged: activeTab, scripting, downloads, clipboardWrite, storage — no host_permissions, no content_scripts (decision D1, docs/development.md)
  • Settings in storage.sync keep their keys, types and meaning (Settings in src/shared/types.ts); a value stored by an older version still loads
  • Add-on id stays snapii@qmmq.de
  • Privacy holds: no network request of any kind, data_collection_permissions stays none, the saved file reveals nothing new about the user
  • No new dependency shipped in dist/ — or NOTICE and the license texts are updated with it

Saved-file impact

  • Golden file tests/unit/golden/simple.svg changed: no — byte-identical (the golden test passes). Bytes of a real save change only where a title was cut (non-ASCII titles get shorter), where an attribute carried a tab/newline/CR (now a reference, same value after parsing), and in vector output for the two fixed defects (forwarded content present, calc-radius boxes as patches).
  • Opened a new file in Firefox, selected the text and followed a link: yes (the roundtrip and vector specs open every fixture's SVG in both browsers)

Risk and rollback

  • What breaks if this is wrong: a page whose links the validator now refuses would fail to save (INVALID_MODEL) — linkFor only ever emits absolute URLs of the three kept schemes, which isKeptHref accepts; a forwarding slot that the walk now yields counts one more element towards MAX_ELEMENTS
  • How to tell from the outside: a save that used to work reports "invalid model"; vector output of a web-component page shows text that was missing before
  • Revert is clean: yes, six independent commits

Out of scope

  • Re-sanitizing Copy-text HTML in the background (Chromium's service worker has no DOM; pages cannot send runtime messages)
  • Keyboard handling when the focus sits in an iframe (needs allFrames injection — a design question under D1)
  • A time budget for trimToClip in collect.ts (to be measured first on a 1 MB single-line <pre>)
  • Pseudo-elements and shadow content in #extent, OCR message size vs. the measured limit, a stale anchorSources filter order — each tracked separately

Assistance

  • I wrote or reviewed every line and can explain why each change is there
  • I ran the commands under Verification myself and pasted their real output

bmmmm added 6 commits October 7, 2026 08:29
…slot

The flat-tree walk flattened nested slots, so the slot a forwarded node is
assigned to (its flatParent) never got a box: the node had no parent in
the scene, its text was drawn transparent and a slotted element's subtree
was never painted. The walk now yields the forwarding slot itself and
takes an empty slot's fallback children explicitly.

Red before the fix: vector-boxes.html (fill of "forwarded text" #000,
diff 2.6 %), the closed-tree case in vector.spec.ts (paint null); the
fallback line in shadow.html goes red when the length check is mutated.
…http(s) or mailto URL

messages.ts accepted any string as a run's or link area's href and
build.ts emitted it as <a href>; linkFor's scheme allowlist ran only in
the content script, which messages.ts itself treats as hostile. isKeptHref
(links.ts) now applies the same allowlist in the validator.

Red before the fix: messages.test.ts rows "run href javascript:" and
"link href relative"; links.test.ts isKeptHref.
A CJK or emoji title could reach 360 bytes, past the 255-byte name limit
file systems and Firefox's download enforce (folder.ts records the folder
case). The cut now counts UTF-8 bytes per code point; ASCII names are
unchanged.

Red before the fix: filename.test.ts, 207 bytes for an emoji title.
… instead of a square shape

Mixed units (10px / calc(50% - 2px)) stay a calc() in the computed value,
which parseFloat read as 0: background, border and content clip came out
square. Such a box is now pixels (reason "border"), checked on the whole
value so a two-value radius with a plain first number is caught too.

Red before the fix: vector.spec.ts "boxes: a radius the engine keeps as
calc()" (unsupported {} in both engines; the test asserts the premise
that the computed value keeps the parenthesis).
…region, while the page scrolls

The overlay stored the element's document rect once at selection and only
redrew it on scroll, so a fixed header drifted away from its box and Save
took the document region the element had left. The rect is read anew on
every scroll while an element is selected.

Red before the fix: overlay.spec.ts "a selected fixed element keeps its
box on screen while the page scrolls" (Firefox: box at client y 170
instead of 670; Chromium: saved rect y 670 instead of 1170). The test
waits for the scroll event itself: Chromium fires it a frame after
scrollTo, and Enter before it would save the rect of before.
…nders; attribute whitespace survives

- flag.ts: one timer per tab, a new notice cancels the previous one (before,
  the first timer cleared a notice shown 2.5 s later after 0.5 s).
- build.ts: the line separator's x is found by a loop, not Math.max(...spread),
  which threw RangeError at 200 000 runs (measured).
- xml.ts: xmlAttr writes tab, newline and carriage return as character
  references; a parser normalizes the literal ones to spaces.

Red before the fix: flag.test.ts (second notice cleared early),
svg-build.test.ts (RangeError), xml.test.ts (references). Golden unchanged.
@bmmmm
bmmmm merged commit 8802691 into main Oct 7, 2026
2 checks passed
@bmmmm
bmmmm deleted the fix/review-2026-10-07 branch October 7, 2026 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant