n.siegfried
64a5fd5612
Closes known issue 3. server/smoketest.py predated the login portal and had no
login step at all, so auth_gate refused every route after /api/health and the
documented way to verify a deploy reported a wall of failures against a healthy
stack.
- Signs in first, holding the session in an http.cookiejar on a shared opener.
urlopen() has no cookie support, which is why the session was dropped.
- Credentials from WP_SMOKE_USER / WP_SMOKE_PASSWORD, or --user/--password, so
a password need not land in shell history. Refuses to start without them
rather than running headlong into 401s.
- Checks the signed-in role up front and warns when it cannot archive or delete
a project, instead of failing six checks later for an unexplained reason.
- New exit code 2 for "could not run" (unreachable, or credentials missing or
rejected), kept distinct from 1 "ran and found problems".
- Also asserts the session is accepted on an authenticated route and refused
after sign-out; signs out at the end so a run on a shared host leaves none.
The working smoke test immediately caught a real bug: SQLite ships with foreign
keys disabled and the pragma is per-connection, so every ondelete="CASCADE" was
silently a no-op on dev while working on Postgres. Deleting a project orphaned its
SOPs, work packages and membership rows; deleting a user orphaned theirs. db.py
now sets PRAGMA foreign_keys=ON for SQLite, so dev matches production.
Enforcing them exposed two things that had been getting away with it:
- create_user adds an account and its ProjectMember rows in one flush, and the
ORM takes flush order from relationship() declarations. models.py has none by
design, so it emitted the child INSERT first and the database rejected it.
Fixed with a db.flush() after the account, and documented at the top of
models.py so the next same-flush pair does not rediscover it. The other three
call sites already commit the parent first.
- A write aimed at a since-deleted project used to leave an orphan row; with FKs
enforced it would have been an IntegrityError surfacing as a 500, which the
browser outbox retries forever (it only retires 4xx). require_project_writable
now refuses a vanished project with 409, like the archived case beside it.
Verified: smoke test 27/27 exit 0 against a live server (the cascade assertion now
passes on SQLite, which is what used to fail); credentials missing and credentials
rejected both abort cleanly with exit 2 and no stray PASS lines; a project_user run
warns up front and fails as described. Scope tests 93/93, live HTTP checks 29/29,
static JS checks 33/33. No orphan rows left in the database afterwards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
227 lines
11 KiB
Markdown
227 lines
11 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 |
|
|
| 3 | User Directory and nav drawer have not been run in a browser | Low (verification gap, not a known defect) | 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.
|
|
|
|
---
|
|
|
|
## 3. User Directory and nav drawer have not been run in a browser
|
|
|
|
**Files:** `html/users.html`, `html/users.js`, `html/wp-sidenav.js`,
|
|
`html/wp-sidenav.css`, `html/console.css`
|
|
**Raised:** 2026-08-05, with the Project Super User work.
|
|
|
|
### What is wrong
|
|
|
|
This is a **gap in what has been verified, not a known defect.** The machine the code
|
|
was written on has no JavaScript engine available (no node, deno, or npx), so these
|
|
files have never been executed. What *has* been checked:
|
|
|
|
- The server side they talk to: 93 scope/permission tests and 29 live HTTP tests
|
|
through the real dependency stack, all passing.
|
|
- Static analysis of the browser code: delimiter balance across all scripts (with a
|
|
tokenizer that handles nested template interpolation), every inline handler
|
|
resolving to a defined function, every `getElementById` target existing in its page,
|
|
and no leftover references to the functions that moved out of `admin.js`.
|
|
|
|
What that cannot cover is anything only a browser decides: CSS layout, the drawer's
|
|
open/close transitions and focus trap, event wiring, and how the three role
|
|
renderings actually look with real data.
|
|
|
|
### What it costs
|
|
|
|
Unknown by definition, bounded by blast radius. A syntax or boot error in `users.js`
|
|
means a blank User Directory — obvious the moment anyone opens it, and it takes
|
|
nothing else down: `users.html` is a new page, and the drawer is additive to pages
|
|
that already worked without it. The server-side role and its scope rules are
|
|
independently tested and unaffected either way.
|
|
|
|
The one thing to watch is `console.css`, which was extracted from `admin.html`'s
|
|
inline styles and is now shared. A rule lost in that move would show up as the Admin
|
|
Console losing its dense-table styling — the same symptom the deploy runbook already
|
|
tells you to look for ("one line per user", not three).
|
|
|
|
### Why it is still open
|
|
|
|
It closes with one manual page load, which needs a browser signed in to a real
|
|
deployment — not something to fake from a test harness.
|
|
|
|
### What closing it takes
|
|
|
|
Minutes. Hard-reload first (Ctrl+Shift+R) — the service worker caches the app shell
|
|
and `sw.js` bumped to `wp-suite-shell-v6`, so a normal reload can serve the old file
|
|
list and make a good deploy look broken.
|
|
|
|
1. Open **users.html** as an admin: the table renders, rows are one line tall, and
|
|
the Permissions and Project role dropdowns are populated.
|
|
2. Open it as a **Project Super User**: the blue scope banner names their project(s),
|
|
the create form demands at least one project, and any account also on a job they
|
|
don't administer shows as `read-only` with a reason on hover.
|
|
3. Open it as a **project user**: six columns, no controls, no create form.
|
|
4. Open **field.html** on a phone or a narrow window: the ☰ opens the drawer, Escape
|
|
and the scrim close it, and Admin Console appears only for admins.
|
|
5. Open **admin.html** and confirm the tables still look dense and correct — that is
|
|
the `console.css` extraction check.
|
|
|
|
Delete this entry once those five are done.
|