# v1.5 "Tag Regression Fix, Reading-Shell Fold-In & v1.3 Closure" — Security Review

**Reviewer:** security-engineer (independent judge) · **Date:** 2026-07-01
**Standard:** ASVS L1 · **Policy:** BLOCK-ON-HIGH (project config)
**Scope (TC-02):** the tag surface as a whole — the v1.3 tag-admin endpoints + admin
UI (never security-reviewed in v1.3) PLUS the NEW Phase-29 server-authoritative body
rewrite + body-inclusive enumeration. Code span `95ca67d..1435ea6` on `main`
(Phase-29 tag surface `da4adc5..1435ea6`); docs HEAD `c343a30`.

**Verdict: SHIP.** No critical/high findings. Two LOW findings (one new-but-fail-closed
validator divergence; one pre-existing/carried DoS-hardening item) + one informational
tooling note. **No code fix is required before v1.5 milestone completion.**

**Substrate (stated honestly):** the **SMTC daemon is OFFLINE**, so structural/behavioral
claims rest on **code-trace (Structural/Honest tier — MEDIUM for a grep/read-only claim
about behavior)** unless backed by a **re-run test or a live endpoint probe (Empirical —
HIGH)**. Each control below carries its tier. `go vet` clean; the five surface packages'
test suites re-run green; `npm audit` (web, prod) **0 vulnerabilities**; `govulncheck`
could not run (toolchain panic — see F-03).

---

## 1. Threat model summary

The regression fix turns an untrusted admin tag string + untrusted page bodies into
**on-disk `.md` bytes** — a strictly larger surface than v1.3's frontmatter-only writes,
and one v1.3 never had reviewed at all. Sources → sinks → sanitizers:

| # | Surface | Source (untrusted) | Sink | Asset at risk | Sanitizer / control |
|---|---|---|---|---|---|
| A | Tag-admin endpoints (TM-01..04) | HTTP client (session-bound admin) | rename/merge/delete/list dispatch | privilege boundary (workspace-wide mutation) | `RequireRole(admin)` from SESSION (`router.go:298`) + global nosurf CSRF (`router.go:358`) |
| B | Inline-tag re-validation (TR-03) | `new_name` / `target` strings | the token spliced as `#`+name into a page body | prose/code integrity (body injection) | `okf.ValidateInlineTag` at the HTTP layer (`handlers_tags_admin.go:278,362`), 400 before any write |
| C | Frontmatter tags-region write | validated tag set | YAML `tags:` sequence bytes | page/YAML integrity (injection, corruption) | `SetTagsFrontmatter`→`okf.SetTags` (byte-stable, structural yaml.Node edit) + `ValidateTagCharset` (SEC-02) |
| D | Body `#tag` rewrite (TR-01) | page body bytes + the tag token | matched `#tag` byte spans in the body | prose/code-region integrity (markdown injection, region breakout) | conservative region-aware `okf.RewriteHashtags`/`FindHashtags` (skip-when-uncertain, offset-parity-pinned) |
| E | Body-inclusive enumeration (TR-04/06) | repo tree + page body bytes | enumeration file reads (`PagesWithBodyTag`, `DetectStaleBodyTags`) | FS path safety (traversal / trashed / non-`.md` read) | SEC-01 resolver `repo.Resolve` (lexical + `os.Root` + `EvalSymlinks`) + `isLivePage` skip rules |

Trust boundaries crossed: **(1)** authenticated-admin client payload → server filesystem
write (path-safety + content-integrity lens); **(2)** untrusted page bytes → server byte
scanner → rewritten on-disk bytes (noninterference / injection lens). Actor model is
**admin→workspace**: the mutating operations are admin-gated, so the residual risk class is
*corrupt-page / prose-injection / traversal*, not privilege escalation — but the body-splice
surface is new, so it gets the deepest focus (surfaces B, D, E).

Confirmatory tooling: `go vet ./internal/{okf,pages,server,repo,graph}/...` **clean**;
`go test` on those five packages **all pass** (re-run this review, cached+fresh); `npm audit`
(web, `--production`) **0 vulnerabilities**; `govulncheck` **could not run** (F-03), mitigated
by an **empty `go.mod`/`go.sum` diff** across `95ca67d..HEAD` (zero new Go deps this milestone).

---

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

