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.