Direct manipulation existed but was awkward, and one part of it was outright
broken. A drag was recognised on ANY pointer movement, so the pixel or two of
travel in an ordinary click counted as a drag and suppressed click-to-edit —
the most common interaction on the calendar would have felt broken. A press now
has to travel a few pixels before it becomes a drag.
At 28px per hour a fifteen-minute block was seven pixels tall. Legible, but not
something a pointer can reliably hit, and its resize grip would have covered the
whole block. Rows are 44px, which makes the smallest block an 11px target while
still fitting a full day on a laptop screen; a test pins both halves of that
trade so neither can be tuned away silently. That height had been written as a
bare 28 in five places in the view that all had to agree with the module — it is
now one constant.
The rest is feedback. A block shows a grab cursor, dims while it is being moved
so it is clear what is travelling, and its grip is taller with a visible edge.
While dragging, the grid switches to a grabbing cursor and suppresses touch
scrolling, so the gesture works on a touchscreen instead of panning the page.
Pointer capture is released and the chrome reset on every exit path, including
a cancelled drag.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
The calendar rendered schedules but could not be used to change them. Creating
or moving anything meant opening a dialog and typing times, which is the wrong
instrument on a week grid: the grid already shows exactly where a thing goes, so
the grid should be where it is put. My previous change made the grid easier to
READ — all screens at once, a colour and a name per target — and left the
interaction untouched, which was only half of what was asked for.
Three gestures now share one pointer loop. Dragging empty space draws a slot and
opens the dialog prefilled with the time drawn, so the gesture supplies the
times and the dialog supplies only what it alone knows. Dragging a block moves
it. Dragging its bottom grip resizes the end. A live ghost shows the range as a
readable time while dragging, and nothing is committed until release, so an
accidental nudge costs nothing. Right-click acts on what is under the pointer:
new here, or edit, duplicate and delete on a block.
Dragging a repeating schedule sideways is refused. A one-off's day IS its date,
but a repeating one's day comes from its rule, so moving an instance across
columns would rewrite the recurrence for every other occurrence — a different
operation, and not one a mouse gesture should perform silently. Changing a
repeating schedule's TIME does still edit the whole series, since a series has
one time of day, so that is confirmed out loud rather than assumed.
The arithmetic is a separate module of pure functions, because it is the part
that fails quietly: a block that ends before it starts, a move near midnight
truncated instead of slid back, or a stamp built with toISOString() putting
anyone west of Greenwich on the previous day. Tests pin each of those. That last
one was already present in the create path and is fixed here too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
socket.io does not retry every disconnect. On 'io server disconnect' it stands
down deliberately and waits to be told to reconnect. The player assumed the
opposite in two places: the disconnect handler stopped the watchdog because
"socket.io owns the reconnect once it KNOWS it's down", and verifyLivenessSoon
skipped a present-but-disconnected socket for the same stated reason.
So when the server closed a socket — a handler throwing, a deploy, an eviction
— nothing was left watching and the player stayed down until someone reloaded
the page. That is what it does on a wall: nothing, indefinitely, with no error
on screen. It happened to a live panel whose heartbeat hit a constraint error;
the server dropped the socket and the display sat dark until reloaded by hand.
A supervisor now backs up every disconnect the client did not itself initiate.
It re-establishes only a socket that is genuinely not connected, and only after
a grace longer than socket.io's maximum backoff, so the reconnection socket.io
does own is never raced. Our own teardown is excluded, since connect() closes
the previous socket before opening the next and supervising that would fight
the attempt already in flight. A resume now hands a stranded socket to the
supervisor rather than assuming someone else has it.
The decisions are pure functions alongside the existing watchdogShouldReconnect,
so they are testable without a browser, and a test asserts the grace still
exceeds the configured backoff ceiling if either is ever retuned.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
A display panel has no keyboard, no pointer and usually no way to clear site
data, but the URL it loads is configurable from whatever manages it. Loading
the player with ?reset=<token> now discards this install's identity so the
panel returns as a new device with a fresh pairing code — the recovery path
when a panel is holding an identity that belongs to a different screen, and the
ordinary path when redeploying a panel to another site.
It applies once per token, which is the whole design. A configured URL is
permanent; nobody goes back and removes the parameter. A reset that fired on
every load would drop the pairing on every reboot and present as a screen that
cannot hold its pairing at all — which reads as an intermittent server fault
rather than the URL doing exactly what it was told. The applied token is
remembered, so ?reset=1 left in place forever resets exactly once; any other
value resets again.
The server URL is deliberately kept, since clearing it would strand a panel
that cannot be typed into, and the cached playlist and layout are dropped so
the new device does not come up showing the previous screen's content.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
The web player derived its fingerprint entirely from hardware traits: user
agent, screen geometry, colour depth, timezone, core count, platform and a
canvas raster. Every one of those describes a model rather than a unit, so two
identical panels produced the same value and the server, which matches on that
value globally, treated them as one device. Two UniFi Pro Displays at different
sites both produced web-m73u8w-5f; the second could not be brought online, and
the row ended up shared, each display evicting the other every thirty seconds.
The identity a player presents is now hardware plus a random per-install salt
kept in localStorage, so two identical panels differ from their first
connection. This is what the Tizen player has always done; the web player is
brought in line with it rather than given a new scheme.
The hardware value is still sent, but only as a hint, and only to move a caller
that has ALREADY authenticated with a device id and token onto its own row —
which is how an existing player carries its identity across this change. A
caller without credentials never resolves through it, however few rows it
appears to match: one row recorded does not mean one display exists, and that
distinction is the whole bug. Such a caller is provisioned a new device, which
costs one pairing code and cannot be wrong.
Older clients are unaffected. They send no hardware value, so they take the
exact-match path exactly as before, and both keep working: the APK's
fingerprint already includes ANDROID_ID and the Tizen player's is already a
stored random id, so neither ever shared an identity between units.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
Three players died with "Cannot set properties of null (setting 'textContent')"
and it could not be traced. The message names no file, and every candidate line
in the current player was ruled out by inspection: the unguarded writes all
build their element with createElement, every getElementById target exists in
the markup, and the script runs after the markup. That points at an older
cached build still served by the service worker, which is exactly the case
where reading current source proves nothing.
The ErrorEvent already carried filename, lineno and colno. They were being
discarded. Keeping them makes the next occurrence name its own line.
Composed to fit the 200 characters the server stores, so the location is not
truncated away: message plus one location, basename only since the origin is
already known from the device. A promise rejection has no filename, so it falls
back to the first stack frame. A cross-origin script, which reports a bare
"Script error." with nothing else, says so rather than emitting :0:0 as if that
were an answer.
A resource load failure still is not a crash; a test guards that, since this
touched the handler that decides it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
A sampling window that recorded nothing leaves the histogram empty, and an
empty IntervalHistogram reports its mean as NaN. Its percentiles return a floor
instead, which is why only the mean was affected and why this went unnoticed.
NaN then survives every arithmetic step in the sampler without complaint and
becomes visible only at the edge, where JSON.stringify renders it as null. So
/api/status served "mean_ms": null while nothing raised an error anywhere, and
any consumer of that gauge read null instead of a number.
Non-finite readings now report 0, which is the honest value: no samples means
no measured delay. Applied to every field so a later change to the histogram
source cannot reintroduce this one field at a time.
Found by CI rather than locally, because an idle window is far likelier on a
loaded runner with several test servers in flight. The failure was real; the
new tests establish the NaN premise and the null serialisation directly rather
than relying on that timing to reproduce.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
The auth limiters are app.use middleware that return 429 before the handler
that writes activity_log, so a rejection left no trace anywhere — the limit
suppressed the record of itself. Four production IPs sit at exactly ten logins
a minute and there was no way to tell whether that is one attacker or an office
whose staff share an egress address, which is the difference between the
limiter working and the limiter locking out customers.
The rejection count does not answer that. The number of distinct accounts per
IP does: one account hammered is the limiter doing its job, several accounts
each denied a few times is a shared egress. Both are now recorded, and a
platform-admin-only endpoint reads the tally back.
Identifiers are salted-hashed with a per-process salt and only ever counted, so
this cannot accumulate into a roster of a customer's addresses. Memory is
bounded per key and overall, and says when a count was capped rather than
silently undercounting.
Behaviour is unchanged: same status, same body, and the recording is wrapped so
telemetry can never break the limiter. A test asserts ten through then 429 with
the identical response shape, since a diagnostic that alters what it measures
is worse than none.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
A player that reconnects after its row was deleted sends the id it still has
cached. device_fingerprints.device_id has a foreign key to devices(id), so
writing that id back fails the constraint. The throw was caught, which is why
this looked harmless, but the catch abandons the whole fingerprint block:
last_seen is not updated, the reinstall link is not made, and the settings
restore never runs. That restore exists specifically for the post-delete
re-pair, so the failure landed exactly where the feature was meant to help and
a re-paired panel came back with its orientation, name and playlist reset.
Production shows 37 of these, timestamped identically to the "sending unpaired"
log lines — the same event seen from the other side.
The incoming id is preferred, then whatever is already stored, and only an id
that still resolves is written; otherwise NULL, which the column allows and
which ON DELETE SET NULL already leaves behind. The INSERT path a few lines
below had this guard; the UPDATE was missed, and it is the one that fires.
Tests cover the deleted-id reconnect, that last_seen still advances, and that
live ids are unaffected. One asserts the raw unguarded statement really does
raise FOREIGN KEY constraint failed, and another asserts the guard is present
in the handler itself, since the others exercise a mirror of that statement.
Also ignores *.sqlite / *.sqlite3, which the existing *.db rules missed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
A display panel usually has no keyboard and no pointer, so a recovery path that
waits for input is not a recovery path. When the server stopped recognising a
device, the player revealed the server-URL form — typing that cannot happen on a
screen-only panel — and hid the pairing section, which was the one thing that
would have rescued it. The screen then sat on "Device was removed from server"
until someone physically reloaded it, even though the player was still connected
to the right server and could have asked for a new code itself.
Both handlers now drop the stale credentials and reconnect on a short countdown.
Reconnecting re-registers with no device_id, so the server issues a fresh pairing
code and the existing registered handler puts it on screen. config.serverUrl is
known-good by construction — we are talking to that server at the moment we are
rejected — so there is nothing for a human to re-enter.
The URL field stays editable throughout, and typing cancels the countdown, so
someone who does have a remote and wants to repoint the player is not yanked
mid-edit. The countdown is the same helper the first-boot path already used,
lifted out and shared rather than duplicated; its input listener is bound once
at setup instead of per countdown, which would have stacked a listener each time.
The Android player already behaved this way (ProvisioningActivity repair mode);
this brings the web player in line.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
A display going offline is one event, but the alert loop re-evaluated every
still-offline device on each 60s tick, so the 2-hour dedup window re-qualified
the same outage over and over. One closed browser tab produced six "your
display is offline" mails overnight, and would have kept going to the 24h cap.
Repeat suppression now keys on devices.offline_alert_heartbeat: the heartbeat
value an alert was already sent for. A device can only come back by sending a
heartbeat, so a later outage always carries a later value and the marker
invalidates itself on recovery — no cleanup, no state to reset. Keeping it on
the row also fixes a second source of duplicates: the in-memory window used to
empty on restart and re-alert the whole offline fleet.
The window stays, doing the job it is actually suited to — bounding how often a
flapping device can alert — and is checked before the marker is written, so a
rate-limited alert is deferred rather than marked and dropped.
The backfill runs once, via schema_migrations rather than the migrations array:
statements there re-run every boot, and an IS NULL backfill would then swallow
the first alert of any outage beginning after the last restart. It marks
currently-offline devices as already-alerted so upgrading does not itself mail
about outages the owner has already been told about six times.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
The week view could only answer "what plays on THIS screen". With one screen
at a time an empty grid is ambiguous — nothing scheduled, or the schedule
points at a different screen? That ambiguity is what a user actually hit.
Adds an "All screens" scope alongside the per-screen one. Every block now
names its target, with a stable per-target colour and a legend, so a full
grid stays readable.
The scope for all=1 comes from the request's resolved tenancy and is filtered
on nothing else, so the tenant boundary rests entirely on that resolution.
Tests pin both halves: an ordinary tenant gains nothing by naming another
workspace in the query string, and the platform-admin act-as path still
resolves the workspace it asks for — the two are easy to mistake for each
other, so they are asserted separately.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL
Creation and playback disagreed about which clock a schedule's hours are on.
The player evaluated blocks in the device's zone — an operator override, else
whatever the player's OS reported. Creation defaulted to a bare 'UTC', because
the dialog never asked for a zone and the server filled the silence with one.
So hours typed as "09:00 to 17:00" were stored as UTC and evaluated somewhere
else. For anyone outside UTC the schedule was correct and appeared to do
nothing, opening hours later than intended, with nothing on screen to explain
why. A user in Asia/Tokyo hit exactly this and reported it as "I added
something and it didn't appear".
Both sides now resolve through lib/device-timezone, so they cannot drift: an
explicit device override wins, then the OS-reported zone, then null. A legacy
'UTC' override counts as unset, since that was the old default rather than a
deliberate choice and a genuine UTC deployment is indistinguishable from an
unconfigured one.
A new schedule inherits its target's zone — the device's, or for a group its
leader's, falling back to the oldest member that reports one. A zone named
explicitly by the caller still wins; this only fills the silence. A target that
has never reported one still lands on UTC, which is the previous behaviour made
explicit rather than assumed.
The dialog now states which clock the hours are on, and says so differently when
that clock is not the operator's own. Stating it is the other half of the fix:
the server can pick the right zone, but the user still has to be able to see it.
Tests pin both directions and, most importantly, that creation and playback
resolve identically from the same device row.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The kiosk page interpolates style.fontFamily and style.background into a <style>
block, escaped with escapeHtml. That is the wrong tool twice over: it escapes
& < > " ' but not { } ;, and inside a raw-text <style> element the entities it does
produce are never decoded, so it neither contains the value nor renders it correctly.
A value could therefore close the declaration, close the rule, and append its own —
putting an attacker-chosen rule on every panel showing the page. There is no XSS,
since </style> stays unreachable, but a url() in an injected rule is an outbound
request from every display, which is a beacon and a cross-site tracking channel.
Both values are now checked structurally rather than against a value allowlist,
because background is a free-text field: linear-gradient(), rgb() and url() are all
legitimate and keep working. Only characters that could terminate the declaration or
open a new rule are refused, along with comment syntax (which can swallow the
declarations that follow) and control characters. font-family needs no parentheses,
so it gets a tighter allowlist.
Tests cover both directions — injection refused and falling back to the default, and
ordinary gradients, colours and font stacks passing through untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The update check is deliberately unauthenticated — every client version has to be
able to ask, including old ones that never learned to send a token — and it keys the
rate breaker on the caller-supplied device_id. Keying on IP is not available either:
the fleet SNATs behind one address, so per-IP would collapse a whole site into a
single bucket.
The result was that the bucket belonged to whoever cited the id rather than to the
device that owns it. A handful of requests naming a panel's UUID left that panel in
rate-backoff, un-updatable for up to half an hour at a time and renewable
indefinitely, while every other device stayed healthy.
Rather than adding auth (which would strand old clients) the state is now
self-healing: when a device registers on the /device socket with a valid device_token
its bucket is cleared. Noise is still possible, but it now lasts until the panel's
next genuine reconnect instead of as long as someone keeps poking.
This is not an escape hatch from the breaker's real job. A device stuck in an update
loop is re-registering legitimately, and clearing its rate state on each genuine
reconnect is what a healthy device looks like; the loop protection that matters is
the download guard. The version-keyed bucket that covers old clients sending only
?version= is a separate namespace and is deliberately not reachable this way.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Players replay a cached playlist, so the id reported on play_start can outlive the
row it names. play_logs.content_id carries a foreign key to content(id), and the id
went straight into the INSERT — so deleting a piece of content made every subsequent
play of it throw, and the whole event was discarded by a catch that logged no
identifiers. On production this fired roughly 360 times in six hours and wrote zero
rows in 24h: Reports was recording nothing at all, for everyone.
Widgets had a quieter version of the same bug. play_logs.widget_id exists and was
never written, so a widget play could not be attributed even when it did insert, and
play_end matched on content_id alone and so could never close a widget's open row.
The reported id is now looked up before use and written to whichever column it
belongs to. An id matching neither degrades to null references rather than losing the
event — content_name still records what played. A play event for a device that does
not exist is still refused; that foreign key is a real invariant, not an obstacle.
play_end matches on either column, and breaks ties on id: started_at has second
granularity, so two plays inside one second tie on it and the wrong row could be
closed. The new tests caught exactly that as flakiness before it was pinned.
The catch now logs the event, device, content and zone. Without them this was
undiagnosable in production.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sharp decodes uploaded files directly (lib/content-ingest.js, routes/content.js
both call sharp(file.path) on whatever a user uploaded), so its bundled libvips is
part of the request path rather than a build-time detail. Moves 0.33.5 -> 0.35.3,
libvips 8.15 -> 8.18.
Validated against the calls this codebase actually makes, because it is a major
bump: metadata() still reports EXIF orientation (1/3/6/8 all round-trip, which is
what lib/media-orientation.js exifSwapsWH and the rotation-aware dimensions depend
on), a bare .rotate() still auto-orients, and resize().jpeg().toFile() is unchanged.
png/webp/jpeg/gif/avif all still encode and decode, and malformed input still throws
rather than crashing.
The new libpng is stricter, which surfaced a latent problem in the AUTH-01 test: its
1x1 PNG literal had a corrupt IDAT chunk whose stored CRC did not match its data. The
old decoder accepted it; the new one refuses with "vipspng: libpng read error", so no
thumbnail was written and the content-gate assertions failed with a 404 that reads
like an auth regression. Replaced with a PNG whose every chunk CRC verifies. The
stricter decode is the correct behaviour and is kept.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A device row carries two fields that are not ordinary data: device_token, the
credential the player proves with on the /device socket, and settings_pin, which
unlocks the player's on-device settings menu and so hands physical control of the
panel to anyone holding it.
device_token was already stripped everywhere. settings_pin was not — it went out on
both the collection and the detail endpoint. The dashboard does show it, but on one
screen only: the device detail page, which fetches a single device. The collection
endpoint had no consumer for it and was returning the PIN for every device in the
workspace on every load.
The detail endpoint keeps it, so that page is unchanged. The list no longer sends it.
Same data, much smaller blast radius, no feature lost.
Tests pin the split in both directions — absent from the list, present on the detail,
and the socket credential absent from both (asserted on the whole serialized payload,
not just the top-level key, so a nested echo would fail too).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two problems on a panel showing one fullscreen widget, both visible as flashing.
The player re-navigated the WebView every duration_sec. PlaylistController.next()
requests a playlist refresh between plays and playCurrentItem() re-issues the item
unconditionally, so a one-item playlist reloaded the same URL forever. The existing
dedupe guard only covers the playlist-update path, so it logged "not restarting"
AFTER the reload had already happened. On an interactive widget that also discarded
whatever the viewer had typed.
showWidget() is now idempotent: same URL with the widget already on screen returns
without re-navigating, and the cached URL is cleared at every media-type transition
so switching away and back still reloads. The refresh itself is untouched — schedule
re-evaluation and dayparting still run on the timer, and widgets keep refreshing
their own data client-side (directory-search polls its board every 30s and preserves
the current query). The web player already behaved this way via reevaluateHeldWidget;
this brings the Android player to parity.
Separately, the directory-search keyboard was laid out in fixed pixels for a
1920-wide viewport. A panel's CSS viewport is its resolution over its density, so a
1080p screen at 240dpi presents 1280x720 — where four rows of 56px keys took ~37% of
the height instead of ~24%, and the lone max-width:700px breakpoint never fired to
correct it. Key metrics are now clamped against vh. The clamp maxima are the previous
fixed values and both vh terms exceed them at 1080 tall, so a 1080 viewport renders
pixel-identically; shorter viewports scale down. The breakpoint no longer re-pins .key,
which would have undone the clamp.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Until now the only ways back into an account were an admin setting your password for you
or shell access to run scripts/reset-admin.js. A self-hosted operator who forgot their
password had no path at all, and the admin-reset route explicitly refuses to reset a
platform admin's password — so a single-admin instance was unrecoverable without a shell.
The per-account login lockout added recently makes that sharper: a user who forgets their
password will hit the lockout and see the same generic error, with no way out.
Two unauthenticated endpoints (they must be — the user cannot log in):
POST /api/auth/forgot-password { email } -> always the same 200
POST /api/auth/reset-password { token, password } -> 200 / 400
The properties that matter, each covered by a test:
- NO ENUMERATION. The request endpoint answers identically — same status, same body —
for a real address, an unknown one, an SSO identity with no local password, and a
malformed string. The frontend shows the same confirmation even on a network error,
so the client cannot leak what the server refused to.
- NO MFA BYPASS. Completing a reset does NOT issue a session; the user signs in
afterwards, so a TOTP-enabled account still clears its second factor. Returning a token
here would turn "read one email" into a full session without the second factor.
- SINGLE USE, SHORT LIVED. 32 random bytes, stored only as a SHA-256 hash (same
discipline as email verification, recovery codes and API tokens), 1h TTL, and the
redeeming UPDATE is conditioned on the hash still being present so concurrent
redemptions cannot both win.
- LOCAL ACCOUNTS ONLY. SSO identities have no local password; no token is minted.
- IT ACTUALLY UNBLOCKS YOU. A completed reset clears the per-account login lockout and
must_change_password, otherwise someone who locked themselves out would reset and still
be locked out.
Rate limited: 5/min on the request (it sends mail to a caller-supplied address), 10/min
on the redeem. If no email transport is configured the response is unchanged — no oracle —
but the server logs loudly, because the user will otherwise wait for mail that cannot
arrive and the generic response cannot tell them.
Frontend: a "Forgot your password?" link on the sign-in card, a request card, and a
new-password card. app.js had to learn #/reset-password explicitly — the auth guard
rewrites any unauthenticated hash to #/login, which would have discarded the one-time
token in the emailed link and made it silently do nothing.
Migration adds users.password_reset_hash / password_reset_expires: additive, nullable,
idempotent; a code-only rollback leaves two dead columns.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A screen that was still connected and still displaying its pairing code could not be
paired. Reloading the player produced the same code, and the on-screen instruction
("restart the display to get a new code") could not help.
devices.created_at is written once, at first registration, and the row is never recreated:
a player persists its device_id and its pairing code in local storage and re-registers
with them forever. Expiry was measured from created_at, so 15 minutes after first boot the
row became permanently unclaimable while the device kept heartbeating — and a restart
reused the stored identity and reproduced the same code, so there was no way out.
Observed in production: an unclaimed web player, still online and heartbeating, whose row
was created 4 days 20 hours earlier and had been unpairable for all but its first 15
minutes. Prod is carrying several such rows; alpha has some 13 days old.
Key expiry on last_heartbeat instead, falling back to created_at for a row that has never
checked in. That answers the question the operator actually has — is this screen still
there showing me this code? — while keeping the property the expiry exists for: a device
that has genuinely gone away still expires.
Trade-off, taken deliberately: a code stays claimable while its screen is connected rather
than for a fixed 15 minutes. That is what the product implies, since the code is on the
screen the whole time, and guessing is bounded by lib/pair-lockout (5 failures per IP per
15 min) and the 5/min route limit rather than by this TTL.
SERVER-ONLY. The player's device:registered handler reads only device_id and device_token
and has no way to display a server-issued code, so reissuing one would have left fielded
players showing a stale code — strictly worse. This fix needs no player update and
un-strands every already-affected device in the field on deploy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
scripts/reset-admin.js mints a JWT carrying `recovery: true`, and middleware/auth.js
accepted that claim on its own with no database involvement. Three consequences:
- NOT REVOCABLE. The only way to invalidate an outstanding recovery token was to rotate
JWT_SECRET, which logs out every user on the instance.
- NOT ENUMERABLE. Nobody could answer "is a recovery token outstanding right now?"
- NOT AUDITED. The synthetic id ('recovery-<nonce>') is not a users row, so every
activity_log insert for it failed the user_id foreign key and was swallowed by a catch —
a break-glass session left no trace at all.
A `recovery_grants` row per minted token turns all three around: DELETE revokes, SELECT
enumerates, expires_at bounds, and used_at + source_ip record when and from where it was
first exercised. The migration is additive and idempotent, so re-running is a no-op and a
code-only rollback just leaves an unused table.
The grant is session-scoped, NOT single-use-per-request. Recovery means many requests —
load the dashboard, list users, reset a password — so consuming the grant on the first
would make break-glass unusable, a worse outcome than the narrow replay window it closes.
Revocation and expiry are the controls; used_at is the audit stamp.
Also fixed, because it is the mechanism that hid this: logActivity now rewrites a
'recovery-*' id to a NULL user_id with the identity in `details`, so break-glass actions
are actually recorded instead of failing the FK; and a dropped audit row now logs a loud
[AUDIT-DROP] line naming the action and increments a counter, rather than vanishing into
console.error.
The token is written to a 0600 file instead of stdout — under systemd or Docker, printing
it meant journald captured a live admin credential well past its lifetime. Added --list
and --revoke-all.
In-flight recovery tokens minted before this change stop working; they live one hour and
were unrevocable, which is the problem being fixed. Minting already required a working DB,
so redeeming against one is not a new dependency.
test/session-token-resolution.test.js now mints a real grant for its recovery token, so
its assertions keep testing that break-glass is refused on those surfaces for lack of a
users row — not for the unrelated new reason that the token is invalid.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The on-device settings PIN (devices.settings_pin, minted at pairing) and the pairing code
assigned to imported devices both came from
`Math.floor(100000 + Math.random() * 900000)`.
Math.random is not a CSPRNG. V8 implements it as xorshift128+, whose internal state is
recoverable from a handful of consecutive outputs, and every call in a process draws from
that one shared stream. Both values are also observable by ordinary users — settings_pin
is returned in device API responses today — so a user who collects a few outputs could
predict the values minted around them, including for other tenants.
lib/numeric-code.sixDigitCode() uses crypto.randomInt, which is CSPRNG-backed and
rejection-samples so the distribution stays uniform. Range is 100000..999999 inclusive,
identical to the old expression, so codes are still exactly six digits with no leading
zero — the on-device keypad and pairing UI are unchanged.
Deliberately NOT converted, because neither gates access: the image-generation seed in
lib/image-gen.js, and the anti-burn-in pixel jitter inside generated widget HTML.
Also unchanged: the settings_pin backfill in db/database.js, which uses SQLite's random()
— that is ChaCha20 seeded from OS entropy, not a weak PRNG.
This is the generator half of the finding only. The separate half — that settings_pin is
returned to every workspace member, including read-only roles — is a response-shape change
and waits on the consumer enumeration.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GET /api/devices/:id/screenshot returns a live picture of what a screen is showing, but
it was still authorized pre-tenancy: `device.user_id !== user.id`, with a role bypass
listing 'admin'/'superadmin'. Three consequences, all now covered by tests:
- `device.user_id &&` SHORT-CIRCUITED. A device with no user_id — never paired, or its
owner deleted — skipped the ownership test entirely, so any authenticated account on the
instance could read it. An unpaired panel displays its pairing code on screen, so that
image is also a route to claiming the device (AUTH-10, out of scope here but connected).
- 'platform_admin' was absent from the bypass list. #14 renamed 'superadmin' to
'platform_admin', so an actual platform admin fell through to the ownership test and was
denied unless they happened to own the row.
- Workspace members other than the owner were denied a device they administer through
every other endpoint.
Now uses accessContext() against the device's workspace — the same helper routes/devices.js
uses — which covers direct membership, org-level access and platform staff in one call. A
device with no workspace is denied outright rather than defaulting open.
Deliberately unchanged: the ?token= query-parameter mechanism on this route, which is a
separate finding with its own blast radius.
No response shape change: still 200 / 401 / 403 / 404 with the same bodies.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The only throttle on POST /api/auth/login was the per-IP limiter in server.js. That
bounds one noisy source and nothing else: it does not bound a distributed attempt, and
it is only as accurate as a deployment's proxy configuration. Nothing counted failures
against the account actually being attacked, and nothing cleared such a count on success
because no such count existed.
lib/login-lockout.js mirrors lib/totp-lockout.js and lib/pair-lockout.js so there is one
lockout idiom here rather than three. 10 failed passwords lock an account for 15 minutes.
Keyed on user.id, never on the submitted email: the email is attacker-supplied and
unbounded, so keying on it would let anyone grow the Map without limit — the same class
of bug fixed elsewhere in this campaign. A user id only exists for a real account, so the
key space is bounded by the user table and needs no eviction sweep, exactly like
totp-lockout.
A locked account returns the SAME 401 and body as a wrong password. A distinct 429 would
tell an attacker "this account exists and is under attack", turning login into an
account-existence oracle; the test asserts the locked response is byte-identical to both
the wrong-password and unknown-account responses. The trade is that a locked-out
legitimate user sees the generic message, so the trip is recorded in activity_log
(auth:login_locked) for the operator instead.
The counter is cleared as soon as the password verifies — before the TOTP and
email-verification branches, which return early and never reach issueSession, so a reset
placed there would never fire for those accounts. SSO paths do not share this code and
are unaffected.
Frontend needs no change: login.js renders any non-ok body's `error` string verbatim, and
the body is unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The diag widget runs in a null-origin sandboxed iframe, so it cannot carry a session and
its telemetry POST must stay unauthenticated. But the handler stored into a plain Map
keyed on a value taken from the request body, with no cap, no TTL and no eviction — an
unauthenticated caller could add entries until the process died. On this product a dead
server is a fleet-wide reconnect, so a bound here is a fleet-safety control.
Two changes:
- lib/bounded-snapshot-store.js: a "latest snapshot per key" store with a global entry cap
and a TTL, evicting least-recently-WRITTEN. The cap is GLOBAL rather than per-IP on
purpose — signage sites egress through one NAT address, so a per-IP limit punishes a
whole venue for one noisy panel and does nothing about a distributed writer. Same
reasoning the OTA download guard already documents ("NEVER per-IP (SNAT)"). A live panel
rewrites its key every 2.5s, so only entries the dashboard already treats as stale
(>15s) are ever eligible for eviction.
- The POST now answers 204 instead of res.json({ok:true}). The reporting widget ignores
the response (fetch(...).catch()), and services/activity.js activityLogger wraps
res.json — so this also stops an anonymous caller from writing one activity_log row, and
running two synchronous statements, per report.
Read contract unchanged: a live key returns its object, an unknown OR expired key returns
null — the shape frontend/js/views/device-detail.js already handles ("no report yet"), and
it treats anything older than 15s as stale regardless, so the 60s TTL is 4x looser than
what the UI honours. No client change; no rate limiter added.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
getClientIp() decides the value every per-IP control keys on — the auth/pairing rate
limiters, lib/pair-lockout, and activity_log.ip_address — so a caller must never be able
to choose it. It believed CF-Connecting-IP whenever the immediate peer was in the
`trust proxy` list, which includes loopback/linklocal/uniquelocal.
Those entries are correct for X-Forwarded-For: a proxy APPENDS to that header and Express
walks the chain right-to-left, so a client-supplied value cannot become the resolved
address. CF-Connecting-IP has no chain — a local reverse proxy passes through whatever
single value the client sent — so treating a loopback peer as evidence the request came
through Cloudflare means trusting the client.
Gate it on the published Cloudflare ranges alone. This is also the portable behaviour:
most self-hosted installs are not behind Cloudflare, and for them the header is now
simply ignored, with attribution falling back to req.ip under whatever `trust proxy` the
operator configured. Installs that do front with Cloudflare are unaffected — their peer
really is a CF edge.
Documented the distinction at config/cloudflareIps.js so the two lists are not conflated
again. No response shape or DB change; no client impact.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Uploaded files are served from the SAME ORIGIN as the dashboard, so how a browser
interprets them is a security boundary. Two things decided that interpretation, and
both were caller-controlled: the stored extension came from
`path.extname(originalname)`, and the only type check read `file.mimetype` — a request
header. A caller could therefore choose to have their bytes served as an active
document from the app origin.
Two independent invariants now hold the boundary:
1. INGEST — lib/upload-sniff.js sniffs magic bytes after multer writes a neutral
`.part` file (diskStorage names the file before any bytes exist, so the sniff cannot
happen there), maps the result through a hardcoded mime->extension allowlist, renames
accordingly, and stores the sniffed mime. Unsupported bytes are refused with a 400.
2. SERVING — upload responses carry `Content-Security-Policy: sandbox`, so if a response
is ever treated as a document it lands in an opaque origin with scripts disabled.
Anything outside the inline-safe extension set is additionally forced to download.
This holds regardless of how a file reached disk, so a future gap in (1) is contained
rather than exploitable.
Applied at every instance of the pattern, not just the first: lib/content-ingest.js,
the /replace route, the four content-serving paths across server.js and routes/content.js
(the latter pair currently shadowed by mount order, which is not a guarantee), and the
ZIP-import path in routes/status.js, which took its extension from the archive entry.
SVG stays accepted and stays inline: white-label logos are SVG, and octet-stream +
nosniff makes <img> fail. Scripts in an SVG never run in an image context, and the
sandbox CSP covers the one case where they would — a direct navigation. SVG is also no
longer handed to sharp, which removes the librsvg path where the open libvips CVEs live.
Existing rows are untouched — no migration. The four upload fixtures in agency.test.js
uploaded `Buffer.from('x')` declared as image/png; that is the exact "declared type is a
lie" case this closes, so the fixtures now use real PNG bytes. No assertion changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
optionalAuth was exported but never mounted on any route (verified by grep across
server, frontend, scripts and tests: only its own definition, its export, and one
stale comment referenced it). It also carried a second, slightly different copy of
the token-resolution logic - its own user column list, and no forced-password-change
check - which is exactly the drift the preceding commit consolidates away.
Removing it rather than porting it to resolveSessionUser: a "set req.user if a token
happens to be present" middleware is a few lines on top of the shared resolver if a
route ever needs one, and an unused export is a standing invitation to mount it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six places verified a session JWT inline instead of going through requireAuth,
each repeating a slightly different subset of its checks. Introduce
resolveSessionUser() in middleware/auth.js as the single definition of "this
token is a usable session, and here is whose it is", and route all of them
through it: the three /api/status token routes, the screenshot route, the
content-reference gate, and the /dashboard socket handshake. requireAuth is now
a thin wrapper over the same helper, so the two cannot drift.
Also:
- Give the pre-TOTP token a distinct audience so it is redeemable only through
verifyMfaPendingToken (POST /api/auth/totp/verify). verifyToken refuses any
token carrying an audience, so a token minted for one purpose cannot be
redeemed on another path.
- The dashboard socket handshake now takes userId/userRole from the live users
row rather than from the token claim, so role changes take effect on the next
connection instead of riding the token's remaining lifetime.
- Add test/session-token-resolution.test.js covering all six surfaces,
including the socket handshake.
Every call site keeps the status code and error body it returned before.
Net query cost: the content-reference gate and the socket handshake each gain
one users-by-id lookup (the same one requireAuth already does per request); the
other four are unchanged or replace an equivalent lookup.
In-flight pre-TOTP tokens are invalidated by the audience change; they live 5
minutes, so the window is a re-login at worst.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The webpage-widget preview note claimed: "the site blocks embedding in a browser
— it will still display on the device screen." The second half is false. The
widget renders the URL in an <iframe> (renderWebpage), and the device player
loads that page in a Chromium WebView, so a site sending X-Frame-Options /
CSP frame-ancestors (Amazon, Google, most large sites/banks) is refused on the
device exactly as in the browser preview. The note set the wrong expectation —
a customer (and we) chased CORS and "should work on device" when the live
device screen was blank too.
Reword to tell the truth in all 6 languages (en/es/fr/de/it/pt), both the
frontend i18n key (widget.webpage_blocked_note) and the player's
preview_webpage_blocked string: if the preview is blank the site blocks
embedding and won't display on the device either — try a page that allows it.
Copy-only; no behaviour change. This is not an Amazon-side fix (embedding refusal
is the site's choice) — just accurate messaging.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getUserPlan()'s auto-downgrade was guarded on `subscription_status !== 'active'`,
but that column DEFAULTs to 'active' and is only ever changed by Stripe webhook
events. For trial users who never touch Stripe — the entire population it's meant
to catch — the condition was always false, so the downgrade never ran and every
signup kept Pro free forever.
Re-key the guard on the real signals:
- trial expired (!trial_active), AND
- stripe_subscription_id IS NULL (never paid), AND
- plan_id === trial_plan (still on the plan the trial granted), AND
- plan_name !== 'free'
The plan_id === trial_plan clause is load-bearing: it protects comped / hand-
granted plans (e.g. a manually-set enterprise plan, where plan_id !== trial_plan)
from being silently downgraded. Grandfathered accounts (trial_started IS NULL)
never enter the block at all, so the ~home cohort is untouched. Added a comment
documenting the subscription_status-default trap so it isn't reintroduced.
Enforcement stays forward-only/lazy — the downgrade happens in the resolver on a
user's next request; no mass update here.
Downstream (deviceSocket.checkDeviceAccess, traced, unchanged): a genuinely-
expired free-tier trial now resolves to free and its device-limit block correctly
caps it to 1 device; grandfathered home (2 devices) and paid users are not
blocked. NOTE: the separate "Trial Expired" screen branch there is a pre-existing
dead condition (it needs trial_started set AND plan_name='free' at once, but the
downgrade clears trial_started) — left as-is per scope; flagged for follow-up.
Tests (new trial-expiry.test.js — there was none, which is how this shipped):
lapsed trial downgrades; comped enterprise (plan_id!=trial_plan) not downgraded;
grandfathered home (trial_started NULL) not downgraded; paid user not downgraded;
in-window trial not downgraded; plus a regression pinning that subscription_status
='active' no longer shields a lapsed trial. Suite 563/563.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add allow_promotion_codes: true to the checkout.sessions.create call in
POST /checkout. This is what renders the "Add promotion code" field on Stripe's
hosted checkout page; for API-created sessions there is no Dashboard equivalent
(that toggle only exists for Payment Links, which we don't use), so a comment
warns against removing it as "redundant". The billingPortal branch is untouched
— portal sessions handle discounts separately.
Testing: no Stripe-SDK test/mock existed (the billing-*.test.js files cover the
#146 usage-metering path, not Stripe). Added stripe-checkout.test.js using the
repo's in-process router-mount convention with a minimal `stripe` stub injected
via require.cache, asserting the checkout payload carries
allow_promotion_codes:true (and still builds a subscription session for the
requested price). Full suite 557/557.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
