Skip to content

SECURITY.md

Latest commit

 

History

History
699 lines (602 loc) · 40.2 KB

SECURITY.md

File metadata and controls

699 lines (602 loc) · 40.2 KB
Raw

Security Notes

Working notes for this codebase: the checks to run before shipping, and the history of the CSP gap that is now closed — kept because the reasoning is what stops it being reopened, and because the conversion that closed it is still the main source of regressions.

Last reviewed: 2026-10-26 (v1.12.8).

Pre-push sweeps

Run these before any push that touches more than one PHP file, and again against the production container after a deploy. Each takes about two seconds. There is no php binary on the build host, so anything invoking it runs in a container.

1. Parse errors, every page

docker exec gamenight-dev sh -c 'find /var/www/html -name "*.php" | while read f; do php -l "$f" 2>&1 | grep -v "^No syntax errors"; done'

A PHP parse error takes down the whole file, not the branch containing it, so the page returns an empty 500 on every request. This exists because v0.2064 rewrote the style.css cache-buster across all 54 pages that link one, and in timer.php that link sat inside a PHP string where the short-echo tag is not evaluated and its quotes closed the string. The timer was a white page for the entire release. Any pattern-based edit across many files will eventually hit a context where the pattern is wrong.

Related trap: never write the PHP closing tag inside a // comment. It ends PHP mode and reintroduces the same class of fatal.

1a. Parse errors in PHP-GENERATED JavaScript

cd ~/qa-headless && node inline_js_sweep.js

Sweep 1 cannot see this. Most of the JavaScript in this app is emitted from inside .php files — checkin.php alone renders about 3,500 lines of it — and php -l is perfectly happy with PHP that prints broken JS. The failure is total rather than local: one SyntaxError discards the entire <script> block, so every function it declared is undefined and the whole console goes dead. There is no partial degradation and no error on screen; the page simply renders and nothing works.

The sweep fetches each rendered page, extracts every inline <script> (skipping src= tags, which are real files a plain node --check already covers) and parse-checks each one, so it reads exactly what the browser is asked to parse. It walks 22 pages, logged out for the two that are reachable that way. Add a page to its PAGES list whenever a new one starts generating JS — a page that is not listed is not checked, and the sweep will still report all-clear.

It exists because an edit inserted a statement between an if and its else:

if (!PAYOUT_STRUCTURES.length) loadPayoutStructures();
refreshPresetState();          // ← inserted here
else renderPayoutStructureSelect();

php -l passed. The file looked right in a diff. The check-in console was completely non-functional, and the first symptom was an unrelated test failing on SESSION is not defined — the global was never declared because the block containing it never parsed.

Any insertion into generated JS deserves this sweep: the surrounding lines you are editing may be PHP-interpolated, brace-matched across a <?php ?> boundary, or (as above) one half of a control-flow statement that a diff does not show you.

1b. Re-run the browser suites for the pages AFFECTED

