Harden per review (SSRF/theme-injection/round-trip), add class guards, green CI gate + source map #33

Merged

brenno merged 8 commits from fix/review-t1-s2-hardening into main 2026-06-29 22:24:49 +00:00

Conversation 0 Commits 8 Files changed 35 +1269 -233

brenno commented 2026-06-29 22:23:53 +00:00

Owner

Copy link

Summary

Addresses every finding of the 2026-06-29 review (security scan + code review),
adds class-level guards so each bug family can't recur, and restores a green,
enforceable quality gate. Also adds a per-file source map and refreshes the docs.

The gate was red on a clean checkout (format drift + a bare catch (_)); after
this branch, make check passes end-to-end (formatting, analyze, conventions,
897 tests) and local now uses the exact CI-pinned toolchain.

Changes

Security

• S1 — SSRF on live remote-media: remote image/video URLs from an imported
  deck now pass NetGuard.isAllowedMediaUrlResolved (lexical + DNS resolve,
  per-host memoised) before NetworkImage / VideoPlayerController.networkUrl.
  Closes the static-internal, resolves-to-internal and decimal-IP cases;
  residual DNS-rebind window documented on the method.
• S2 — HTML/CSS injection via imported theme profile: ThemeProfile.fromJson
  validates every colour to #RRGGBB and whitelists fonts, at the single
  choke point that feeds both the HTML export and the audience-window styles.

Round-trip data-loss (silent)

• C1 caption pipe, C2 video-trim precision (decimal seconds), C3 notes
  -->, C4 table-cell literal <br>. Each fixed with a round-trip test.
• The new fuzz also surfaced a bug the review missed: a single image caption
  containing " | " was truncated too — now every caption write pipe-encodes.

Conventions / cleanups

• T1 typed the three bare catch (_) sites (CI gate back to 0).
• C5 fence-balance validated per slide; C6 removed a dead <!-- _style:
  branch.

Prevention — catch the whole class, not the case