### F-01 — Inline-grammar gate accepts nested `a/b` that the frontmatter write then rejects → 500, not 400 (LOW · new · fail-closed)

- **Location:** gate `internal/server/handlers_tags_admin.go:278` (rename), `:362` (merge);
  reject `internal/okf/repair.go:180-184` (`SetTags`→`ValidateTagCharset`, regex `repair.go:153`).
- **Source→sink:** `new_name`/`target` = `"a/b"` → `ValidateInlineTag` **accepts** (nested tag
  is a legal inline `#a/b`, `validate.go:44` treats `/` as a separator) → the op proceeds → for
  a page carrying the tag in **frontmatter** (`changed==true`), `ApplyTagsBatch` calls
  `SetTagsFrontmatter`→`okf.SetTags`, whose defense-in-depth `ValidateTagCharset`
  (`^[a-zA-Z0-9_ -]+$`) **forbids `/`** → the batch returns an infrastructure error → the
  handler emits **HTTP 500**, not a clean 400.
- **Evidence tier: Empirical (live endpoint probe).** A throwaway `server_test` seeded a page
  with frontmatter tag `ops`, then POSTed `{"old_name":"ops","new_name":"a/b"}`:
  **apply → 500** `{"error":"Failed to apply tag rename."}`; **preview → 200** (preview never
  hits the write path). The two validators' divergence is itself pinned by the shipped unit test
  `TestValidateInlineTag_DoesNotLoosenCharset` (`validate_test.go:80-81`:
  `ValidateTagCharset("a/b")` errors; `validate_test.go:21`: `ValidateInlineTag("a/b")` ok).
