---
author: security-engineer
responsible: security-engineer
milestone: v1.6
milestone_name: Tag System Completion & Authoring Polish
phase: 31, 32, 35, 36
status: passed
standard: ASVS L1
policy: block-on-high
confidence: manual_review + empirical (re-run tests)
head: 1653933
range: 36accfa..1653933
---

# v1.6 "Tag System Completion & Authoring Polish" — Security Review

**Reviewer:** security-engineer (independent judge of security risk) · **Date:** 2026-07-02
**Standard:** ASVS L1 · **Policy:** BLOCK-ON-HIGH (project config)
**Scope:** the four roadmap-flagged security-relevant phases of v1.6 — **31** (bulk
multi-tag write surface), **32** (net-new on-disk tag-metadata store + color/description
injection surface into badge DOM), **35** (tag-operation undo write surface), **36 EP-01**
(the `#tag` stored-XSS boundary in `MarkdownProse`). Code span `36accfa..1653933` on `main`
(v1.5 close → v1.6 HEAD). Phases 33 (hierarchy), 34 (analytics), 37 (insert templates)
carry skip-witnesses (§5) — the roadmap does not flag them security-relevant and the audit
confirms why.

**Verdict: `passed`.** No critical, high, medium, or low security findings. One
**informational** observation (the Phase-36 copy-button `textContent` leak — a test/UX
gap that carries **no security impact**, and which was *already closed in-milestone* by
the engineer at HEAD `1653933`). **No code fix is required before v1.6 milestone
completion.**

**Substrate (stated honestly):** the **SMTC daemon is not available in this environment**,
so every structural/behavioral claim rests on **code-trace (Structural/Honest tier — MEDIUM
for a grep/read-only claim about behavior)** unless it is backed by a **re-run test
(Empirical — HIGH)**. Each control below carries its tier. Confirmatory tooling this pass:
`go vet ./internal/{tagmeta,server}/...` **clean**; the four surface test suites **re-run
green** (Go server 20/20 tag-admin handler tests, tagmeta `TestValidateColor` +
`TestValidateDescription`, web `widgets.test.ts` **32/32**, `AgentAnswer.test.tsx` **4/4**);
`npm audit --omit=dev` (web, prod) **OK / 0 vulnerabilities**; **zero new Go deps and zero
new npm deps** across `36accfa..HEAD` (empty `go.mod`/`go.sum` and empty
`web/package.json`/`package-lock.json` diffs). `govulncheck` not run (same Go-1.26 toolchain
incompatibility recorded as v1.4/v1.5 F-03; mitigated by the empty Go-dep delta).

---

## 1. Threat model summary

v1.6 completes the tag system. The security-relevant surfaces are all **admin→workspace**
mutations (or, for EP-01, an untrusted-page-bytes → rendered-DOM boundary), so the residual
risk classes are *privilege-boundary breach*, *injection into on-disk bytes*, *stored XSS*,
and *concurrent-edit clobber* — **not** anonymous-user escalation. Sources → sinks →
sanitizers:

| # | Surface (phase) | Source (untrusted) | Sink | Asset at risk | Sanitizer / control |
|---|---|---|---|---|---|
| A | Bulk multi-tag (31) | session-bound admin payload (`add/remove/filter_tags`, `page_paths`) | frontmatter `tags:` region via `ApplyTagsBatch` | privilege boundary + YAML/page integrity | `RequireRole(admin)` from SESSION + global nosurf CSRF; `ValidateTagCharset` on each **added** tag; reuses the reviewed byte-stable batch path (no new write path) |
| B | Tag-metadata color/description/parent (32) | admin payload (`color`, `description`, `parent`, `tag`) | `Tags/{tag}.md` on-disk bytes **and** `span.style.color` on badge DOM | CSS/style injection, HTML injection, XSS, path traversal | `ValidateColor` (hex/palette subset), `ValidateDescription` (single-line, HTML-metachar-free, ≤200), `ValidateTagCharset` (tag→path), `CheckCycle`; write via SEC-01 resolver; badge via `createElement`+`textContent`+`style.color` (no `innerHTML`) |
| C | Tag-operation undo (35) | admin request (no free-form payload) + stored journal row | inverse `ApplyTagsBatch` replay | concurrent-edit clobber, history integrity | `RequireRole(admin)` + CSRF; **fresh** per-page revision read + **semantic stale check** (409-guard); forward-commit only; op-journal is SQLite operational data |
| D | `#tag` badge in `MarkdownProse` (36 EP-01) | committed page body bytes (history preview + agent answer) | react-markdown → DOM | stored XSS | badge is a **typed hast element node** (`data.hName`/`hProperties`/`hChildren`), NOT raw HTML; `rehype-raw` OFF / `rehype-sanitize` ON; tag text is a hast `text` node (inert) |

