n.siegfried
4ace2afb1c
User accounts lived in the Admin Console, which is admins-only. Project admins
need to create the accounts on their own jobs without an app admin on the phone,
so accounts move to a new User Directory page and a new role carries the right.
server/auth.py, server/app.py
New permissions role `project_super_user`, between admin and project_admin:
everything a project admin may do, plus user administration SCOPED to the
projects they hold the role on. Four limits make it safe to hand out, all
enforced server-side:
* Scope comes from projects, not the job title. It resolves per membership
(managed_project_ids), so an ordinary account can hold it on one job via
ProjectMember.role, and a super user demoted on one job administers
nobody there. No projects, no authority.
* Account-level changes (password, disable, rename, permissions, delete)
require EXCLUSIVE scope: refused when the target is also on a project the
caller does not administer, because those changes are global. The
directory renders such rows read-only with the reason.
* No admin or super-user targets, and neither role can be granted by a
super user -- that is the line that stops it becoming app-wide control.
* PUT .../projects rebuilds only the caller's own slice; memberships on
projects they do not administer are left untouched. A payload that simply
omits them must not cut someone off a job the caller cannot see.
Creating requires naming at least one of your own projects: an account with
none would be one the creator instantly cannot manage.
/api/auth/users is now scoped rather than admin-only, and carries a per-row
`manageable` verdict plus the reason. Non-managers get a contact card only --
a project user has no business reading colleagues' login history. New
/api/auth/user-scope tells the page what it may offer. Administrative
password resets are now audited; they were the one account change that left
no trace. Settings, feature flags and the auto-add rule stay admin-only.
While here: one definition of "is a user manager", derived from the managed
set. An account-role-only version disagreed with the scoped one and locked
per-project super users out of routes they were entitled to.
html/users.html, html/users.js
The directory: three renderings from one page -- admin (everything), super
user (controls per row, read-only where scope is shared), everyone else (a
read-only directory of the people on their own projects).
html/console.css, html/console-util.js
Extracted from admin.html/admin.js so both console pages share them. A
divergent jsq() is an XSS and a divergent role list offers permissions the
server refuses, so neither may exist twice.
html/wp-sidenav.{js,css}
Global nav drawer, role-gated, carrying ?project= across links. Mounted on
the field view (which had no way to anywhere) plus both console pages.
No migration: users.role is already String(20) and the new value fits.
Verified: 93 scope/gate tests, 29 live HTTP tests through the real dependency
stack, 33 static JS checks. Not verified in a browser -- no JS engine on this
machine -- so users.html and field.html want one manual load.
server/smoketest.py still fails with 401s. Pre-existing: it has no login code,
so auth_gate refuses it. Confirmed unchanged by stashing this work.
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.
|