Project-SDE-WP-Suite/KNOWN-ISSUES.md

# 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 |
| 3 | Export is not one merged PDF; drawings ride along as a list | Low | 2026-08-20 | Open — decided |

---

## 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`.

**The `project_super_user` role (added 2026-08-05) widens the set of victims whose
session is worth stealing, without raising the ceiling.** Previously only an app
admin's session could create accounts or change permissions; now a super user's can
too, within the projects they administer. The ceiling is unchanged — it was already
`admin` — but the odds of landing on a session that can mint an account go up, and a
super user is likelier than an admin to be reading a WP creator on a live job. It is
one more reason the accidental-breakage case is not the only one that matters.

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.

Note that the second of those got easier to reach without anyone deciding to: a
Project Super User can now issue accounts on their own job without an app admin
involved, so "accounts are issued to subcontractors" can become true by ordinary
delegated use rather than by a policy change. Worth checking the directory
occasionally against who is actually on staff.

### 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/console-util.js` (it moved
out of `html/admin.js` on 2026-08-05 when the User Directory started needing it) —
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.

---

## 3. Export is not one merged PDF; drawings ride along as a list

**Files:** `html/wp-creation-app.js` (the T9.1 export walk), `CR-008`
**Decided:** 2026-08-20, by Nick — "add this to known issues."

### What is wrong

CR-008 asked for the work package "as one document." What shipped (T9.1)
renders every section inline — including images — and lists PDF drawing
attachments with links, rather than merging their pages into a single PDF.

### What it costs

A crew printing the package gets the form and the inline images in one pass,
but linked PDF drawings are separate opens/prints. For field hand-offs that
want literally one file, someone stitches it manually.

### Why it is still open

Real PDF merging needs either a server-side PDF library (a new dependency and
a render pipeline for arbitrary uploaded PDFs) or a client-side one (heavy,
and the creator is deliberately dependency-free). The recommendation made at
T9.1 — inline images + listed PDFs — was accepted as the shipped behaviour.

### What closing it takes

A server-side merge endpoint (e.g. pypdf) that concatenates the rendered
package with each attached PDF, streamed back as one download; plus a size
ceiling consistent with D8's upload limits. One task, one new dependency.