Trust boundaries crossed: **(1)** authenticated-admin payload → server filesystem/DB write
(path-safety + content-integrity + RBAC lens); **(2)** untrusted committed page bytes →
client Markdown renderer → live DOM (noninterference / stored-XSS lens). The load-bearing
invariant the whole tag track rests on — **all tag writes go through
`ApplyTagsBatch`/`SetTagsFrontmatter` byte-stably** — is preserved by every new write
surface (31 and 35 reuse it verbatim; 32 writes `Tags/*.md` through the same single-writer
commit path page saves use).

---

## 2. Per-phase audit (ASVS L1)

### Phase 31 — Bulk Multi-Tag Operations (`handleTagBulk`) — CLEAN

- **RBAC + CSRF — Empirical (re-run test) + Structural (router trace).** The route
  `admin.Post("/admin/tags/bulk", h.handleTagBulk)` (`router.go:362`) is wired **inside**
  the admin subgroup `authed.Group(admin.Use(auth.RequireRole(auth.RoleAdmin)))`
  (`router.go:308-309`). Authorization is read from the **session-bound** user
  (`loadCurrentUser`), never client input; the global nosurf `csrfProtect` wraps all
  mutating POSTs. `TestBulkTagRBAC` re-runs **green** (editor → 403).
- **Input validation — Empirical + Structural.** Each **added** tag is re-validated with
  `okf.ValidateTagCharset` at the handler, 400 before any write
  (`handlers_tags_admin.go:653-658`). Removed/filter tags are *matched, not written*, so
  they correctly need no charset gate (`:651-652` documents this). A missing page-set or
  op-set yields 400 (`:642-649`). `TestBulkTagValidation` re-runs **green**.
- **No new write path — Structural.** The handler resolves the page set server-side (explicit
  paths ∪ `PagesByTag` filter — a **parametrized** graph read), then calls the *existing*
  `buildTagApplyItems` + `pages.ApplyTagsBatch` (`:687,:709`) with **`bodyOld=""`** — the
  body channel is disabled, so bulk is **frontmatter-only** and never invokes
  `RewriteHashtags`/`ValidateInlineTag`. N pages → **1** commit; per-page stale is 409-guarded
  inside the batch. `TestBulkTagGolden` (add `[a,b]`+remove `[c]` across 3 pages → 1 commit)
  and `TestBulkTagStale` (a stale page reported + excluded) re-run **green**.
- **Verdict:** the bulk surface adds no new taint path; it is admin-gated, CSRF-protected,
  charset-validated, and routes through the v1.5-reviewed byte-stable batch. **No finding.**

### Phase 32 — Tag Metadata Store (`handleTagMeta` + `internal/tagmeta`) — CLEAN

- **RBAC + CSRF — Empirical + Structural.** `admin.Post("/admin/tags/meta", h.handleTagMeta)`
  (`router.go:373`) inside the same admin subgroup + global nosurf CSRF. `TestTagMetaRBAC`
  re-runs **green**.
- **Color — no CSS injection — Empirical + Structural.** `tagmeta.ValidateColor`
  (`validate.go:27-42`) accepts **only** a hex color (`#RGB`/`#RRGGBB`, hex-digit-checked) or
  a fixed 6-token palette (`red/blue/green/amber/purple/gray`, `validate.go:13-20`). `url(...)`,
  `var(--x)`, `expression(...)`, `<script>`, wrong-length hex, and `red;` are all **rejected**
  with 400 — asserted by `TestValidateColor` (`store_test.go:204-211`, re-run **green**). The
  color is the only value that flows into `span.style.color` on badge DOM, so this closes the
  CSS/style-injection boundary at the server gate.
- **Description — HTML-metachar-safe — Empirical + Structural.** `tagmeta.ValidateDescription`
  (`validate.go:59-73`) rejects newlines (single-line only), `>200` chars, and any of
  `<>&"'` — defense-in-depth atop the `textContent` rendering. `TestValidateDescription`
  re-runs **green**.