- **Why it is NOT high, and NOT a security defect:** the failure is **fail-closed** — the batch
  errors *before* any commit is enqueued (`batch.go:162-165` returns the error up; no
  `EnqueueCommit`), so **nothing is written, nothing is corrupted, no partial state, no
  injection**. It is admin-gated. The security invariant ("no unsafe token reaches a body/YAML
  sink") **holds** — the token is rejected, just at the wrong layer with the wrong status code.
  Note a body-only page (`SkipFrontmatter==true`, `batch.go:161`) would accept `#a/b` as a valid
  inline nested tag — which is correct; the divergence bites *only* the frontmatter-backed path.
- **Disposition: ACCEPT for v1.5; route to architect as a robustness gap.** Recommended fix
  (non-blocking): either (a) also gate the handler with the frontmatter charset (reject `/` at
  400 for consistency), or (b) teach `SetTags`/`ValidateTagCharset` to accept the nested-tag
  grammar so nested tags are first-class end-to-end. Pick one so the inline gate and the
  frontmatter gate agree. Not a milestone blocker.

```finding
id: SEC-v1.5-F01
severity: low
title: Inline-grammar gate accepts nested a/b but frontmatter charset rejects it → 500 not 400 (fail-closed)
location: internal/server/handlers_tags_admin.go:278
status: open
disposition: accept-and-route-up
```
```evidence
finding: SEC-v1.5-F01
warrant_kind: live_endpoint_probe
tool: go-test (throwaway server_test probe) + code-trace
detail: "Seeded page frontmatter tag=ops; POST rename ops->a/b: APPLY=500 {\"error\":\"Failed to apply tag rename.\"}, PREVIEW=200. Divergence pinned by shipped units: ValidateInlineTag(\"a/b\")=ok (validate_test.go:21), ValidateTagCharset(\"a/b\")=err (validate_test.go:80). Fail-closed: SetTags errors before EnqueueCommit (batch.go:162-165) — no write, no corruption, no injection. Admin-gated."
```

### F-02 — Tag-admin POST bodies have no `MaxBytesReader` cap (LOW · pre-existing, carried from v1.4 F-02)

- **Location:** `internal/server/handlers_tags_admin.go:268` (rename), `:348` (merge), `:425`
  (delete) — `json.NewDecoder(r.Body).Decode(...)` with no body cap. Same class as v1.4 F-02
  (`handlers_pages.go`), which is already a tracked v1.4-carried follow-up (REQUIREMENTS Future).
- **Source→sink:** unbounded HTTP request body → in-memory JSON decode. The tag-admin request
  shapes are tiny (`{old_name,new_name}` etc.), so the practical exposure is negligible, and the
  route is **admin-authenticated**. This is a milestone-wide hardening item, not a v1.5 regression:
  v1.5 adds three more uncapped decode sites of the *same kind*, it does not introduce the pattern.
- **Evidence tier: Structural (code-trace).** No `http.MaxBytesReader` on any tag-admin decode;
  contrast `handlers_attachments.go` which caps. YAML alias-bomb is not additive (the write path
  decodes structured `yaml.Node`s, not the request body; the request body is a fixed-shape struct).
- **Disposition: ACCEPT for v1.5 (pre-existing); fold into the existing v1.4 F-02 follow-up.**
  A single global `MaxBytesReader` middleware on `/api/v1` write routes closes v1.4 F-02 and this
  together. Does not block v1.5 — authenticated, admin-only, tiny expected bodies.

```finding
id: SEC-v1.5-F02
severity: low
title: Unbounded JSON request body on tag-admin rename/merge/delete (no MaxBytesReader)
location: internal/server/handlers_tags_admin.go:268
status: open
disposition: accept-preexisting-fold-into-v1.4-F02
```

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

- `govulncheck ./internal/{okf,pages,server,repo,graph}/...` **panics**:
  `ForEachElement called on type containing *types.TypeParam` (a govulncheck/`x/tools`↔Go-1.26
  toolchain incompatibility; exit 2, NOT the exit-3 "affected" path) — the *identical* failure
  v1.4 recorded as its F-03.
- **Mitigated:** `git diff --stat 95ca67d..HEAD -- go.mod go.sum` is **empty** — v1.5 introduced
  **zero new Go dependencies**, so the Go supply-chain delta for this milestone is nil. The
  frontend delta is covered by the clean `npm audit`. Recorded so the Go-side dep-vuln
  confirmation is logged as *unavailable-this-environment*, not silently *passed*.

---

## 3. Verified clean (on the record)

The plan's five scope areas, traced (and where noted re-run) and confirmed sound. v1.3's
RBAC/CSRF is confirmed here for the first time (v1.3 received no review at all).

- **(a) Admin-only RBAC + CSRF — Empirical (re-run tests) + Structural (router trace).**
  All four endpoints — `GET /admin/tags`, `POST /admin/tags/{rename,merge,delete}` — are wired
  **inside** the admin group `authed.Group(admin.Use(auth.RequireRole(auth.RoleAdmin)))`
  (`router.go:297-298`, routes `:335-338`). Authorization enters the request at exactly **one**
  place — `loadCurrentUser` attaches the session-bound user (`handlers_users.go:53-56`) and
  `RequireRole` reads **only** from there, **never client input** (T-00.03-01). CSRF: the global
  `csrfProtect` (nosurf) wraps the **whole** handler (`router.go:358`, `middleware.go:16-28`) with
  **no path exemption for mutating routes** — nosurf enforces a valid token on every POST while
  passing safe GETs. Re-run test `TestTagAdmin*` RBAC matrix asserts **editor → 403** on all three
  mutations (`handlers_tags_admin_test.go:164-169`); sibling missing-CSRF tests
  (`handlers_describe_test.go:150`, `handlers_templates_test.go:148`) pin the global gate. *(Test-
  coverage note, not a finding: the `GET /admin/tags` list has no independent 403 test — it inherits
  the same group gate the three tested mutations prove; worth a one-line test for completeness.)*

- **(b) Server-side inline-tag re-validation (the `ValidateTagCharset`↔inline-grammar gap CLOSED)
  — Empirical (re-run gate tests).** `okf.ValidateInlineTag` gates rename `new_name`
  (`handlers_tags_admin.go:278`) and merge `target` (`:362`) **at the HTTP layer, 400 before any
  write**. Delete carries no new token (removal), so it needs none. The gap is genuinely closed:
  `ValidateInlineTag` (`validate.go:32-66`) enforces the body alphabet `[A-Za-z0-9_/-]`, ≥1
  non-digit, and a slash policy — so it **rejects** a space (`"a b"`), an all-digit run
  (`"1984"`), and anything with a `.`/metachar/traversal shape (`"../etc"` fails the alphabet on
  `.`). The endpoint gate tests `TestTagRename_InlineGrammarGate` and
  `TestTagMerge_InlineGrammarGate` (`handlers_tags_admin_test.go:282-296,354-370`) drive exactly
  the tokens the *old* `ValidateTagCharset` accepts but the *new* gate must reject (`"a b"`,
  `"1984"`) → **400**, so a 400 can only come from the new gate being the active HTTP validator.
  `TestValidateInlineTag` (`validate_test.go`) covers the full Lezer positive/negative corpus, and
  `TestValidateInlineTag_DoesNotLoosenCharset` guards that the two gates stay distinct. **Net: no
  space/metachar/all-digit token can be spliced into a body.** (The one residual — nested `a/b`
  reaching the frontmatter charset — is F-01, and it is fail-closed.)

- **(c) Byte-stable frontmatter + body writes — Empirical (re-run goldens) + Structural.** The
  frontmatter tags region is edited by `okf.SetTags` as a **structural `yaml.Node` replacement**
  (`repair.go:200-217`) — it builds a `SequenceNode` of `!!str` scalars and swaps the value node
  **in place**, so keys/comments/ordering are untouched and the YAML cannot be injected (a crafted
  value is a scalar node, not raw YAML text; and it already passed the charset gate). The body is
  edited only by `RewriteHashtags` (surface d). The batch's **no-op guard**
  `bytes.Equal(out, raw)` (`batch.go:176`) never stages a byte-identical write. Goldens
  `batch_bodyrewrite_test.go` (rename/delete/merge/**body-only**) assert body `#old`→`#new`,
  frontmatter `[new]`, byte-identical frontmatter region on the body-only `SkipFrontmatter` path,
  and the regression-closed property (the committed body no longer recognizes the old tag, so the
  client reconcile cannot re-materialize it). **No YAML/Markdown injection, no prose corruption,
  no malformed document from a crafted tag/body.**

- **(d) `RewriteHashtags` untrusted-input surface — Structural (code-trace) + Empirical (corpus
  parity re-run). DEEPEST FOCUS.** The splice-produces-invalid-structure risk the plan flags is
  **structurally impossible for validated input**, for two independent reasons: **(1) it never
  edits inside an excluded region** — `FindHashtags` (`hashtag.go:83-126`) is the SINGLE recognizer,
  region-aware via `classifyBlocks` (fenced ``` /~~~, indented 4-space/tab code, HTML type-1/type-6
  blocks) + inline-code backtick skip + `\#` escape skip + ATX-heading marker exclusion +
  blockquote content-start boundary; a `#` in any excluded region is left untouched. **(2) the
  replacement bytes carry no structural metacharacter** — a rename/merge splices `#`+`new`, and
  `new` already passed `ValidateInlineTag`, so it is `[A-Za-z0-9_/-]` only: **no backtick, no `<`,
  no newline, no `*`/`_`/`[`/`]`, no space** — it cannot open a code fence, an HTML block, an
  emphasis run, or a link, and cannot merge/split lines. A delete removes `#`+token and, only when
  space-bounded on **both** sides, exactly one space (`hashtag.go:442-450`), never crossing a
  newline — it removes bytes, it cannot add structure. The scanner is **conservative
  (skip-when-uncertain)**: an unclassifiable `#` is left unchanged, so the worst case is a *stale
  tag* (self-healing on the next correct op), never a corrupted region. Correctness of the boundary
  scan itself (the only residual) is pinned **offset-for-offset against the real Lezer grammar** by
  the whole-document parity corpus (`hashtag_test.go` + `testdata/hashtag_corpus.json`, re-run
  green), and the Go predicates (`isBodyByte`/`isNonDigitByte`/`scanTagBody`, `hashtag.go:244-282`)
  mirror `web/src/lib/cm/hashtag.ts:49-88` exactly. **A crafted tag/body cannot break out of a
  code region or inject Markdown.**

- **(e) Body-inclusive enumeration read path (TR-04/TR-06) — Structural (code-trace) + Empirical
  (re-run pages/graph suites). HIGHEST-VALUE NEW SURFACE.** `PagesWithBodyTag` and
  `DetectStaleBodyTags` (`tagscan.go:65-159`) live in the **pages service** and read every body
  through the **SEC-01 resolver**: `s.repo.Tree()` enumerates paths, then `s.repo.Read(it.Path)`
  routes through `Repo.Resolve` (`files.go:27-28` → `path.go:83`), which enforces, in order,
  reject-empty/NUL → percent-decode-and-re-validate → Windows-separator normalize → reject
  absolute → `filepath.Clean` + reject any `..` segment → `os.Root.Stat` (OS syscall guard) →
  `EvalSymlinks` boundary-prefix check (`withinRoot`). `Tree()` uses `filepath.WalkDir`
  (`files.go:96-98`), which **does not follow symlinks** (Lstat-based), and any symlinked `.md`
  that WalkDir *reports* is re-resolved by `Read`→`Resolve`, whose `EvalSymlinks` rejects an
  escape — so a symlink cannot smuggle an out-of-root read. `isLivePage` (`tagscan.go:29-37`)
  applies the **same skip rules as the graph rebuild**: directories skipped, non-`.md` skipped,
  the trash dir and its subtree skipped. A per-page read error is skipped, never sinking the scan
  (mirrors the rebuild). The graph layer's **"reads NO bodies — only path enumeration"** invariant
  (`query.go:256`) is preserved: `PagesByTag` is a **parametrized** SQL read of the frontmatter-
  derived `page_tags` cache (`query.go:433-434`, `LOWER(tag)=LOWER(?)` — no SQL injection, no body
  read), and the body scan was correctly placed in the pages service, **not** bolted into
  `query.go`. **No path traversal, no trashed/non-`.md` read, no resolver bypass.** The resolver
  itself is independently fuzzed (`path_test.go:112` `FuzzResolve` asserts no input ever yields an
  escaping path) and symlink-escape tested (`:104`).

- **(f) Admin UI (`TagManagement.tsx`) — Structural (code-trace).** Every tag name and page path
  in the preview renders via **JSX text interpolation** (`{p.page_path}`, `{p.current_tags.join}`,
  `{t.tag}` — `TagManagement.tsx:154-156,213,228-229`), which React auto-escapes; there is **no
  `dangerouslySetInnerHTML`** anywhere in the component. The merge target is chosen from a
  `<select>` of existing tags (`:210-215`), so the UI cannot even submit a free-form target — and
  regardless, the **endpoint** (not the UI) is the enforced trust boundary. No DOM-XSS surface.

---

## 4. Risk verdict

| Dimension | Assessment |
|---|---|
| Authz / CSRF on the tag-admin endpoints | **Correct.** Admin-gated from SESSION (`RequireRole`), global nosurf CSRF on all mutations; editor→403 re-run green. v1.3's never-reviewed RBAC confirmed sound. |
| Body injection via a crafted rename/merge target | **Controlled.** `ValidateInlineTag` gates at the HTTP layer (400 before write); no space/metachar/all-digit token can splice into prose. Gate tests re-run green. |
| Markdown/region breakout via `RewriteHashtags` | **Controlled.** Region-aware + skip-when-uncertain + alphabet-constrained splice ⇒ breakout structurally impossible for validated input; offset-parity corpus re-run green. |
| YAML injection / prose corruption / malformed doc | **Controlled.** Structural `yaml.Node` edit + byte-stable emit + no-op guard; goldens re-run green. |
| Path traversal / trashed / non-`.md` read via the NEW enumeration | **Controlled.** `PagesWithBodyTag`/`DetectStaleBodyTags` route every read through the SEC-01 resolver with graph-rebuild skip rules; resolver fuzzed + symlink-escape tested. |
| Admin-UI XSS | **Controlled.** React text-interpolation only; no `dangerouslySetInnerHTML`. |
| Robustness (validator consistency) | **One LOW (F-01):** nested `a/b` → 500 not 400 — fail-closed, no write, no injection. Route up. |
| DoS | **One LOW (F-02):** uncapped JSON body on tag-admin POSTs — pre-existing, admin-only, tiny bodies; fold into v1.4 F-02. |
| Supply chain | npm clean (0 vulns); **zero new Go deps** (empty go.mod/go.sum diff); govulncheck N/A (F-03). |

**Overall: SHIP.** No critical/high findings ⇒ **BLOCK-ON-HIGH not triggered.** The two LOW
findings are accept-for-ship and routed to the architect (F-01 as a robustness gap; F-02 folded
into the existing v1.4 F-02 follow-up). **No fix is required before v1.5 milestone completion.**
The milestone-close security gate v1.3 never received is now on the record: the tag-admin
RBAC/CSRF is confirmed sound, and the new body-splice + enumeration surface is controlled.