If a change touches a shared file — _footer.php, _nav.php, auth.php, db.php, anything under www/*.js — the blast radius is every page that includes it, not the file you edited. Run the full set in ~/qa-headless, not just the suite for the page you were working on.

This is not hypothetical: v0.2077 added pk-dispatch.js to _footer.php and shipped, which made every control in the check-in console fire twice for a whole release. The suite that detects it (checkin_args.js, reporting calls: 2) was not run, because checkin.php had not been edited.

1c. Double-dispatch sweep

cd ~/qa-headless && node double_dispatch_sweep.js

Walks 37 pages and, for every data-act* control and generic behaviour, fires one event and asserts the handler ran exactly once. It catches the v0.2078 class of bug: two dispatchers on a page, a duplicated listener, or a script included twice. It also flags a control whose named function does not resolve, i.e. a dead control.

A control that fires twice is silent on anything idempotent and fatal on anything that toggles, which is why it survived a whole release before a user hit it on a phone. Run this after any change to a shared partial or to pk-dispatch.js.

Note checkin.php legitimately loads the shared script and sets window.PK_DISPATCH_LOCAL; the flag makes the shared one return early, and the sweep reports it as standing down rather than as a fault.

It needs an admin session, and it says so. For its first several releases the sweep ran as a plain user, so every admin page returned 403 and reported zero controls — 296 of 558 controls silently unchecked, including five files that later turned out to be broken. It now probes admin_settings.php first and fails loudly if it is unreachable. Promote the dev test user, run, then put it back:

docker exec gamenight-dev php -r '(new PDO("sqlite:/var/db/app.db"))->exec("UPDATE users SET role=\"admin\" WHERE id=269");'
cd ~/qa-headless && node double_dispatch_sweep.js
docker exec gamenight-dev php -r '(new PDO("sqlite:/var/db/app.db"))->exec("UPDATE users SET role=\"user\" WHERE id=269");'

The generic behaviours need watching differently from data-act*: data-confirm names no function, so there is nothing to count. The sweep stubs pkConfirmForm() and asserts exactly one call per data-confirm form and button. Without that it passed a release in which every delete on the home feed raised two stacked dialogs.

2. Known-bad escaping patterns

grep -rnE "addslashes\(htmlspecialchars\(|on[a-z]+=\"[^\"]*<\?=[[:space:]]*json_encode|value=\"'\+[a-zA-Z_]" www/ --include=*.php \
  | grep -v "escAttr(\|escHtml("

The trailing filter matters: the third pattern also matches correctly-escaped code such as value="'+escAttr(col)+'", and a check that reports clean code as a problem quickly gets ignored. On a clean tree the whole command prints nothing.

2d. JSON written into a script block

grep -rn '<?= json_encode(' www/ --include=*.php

Prints nothing on a clean tree. A value written into <script> as var X = <?= … ?>; goes through js_json() (db.php), never bare json_encode(). The HTML parser reads a script block before JavaScript does: json_encode() escapes /, so </script> cannot appear, but <!-- and <script> can, and that pair moves the tokenizer into its "double escaped" state so the real </script> no longer ends the block (the page's next script tag does), which turns the block into a SyntaxError and discards every function in it. An event title is enough to do it, on every console the title is shown on. js_json() carries JSON_HEX_TAG and friends, substitutes an invalid UTF-8 byte instead of returning false (which printed var X = ;, the same outcome), and prints null as the last resort. Extra flags such as JSON_FORCE_OBJECT are passed through: js_json($v, JSON_FORCE_OBJECT). This is the script-block rule; the attribute rule below is different (an attribute holds data, not JSON).

2b. Invites matched to accounts by name

grep -rnE "LOWER\((u|users)\.username\) = LOWER\((ei|event_invites|i|a|b)\.username\)|LOWER\((ei|event_invites|i|a|b)\.username\) = LOWER\((u|users)\.username\)" www/ --include=*.php \
  | grep -v "ONE-SHOT BACK-FILL"

Prints nothing on a clean tree. The one sanctioned match is the one-shot back-fill in db_init(), which carries that marker as a SQL comment and runs once, under the event_invites_user_id_backfilled setting. An invite's account is event_invites.user_id, set by the host at write time (resolve_invite_user_id(), or the caller's own id); a username is user-chosen, so matching on it at run time lets whoever registers a name that a host typed become that invitee (reminders, the comment thread, the roster row their results attach to, and for a typed-in manager the event's controls). The join was removed from the authority checks in the first hardening and crept back into ten readers and the API's writers by v1.8.4; the v1.8.5 pass cleared them. Any new reader selects ei.user_id or joins users u ON u.id = ei.user_id; any new writer records user_id. The alias list grew in v1.12.3 when the audit found the join alive under i in two more places.

2c. The signed-in account's username used as a query key

grep -rnE -A8 "LOWER\(([a-z_]+\.)?username\) *= *LOWER\((\?|:[a-z_]+)\)" www/ --include=*.php \
  | grep -E "\\\$(current|user|cu|me|sender|recipient|viewer)\['username'\]"

A statement that compares username with a bound value, followed within eight lines by an execute that binds the signed-in (or phone-resolved) account's username. Prints nothing on a clean tree. Three rules, from the 2026-10-24 audit (fixed in v1.12.3):

  1. Which invite is mine? is ei.user_id = <account id>: never the signed-in account's username, the phone-resolved account's username (the SMS and WhatsApp webhooks), or an API caller's user_id turned into a username.
  2. Which account does this invite belong to? is ei.user_id (JOIN users u ON u.id = ei.user_id); NULL means no account.
  3. A queued notification is addressed to pending_notifications.user_id (the caller's account id, or the invite row's own link copied when it was queued), else to the contact details on the invite row. dispatch_queued_notification() never resolves a username to an account.

The bound-parameter form LOWER(username) = LOWER(?) is still legitimate in two places, and the sweep leaves them alone: a host acting on a roster name inside an event they already control (approve, deny, resend, the check-in console's poker_players.display_name writes), where the name identifies a row and not an account; and resolving a typed name to an account at write time (resolve_invite_user_id(), canonical_username(), add_walkin), which is the host's decision. event_notifications_sent.user_identifier is a dedup log keyed by lowercase name on purpose and is not a username comparison, so the sweep does not see it. Before v1.12.3 this sweep printed 33 lines: My Events, the RSVP and sign-up paths, two manager checks, the DM picker and the webhooks all found "my" invite by a name a stranger could register, and the notification sender handed that stranger the reminders and one-click RSVP links meant for the person the host typed.

Anything it prints is a real bug, not a style preference. In order, it flags:

Pattern Why it is wrong
addslashes(htmlspecialchars($v)) htmlspecialchars turns ' into &#039; first, so addslashes finds no quote to escape and is a no-op. The browser decodes the entity back to a quote before compiling an inline handler, so the JS string still breaks.
on…="… <?= json_encode($v) ?> …" json_encode() emits its own " delimiters, which terminate the surrounding attribute. Everything after is parsed as further attributes on the element.
value="'+x+'" in JS-built markup An unescaped value closes the attribute and injects markup into the DOM.

All three shipped to production and were fixed in v0.2070 and v0.2071. The first two also caused a visible functional bug: the admin delete confirmation silently did nothing for any event whose title contained an apostrophe.

Correct forms — and note that "attribute context" is now two different things:

Context Correct form
PHP, attribute holding JS source (onclick="fn(…)") htmlspecialchars(json_encode($v), ENT_QUOTES | ENT_SUBSTITUTE) — historical only, no such attributes remain
PHP, attribute holding data (data-a1, data-confirm, title) htmlspecialchars($v, ENT_QUOTES | ENT_SUBSTITUTE), or a cast such as (int)$id
JS in timer_classic.php, attribute context escAttr($v)
JS, text context escHtml($v)
PHP, value inside a <script> block js_json($v) (sweep 2d)
JS, text passed to pkConfirm() / pkAlert() / pkConfirmForm() the message is rendered as HTML: a name, title or other typed value goes through escHtml() (or the file's own escaper) first; pk-dispatch.js escapes data-confirm text itself

The json_encode rule is a trap now. It was correct while attributes carried JS source: onclick="viewEvent(<?= json_encode($ev) ?>)" needed a JS literal, and the encoder produced one. After the handler conversion, data-a* attributes carry data — pk-dispatch.js reads the attribute and passes the string through, with no parse step. A json_encode()d object therefore arrives at the handler as text.

This is not hypothetical. calendar.php followed the documented rule:

data-act="viewEvent" data-a1="<?= htmlspecialchars(json_encode($ev)) ?>"

viewEvent(ev) read ev.id off a string, so every event chip in the month view navigated to /event.php?id=undefined until v0.2083. Pass the scalars the handler actually needs (data-a1="<?= (int)$ev['id'] ?>"), never an object.

escHtml() escapes <, > and & but not quotes, so it is safe for text nodes and unsafe for attributes. That distinction is the whole reason escAttr() exists.

Always pair ENT_QUOTES with ENT_SUBSTITUTE. On its own, ENT_QUOTES makes htmlspecialchars() return an empty string when the input contains an invalid UTF-8 byte. An event title carrying one produced an empty data-confirm, so the delete confirmation silently did not appear (fixed across 8 files in v0.2082). html_entity_decode() is unaffected and correctly does not use it.

3. Conversion defects in declarative markup

grep -rnE 'data-[a-z0-9-]+="[^"]*""|data-[a-z0-9-]+="[^"]*&lt;\?=|data-[a-z0-9-]+="[^"]*" \+ |data-[a-z0-9-]+="<\?= *htmlspecialchars\(json_encode' www/ --include=*.php

Prints nothing on a clean tree. Every hit is a real defect. Converting an inline handler means retyping an attribute by hand, and the four ways that goes wrong are all invisible to php -l and to a browser — the page renders, the control just misbehaves.

Pattern What it means
data-x="…"" a stray duplicated closing quote; everything after it is parsed as a further attribute
data-x="…&lt;?= a PHP tag that got HTML-escaped during conversion, so the literal <?= … ?> shows in the dialog
data-x="…" + a JS concatenation emitted inside a single-quoted JS string, so ' + i + ' lands in the markup verbatim
data-x="<?= htmlspecialchars(json_encode( an object passed where the dispatcher will deliver a string (see the escaping table above)

Measured, not assumed: against the tree as it shipped before v0.2082 this prints 20 hits across 11 files — the entire markup half of that batch, four of its nine findings. On the fixed tree it prints nothing. It runs in well under a second and needs no container and no browser, so there is no reason to skip it.

It does not catch the other five (a duplicated listener, a wrong query parameter, a missing data-stop check, a JSON-quoted key, a missing ENT_SUBSTITUTE) — those need sweep 1c or a browser. Use both.

Word-boundary warning. If you write an ad-hoc grep for handlers, anchor it. on[a-z]+=" matches action=, position= and direction=, and reported 130 inline handlers on a tree that has zero. Use [[:space:]]on[a-z]+=" and discard comment lines. A check that cries wolf gets ignored, which is worse than not having it.

Outbound calls and inbound webhooks (FinalTable, v0.2130)

The first thing in the tree that both calls another server with a credential and accepts signed requests from it. What holds it:

  • The game key (sso_apps.api_key) is stored with encrypt_value() under APP_SECRET, decrypted only inside finaltable_request() for the Authorization: Bearer header, and never echoed: the admin page selects its columns by name and renders Set / Not set; the activity log records that a key was set, never the key. CURLOPT_FOLLOWLOCATION is off, so a redirect cannot carry the bearer to another host. Every outbound call is a person pressing a button behind csrf_verify() and can_manage_event() (or an admin on the Connected Apps page); there is no queue that could replay one.
  • The webhook secret is per game: 48 hex characters from random_bytes(), sent to FinalTable once at creation and kept encrypted in finaltable_games.webhook_secret. It never reaches a browser (finaltable_public_game() strips it).
  • The receiver (finaltable_webhook.php) fails closed the way sms_webhook.php does, with an explicit $verified flag: POST only, body capped at 64 KB and read once, a millisecond timestamp within 300 s (a seconds comparison copied from the SMS receiver rejects everything — that was the first bug), a sha256=<64 hex> signature, then hash_equals('sha256=' . hash_hmac('sha256', $ts . '.' . $body, $secret), $sig) with the raw header string and the body exactly as sent. The game row is looked up from the unverified body's game.id before the check because the secret is per game; nothing else is read until the signature passes. An unknown id is a 404 on purpose (a deleted event's stragglers; FinalTable gives up after a day and logs it there). Dedupe is an INSERT into finaltable_deliveries with a UNIQUE (game_id, delivery_id): the constraint is the lock, a repeat answers 200 and does nothing. It never includes auth.php, so no session, CSRF or CSP is in play.
  • What the panel's state read gives back is filtered by the caller, not left to the page to hide: the join code and link only for someone on the guest list (a manager, or an approved invite matching their account id - the same test event.php makes), the FinalTable roster and the seat-by-seat entrants only for a manager, a bare seated count for everyone else, and the finishing order never - it stays in last_status for the server-rendered line and Manage Game. event_visibility_sql() alone is too loose here: on a public or league event it is true for every signed-in member, so a read authorised only by it would have walked past the organiser's hide guest list setting.
  • What it writes is bounded: finaltable_games columns and the poker_players rows of the one session tied to that game, matched by user_id (never by the name in the payload) and only for an id on the roster this side sent, which is kept as JSON on finaltable_games.roster when the table is set up. Without that test a signed delivery naming a stranger would have this side make them a player and give them an approved event_invites row — which is an authorisation grant, since event_visibility_sql() reads it. Ids that were never sent are refused and logged once per delivery, however many a standings list names. A game set up before the column existed has roster NULL, which means "do not check", so a table already running keeps recording; that fallback can go a release later. A player it does have to create gets an invite row beside it so sync_invitees() cannot soft-remove it.
  • The watch link is not gated, and that is the decision. The state read withholds the roster and the entrants from a non-manager, but the rail link goes to every viewer of the event and FinalTable's rail shows the field by name. Gating the button would not make the night private — every invitee already has the link by email, and the link is pass-on-able by design on both sides — so hide guest list binds the guest list on the event page and nothing else. DOCS.md says so where a host will read it.
  • The new data-act controls (ftSetup, ftControl, ftCancel, ftRemove, ftCopy) live in finaltable.js and render only when the event carries online_app_id; give a dev event one before the double-dispatch sweep, or it walks event.php without them.

CLOSED: CSP no longer allows inline script (v0.2079)

auth.php now sets:

script-src       'self' 'nonce-<per-request>'
script-src-elem  'self' 'nonce-<per-request>'
script-src-attr  'none'

An injected <script> is refused because it cannot carry the nonce, and an injected on* attribute is refused because attribute handlers are disallowed outright. CSP is now a real second line of defence against XSS, which it was not for any of the findings in the v0.2070 review.

The nonce is carried on script-src as well as script-src-elem deliberately: browsers without CSP3 granularity fall back to script-src, and a nonce there makes 'unsafe-inline' ignored, so they get the same guarantee rather than none.

How it used to read, and why that was no protection at all

'self' stops an attacker loading script from another origin. 'unsafe-inline' additionally permits script written inside the HTML, covering both <script> blocks and onclick="…" attributes. Because the browser cannot distinguish the app's own inline script from an injected one, the CSP originally provided no XSS mitigation at all — every XSS issue found in the v0.2070 review would have executed without friction.

v0.2072 closed half of it (injected <script> elements are refused by the nonce) and v0.2079 closed the rest (script-src-attr 'none', once the tree reached zero inline handlers). The paragraphs below describe the intermediate states because the reasoning is what keeps the fix from being undone, not because any of it is still live.

Why it took six releases to remove

'unsafe-inline' was load-bearing until the very end. The tree today:

Count
Inline on* handler attributes 0 (was 401; converted across v0.2073-v0.2078)
Inline <script> blocks 64, all nonced as of v0.2072
External .js files 7 (pk-dispatch.js added in v0.2077)

At the start checkin.php alone had 165 inline handlers and timer.php 157, so dropping the directive on day one would have broken the application completely. That is why the path below was split into three independently shippable steps rather than one flag change.

Verify the count with the anchored pattern, not a bare one:

grep -rEh '[[:space:]]on[a-z]+="' www/*.php www/*.js | grep -vE '^\s*(//|\*)'

Why a nonce is only half a fix

A nonce authorizes a <script> block. There is no syntax to nonce an on* attribute — inline event handlers cannot be authorized by nonce at all. They have to become addEventListener with their data carried in data- attributes.

Note the trap: adding a nonce to script-src makes a browser ignore 'unsafe-inline' entirely, which blocks event handler attributes too. Done naively that breaks every button on the site. The split is what makes step 1 shippable on its own:

The intermediate split that made step 1 shippable on its own — superseded by the policy at the top of this section, kept to explain the sequencing:

Directive (v0.2072-v0.2078, no longer in use) Purpose
script-src 'self' 'unsafe-inline' fallback for browsers without CSP3 granularity (Firefox); behaviour there is unchanged
script-src-elem 'self' 'nonce-…' script elements must carry the nonce, so an injected <script> is refused
script-src-attr 'unsafe-inline' event handler attributes stay allowed until they are converted

Most of this codebase's handlers are built inside JS strings at runtime, e.g.

h += '<button onclick="toggleBuyin(' + p.id + ')">';

which is exactly the pattern that has to move to event delegation.

Agreed path, each step independently shippable

  1. Add a per-request nonce via the three-directive split above. Done in v0.2072. auth.php mints a per-request CSP_NONCE; csp_nonce() stamps it on all 64 inline blocks across 33 files. External <script src="/..."> needs no nonce, it is covered by 'self'. A standalone page that sends no CSP header of its own must not call csp_nonce(); cast_receiver.php was the only one and has since been deleted as dead code. What this buys: an injected <script> element is now refused by the browser. What it does not: an injected on* attribute still runs, since script-src-attr still allows them. That is what step 2 closes.
  2. Convert handlers file by file. DONE as of v0.2078 — the tree is at zero. The pattern below is kept because any new markup must follow it. Started in v0.2073: _nav.php (7) and _post_card.php (9) are at zero. The established pattern:
    • Tag the control with a data-* attribute carrying whatever the old handler passed as arguments (data-nav="dropdown", data-pc="toggle-comments" data-post="12").
    • Dispatch from a delegated listener on document, not a per-element binding. Delegation is required, not stylistic: posts_chunk.php injects cards by AJAX after load, and per-element listeners would miss them.
    • Prefer an existing external .js file as the destination (nav.js). External scripts are covered by 'self' and need no nonce. Otherwise put the listener in the page's nonced block, next to the functions it calls.
    • For onsubmit="return pkConfirmForm(...)" use the generic data-confirm="…" data-confirm-ok="…" data-confirm-danger="1" handled by one delegated submit listener. pkConfirmForm() calls formEl.submit(), which does not re-fire the submit event, so it cannot recurse.
    • If the destination asset is not cache-busted, bust it in the same change. Moving behaviour into a .js file turns a stale cached copy from an out-of-date file into a broken feature: the new markup emits data-* attributes the old file has never heard of, and the control silently does nothing. This shipped as v0.2073 and was fixed in v0.2074 — nav.js was the only local script tag without a ?v= and the entire nav went dead for anyone holding a cached copy. Every local script tag now carries ?v=<?= APP_VERSION . '.' . filemtime(...) ?>; keep it that way. The same trap hit style.css in v0.2062.
    • Namespace argument attributes per event. One element can carry two handlers (the walk-in box has both input and keydown). A shared data-a1..a4 namespace produces a duplicate attribute, HTML keeps the first, and the second handler silently receives the wrong arguments. Click uses the bare data-aN; every other event prefixes its own (data-keydown-a1). This shipped in v0.2073's pattern and broke Enter-to-add-walk-in plus Enter in cash-in fields; fixed in v0.2075.
    • Test the actual gesture, not just the dispatch. A sweep that reads the same attributes the dispatcher reads will agree with a bug rather than catch it. The collision above survived 315 passing dispatch assertions and was found by a human pressing Enter. Add one real-gesture test per interaction class (type-and-Enter, click, select-change).
    • Use the shared www/pk-dispatch.js for any page that includes _footer.php. It is external, so it needs no nonce, and it carries generic declarative behaviours for the idioms that recur: data-confirm / data-confirm-ok / data-confirm-danger on a form, data-href, data-toggle-class="id:class", data-click-file="inputId", data-select-all-on-focus, data-uppercase. Reach for those before writing yet another near-identical named helper. checkin.php and timer_classic.php carry their own copies from before it existed; leave them. The Tournament Timer (timer.php) has no shared dispatcher at all and reads its own data-tb attributes, which is why it does not claim the shared data-act name.
    • Delegate in the CAPTURE phase. An ancestor that calls stopPropagation() makes a bubble-phase listener on document blind to everything beneath it. timer.php's control tray does exactly that, so the play, sound and level buttons dispatched nothing until the listeners moved to capture. Inline handlers were immune because they run at the target; capture restores that ordering.
    • Count with a complete on[a-z]+= pattern. An enumerated list of event names will miss some: ondragstart/ondragover/ondrop/ondragend were absent from the first sweep, so timer.php was reported as 154 rather than 157 and drag-to-reorder would have been left inline.
    • Capture a before/after baseline of the live DOM. Snapshot every element carrying a handler, convert, snapshot again, and diff on (element, event). That is what proves nothing was dropped; a passing dispatch sweep only proves that what remains works.
    • A change to a shared partial changes every page that includes it, so run the suites for the pages AFFECTED, not the files EDITED. v0.2077 added pk-dispatch.js to _footer.php and shipped; checkin.php includes that footer and already had its own dispatcher, so every control in the check-in console fired twice for a full release. Toggles are where this is fatal and silent: tapping a player card ran classList.toggle('open') twice so it opened and shut again, and ticking buy-in set it on then off. The dispatch sweep did detect it (calls: 2 on 289 of 294 controls) — it simply was not run, because checkin.php had not been edited. The user found it on a phone before the suite did. If a change touches _footer.php, _nav.php, auth.php, db.php or any file under www/*.js, re-run everything.
    • One dispatcher per page. checkin.php and timer.php carry their own and set window.PK_DISPATCH_LOCAL = 1; pk-dispatch.js returns early when it sees that flag. Do not add a second dispatcher to a page without the same guard.
    • When testing a conversion, make sure the fixture actually exists. A check that skips because no post card rendered will report a pass for work it never verified.
    • Verify with the old asset forced back in (Playwright page.route() can serve the previous file) to prove the cache-buster is what fixes it, rather than assuming.
  3. Drop script-src-attr 'unsafe-inline'. DONE in v0.2079. Verified before switching: a DOM scan across 36 pages found zero on* attributes in rendered markup (including JS-built) and zero unnonced inline <script> blocks. After switching, an injected on* handler and an injected nonce-less <script> are both confirmed blocked, while every nonced inline block still runs.

Anything added from here must follow the conversion pattern below, because an inline handler will now simply not fire. pk-dispatch.js logs [pk-dispatch] no handler named X when a name does not resolve, and the sweeps in the pre-push section catch the rest.

The sweeps above are no longer a compensating control for an open gap — the gap is shut. What they now protect is the conversion: 401 hand-edited attributes produced nine defects that a parse check could not see, four of them still live weeks later. They are the difference between catching that in two seconds and catching it in a security review months later.

What the v0.2070 / v0.2071 review changed

Recorded because roughly half the fixes are structural (future code inherits them) and half are conventions someone has to follow.

Structural, no discipline required:

  • sanitize_html() now walks the subtree of an element it is about to unwrap, so every caller gets correct sanitization.
  • Event authority resolves through event_invites.user_id rather than a username string match. Since v1.8.5 so does everything else that turns an invite into an account: roster imports, notification recipients, SMS matching and the API (sweep 2b keeps it that way). Since v1.12.3 the reader's side too: a signed-in account finds its own invite by id, and a queued notification carries the account it is addressed to (sweep 2c).
  • Account self-service (v1.12.4): a signed-in session is not proof enough to re-home an account. confirm_current_password() in auth.php gates every change of username, email or phone, two-factor enrolment, recovery-code regeneration, disabling two-factor, a new password and deleting the account (five wrong guesses lock those prompts for 15 minutes). A new email waits in users.pending_email until the link sent to it is used (email_verifications.new_email, verify_email.php), and the old address is told. A changed phone is unverified and send_mfa_sms_code() refuses to text it; the sign-in page asks for a recovery code instead. users.session_epoch is bumped by every password change or reset (invalidate_other_sessions()) and current_user() signs out any session carrying the old value. session.use_strict_mode is on, and complete_login() discards the anonymous session's CSRF token so the signed-in session mints its own.
  • League membership follows approval (v1.12.5): on a league event an approved invite that has an account is a member, a pending or denied one is not. walkin.php enrols only an approved walk-in, and every path that approves an invite later calls league_enrol_if_approved() in db.php.
  • Pending league and contact rows are claimed by a PROVEN address only (v1.12.5): claim_verified_contact() in auth.php runs from the verification pages, the Surge code in Settings and admin imports, never from register_user(); league_invite.php links a signed-in account only through a verified email or phone. The unlinked register_dl.php copy of the sign-up page, which had no attempt limit, is gone.
  • One local-redirect rule (v1.12.5): safe_local_path() in auth.php accepts a single leading slash followed by printable ASCII only, so //host, /\host and /<tab>/host (folded by browsers into //host) are all refused; referer_local_path() applies it to "go back" redirects. login, register, comment, the league post and API-key endpoints and the calendar all use it.
  • Read-write gaps closed in v1.12.6: an event comment needs the event visible to the commenter (event_visibility_sql(), as event.php); the event editor's contact picker gives a plain league member names only and never a pending contact's typed details; adding a registered account to a league creates an invitation addressed to that account (league_invite_account() in db.php, league_members.invited_user_id, accepted only by that account on league_invite.php, the row carrying only what the inviter typed) instead of enrolling it; per-player league history is members-only; a timer-layout save or delete unlinks only the caller's own uploads (u<uid>_ prefix) and never a file a chip set still names (pk_lo_gc_images()); push endpoints must be a known push service or an admin-listed host (wp_endpoint_allowed()), ten per account, 3 s connect / 5 s total, https only, the test button rate-limited; a payout structure loads only through the list's own visibility filter (pk_payout_structure_for()).
  • The Low list, first half (v1.12.7): every MFA text is counted (send_mfa_sms_code() refuses past five in fifteen minutes, activity_log mfa_sms_send) and every verification code per account (send_verification_code(), five an hour); a phone number already registered gets the DECOY code session (verify_user_id = -1, same answers as a real signup) so the verify page is not a lookup; every SMS event command needs a verified number (sms_admin.php); the API log stores the canonical path (api_canonical_path()), so a direct-file call or a trailing slash counts against the same per-endpoint limit, and the create limits match exactly; API answers are private (a cached GET) or no-store; contacts' save responses no longer say whether a typed address matches an account; TV pairing codes are minted at most thirty an hour per address, always unique (tv_pair_new() throws rather than duplicate), misses on tv_send.php are counted per address, and the "sent" banner names only a game the caller runs; connect.php refuses a missing or malformed state; the FinalTable webhook logs one bad-signature line per game and address per five minutes; a missing or unwritable app secret stops the site (db.php) instead of minting a per-request one; every CSV export goes through csv_cell(); privacy.php and terms.php carry the headers; the service worker opens only a path on this site; script-block JSON goes through js_json() (sweep 2d); the dispatchers (pk-dispatch.js, checkin.php, timer_classic.php) call only an identifier-named, page-defined function, never a browser built-in; data-confirm text is escaped before the dialog; the timer's custom-element maps have no prototype and the seven Object.prototype names are refused on save.
  • The Low list, second half (v1.12.8): get_client_ip() returns REMOTE_ADDR and nothing else (mod_remoteip in the Dockerfile already rewrites it from X-Real-IP for connections from the Docker bridge, so a header from anywhere else now counts for nothing); the SMS log keeps outbound bodies through redact_secrets() (six-digit codes and link tokens blanked) and the mail retry queue stores bodies through encrypt_value(); the cron token travels in the X-Cron-Token header (?token= kept for an external cron) and reaches cron_drain.php through the environment, never an argument; cron_token, waha_api_key and vapid_private_pem are in ENCRYPTED_SETTINGS (one-shot settings_encrypted_v2 re-saved the stored values); sanitize_html() allows no class or id and only the properties sanitize_style() lists (no position, z-index, display, opacity, transform or url()); Jodit is pinned by JODIT_VERSION in the entrypoint with a .version marker so a bump reaches an existing install; Apache's twelve workers and the 384 MB container limit are sized together.
  • Message and comment pictures (v1.13.0): messages_dl.php send and comment.php add apply upload.php's checks (real bytes, getimagesize(), 8 MB, the daily cap) before anything touches disk, name the file u<uid>_<32 hex>.<ext> under uploads/dm/ or uploads/comments/ (unguessable, owner in the name, served static with execution off), and dm_unlink_attachments() (_dm.php) / comment_unlink_attachments() (db.php) remove the files on every path that deletes the rows (a comment, post, event or account deleted, the last member leaving a group); cron.php's sweep treats dm_messages.attachment and comments.attachment as references. A league's known-as name (league_members.known_as, v1.13.0) is set only by an owner or manager through update_member, capped at 60 characters, and shown only on the league's own pages (never the public page).
  • username_format_error() in auth.php is the single username rule, applied by registration, the API and the profile editor.
  • pk_theme_sanitize_props() constrains stored theme JSON on every write path.

Conventions that must be followed by new code:

  • Attribute escaping (see the table above).
  • escAttr() for JS-built attributes in the timer.
  • Scope filters and copy-on-edit on any new theme or preset action in timer_dl.php, matching what get_theme / get_presets / delete_theme already do.
  • State-changing endpoints are POST plus csrf_verify(); a GET may render a confirmation but must not write. See rsvp.php, verify_email.php, poll.php and join_league.php.
  • form-action 'self' stays. The one exception is connect.php, whose confirm POST answers with a redirect to a connected app, and Chromium checks form-action against that redirect target (Firefox does not, which is how a single-browser test misses it). It calls csp_allow_form_action_to() with the origin of the app's registered sso_apps.base_url, never with anything from the request, and the helper validates the origin to a bare scheme://host[:port] before splicing it into the header. Do not widen it to * or call it from any other page.
  • A redirect target that comes from a request parameter is validated origin-exact (sso_validate_return() in _sso.php: scheme, host and port equal, path under the registered path, no fragment or userinfo) and a failure renders an error page rather than redirecting anywhere. A prefix compare on the string is not a check: http://a.com is a prefix of http://a.com.evil.com/.