- **HashtagWidget XSS invariant — Structural (code-trace).** The badge is built
  `createElement`+`textContent` ONLY: `span.textContent = "#" + this.tag` and the color is
  applied via `span.style.color = this.color` (`widgets.ts:373-384`) — **no `innerHTML`**.
  `widgets.test.ts` "builds DOM via createElement + textContent ONLY … markup stays inert"
  asserts a single inert text node + zero element children even for a `#a<b>c` tag
  (`widgets.test.ts:175-185`); re-run **green** (32/32).
- **On-disk write + tag→path safety — Structural.** The store holds `*repo.Repo`, "the SEC-01
  resolver path for file I/O" (`store.go:20-25`); `SetMeta` writes `Tags/{tag}.md`
  (`tagPath`, `store.go:112-114`) through `s.repo.Write` / the single-writer commit path
  (`store.go:186`). The tag flowing into the path is first `okf.ValidateTagCharset`-gated at
  the handler (`handlers_tags_admin.go:954`, regex `^[a-zA-Z0-9_ -]+$` — `repair.go:159`),
  which **forbids `/` and `.`** → no traversal into the path segment; and `repo.Write` re-checks
  through the resolver regardless (defense-in-depth). Parent is `ValidateParent`-gated +
  `CheckCycle` runs **before** the write (`handlers_tags_admin.go:966-976`) so an
  un-rebuildable on-disk cycle is never written. `TestTagMetaSetGet`/`Clear`/`Absent`/
  `Validation` re-run **green**.
- **Verdict:** color/description/parent/tag are all validated at a fail-closed 400 gate;
  the badge DOM upholds the no-`innerHTML` XSS invariant; the on-disk write cannot traverse.
  **No finding.**

### Phase 35 — Tag-Operation Undo (`handleTagUndo`) — CLEAN

- **RBAC + CSRF — Empirical + Structural.** `admin.Post("/admin/tags/undo", h.handleTagUndo)`
  (`router.go:369`) inside the admin subgroup + global nosurf CSRF. `TestUndoRBAC` re-runs
  **green**.
- **409-guard reads fresh revision, never clobbers a concurrent edit — Empirical + Structural.**
  `BaseRevision` is read **fresh per page at undo time** (`h.pages.Get` → `pg.Revision`,
  `handlers_tags_admin.go:1112,1210`), not a stored revision; the `ApplyTagsBatch` 409-guard
  protects the read→commit window. On top of that a **semantic stale check** compares the
  page's *current* frontmatter tags to what the original op would have produced from the
  journal's `pre_op_tags`; a mismatch → the page is reported `stale` and skipped, never
  written (`:1191-1200`). `TestUndoStale` re-runs **green** (an intervening-edit page is
  skipped with a report).
- **Forward-commit, never history rewrite — Structural.** Undo builds inverse `TagApplyItem`s
  and replays them through `pages.ApplyTagsBatch` (`:1223`) — a normal forward commit,
  consistent with `pages.RestoreVersion`; there is **no git history rewrite**. Undo-of-undo
  (redo) is itself journaled (`:1252`). `TestUndoRenameGolden`/`UndoDelete`/`UndoBulk`/
  `UndoOfUndo` re-run **green**.
- **Op-journal is operational data — Structural.** The journal is a SQLite table
  `tag_op_journal` (migration `0013_tag_op_journal.sql`); inserts are **parametrized**
  (`INSERT INTO tag_op_journal (op_type, op_detail, pages) VALUES (?, ?, ?)`,
  `:1053-1055`) — no SQL injection. The journal write is best-effort/non-fatal (`:1056-1059`),
  so a journal hiccup cannot corrupt an already-committed op. `TestJournalBulk` re-runs
  **green**.
- **Verdict:** admin-gated, CSRF-protected, concurrency-safe (fresh revision + semantic stale
  guard), forward-commit only, parametrized journal. **No finding.**

### Phase 36 EP-01 — `#tag` badge in `MarkdownProse` (stored-XSS boundary) — CLEAN

- **Badge is a sanitized element node, NOT raw HTML — Structural (code-trace).** `remarkHashtag`
  emits a typed mdast node whose `data.hName="span"`, `data.hProperties.className=["cm-hashtag"]`,
  and `data.hChildren=[{type:"text", value:tagText}]` (`remark-hashtag.ts:172-180`).
  `mdast-util-to-hast` turns this into a hast **`element`** node with an inert **`text`**
  child — the tag bytes are never parsed as markup.
