# v1.4 "Editor Enhancements & Authoring Polish" — Security Review

**Reviewer:** security-engineer (independent judge) · **Date:** 2026-07-01
**Range:** `b634050~1..HEAD` on `main` (phases 21–27; deep focus 25/26/27)
**Verdict: SHIP.** No critical/high findings. Two LOW findings, both accept-for-ship
(one delegated-but-sound upstream control to track; one pre-existing DoS-hardening item).
**No code fix is required before milestone completion.**

---

## 1. Threat model summary

The milestone adds three new untrusted-input → sink paths.

| # | Surface (phase) | Source (untrusted) | Sink | Asset at risk | Sanitizer / control |
|---|---|---|---|---|---|
| A | Mermaid (26, ED-05a) | page markdown = diagram source | rendered `<svg>` in the live DOM | DOM integrity → stored XSS | mermaid `securityLevel:'strict'` → DOMPurify (upstream) **+** `DOMParser('image/svg+xml')` + parsererror/non-svg-root check + `importNode`/`appendChild` (never `innerHTML`) |
| B | Syntax highlight (25, ED-04a) | fenced code bytes | token `<span>`s in `<pre><code>` | DOM integrity → stored XSS | `createElement` + `textContent`/`createTextNode` only; `matchLanguageName(…, fuzzy=false)` |
| C | Page templates (27, ED-06a) | client-submitted seed frontmatter+body; `folder` | new `.md` file on disk | FS path safety; page/YAML integrity | `okf.ValidateFrontmatter` (fail-closed), `okf.SetField`/`Repair`, `repo.Resolve` |

Trust boundaries crossed: **(1)** untrusted page bytes → browser DOM (noninterference / XSS
lens); **(2)** authenticated-editor client payload → server filesystem write (path-safety +
content-integrity lens). Assets: the session/DOM (XSS), the on-disk repo (traversal), page
and index integrity (corrupt-YAML). Actor model for surface C is editor→editor (templates are
authored by editors, create is editor-gated) — no trust elevation, so the C risk is
*corrupt-page / DoS*, not privilege escalation.

Confirmatory tooling: `go vet ./...` clean; `npm audit` (web, prod) **0 vulnerabilities**
(covers all new deps — CodeMirror `lang-*` grammars, mermaid 11.16.0, DOMPurify 3.4.11).
`govulncheck` could not run (toolchain panic — see F-03); mitigated by an empty `go.mod` diff.

---

## 2. Findings (most-severe-first)

### F-01 — Mermaid event-handler XSS defense is single-sourced to upstream DOMPurify (LOW · delegated control)

- **Location:** `web/src/lib/cm/widgets.ts:284-312` (MermaidWidget.render), `web/src/lib/cm/mermaid.ts:24-29`.
- **Source→sink:** page markdown (diagram source) → `renderMermaid` → svg string → `DOMParser('image/svg+xml')` → `importNode(true)` → `wrapper.appendChild(svgEl)`.
- **Evidence tier: confirmed via code trace.** The injection path is genuinely safe against
  `<script>`: a script node created by `DOMParser` has its "already-started" flag set, so it
  will not execute on `appendChild` — real defense-in-depth. **However**, `importNode` +
  `appendChild` copies attributes verbatim: an inline SVG event handler (`onload=`, `onclick=`)
  would still fire if it reached this widget. The DOMParser boundary does **not** strip event
  handlers — only `<script>` execution. Therefore the *event-handler* XSS defense rests
  **entirely** on the upstream control: mermaid `securityLevel:'strict'` (verified set,
  `mermaid.ts:26`, and asserted `mermaid.test.ts:49`) piping output through DOMPurify before the
  string ever reaches the widget. `bindFunctions` is verified never called (grep: comment-only);
  `suppressErrorRendering:true` verified set; no `securityLevel` override anywhere; MermaidWidget
  is the sole render sink (`livePreview.ts:529`).
- **Is the delegation sound? Yes, for ship.** DOMPurify 3.4.11 and mermaid 11.16.0 are current;
  `npm audit` reports 0 vulnerabilities across the tree; strict + DOMPurify is the vendor-
  prescribed sanitization path; the widget correctly avoids re-sanitizing (hand-rolling a second
  SVG sanitizer would be a footgun). This is a *sound delegation of a single-point control*, not
  a gap — but it is a single point, on a third-party lib, defending an XSS boundary.
- **Disposition: ACCEPT for v1.4; track.** Pin mermaid + dompurify (currently `^` ranges in
  `web/package.json`), keep `npm audit` in CI, and re-run this trace on any mermaid **major**
  bump (a strict-mode regression upstream is the realistic failure mode). Not a blocker.

```finding
id: SEC-v1.4-F01
severity: low
title: Mermaid event-handler XSS defense delegated to upstream DOMPurify (single point)
location: web/src/lib/cm/widgets.ts:284-312
status: open
disposition: accept-and-track
```
```evidence
finding: SEC-v1.4-F01
warrant_kind: manual_review_attested
tool: code-trace + grep + npm-audit
detail: "DOMParser/importNode strips <script> exec (already-started flag) but NOT event-handler attrs; onload/onclick stripping delegated to mermaid securityLevel:'strict'+DOMPurify 3.4.11; strict verified set (mermaid.ts:26), bindFunctions never called, no override; npm audit 0 vulns"
```

### F-02 — JSON write bodies (`POST/PUT /pages`) have no `MaxBytesReader` cap (LOW · pre-existing, milestone-wide)

- **Location:** `internal/server/handlers_pages.go:209` (create), `:259` (save) — `json.NewDecoder(r.Body).Decode(...)` with no body cap. Contrast `internal/server/handlers_attachments.go:58,120` which *do* cap via `http.MaxBytesReader`.
- **Source→sink:** unbounded HTTP request body → in-memory decode. The template-seeded create
  adds `seedFrontmatter`+`seedBody` to an already-uncapped body, so v1.4 enlarges (does not
  create) the surface.
