n.siegfried
7f831bf1ca
Deletes known issue 3. users.js, wp-sidenav.js and the extracted console.css had
never been executed, because there is no node/deno on the machine they were written
on. Edge is, so the pass was driven through the DevTools Protocol with a hand-rolled
stdlib WebSocket client, signing in by minting a session with the app's own
auth.create_token() rather than scripting the login form.
70 checks, twice, all passing — covering the five steps that entry listed:
1. users.html as an admin: 9 columns, one-line rows, no sideways scroll, all four
grantable roles, the project-access dialog opening and closing on Escape, and
your own permissions cell locked to a tag while your job function stays editable.
2. As a Project Super User: banner naming the project, only in-scope accounts
listed, out-of-scope rows read-only with the reason on hover, and exactly the
two roles they may grant.
3. As an ordinary project user: 6 columns, no create form, zero controls, emails
still reachable as mailto links.
4. field.html: drawer opens, closes on Escape and on the scrim, aria-expanded and
aria-current correct, focus moves inside, 44px tap targets, Admin Console hidden
from non-admins, and ?project= carried onto project-scoped links only.
5. admin.html: console.css loaded, --ctl resolving, cards and headings and sticky
dense tables intact after the extraction, user administration gone and replaced
by a link, and the admins-only gate still holding for a non-admin.
Every page boots with no JavaScript errors, which was the actual unknown.
Two things the pass surfaced, neither a defect: a 404 on /api/sops/latest is the
API's designed answer for a project with no SOP ("No SOP found") and the browser
logs every 4xx, so the fixture now seeds one; and role pills only appear where a row
is rendered read-only, since an editable row shows a dropdown instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
164 lines
7.8 KiB
Markdown
164 lines
7.8 KiB
Markdown
# 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`.
|
|
|
|
**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.
|