- **`rehype-raw` OFF / `rehype-sanitize` ON preserved — Structural.** `MarkdownProse` runs
  `rehypePlugins={[rehypeSlug, [rehypeSanitize, headingIdSchema]]}` (`MarkdownProse.tsx:91`);
  the raw-HTML rehype plugin is **deliberately not enabled** (`:38-39`). The schema permits
  only `className="cm-hashtag"` on the already-allowed `span` (`:30-33`) — a specific class on
  a safe element, **never** raw HTML. Raw HTML in page content therefore still cannot become
  live markup.
- **Stored-XSS test passes — Empirical.** `AgentAnswer.test.tsx` "does NOT render raw HTML in
  the answer (stored-XSS guard, T-04-21)" (feeding `<img src=x onerror="alert(1)">`) re-runs
  **green** (4/4). Code/escape/heading/URL-fragment `#` cases stay literal by construction
  (the plugin walks `text` nodes only, skips `code`/`inlineCode`, honors `\#` and start-boundary
  rules — `remark-hashtag.ts:104-183`).
- **Verdict:** EP-01 renders the badge without weakening the sanitize guard; the stored-XSS
  boundary holds. **No finding.**

---

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

**No critical / high / medium / low security findings.** One informational observation:

### F-01 — Copy-button label leaked into the read-mode code block `textContent` (INFORMATIONAL · test/UX, no security impact · **RESOLVED in-milestone**)

- **Location:** `web/src/lib/cm/widgets.ts:149-170` (`CodeBlockWidget.toDOM` appends the
  EP-02 copy `<button>` as a sibling of `<code>` inside `<pre>`); surfaced by the test
  `web/src/lib/cm/livePreview.test.ts:473-480`.
- **What it was:** the Phase-36 EP-02 copy button (`btn.textContent = "Copy"`) lives inside
  the `<pre>`, so `pre.textContent` became `"fmt.Println(1)Copy"` — the prior test
  (`livePreview.test.ts`) asserted `pre.textContent === "fmt.Println(1)"` and regressed, and a
  user select-all + copy of the rendered read-mode block would have swept the word "Copy" into
  the clipboard.
- **Why it is NOT a security finding.** The leaked bytes are a **static literal** (`"Copy"`),
  not untrusted or secret data; there is no injection, no XSS, no privilege, no taint crossing
  any boundary — the button is `createElement`+`textContent`-built and the no-`innerHTML`
  invariant is intact (`widgets.test.ts:218-243`, re-run green). This is a
  **test-maintenance + UX** concern only. Per the honesty floor it routes as
  **informational**, not `low`.
- **Resolution (already on disk at HEAD `1653933`).** The engineer's fix kept the button a
  **sibling of `<code>`** (so `<code>.textContent` is the clean rendered code), added
  `user-select: none` to `.cm-copy-btn` (`LivePreviewEditor.css:173`) so a select-all/copy of
  the block excludes the button label, and corrected both tests to assert on the `<code>`
  child (`livePreview.test.ts:476-480`, `widgets.test.ts:224-226`). The full `widgets.test.ts`
  suite (**32/32**) and the corrected `livePreview` case re-run **green**. The finding is
  **closed** in the same milestone; recorded here for the milestone-close record.

```finding
id: SEC-v1.6-F01
severity: informational
title: Copy-button label leaked into read-mode code block textContent (test/UX, no security impact; resolved in-milestone)
location: web/src/lib/cm/widgets.ts:149
status: closed
disposition: resolved-in-milestone
```
```evidence
finding: SEC-v1.6-F01
warrant_kind: manual_review_attested
tool: code-trace + go/vitest re-run
detail: "Leaked bytes are the static literal \"Copy\", not untrusted/secret data — no injection/XSS/privilege/taint, so informational not low. Resolved at HEAD 1653933: button kept sibling of <code> (widgets.ts:170), user-select:none on .cm-copy-btn (LivePreviewEditor.css:173), tests corrected to assert <code>.textContent (livePreview.test.ts:478, widgets.test.ts:224-226). widgets.test.ts 32/32 + livePreview case re-run green."
```

---

## 4. Verified clean (on the record)