- **Evidence tier: confirmed via code trace.** No `MaxBytesReader` on any JSON page endpoint.
  An authenticated editor can post an arbitrarily large body → memory-pressure DoS. YAML
  alias-bomb / billion-laughs is *not* additive here: `okf.ValidateFrontmatter` decodes into
  `map[string]any` via `gopkg.in/yaml.v3`, which bounds alias expansion; and there is **no
  server-side `{{}}` template engine** (verified — substitution is client-only), so the
  substitution injection/DoS vector the plan asked about does not exist server-side.
- **Disposition: ACCEPT for v1.4 (pre-existing); recommend a milestone-wide fix.** Add a global
  JSON-body cap (a small `MaxBytesReader` middleware on `/api/v1` write routes). Does not block
  v1.4 — the exposure is unchanged in kind from the shipped `Save` path and is authenticated.

```finding
id: SEC-v1.4-F02
severity: low
title: Unbounded JSON request body on page create/save (no MaxBytesReader)
location: internal/server/handlers_pages.go:209
status: open
disposition: accept-preexisting-recommend-fix
```

### F-03 — `govulncheck` could not run (INFORMATIONAL · evidence-quality, not a code defect)

- `govulncheck ./...` panics: `ForEachElement called on type containing *types.TypeParam`
  (a govulncheck/Go-1.26 toolchain incompatibility, not a code issue; exit 2, not the exit-3
  "affected" path).
- **Mitigated:** `git diff --stat b634050~1..HEAD -- go.mod go.sum` is **empty** — v1.4
  introduced zero new Go dependencies, so the Go supply-chain delta for this milestone is nil.
  All new deps are frontend and are covered by the clean `npm audit`. Flagged only so the
  Go-side dep-vuln confirmation is recorded as *unavailable-this-environment*, not *passed*.

---

## 3. Verified clean (on the record)

These are the plan's explicit questions, traced and confirmed sound:

- **Syntax highlight (25):** every node — plain `code.textContent` (`widgets.ts:142`) and each
  token via `span.textContent`/`createTextNode` (`:184-189`) — no `innerHTML`. Grammar match is
  `fuzzy=false`, so unknown/`mermaid` info strings stay plain. Async teardown guarded
  (`destroyed` + `isConnected`, detached-fragment atomic swap). **No finding.**
- **Mermaid injection (26):** `securityLevel:'strict'` + `suppressErrorRendering:true` set;
  SVG injected only via `DOMParser('image/svg+xml')` + parsererror check (`:292`) + non-`<svg>`-
  root check (`:294`) + `importNode`/`appendChild`, never `innerHTML`; `bindFunctions` never
  called; whole-pass `try/catch` → raw-code fallback that cannot be bypassed; teardown/stale
  guards present. Sound **modulo F-01's delegation note.**
- **Templates (27):** **no server-side `{{}}` substitution engine exists** (grep-confirmed — the
  only `{{` are Go struct literals); substitution is client-only, so no server injection/DoS
  surface. `okf.ValidateFrontmatter` strict-decodes into `map[string]any` → rejects duplicate
  keys + malformed YAML → `ErrTemplateInvalid` → clean 400 (fail-closed). User title wins via
  `SetField` (no second `title:` key). **The server never trusts a client template path** —
  `CreateFromTemplate` takes CONTENT only; the new-page path derives solely from the user's
  slugified title. `folder` reaches `uniquePath`→`repo.Resolve`, a layered resolver (lexical
  `..`/NUL/absolute reject + percent-decode + Windows-separator normalize + `os.Root` syscall
  guard + `EvalSymlinks` boundary check — `internal/repo/path.go`), so a traversal-shaped folder
  fails closed. `ListTemplates` re-gates each candidate through `repo.Resolve` (SEC-01). **No
  finding.**
- **Authz/CSRF (cross-cutting):** `GET /templates` → any-authed group (safe method, non-secret
  content) ✔; `POST /pages` (template-seeded create) → editor subgroup
  (`auth.RequireRole(RoleEditor)`, role from session, never client input) ✔; global nosurf CSRF
  wraps the whole handler (`router.go:359`), enforcing on the mutating POST while exempting the
  GET. **No finding.**

Cross-reference (not v1.4-scoped, from qa-phase27 note): `client.ts getPage(path)` interpolates
the path without `encodeURIComponent` — a routing/correctness issue for exotic filenames, **not
a security issue** (reads are any-authed and resolver-gated server-side). Informational only.

---

## 4. Risk verdict

| Dimension | Assessment |
|---|---|
| XSS via new render surfaces (mermaid/highlight) | Controlled. Highlight is textContent-only (sound). Mermaid is strict+DOMPurify+DOMParser; event-handler defense delegated-but-sound (F-01, track). |
| Path traversal via templates | Controlled. No client path is trusted; `repo.Resolve` gates all writes. |
| Corrupt-page / YAML injection via templates | Controlled. `ValidateFrontmatter` fails closed; no server `{{}}` engine. |
| Authz / CSRF on new endpoints | Correct (any-authed read, editor+CSRF write). |
| Supply chain | npm clean (0 vulns); no Go dep change (govulncheck N/A — F-03). |
| DoS | One pre-existing low (F-02, unbounded JSON body). |

**Overall: SHIP.** The two LOW findings are accept-for-ship. **No fix is required before v1.4
milestone completion.** Recommended (not blocking): F-02 global JSON body cap; F-01 pin
mermaid/dompurify + CI `npm audit` + re-review on mermaid major.
