From 153fe97a314e383642bb7dec6dba14de5171757e Mon Sep 17 00:00:00 2001 From: "n.siegfried" Date: Wed, 5 Aug 2026 13:56:27 -0700 Subject: [PATCH] Record the SOP discipline-name XSS as a known issue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is a real hole and we are shipping without fixing it, so it needs to be written down somewhere that outlives the conversation it came up in. Discipline names are rendered into inline handlers in the WP creator escaped with esc(), which maps ' to '. That is right for text and wrong here: the browser decodes entities in an attribute before the JS parser sees it, so the entity becomes a bare quote and closes the handler's string literal. Escaping for a handler argument has to go backslash, then quote, then HTML — esc() only does the last part. Same bug, same ordering, as the two fixed on 2026-08-05 (jsq() in admin.js, escHandlerArg() in work-package-suite-app.js); this one predates that work and sits in a file it did not touch. Left open rather than fixed because the suite is internal, behind a login, with named employee accounts and no anonymous input path — the likely cost is a discipline named "Owner's Equipment" silently breaking its own pill, not an attack. The entry records the conditions that change that judgement (exposure outside the corporate network, accounts for subcontractors or clients, self-registration), so the rating cannot go quietly stale if the deployment story changes. Neither CSP nor the CSRF gate mitigates it, and both are noted so nobody re-derives that hopefully. Also records the archived-project rough edge from the same day: the server refuses writes with 409, but the two big apps still present Save and Issue buttons, so the failure is safe but late. data-wp-archived is already on the document element for whoever closes it. Each entry says what closing it takes, and entries get deleted in the commit that fixes them — otherwise this file becomes a museum instead of a queue. Co-Authored-By: Claude Opus 5 (1M context) --- KNOWN-ISSUES.md | 148 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) create mode 100644 KNOWN-ISSUES.md diff --git a/KNOWN-ISSUES.md b/KNOWN-ISSUES.md new file mode 100644 index 0000000..aa88274 --- /dev/null +++ b/KNOWN-ISSUES.md @@ -0,0 +1,148 @@ +# Known issues — Work Package Suite + +Defects and limitations we know about and have decided not to fix yet. An entry +here is a commitment to a decision, not a bug tracker: it says what is wrong, what +it costs, why it is still open, and what closing it takes. + +Anything genuinely urgent does not belong here — it belongs in the next deploy. + +Close an entry by deleting it in the same commit that fixes it. + +| # | Issue | Severity | Raised | Status | +|---|-------|----------|--------|--------| +| 1 | XSS via SOP discipline names in the WP creator | Medium (internal), High if externally reachable | 2026-08-05 | Open | +| 2 | Archived projects: the two big apps don't grey out their own controls | Low | 2026-08-05 | Open | + +--- + +## 1. XSS via SOP discipline names in the WP creator + +**Files:** `html/wp-creation-app.js` lines 684, 723, 727, 729 · escaping helper at +line 63 +**Predates:** the 2026-08-05 archive/admin-console work. Not introduced by it. + +### What is wrong + +Discipline names are rendered into inline event handlers escaped with `esc()`, +which maps `'` to `'`. That is correct for text and wrong here. The browser +decodes entities in an attribute value **before** the JavaScript parser sees it, so +`'` becomes a bare `'` inside the handler's string literal and closes it early. + +```js +// html/wp-creation-app.js:684 — esc() is not sufficient for a handler argument +onchange="toggleDiscipline('${esc(d)}',this.checked)" +``` + +Escaping for an inline handler has to happen in this order: **backslash, then +quote** (for the JS string literal), **then HTML** (for the attribute carrying it). +`esc()` only does the last part. + +### How it is reached + +1. `gov_disciplines` (`html/work-package-suite.html:219`) is a free-text field. Its + value is comma-split with no validation at `work-package-suite-app.js:1224`. +2. It is saved into `sops.data` and syncs to the server via `ProjectData.pushSOP`. +3. Every other member of that project pulls it with `pullProject()` and renders it + in the WP creator — so this is **stored** and **cross-user**, and it fires on + page load rather than needing the victim to click anything. + +Any **project_user** on the job can set it while the SOP is a draft (after the SOP +is marked complete it takes project_admin). The victim is anyone who opens the WP +creator for that project, which includes administrators. + +### What it costs + +**The likely cost is a broken screen, not an attack.** A discipline named +`Owner's Equipment` — an ordinary thing to type — produces a syntax error in the +handler, so the discipline pill and its scope-step buttons silently stop +responding. No error message, nothing a field user can diagnose. + +**The security ceiling is project_user → admin.** The session cookie is HttpOnly so +the token cannot be read, but the injected code does not need it: it runs in the +victim's page and can call any API the victim can, including +`POST /api/auth/users/{id}/role`. + +Two controls that look like they would contain this do not: + +- **CSP does not mitigate it.** `nginx-wp-suite.conf:58` serves + `script-src 'self' 'unsafe-inline'`, and `'unsafe-inline'` is what permits inline + event handlers in the first place. +- **The CSRF gate does not mitigate it.** `_csrf_ok` (`server/app.py:67`) only + requires a same-origin `Origin`, and code running inside our own page is + same-origin. + +### Why it is still open + +The suite is internal, behind a login, on the corporate network, with a small set +of named employee accounts and no anonymous input path. Exploiting it means an +employee deliberately attacking colleagues, and the audit log carries their name on +the SOP edit. The accidental-breakage case is far more likely to be met than the +malicious one. + +**Re-rate this as High and fix it immediately if any of these become true:** the +suite is exposed outside the corporate network, accounts are issued to +subcontractors or clients, or self-registration is added. + +### What closing it takes + +Small — roughly half an hour. The helper already exists; it was added to the SOP +builder on 2026-08-05 for the same bug in custom constraint names: + +```js +// html/work-package-suite-app.js:1120 +function escHandlerArg(v){ return escAttr(String(v==null?'':v).replace(/\\/g,'\\\\').replace(/'/g,"\\'")); } +``` + +1. Add the same helper to `html/wp-creation-app.js` alongside `esc()`. +2. Use it at lines 684, 723, 727 and 729 in place of `esc(d)`. +3. Sweep the other inline handlers in that file for the same pattern. The remaining + ones interpolate server-generated ids that `check_id()` already constrains to a + safe charset, or hardcoded enum values, so they are not currently reachable — + converting them anyway keeps the pattern from coming back. +4. Confirm with a discipline named `Owner's Equipment`: the pill must respond to + clicks and the name must display intact. + +The equivalent fix on the admin side is `jsq()` in `html/admin.js` — same ordering, +same reasoning, worth reading before starting. + +--- + +## 2. Archived projects: the two big apps don't grey out their own controls + +**Files:** `html/wp-creation-app.js`, `html/work-package-suite-app.js` +**Raised:** 2026-08-05, with the project-archiving work. + +### What is wrong + +Archiving a project freezes it server-side — every write returns 409 (see +`require_project_writable` in `server/app.py`, and the *Archiving a project* +section of `DEPLOYMENT.md`). The front end tells the user, but does not stop them: +`wp-chrome.js` shows a read-only banner and sets `data-wp-archived="1"` on the +document element, and nothing reads that attribute yet. So on an archived project +the WP creator and the SOP builder still present working Save and Issue buttons. + +### What it costs + +Low, and it fails safe — the server refuses the write, so nothing is corrupted and +no data is lost. The cost is wasted effort and a confusing moment: someone deep- +linked to an archived job can fill in a form and only learn it was refused when the +sync indicator reports the change did not save. + +Reaching an archived project at all takes a deep link or a stale tab, since it is +gone from every picker, switcher and search — which is why this is a rough edge +rather than a defect. + +### Why it is still open + +Gating every control in two large single-page apps is materially bigger than the +archive feature itself, and the server is the real enforcement boundary either way. +The banner plus the sync indicator were judged enough for a first release. + +### What closing it takes + +`data-wp-archived` is already on the document element for exactly this purpose. +Either add `[data-wp-archived]` rules in `wp-chrome.css` that disable and dim the +save/issue controls, or add a boot check in each app that disables them and shows a +read-only notice inline. Decide separately how the embedded creator +(`wp-creation-index.html`) surfaces it, since it runs in an iframe where the shared +app bar — and therefore the banner — is deliberately skipped.