• network_sink_guard_test — source scan: network sinks only in NetGuard files.
• markdown_roundtrip_fuzz_test — adversarial corpus through every lossless field.
• ThemeProfile.fromJson fuzz — every style field must sanitise.
• tool/mutation_check.dart + make mutate — mutation check for the dead/untested
  branch class that analyze and line coverage both miss (C6's class).

CI gate + toolchain pin

• Normalized formatting to the CI-pinned toolchain (Flutter 3.44.2 / Dart 3.12.2)
  so make format-check — and the CI gate — is green.
• Added .tool-versions; documented the pin and the "use the Flutter-bundled
  dart" caveat in BUILD.md / CHECKS.md.

Docs

• docs/SOURCE_MAP.md: one-line description of every file under lib/ (178),
  linked from README / ARCHITECTURE / CHECKS.
• CHECKS.md refreshed (scale numbers, coverage floor, new guards, make mutate).

Checklist

• make check passes (format-check, analyze, full test suite).
• Added/updated tests for the behaviour I changed.
• No new UI strings (logic/security/docs only).
• No storage-format change to author content (the .md round-trips
  identically; escaping is internal). FILE_FORMAT.md unaffected.
• Docs updated (README, ARCHITECTURE, BUILD, CHECKS, new SOURCE_MAP).

Notes for reviewers

• The "Normalize formatting" commit is whitespace-only but touches files outside
  this branch's logic — review it separately; it's what makes the gate green.
• One residual is left as a follow-up (noted in code/commit): an imported theme
  profile's logoPath still resolves via resolveTrustedAssetPath.
• make mutate is intentionally not in make check (slow; survivors need
  triage). Running it on the validator flagged real untested branches.

🤖 Generated with Claude Code

## Summary Addresses every finding of the 2026-06-29 review (security scan + code review), adds class-level guards so each bug *family* can't recur, and restores a green, enforceable quality gate. Also adds a per-file source map and refreshes the docs. The gate was red on a clean checkout (format drift + a bare `catch (_)`); after this branch, `make check` passes end-to-end (formatting, analyze, conventions, **897 tests**) and local now uses the exact CI-pinned toolchain. ## Changes **Security** - **S1 — SSRF on live remote-media:** remote image/video URLs from an imported deck now pass `NetGuard.isAllowedMediaUrlResolved` (lexical + DNS resolve, per-host memoised) before `NetworkImage` / `VideoPlayerController.networkUrl`. Closes the static-internal, resolves-to-internal and decimal-IP cases; residual DNS-rebind window documented on the method. - **S2 — HTML/CSS injection via imported theme profile:** `ThemeProfile.fromJson` validates every colour to `#RRGGBB` and whitelists fonts, at the single choke point that feeds both the HTML export and the audience-window styles. **Round-trip data-loss (silent)** - **C1** caption pipe, **C2** video-trim precision (decimal seconds), **C3** notes `-->`, **C4** table-cell literal `<br>`. Each fixed with a round-trip test. - The new fuzz also surfaced a bug the review missed: a *single* image caption containing `" | "` was truncated too — now every caption write pipe-encodes. **Conventions / cleanups** - **T1** typed the three bare `catch (_)` sites (CI gate back to 0). - **C5** fence-balance validated per slide; **C6** removed a dead `<!-- _style:` branch. **Prevention — catch the whole class, not the case** - `network_sink_guard_test` — source scan: network sinks only in NetGuard files. - `markdown_roundtrip_fuzz_test` — adversarial corpus through every lossless field. - `ThemeProfile.fromJson` fuzz — every style field must sanitise. - `tool/mutation_check.dart` + `make mutate` — mutation check for the dead/untested branch class that `analyze` and line coverage both miss (C6's class). **CI gate + toolchain pin** - Normalized formatting to the CI-pinned toolchain (Flutter 3.44.2 / Dart 3.12.2) so `make format-check` — and the CI gate — is green. - Added `.tool-versions`; documented the pin and the "use the Flutter-bundled dart" caveat in `BUILD.md` / `CHECKS.md`. **Docs** - `docs/SOURCE_MAP.md`: one-line description of every file under `lib/` (178), linked from README / ARCHITECTURE / CHECKS. - CHECKS.md refreshed (scale numbers, coverage floor, new guards, `make mutate`). ## Checklist - [x] `make check` passes (format-check, analyze, full test suite). - [x] Added/updated tests for the behaviour I changed. - [x] No new UI strings (logic/security/docs only). - [x] No storage-format change to author content (the `.md` round-trips identically; escaping is internal). FILE_FORMAT.md unaffected. - [x] Docs updated (README, ARCHITECTURE, BUILD, CHECKS, new SOURCE_MAP). ## Notes for reviewers - The "Normalize formatting" commit is whitespace-only but touches files outside this branch's logic — review it separately; it's what makes the gate green. - One residual is left as a follow-up (noted in code/commit): an imported theme profile's `logoPath` still resolves via `resolveTrustedAssetPath`. - `make mutate` is intentionally **not** in `make check` (slow; survivors need triage). Running it on the validator flagged real untested branches. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

brenno added 8 commits 2026-06-29 22:23:54 +00:00

Harden theme-profile import and restore green make check ... 692d14719d

T1 — make check was red on this branch: three bare `catch (_)` sites
against a baseline of 0 (the review found one on main; the WebDAV
feature added two more). All typed now:
- markdown_safety: `on ArgumentError` (code is already range-checked)
- webdav_service: `on FormatException` for a malformed href
- settings_dialog: `catch (e, st)` + logError on the probe fallback

S2 — HTML/CSS injection via an imported theme profile. ThemeProfile
colours/fonts are interpolated raw into the HTML-export <style> block
and the audience-window inline styles, while the profile travels as
base64url JSON in the deck front matter that the import-safety scanner
never decodes. ThemeProfile.fromJson now validates every colour to a
strict #RRGGBB literal (reusing normalizeChartColor) and whitelists
fontFamily/codeFontFamily against the offered sets, falling back to the
trusted defaults. Inherited colours (e.g. checklistCheckedColor ->
accentColor) are sanitised on the resolved value, so a poisoned accent
can't leak through the chain. Lossless for real data: the colour picker
already enforces #RRGGBB and only offered fonts are selectable.

Adds ThemeProfile.fromJson regression tests for the injection payloads,
the inheritance path, font whitelisting and lossless normalisation.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Close SSRF on the live remote-media path (S1) ... 26a5a05f3b

Remote background/inline images and remote <video> URLs from an imported
deck were gated only by the lexical isAllowedMediaUrl/isBlockedHost,
which does no DNS for a hostname. A deck image/video whose host resolves
to 169.254.169.254, 127.0.0.1 or an RFC1918 address (classic DNS-based
SSRF), or a non-dotted encoding like http://2130706433/, slipped through
straight into NetworkImage / VideoPlayerController.networkUrl — unlike
the .ocideck URL import, which already resolves and pins.

Adds NetGuard.isAllowedMediaUrlResolved: lexical prefilter + a DNS
resolution that rejects any host mapping to an internal address, memoised
per host (positives cached, negatives dropped so a transient blip retries
and an internal host stays re-checked). Both media sinks now gate on it:
- video: an async resolve in _MediaPlaybackHost.initMedia before the
  network controller is created (keeps the placeholder on refusal);
- images: a FutureBuilder in _resolvedImage that shows a placeholder
  while resolving and the blocked-placeholder on refusal.

Residual: NetworkImage / VideoPlayerController re-resolve at fetch time,
so the socket is not pinned and a DNS rebind in the check→fetch window
stays possible — documented on the method. Full byte-level pinning would
mean fetching media through a guarded HttpClient ourselves (impractical
for streamed video); deferred.

Adds NetGuard.isAllowedMediaUrlResolved tests (scheme, loopback/private/
link-local literals incl. the metadata endpoint, public literal).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Fix four silent data-loss bugs in the markdown round-trip (C1-C4) ... 95c95d9a1c

Each one lost or mangled author content on save/load with no warning:

C1 — split-image captions share one <div class="image-caption"> joined by
" | "; a caption containing " | " shifted text across the boundary on the
next load. Each caption's pipes now map to a private-use sentinel before
the join and back after the split, so the delimiter is unambiguous.

C2 — video trim wrote whole seconds with start floor() / end ceil(),
widening the segment by up to a second on every save until neighbouring
splits overlapped. Now writes decimal seconds (_msToSec); _secToMs
already parsed fractional seconds, so the window round-trips losslessly.

C3 — a speaker note containing "-->" closed the HTML comment early, so
the note was truncated and its tail leaked into the slide body. The token
is escaped to "--\>" on write and restored on read.

C4 — a table cell with the literal text "<br>" became a newline every
parse (indistinguishable from the newline encoding). A literal "<br>" is
now escaped to "\<br>" on write; _unescapeCell walks char-by-char so an
escaped "\<br>" stays literal while an unescaped "<br>" is the newline.

Adds a round-trip regression test per bug.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Validate fence balance per slide; drop dead _style guard (C5, C6) ... 1cd62855bc

C5 — _validateFenceBalance counted ``` fences across the whole document,
so an unclosed fence in one slide and another in the next summed to an
even count and neither was reported. It now runs per slide block (with a
line offset for accurate reporting), so each imbalance is caught.

C6 — two `t.startsWith('<!-- _style:')` branches in the rich-text parser
were unreachable: the line list is built from `remaining`, which has all
HTML comments (including `<!-- _style: -->`) stripped earlier in the same
method. Removed; the live `<div>`/`</div>` structural checks stay.

Adds a validator test for two slides whose fence imbalances cancel out.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Add class-level guards against the fixed bug families (+1 bug they found) ... 2a16cf1511

The per-bug fixes only close the specific cases; these tests catch the
whole class so a future regression fails CI instead of shipping silently.

- markdown_roundtrip_fuzz_test: an adversarial corpus (pipe, "-->", "<br>",
  backslash, HTML metachars, newlines, unicode) through every lossless
  field of every slide type. Writing it surfaced a real bug the manual
  review missed: a SINGLE image caption containing " | " was truncated too
  (the generic line parser split *every* image-caption div, not just
  twoImages). Fixed by encoding pipes -> sentinel in EVERY caption write
  (single and two-caption funnel through _writeCaptionDiv), so splitting on
  the real " | " separator is unambiguous; single-caption parse de-sentinels.

- network_sink_guard_test: source-scan (like asset_path_guard_test) that
  fails if NetworkImage / VideoPlayerController.networkUrl / HttpClient
  appears outside the files that apply NetGuard — a new sink can't quietly
  reintroduce the SSRF hole.

- settings_provider_test: a fuzz over ThemeProfile.fromJson asserting every
  style colour reads back as #RRGGBB and every font as a whitelisted family,
  for a list of injection payloads — the single checklist of CSS-interpolated
  fields.

Full suite 897 -> green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Add mutation check for the dead-branch class (C6) that coverage misses ... a5ac245af1

C6 was an `||` operand that could never be true (a `<!-- _style:` check on
input whose comments were already stripped). `dart analyze` doesn't flag it
(it's semantically, not syntactically, dead) and line coverage marks the
line as covered because another operand on the same line is hit. Only
mutation testing catches this class.

tool/mutation_check.dart forces each String.startsWith/endsWith predicate
in a target file to false and reruns the given tests; a surviving mutant is
a dead or untested predicate. `make mutate` wires it up (FILE/TESTS
overridable), deliberately outside `check` — it is slow and survivors need
triage (dead -> remove, untested -> add a test). The target file is always
restored (finally + git-checkout safety net).

Running it on markdown_validator.dart surfaced 9 survivors: live but
untested validation branches (the <!-- tlp:/advance:/list_style: comment-key
checks and the malformed <video>/<audio> guards) — real coverage gaps worth
a follow-up.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Normalize formatting to the pinned toolchain (Flutter 3.44.2 / Dart 3.12.2) ... e9f97e8e41

The repo was committed with a different dart-format version than the one CI
pins (flutter 3.44.2 → dart 3.12.2), so `make format-check` — and therefore
the CI gate — was red: 18 files (incl. ones untouched this branch) wanted
the pinned formatter's multi-line + trailing-comma layout. Ran `dart format`
with the pinned toolchain so format-check is 0-changed. Whitespace only; the
full suite stays green (897).

This is the formatter half of "make the gate enforceable": local dev on the
pinned 3.44.2 now produces byte-identical formatting to CI.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

docs: per-file source map, toolchain pin, and class-guard/mutate docs ... 9aa369c5a8

- Add docs/SOURCE_MAP.md: a one-line description of every one of the 178
  files under lib/, grouped by directory (complements ARCHITECTURE.md's
  high-level map). Linked from README, ARCHITECTURE, and CHECKS.
- Pin the toolchain for local dev to match CI: add .tool-versions
  (flutter 3.44.2-stable) and tighten BUILD.md to state the exact version
  and the "use the Flutter-bundled dart, not a standalone one" caveat that
  caused the format-check drift.
- CHECKS.md: refresh the scale numbers (~900 tests / ~105 test files / 178
  lib files), fix the coverage-floor text (60%, was inconsistently 50),
  document the new class-level guards (network-sink scan, round-trip fuzz,
  theme-injection fuzz) and the manual `make mutate` dead-branch check, and
  reference .tool-versions in the version-pin section.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

brenno merged commit c813476777 into main 2026-06-29 22:24:49 +00:00

brenno referenced this pull request from a commit 2026-06-29 22:24:49 +00:00

Merge pull request 'Harden per review (SSRF/theme-injection/round-trip), add class guards, green CI gate + source map' (#33) from fix/review-t1-s2-hardening into main

brenno deleted branch fix/review-t1-s2-hardening 2026-06-29 22:24:49 +00:00

brenno referenced this pull request from a commit 2026-07-24 19:37:34 +00:00

fix(a11y): de themawissel-reparatie sloopte de deckstaat — bescheidener opgelost (#780)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

accepted

Assessed and will be built

  

bug

Something is broken

  

declined

Assessed and will not be built

  

docs

Documentation only

  

duplicate

Already tracked elsewhere

  

enhancement

New feature or improvement request

  

good first issue

Small, self-contained, and a good place to start

  

in-progress

Someone is working on this right now. Do not start it in parallel.

  

needs-info

Waiting for the reporter

  

privacy

Personal data can leak. Handled before anything else.

  

security

A weakness someone could use. Handled before anything else.

  

triage

Awaiting assessment

No labels

accepted

bug

declined

docs

duplicate

enhancement

good first issue

in-progress

needs-info

privacy

security

triage

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

LibreKAT/Ocideck!33

No description provided.