| Dimension | Assessment |
|---|---|
| Admin RBAC + CSRF on the new endpoints (bulk/meta/undo) | **Correct.** All three wired inside `authed.Group(admin.Use(RequireRole(admin)))` (`router.go:308-309,362,369,373`), authorization from SESSION never client input; global nosurf CSRF on every mutating POST. `TestBulkTagRBAC`/`TestTagMetaRBAC`/`TestUndoRBAC` re-run green (editor→403). |
| Bulk write path | **Controlled.** No new write path — reuses `buildTagApplyItems`+`ApplyTagsBatch`, frontmatter-only (`bodyOld=""`), N→1 byte-stable commit, per-page 409-guard; added tags `ValidateTagCharset`-gated. Goldens re-run green. |
| Color → CSS/style injection into badge DOM | **Controlled.** `ValidateColor` hex/palette subset only; `url()`/`var()`/`expression`/`<script>` rejected at a 400 gate (`TestValidateColor` green). |
| Description → HTML injection | **Controlled.** `ValidateDescription` single-line + no `<>&"'` + ≤200; rendered as `textContent` (`TestValidateDescription` green). |
| HashtagWidget / badge DOM XSS | **Controlled.** `createElement`+`textContent`+`style.color`, no `innerHTML`; a `<b>`-bearing tag stays one inert text node (`widgets.test.ts` green, 32/32). |
| Tag-name → on-disk path traversal (`Tags/{tag}.md`) | **Controlled.** `ValidateTagCharset` forbids `/` and `.`; `repo.Write` re-resolves via SEC-01 resolver (defense-in-depth). |
| Undo concurrency (clobber a newer edit) | **Controlled.** Fresh per-page revision read + semantic stale check + `ApplyTagsBatch` 409-guard; stale page skipped with a report (`TestUndoStale` green). Forward-commit only — no history rewrite. |
| Op-journal SQL | **Controlled.** Parametrized inserts/reads on `tag_op_journal` (migration 0013); best-effort/non-fatal write can't corrupt a committed op. |
| EP-01 stored XSS in `MarkdownProse` | **Controlled.** Badge is a typed hast element node (not raw HTML); `rehype-raw` OFF / `rehype-sanitize` ON; `AgentAnswer` stored-XSS test green (4/4). |
| Supply chain | **Clean.** npm audit (prod) OK/0; **zero new Go deps and zero new npm deps** across `36accfa..HEAD`. `govulncheck` N/A (toolchain incompat, carried F-03; mitigated by empty Go-dep delta). |

---

## 5. Skip-witnesses (non-security-relevant phases)

Per the workflow's conditional-gate skip-witness discipline — the roadmap flags only 31/32/35/36
as security-relevant; the remaining v1.6 phases are recorded as `not_applicable`, not silently
omitted:

```gate_result
gate: security-review
phase: "33"
status: not_applicable
basis: "Tag hierarchy is display/organization only — reuses the SAME admin-only + CSRF + validated /admin/tags/meta endpoint as Phase 32; server-side total cycle check (CheckCycle) rejects cycles/self-parent before write; zero page bytes change. No new trust boundary or write path."
```
```gate_result
gate: security-review
phase: "34"
status: not_applicable
basis: "Tag usage analytics is an admin-only, read-only view over the existing parametrized graph.TagPageCounts query — no mutation, no new index, no untrusted sink."
```
```gate_result
gate: security-review
phase: "37"
status: not_applicable
basis: "Insert-at-cursor templates reads a Templates/ snippet via the existing safe-path resolver and inserts substituted text as a normal client-side CM6 transaction; no placeholder executes code, no server write path, no new trust boundary."
```

---

## 6. Risk verdict

**Overall: `passed`.** No critical/high findings ⇒ **BLOCK-ON-HIGH not triggered**; and in
fact no medium/low security findings either. The one informational observation (F-01) is a
test/UX gap with **no security impact** and is **already resolved** at HEAD `1653933`. The
four security-relevant surfaces are each controlled and test-covered: the bulk and undo write
paths reuse the v1.5-reviewed byte-stable batch under admin RBAC + CSRF; the net-new metadata
store validates color/description/parent/tag at fail-closed 400 gates and upholds the
no-`innerHTML` badge invariant; the EP-01 read-surface badge is a sanitized element node that
keeps `rehype-raw` OFF / `rehype-sanitize` ON. **No code fix is required before v1.6 milestone
completion.**

**Substrate caveat (honesty floor):** claims here are code-trace (Structural/Honest — MEDIUM)
elevated to Empirical (HIGH) wherever a re-run test backs them; the SMTC daemon was not
available this environment, so no Decision/VERIFIED-tier certificate is asserted. Every
security-relevant suite re-ran green, `go vet` is clean, and the Go/npm dependency delta for
v1.6 is empty.
