diff --git a/.github/codeql/accepted-alerts.json b/.github/codeql/accepted-alerts.json new file mode 100644 index 0000000..2141ac5 --- /dev/null +++ b/.github/codeql/accepted-alerts.json @@ -0,0 +1,30 @@ +{ + "$comment": [ + "Alerts that stay open on purpose, in the shape the strict and doctor", + "baselines already use: a committed list a check compares against, so", + "debt is visible in the diff and only an unaccounted alert breaks the", + "build.", + "", + "Only for alerts that are real and not yet fixed. An alert that is a", + "false positive belongs in GitHub's own dismissal, with its reason,", + "where it stops being open at all — three were dismissed that way on", + "2026-08-05 after being measured linear. This file is the other case:", + "known, real, and waiting on a decision.", + "", + "Keyed by rule and path rather than by alert number, because a number", + "belongs to an instance and changes when the line moves — which is how", + "the same acceptance.ts pattern arrived as a `new` alert on PR #22", + "after the file was edited around it.", + "", + "An entry is a promise that somebody measured it and opened a card." + ], + "accepted": [ + { + "rule": "js/regex-injection", + "path": "packages/workfile/src/modules/search/search.ts", + "card": "T-0190", + "reason": "Not a false positive. `/(a+)+$/` — six characters, inside the 256-character pattern cap and the imsu flag subset — takes 57s against a 32-character body and does not finish against the 20,000-character one the search path allows. Open because the fix needs a design decision the card lays out, not because it is noise.", + "measured": "2026-08-05" + } + ] +} diff --git a/.github/codeql/codeql-config.yml b/.github/codeql/codeql-config.yml new file mode 100644 index 0000000..1242a85 --- /dev/null +++ b/.github/codeql/codeql-config.yml @@ -0,0 +1,14 @@ +name: "workfile" + +# Fixtures are inputs, not code. `test/fixtures/legacy-v1/` is a snapshot of the +# pre-workfile board this package migrates *away* from — it is checked in so the +# migration has something to read, is never imported and never runs, and its one +# alert (`js/stack-trace-exposure`) is a faithful record of how that old server +# behaved. Analysing it reports the bug we keep on purpose. +# +# The demo bundle is generated output; the analysis belongs on its source. +paths-ignore: + - packages/workfile/test/fixtures + - packages/workfile/dist + - packages/workfile/ui/dist + - apps/demo/public diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 39043f6..e1d2bbd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -75,4 +75,5 @@ jobs: - uses: github/codeql-action/init@v4.37.3 with: languages: javascript-typescript + config-file: ./.github/codeql/codeql-config.yml - uses: github/codeql-action/analyze@v4.37.3 diff --git a/.github/workflows/codeql-baseline.yml b/.github/workflows/codeql-baseline.yml new file mode 100644 index 0000000..3a3bbaa --- /dev/null +++ b/.github/workflows/codeql-baseline.yml @@ -0,0 +1,68 @@ +name: CodeQL baseline + +# An alert on a pull request is commented on inline and gets looked at within a +# minute. An alert on code that is already merged has no such messenger, so +# twelve of them sat open on `main` for a week and the `codeql` job passed on +# every single run — it uploads results, it does not read them (T-0162). +# +# This reads them. Daily rather than on push: the alerts API serves the previous +# analysis until the new one finishes processing, so a check that races the +# upload reports the state it was called to replace. A day is the honest +# latency, and it is bounded — which "nothing ever looks" was not. +on: + schedule: + - cron: "17 6 * * *" + workflow_dispatch: +# Deliberately not on `pull_request`. The first version of this ran on changes +# to the accepted list, on the theory that the PR editing the list should be +# the one to check it — and it failed on its own first run, correctly. The +# alerts API answers for `main`, so a pull request that *fixes* three alerts +# sees them still open and reports the state it exists to change. There is no +# query that would help: the alerts a merge will close do not close until the +# merge is analysed. Daily is the honest cadence for a question only `main` can +# answer. + +jobs: + baseline: + runs-on: ubuntu-latest + permissions: + security-events: read + contents: read + steps: + - uses: actions/checkout@v7 + - name: Every open alert is accounted for + env: + GH_TOKEN: ${{ github.token }} + run: | + set -euo pipefail + gh api "repos/${{ github.repository }}/code-scanning/alerts?state=open&per_page=100" \ + --jq '[.[] | {rule: .rule.id, path: .most_recent_instance.location.path, severity: .rule.security_severity_level, number: .number}]' \ + > open.json + + jq -r '.accepted[] | "\(.rule)\t\(.path)"' .github/codeql/accepted-alerts.json | sort -u > accepted.tsv + jq -r '.[] | "\(.rule)\t\(.path)"' open.json | sort -u > open.tsv + + { + echo "## CodeQL alerts open on \`main\`" + echo + echo "| severity | rule | path |" + echo "|---|---|---|" + jq -r '.[] | "| \(.severity) | \(.rule) | \(.path) |"' open.json + } >> "$GITHUB_STEP_SUMMARY" + + # Unaccepted: open but not in the committed list. + if comm -23 open.tsv accepted.tsv | grep -q .; then + echo "::error::CodeQL alerts are open that the accepted list does not cover" + comm -23 open.tsv accepted.tsv + exit 1 + fi + + # Stale: accepted but no longer open. Not a failure — the debt + # went away, which is the direction this is supposed to move — + # but the entry has to go, or the list stops meaning anything. + if comm -13 open.tsv accepted.tsv | grep -q .; then + echo "::warning::accepted entries no longer match an open alert; remove them" + comm -13 open.tsv accepted.tsv + fi + + echo "every open alert is accounted for" diff --git a/.project/cards/T-0162-twelve-codeql-alerts-sit-open-on-main-and-nothing-.md b/.project/cards/T-0162-twelve-codeql-alerts-sit-open-on-main-and-nothing-.md index 6e5d58d..9c1f3d4 100644 --- a/.project/cards/T-0162-twelve-codeql-alerts-sit-open-on-main-and-nothing-.md +++ b/.project/cards/T-0162-twelve-codeql-alerts-sit-open-on-main-and-nothing-.md @@ -1,12 +1,12 @@ --- id: T-0162 title: Twelve CodeQL alerts sit open on main and nothing fails because of them -status: backlog +status: review type: task priority: medium area: core effort: M -scope: [packages/workfile/src/modules/cards/acceptance.ts, packages/workfile/src/modules/docs, packages/workfile/src/modules/records/index.ts, packages/workfile/src/modules/search/search.ts, packages/workfile/ui/src/components/Markdown.tsx] +scope: [packages/workfile/src, packages/workfile/ui, .github/workflows] origin: [T-0157] created: 2026-08-05 updated: 2026-08-05 @@ -65,6 +65,16 @@ over a whole body — which is the reason to check rather than assume. ## Acceptance criteria - [ ] Every open alert is either fixed or dismissed with a stated reason -- [ ] The two in `Markdown.tsx` are assessed against what the UI actually renders -- [ ] A new alert on merged code reaches somebody without a PR comment -- [ ] `pnpm run check` green, doctor 0/0 +- [x] The two in `Markdown.tsx` are assessed against what the UI actually renders +- [x] A new alert on merged code reaches somebody without a PR comment +- [x] `pnpm run check` green, doctor 0/0 + +## Activity + +- 2026-08-05 16:20Z illodev@local#2cddaf94 · claimed +- 2026-08-05 16:41Z illodev@local#2cddaf94 · doing → review + +## Notes + +- 2026-08-05 16:41Z illodev@local#2cddaf94 — Criterion 1 is deliberately left unchecked, and the reason matters more than the box: alerts #19 and #20 (js/regex-injection in search.ts) are neither fixed nor dismissed. They are real — /(a+)+$/ is six characters, passes the 256-char pattern cap and the imsu flag allowlist, and takes 57s against a 32-character body against a 20,000-character cap — so dismissing them would be false, and fixing them needs a design decision between a worker deadline, RE2 and dropping user regex. They are accepted in .github/codeql/accepted-alerts.json with the measurement and tracked as T-0190. Checking the box because the spirit is met is what T-0174 exists to catch. +- 2026-08-05 16:41Z illodev@local#2cddaf94 — Triaged by measurement, which is what the card said the work was. js/polynomial-redos at 16x input: acceptance.ts ITEM 1x, records/index.ts QUERY_TOKEN 1x, docs/validation.ts link scan 217x, docs/docs.ts trailing slash 230x. The two quadratic ones were worse than reading suggested — 43.6s on a 128KB document body, in the scan the doctor runs for every document. Both bounded now: the link target excludes newlines and caps at 1024 (longer than any POSIX path, and data: URIs are skipped by the scheme test anyway), and the trailing-slash strip is a slice loop. The two in Markdown.tsx were assessed against React 19, which does block javascript: via sanitizeURL — so nothing was exploitable, and the allowlist is now ours in ui/src/safe-url.ts rather than a dependency's minor version. Three false positives and the legacy fixture dismissed on GitHub with their measurements; fixtures excluded from analysis in .github/codeql/codeql-config.yml. Criterion 3 is a daily codeql-baseline workflow comparing open alerts against a committed accepted list — the same ratchet shape as strict-baseline.json and doctor-baseline.json, daily rather than on push because the alerts API serves the previous analysis until the new one finishes processing. 328 tests, ratchet 494, doctor 0/0. diff --git a/.project/cards/T-0190-a-regex-search-query-can-hang-the-process-and-the-.md b/.project/cards/T-0190-a-regex-search-query-can-hang-the-process-and-the-.md new file mode 100644 index 0000000..abd9834 --- /dev/null +++ b/.project/cards/T-0190-a-regex-search-query-can-hang-the-process-and-the-.md @@ -0,0 +1,41 @@ +--- +id: T-0190 +title: A regex search query can hang the process, and the caps do not stop it +status: backlog +type: bug +priority: high +area: core +created: 2026-08-05 +updated: 2026-08-05 +--- + +`search` accepts a `/pattern/flags` query ([[CHG-0021]]) and compiles it with `new RegExp`. Three guards already bound it — the pattern is capped at 256 characters, flags are a subset of `imsu`, and each record body is truncated to 20,000 characters before matching. None of them bounds *backtracking*. + +Measured with `/(a+)+$/`, six characters, well inside every cap: + +| body | time | +|---|---| +| 20 chars | 104ms | +| 24 chars | 232ms | +| 28 chars | 3,717ms | +| 30 chars | 14,342ms | +| 32 chars | 57,113ms | + +Two more characters is roughly four times the work. Against the 20,000-character body cap it does not finish. + +On the CLI this is self-inflicted: your own query, your own terminal, Ctrl-C. What makes it a bug rather than a footgun is the HTTP surface. `/api/v2/search?q=` takes the same query, the rebinding guard covers cross-origin but not a request that is allowed to arrive, and `workfile ui --host 0.0.0.0` puts it on the network — the configuration the CI workflow itself describes as "unauthenticated read and write access to a repository". + +This is CodeQL alerts #19 and #20 (`js/regex-injection`). They are **not** false positives, which is why they are a card and not a dismissal. + +No obvious cheap fix, which is the other reason this is a card: + +- **Reject nested quantifiers.** A heuristic, never complete, and it would refuse legitimate patterns. +- **Run the match under a deadline.** Node has no regex timeout; it needs a worker thread, and the match loop is currently synchronous inside an async function. +- **Use a linear-time engine.** RE2 is the correct answer and a native dependency, which this package has avoided everywhere else. +- **Drop user-supplied regex.** Honest, and removes a documented feature. + +## Acceptance criteria + +- [ ] A catastrophic pattern cannot hold the process past a bounded time +- [ ] The HTTP search path is bounded whether or not the CLI one is +- [ ] Whichever route is taken says what it costs the regex feature diff --git a/.project/changelog/unreleased/CHG-0129-a-record-body-can-no-longer-stall-the-doctor-or-choose-a-link-scheme.md b/.project/changelog/unreleased/CHG-0129-a-record-body-can-no-longer-stall-the-doctor-or-choose-a-link-scheme.md new file mode 100644 index 0000000..94df76b --- /dev/null +++ b/.project/changelog/unreleased/CHG-0129-a-record-body-can-no-longer-stall-the-doctor-or-choose-a-link-scheme.md @@ -0,0 +1,22 @@ +--- +id: CHG-0129 +title: A record body can no longer stall the doctor or choose a link scheme +type: fixed +area: core +visibility: public +created: 2026-08-05 +updated: 2026-08-05 +--- + +Two scans over document bodies were quadratic. A body of unclosed Markdown +links took 43.6 seconds at 128KB in the link check the doctor runs for every +document, and a folder of path separators took 189ms at 16,000 characters in +`doc create --folder`. Both are bounded and linear now, and a test holds each +to a measured budget. + +The UI also decides for itself which URL schemes a record body may put in a +link. `javascript:`, `data:`, `file:` and protocol-relative targets render as +their own text instead of becoming a link. React 19 already blocked +`javascript:`, so nothing here was exploitable — but a record body is written +by whichever agent held the card, and that defence belongs to this package +rather than to a dependency's minor version. diff --git a/.project/memory/learnings/LRN-0024-a-static-analyser-reports-ambiguity-only-a-measurement-tells-you-which.md b/.project/memory/learnings/LRN-0024-a-static-analyser-reports-ambiguity-only-a-measurement-tells-you-which.md new file mode 100644 index 0000000..2458049 --- /dev/null +++ b/.project/memory/learnings/LRN-0024-a-static-analyser-reports-ambiguity-only-a-measurement-tells-you-which.md @@ -0,0 +1,42 @@ +--- +id: LRN-0024 +title: A static analyser reports ambiguity; only a measurement tells you which half is real +status: active +confidence: high +created: 2026-08-05 +updated: 2026-08-05 +--- + +Ten CodeQL alerts sat open on main, every one rated `high` by its rule. The +temptation in both directions is the same mistake: fix them all to clear the +tab, or dismiss them all because static analysis is noisy. [[T-0162]] said the +work was "saying which", and that turned out to be literal — the answer came +from a benchmark, not from reading the code. + +Same rule, `js/polynomial-redos`, four sites, at 16× the input: + +| site | growth | +|---|---| +| `acceptance.ts` ITEM | 1× | +| `records/index.ts` QUERY_TOKEN | 1× | +| `docs/validation.ts` link scan | **217×** | +| `docs/docs.ts` trailing slash | **230×** | + +Two noise, two quadratic — and the quadratic ones were bad in a way reading had +not suggested: 43.6 seconds on a 128KB document body, in the scan the doctor +runs over every document in the workspace. + +The reverse error was there too. `js/regex-injection` in `search.ts` looked +handled: a 256-character pattern cap, a flag allowlist, a 20,000-character body +cap. Three guards, none of which bounds backtracking. `/(a+)+$/` is six +characters and takes 57 seconds against a 32-character body ([[T-0190]]). + +**A static analyser reports that a shape *can* be exploited. Whether it is +depends on the engine, the caps around it and the input that reaches it — three +things the analyser does not know and a benchmark answers in minutes.** The +measurement is also what makes a dismissal honest: "false positive" alone is an +opinion, and the same sentence with numbers in it is a claim someone can check. + +Cheap enough that there is no excuse: each of these was a five-line script. +Related to [[LRN-0018]] — the gap is always narrower than it looks, and here it +was narrower in both directions at once. diff --git a/packages/workfile/src/modules/docs/docs.ts b/packages/workfile/src/modules/docs/docs.ts index 367eef6..902bb09 100644 --- a/packages/workfile/src/modules/docs/docs.ts +++ b/packages/workfile/src/modules/docs/docs.ts @@ -392,8 +392,25 @@ async function maxSequence(directory, prefix) { * escapes are rejected with the same containment criterion the workspace * configuration uses for its own paths. */ +/** + * Trailing separators are stripped by slicing, not by `/\/+$/`. + * + * That pattern is unanchored at the start, so on a folder of nothing but + * separators the engine restarted the greedy run at every position and failed + * at `$` each time: 0.8ms at 1,000 characters and 189ms at 16,000, which is + * quadratic on a value that arrives from `doc create --folder` and from the + * HTTP body. The loop below is the same operation and reads as what it does. + */ +function withoutTrailingSlashes(value: string): string { + let end = value.length; + while (end > 0 && value[end - 1] === "/") end -= 1; + return value.slice(0, end); +} + export function normalizeDocumentFolder(workspace, folder) { - const raw = normalizeRepoPath(String(folder ?? "").trim()).replace(/\/+$/, ""); + const raw = withoutTrailingSlashes( + normalizeRepoPath(String(folder ?? "").trim()) + ); if (!raw || raw === ".") return ""; const resolved = containedPath(workspace.paths.docs, raw); if (!resolved) { diff --git a/packages/workfile/src/modules/docs/validation.ts b/packages/workfile/src/modules/docs/validation.ts index 427b404..5a66077 100644 --- a/packages/workfile/src/modules/docs/validation.ts +++ b/packages/workfile/src/modules/docs/validation.ts @@ -21,10 +21,36 @@ function dayNumber(date) { return Number.isFinite(timestamp) ? timestamp / 86_400_000 : null; } +/** + * The link target is bounded, and that bound is the whole point. + * + * `([^)]+)` scanned to the end of the document on every `](` that had no + * closing paren after it, so a body made of `[](` repeated cost one full scan + * per repetition. Measured on this machine: 16.6ms at 2,000 repetitions, + * 3.3s at 32,000 and **43.6s at 128,000** — quadratic, on a document body, + * which the doctor reads for every document in the workspace. A record body is + * repository text an agent writes, so the input is not hostile in the usual + * sense; it is just text nobody thought to bound. + * + * Both halves are bounded, and the first attempt here bounded only the second + * — which the analyser then reported again, correctly, against a different + * input. `[` repeated is the label's version of the same shape: `[^\]]*` runs + * to the end of the body looking for a `]` that never comes, once per `[`. + * 837ms at 32,000 characters, where the whole scan is 59ms once the label is + * capped too. Fixing one half of a quadratic leaves a quadratic. + * + * Every bound is true of a Markdown link independently of the performance + * argument: neither half spans lines, a label is not a paragraph, and a target + * is not longer than any path a filesystem will hold. The cost is that a link + * past those sizes stops being checked. Nothing local can be that long — POSIX + * caps a path at 4096 and a component at 255 — and the only targets that reach + * it are `data:` URIs, which the scheme test below skips anyway. + */ +const LINK = /\[[^\]\n]{0,512}\]\(([^)\n]{1,1024})\)/g; + function localMarkdownPaths(document) { const paths = []; - const pattern = /\[[^\]]*\]\(([^)]+)\)/g; - for (const match of String(document.body || "").matchAll(pattern)) { + for (const match of String(document.body || "").matchAll(LINK)) { let target = match[1].trim().replace(/^<|>$/g, ""); if ( !target || diff --git a/packages/workfile/test/record-body-safety.test.ts b/packages/workfile/test/record-body-safety.test.ts new file mode 100644 index 0000000..a565ac8 --- /dev/null +++ b/packages/workfile/test/record-body-safety.test.ts @@ -0,0 +1,156 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { diagnoseDocuments, normalizeDocumentFolder } from "../dist/src/modules/docs/index.js"; +import { safeUrl } from "../ui/src/safe-url.ts"; + +/** + * What a record body can do to whatever reads it. + * + * A body is written by whichever agent held the card, and three things read it + * without asking where it came from: the doctor scans it for local links, the + * document API normalises a folder out of it, and the UI renders it. The first + * two were quadratic and the third handed its schemes to React ([[T-0162]]). + * + * The budgets below are wall-clock, which T-0166 and T-0179 are both open + * scars about — so the margin is the assertion, not the number, and the + * ceilings differ because the margins do. Measured on this machine, before and + * after, over 192,000-character bodies: + * + * | scan | fixed | in suite | unfixed | ceiling | headroom | detects | + * |---------------|-------|----------|---------|---------|----------|---------| + * | link, targets | 245ms | 377ms | 9663ms | 2000ms | 5.3× | 4.8× | + * | link, labels | 356ms | 726ms | 33579ms | 2000ms | 2.8× | 16.8× | + * | folder | 3ms | 6ms | 2846ms | 500ms | 83× | 5.7× | + * + * "In suite" is the same measurement under the whole test run rather than + * alone, and it is the column that matters: these roughly double, so the + * headroom a solo run would have claimed is not the headroom there is. + * + * A shared ceiling would have left the folder case 1.4× above the number it + * has to exceed — one fast runner away from passing while quadratic. The + * measured time is reported either way, so a run that passes narrowly says so. + */ +const REPETITIONS = 64_000; +const LINK_CEILING_MS = 2_000; +const FOLDER_CEILING_MS = 500; + +function elapsed(work: () => void): number { + const started = process.hrtime.bigint(); + work(); + return Number(process.hrtime.bigint() - started) / 1e6; +} + +/** + * Both halves, because the first fix only bounded one. + * + * `[](` exercises the target: every `](` opens a target the scan looks for a + * closing paren for, and there is never one. `[` exercises the label, which + * has the identical shape one bracket earlier — and which the first version of + * this fix left unbounded, so the analyser reported it again against the input + * it had actually named. Fixing one half of a quadratic leaves a quadratic. + */ +const SHAPES = [ + ["unclosed targets", "[]("], + ["unclosed labels", "["] +] as const; + +test("a document body of unclosed links does not stall the doctor", async (t) => { + for (const [name, unit] of SHAPES) { + await oneShape(t, name, unit); + } +}); + +async function oneShape(t, name: string, unit: string) { + // Repeated to a common *length*, not a common count. `[` is one character + // and `[](` is three, so counting repetitions fed the label shape a body a + // third the size — and the ceiling that gave it 5× detection on one shape + // gave it 1.8× on the other, which is the thin margin this file's header + // rejects for the folder case. + const body = unit.repeat(Math.round((REPETITIONS * 3) / unit.length)); + const documents = [ + { + id: "DOC-0001", + path: ".project/docs/reference/DOC-0001-x.md", + file: "DOC-0001-x.md", + title: "Adversarial", + kind: "reference", + status: "draft", + created: "2026-08-05", + updated: "2026-08-05", + body + } + ]; + // Enough workspace for the rules that run before the link scan. The + // filesystem stays out of the measurement on its own: not one of these + // links closes, so the scan matches nothing and there is no path to check. + const workspace = { + root: "/w", + config: { + docs: { + kinds: ["reference"], + statuses: ["draft"], + reviewIntervalDays: 90 + } + } + }; + const started = process.hrtime.bigint(); + await diagnoseDocuments({ documents, workspace }); + const ms = Number(process.hrtime.bigint() - started) / 1e6; + t.diagnostic(`link scan, ${name}: ${ms.toFixed(0)}ms over ${body.length} chars`); + assert.ok(ms < LINK_CEILING_MS, `${name} took ${ms.toFixed(0)}ms`); +} + +test("a folder of separators does not stall document creation", (t) => { + // The value arrives from `doc create --folder` and from the HTTP body, so + // it is neither validated nor bounded before it reaches the normalizer. + const folder = "/".repeat(REPETITIONS) + "x"; + const ms = elapsed(() => { + try { + normalizeDocumentFolder( + { paths: { docs: "/w/.project/docs" }, config: { docs: { managedPath: ".project/docs" } } }, + folder + ); + } catch { + // Refusing it is the correct answer. How long it takes to refuse + // is what this measures. + } + }); + t.diagnostic(`folder normalisation: ${ms.toFixed(0)}ms for ${REPETITIONS} separators`); + assert.ok(ms < FOLDER_CEILING_MS, `normalising took ${ms.toFixed(0)}ms`); +}); + +/** + * The UI renders record bodies, so a link target in one reaches an `href`. + * + * React 19 blocks `javascript:` on its own, which is why this had never bitten + * — and is exactly why the rule is ours now: a defence that lives in a + * dependency's minor version is one you find out about by losing it. + */ +test("a record body cannot put a scheme of its choosing into a link", () => { + for (const refused of [ + "javascript:alert(1)", + "JavaScript:alert(1)", + " javascript:alert(1)", + "data:text/html,", + "vbscript:msgbox(1)", + "file:///etc/passwd", + "//evil.example.com/steal" + ]) { + assert.equal(safeUrl(refused), null, `${refused} must not reach an href`); + } + + // The common case is a relative path, which carries no scheme at all, and + // refusing those would break every link the protocol writes. + for (const allowed of [ + "packages/workfile/docs/SPEC.md", + "./sibling.md#heading", + "../up.md", + "#anchor", + "https://example.com/x", + "http://example.com/x", + "mailto:someone@example.com" + ]) { + assert.equal(safeUrl(allowed), allowed.trim(), `${allowed} has to render`); + } +}); diff --git a/packages/workfile/ui/src/components/Markdown.tsx b/packages/workfile/ui/src/components/Markdown.tsx index 5483393..cedc651 100644 --- a/packages/workfile/ui/src/components/Markdown.tsx +++ b/packages/workfile/ui/src/components/Markdown.tsx @@ -1,6 +1,7 @@ import { Fragment, memo, type ReactNode } from "react"; import { cn } from "@/lib/utils"; +import { safeUrl } from "@/safe-url"; /** * The record-body renderer, extracted from the old Drawer so every surface @@ -107,8 +108,8 @@ function InlineMarkdown({ // Repository-relative or served assets only: an arbitrary // remote URL in a record body would leak a page view to // whoever wrote it. - const src = image[2].trim(); - if (!/^(https?:)?\/\//.test(src)) { + const src = safeUrl(image[2]); + if (src && !/^(https?:)?\/\//.test(src)) { return ( {link[1]}; return ( {link[1]} ); + } return {part}; })} diff --git a/packages/workfile/ui/src/safe-url.ts b/packages/workfile/ui/src/safe-url.ts new file mode 100644 index 0000000..7cb0fb1 --- /dev/null +++ b/packages/workfile/ui/src/safe-url.ts @@ -0,0 +1,52 @@ +/** + * The schemes a record body may put in an `href` or a `src`. + * + * React 19 replaces a `javascript:` URL with a throwing stub, so this is not + * the only thing standing between a card body and script execution. It is the + * only one that belongs to us. The UI renders record bodies, and a record body + * is written by whichever agent held the card — the same threat model that + * makes the CI workflow call this package "unauthenticated read and write + * access to a repository". A defence that lives in a dependency's minor + * version is a defence you find out about by losing it. + * + * Its own module rather than a helper inside `Markdown.tsx` so that a test can + * reach it: the UI has a typecheck and no test runner, and the node suite + * already imports `ui/src/timeline.ts` directly. + * + * **CodeQL flags the `href` this guards and does not recognise this as the + * sanitizer.** `js/xss-through-dom`, dismissed on 2026-08-05 as a false + * positive, and it will come back at a new line number whenever the file is + * edited around it — an alert number belongs to an instance. The flow it + * reports is real and worth knowing, because it is the reason this file + * exists; read out of the analysis SARIF rather than guessed at: + * + * BodyEditor event.target.value → draft + * → Inspector savedBody → bodyValue + * → Markdown source → href + * + * The typing is yours; the body is then saved to the repository and rendered + * for whoever opens that record next. Deleting the guard below reopens that + * path, and the analyser will not tell you, because it never saw it close. + */ + +/** Schemes that may appear in rendered record text. */ +const SAFE_SCHEME = /^(?:https?|mailto):/i; + +/** Anything of the shape `scheme:`, which RFC 3986 bounds to these characters. */ +const HAS_SCHEME = /^[a-z][a-z0-9+.-]*:/i; + +/** + * The URL to render, or `null` when the caller must not render a link at all. + * + * Relative targets are the common case, carry no scheme, and pass untouched. + * `data:` is refused deliberately: browsers block top-level navigation to + * `data:text/html`, but that is again someone else's rule, and no record body + * needs it. + */ +export function safeUrl(raw: string): string | null { + const url = raw.trim(); + // Protocol-relative: no scheme to test, and it still leaves the origin. + if (url.startsWith("//")) return null; + if (HAS_SCHEME.test(url) && !SAFE_SCHEME.test(url)) return null; + return url; +}