`server/node_modules` was tracked as a SYMLINK to its own absolute path:
120000 blob ... server/node_modules -> /home/owner/Downloads/remote_display/server/node_modules
It came in with the #283 merge. Any attempt to resolve it is an ELOOP, so the
payload build died at the staging step with
cp: cannot stat 'server/node_modules': Too many levels of symbolic links
and a fresh checkout gets a server/ whose dependencies cannot resolve at all.
.gitignore only had `node_modules/`, and a trailing slash matches DIRECTORIES —
which is exactly how a symlink of that name slipped past it. Both forms are listed
now, so the same mistake cannot be committed again.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014kfhrUPit5MCqxeTQyqr56
A fleet gets one package, and exactly one box per site should host the server.
Defaulting to on would mean every player that ever received this package
started listening on 8181, and the mistake would stay invisible until two of
them fought over the same displays.
st-config.json on the storage root, {"server": 1}, switches it on. Absent,
unreadable, unparseable, or anything other than an affirmative value leaves it
off - there is no reading of a broken config file that should end with a
device deciding to host a server. It sits at the root rather than in data/
because that is where an operator drops it over the DWS, and autozip never
writes it, so a re-provision cannot silently flip a site either way. The
package ships st-config.example.json, never st-config.json, for the same
reason.
With the server off, NOTHING listens: roNodeJs is never created, so there is
no 8181 and no 8182. The page is told through its URL rather than discovering
it, because "nothing is answering" would otherwise render as a fault and send
someone looking for a server that was never meant to exist. It now has a
fourth state that says so and offers the one line of JSON that changes it.
Separately: the status listener was bound to every interface, so anything on
the customer's LAN could read the install log, disk usage, the device's own
address and a tail of the server's console - that last one carries whatever
the server printed most recently. Its only consumer is a page on the same
device. Now 127.0.0.1 only, confirmed against /proc/net/tcp rather than by
probing, after a first attempt at verifying it fell back to loopback and
reported that as the LAN result.
Claude-Session: https://claude.ai/code/session_014kfhrUPit5MCqxeTQyqr56
Co-authored-by: Dan Walters <dan.walters@bytetinker.net>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The box is now both server and player, so its screen has to be one or the
other at any moment. Three states, and the transitions are the point:
installing / down / failed diagnostics, so the fault is visible
up, but no account yet diagnostics plus the address to create one
up, and an account exists the player, full screen
A fresh install has nothing to play and nobody to play it for, so it stays on
the configuration screen until someone has signed up. Hiding that address
would leave the device unsetuppable: it has no keyboard.
⚠️ THE PLAYER IS AN IFRAME LAYER, NOT A NAVIGATION. Setting location.href
would replace the document and take the poller with it - and that poller is
the only thing able to notice the server failing later. As a layer, the
diagnostics are one style change away from being back on screen, which is
exactly what should happen when a server that has been playing for weeks
throws at 3am. A test asserts location.href is never assigned, so this cannot
be quietly simplified back.
Whether an account exists is asked by the wrapper, not the page:
/api/auth/config is public, but the page is loaded from file:// - origin
"null" - and the server sets no CORS headers on its own API, while this
process is already talking to it. The answer is three-valued. null means the
probe has not replied yet and is deliberately NOT treated as false: guessing
would flip a fresh box to an empty player and take the sign-up address off
the screen while someone was reading it.
Verified against a real server rather than by inspection - install, sign up,
watch it flip:
BEFORE signup : needsSetup=null -> diagnostics
POST /api/auth/register -> HTTP 201
AFTER signup : needsSetup=false -> player
Claude-Session: https://claude.ai/code/session_014kfhrUPit5MCqxeTQyqr56
Co-authored-by: Dan Walters <dan.walters@bytetinker.net>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
copyFileBytes replaced fs.copyFileSync because copyFileSync does not merely
copy bytes - it fchmods the destination to match the source, and exFAT has no
permission bits, so the pre-migration snapshot failed with EPERM on a player.
The replacement dropped the chmod entirely, which fixed that and introduced a
worse problem everywhere else: the copy landed at the default 0666 & ~umask.
Measured on ext4, a 0600 database file copied to a 0664 snapshot - the whole
database readable by group and other, on every install, not just on a player.
Removing a permission operation to fix a permission error is not a fix.
The mode is now applied as a separate, failure-tolerant step after the bytes
are written. That is the actual difference from copyFileSync: there the chmod
is inseparable from the copy, so a filesystem without modes fails the whole
operation; here the data is already safe and a refusal simply means there were
never permissions to carry across.
Two tests: the source mode survives the copy (fails on the current main), and
a filesystem that refuses fchmod still gets its bytes.
Claude-Session: https://claude.ai/code/session_014kfhrUPit5MCqxeTQyqr56
Co-authored-by: Dan Walters <dan.walters@bytetinker.net>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* Make a BrightSign say what it is running, and what it is plugged into
A panel on a wall could not answer three questions an operator asks first:
which version am I, which page am I running, and which screen is that. All
three had answers already travelling over the socket; nothing was reading them.
VERSION. device_info.app_version was the literal '1.1.0-web' for every web
player, BrightSign included — the same string as PLAYER_VERSION, which already
travels separately as client_version. So the column carried no information at
all: a panel provisioned this morning and one running a year-old host reported
identically. app_version is now the ON-DEVICE host package, the artifact OTA
replaces and the only one here that can be stale, and PLAYER_VERSION is stamped
at serve time from VERSION rather than being a constant nobody bumped for the
whole 1.x line. No '-web' suffix: client_version is only compared for equality
today, but X.Y.Z-web is a semver PRERELEASE that sorts BELOW X.Y.Z, and this
project has been bitten by exactly that before.
The host version arrives asynchronously and can land after the page registers,
so register sends what it has and the heartbeat corrects the record — which also
catches the version changing under a live page, which is what a self-update is.
THE CARD SHOWED FOR NOBODY. The Info tab's version card sat inside the block
gated on android_version && !startsWith('Web/'). A BrightSign registers as
"Web/<ua>", so the panel that most needed a version never displayed one.
THE PAD THAT COULD NOT BE CLICKED. System View was gated on tier === 2. tier is
an Android device-owner concept, NOT NULL DEFAULT 0, written only by the APK —
so a BrightSign or Tizen panel sat at 0 forever and rendered HOME, BACK, POWER,
the D-pad and OK permanently pointer-events:none, for keys those players
genuinely handle. Greying an Android gate over a working control is the "button
that cannot work" the capability system exists to prevent, inverted. Only
Recents (KEYCODE_APP_SWITCH) and Settings are truly Android-only; those are now
the only things hidden.
THE PACKAGE POINTED AT THE WRONG SERVER. autorun.zip carried the committed
default, so a player self-updating from alpha or a self-hosted box was handed a
config pointing at screentinker.com — which surfaces as a pairing bug, miles
from the packaging code that caused it. It is now stamped with the URL it was
fetched from. The bytes therefore vary per origin, so the cache is keyed by
origin and BOTH routes derive it identically: the manifest checksum and the
served bytes must come from one buffer or every player downloads, fails
verification and retries forever.
EDID. getEdidIdentity() answers seven questions and cannot answer any others —
manufacturer, EDID version, physical size, gamma and the mode lists exist only
in the raw block, which getEdid() returns as 2048 bytes. The player ships those
on the register (identity, not a reading: it changes when someone swaps the
screen) and the SERVER parses them. That split is the point: a new field becomes
a server deploy instead of a bridge update behind a 4h CDN plus an OTA for the
host. Verified against real hardware — an XT245 with a CX101 decodes to RTK /
0x1010 / serial 1 / 2020w26 / 22x13cm, preferred 1920x1200@62, matching the
player's own DWS field for field. The odd-looking 62 is right: 168.5MHz over
2200 x 1245 is 61.5Hz, and rounding it to a nicer 60 would contradict the panel.
Also corrects two comments that had outgrown their reasoning: the BrightSign
capability baseline still explained its exclusions with "a canvas cannot read
the video plane", which native capture made obsolete, and player-parity.md
claimed the bridge is "always current" when a zone-wide Cloudflare Browser Cache
TTL had been rewriting its no-cache to max-age=14400 for months.
Every new guard is mutation-tested — the fix was reverted in the source and each
test confirmed to fail. 1676 -> 1714 tests, all green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014kfhrUPit5MCqxeTQyqr56
* Run the ScreenTinker server on the player it serves
A BrightSign XT245 now downloads, installs and runs the server itself, with
the display showing what it is doing until it is up.
WHY IT NEEDED A NEW SHAPE
BrightSignOS cannot open a large autorun.zip. The 73MB build failed at boot
with "ZipArchive error at line 91", and the OS renamed it autorun.zip_invalid
- which is how a device that had already unpacked once came back up with no
autorun at all. The identical package cut to 32KB and five files boots fine;
paths (182 chars) and depth (8) are unremarkable, so the limit is in the
boot-time reader, not the archive. BrightSign's own notes acknowledge package
size as a problem and point at webpack; that route needs the dynamic requires
in scripts/ removed first, so instead autorun.zip carries only what starts the
process and the payload arrives over HTTP into a Node that has no such limit.
The payload can also be updated without re-provisioning the device.
WHY roNodeJs AND NOT THE WIDGET
The first version ran the server inside an roHtmlWidget with nodejs_enabled.
That is a Node context inside an Electron renderer, and it is not Node. Four
separate boot failures came out of it, each invisible to a local test because
a local test runs on real Node:
- shebangs are not stripped, so any `#!/usr/bin/env node` file dies with
"Failed to construct 'ContextifyScript': Invalid or unexpected token".
Note it names no token - "#" is not one. An ESM file compiled as CJS says
"Unexpected token 'export'" instead, which is how the two are told apart.
- require() of an ESM-only package is unsupported, which plain Node 24
handles. uuid 14 is ESM-only and 21 files import it.
- setInterval is the DOM's and returns a NUMBER, so setInterval(...).unref()
throws. Two call sites were unguarded; sixteen more were written
defensively and had been silently not unreffing.
- worker_threads cannot create a thread at all.
BrightSign's dev-cookbook is explicit: roNodeJs "for long running processes
like ... running a web server", roHtmlWidget "for browser-based apps". Their
cra-template examples do exactly this - server in roNodeJs, widget pointed at
localhost. It also fixes the lifecycle problem that was the original argument
against a server on this hardware: in a widget the server dies with the page,
taking an open SQLite WAL with it.
The shims for the first three are kept in the packager for now rather than
removed in the same change that moves the container, so that if something
breaks it is the move and not four simultaneous removals.
CHANGES THAT ARE NOT BRIGHTSIGN-SPECIFIC
db/database.js, routes/status.js fs.copyFileSync does not merely copy
bytes: it fchmods the destination to match the source. exFAT has no
permission bits, so the pre-migration snapshot failed with EPERM and the
failure path called process.exit(1) - which inside a widget also killed
the page, leaving a black screen and no diagnostic. The guard was right;
the copy was wrong. lib/fsutil.js copies without touching mode.
db/wal-checkpointer.js the module already degraded correctly when its
worker died or could not be respawned, but the FIRST spawn was not
wrapped, so a host that cannot make threads lost the whole server rather
than falling back to inline autocheckpoint.
db/sqlite-compat.js a better-sqlite3 facade over node:sqlite. With it the
bundle contains no native code at all, which is what lets an x86_64
laptop build a package for an aarch64 player. 1719/1719 tests pass on
Node 24 through this shim.
The packager refuses to build if a source file is untracked (git ls-files
decides what ships, and lib/fsutil.js reached a player without shipping
alongside the code that required it), if any .node binary is present, if a
shebang survives, or if a database, upload, cert or .env is staged - the first
build of this package swept up a real 33MB database and 105MB of uploads.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014kfhrUPit5MCqxeTQyqr56
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Dan Walters <dan.walters@bytetinker.net>
On a fresh install with no users, typing an email address made the password
field disappear.
Identifier-first login asks the server which identity provider an address uses
before offering a credential, so someone whose organization requires its own
IdP is never shown a password box that will be refused. First-run setup opted
out of that by setting `identified = true` up front - there is nobody to
identify, the operator is creating the first account - and then the "editing
the address returns to the identifier step" listener fired on the very first
keystroke, set it back to false, and re-rendered. Password gone, mid-typing.
The same re-render also replaced "Create admin account" on the button with
"Sign in" and then "Next".
Initialising a flag to the right value is not the same as the flow being
inert: any later event could undo it, and one did. The decision now lives in
frontend/js/lib/login-form-state.js as a pure function that ignores
`identified` and `ssoOnlyDomain` entirely when there are no users, so no event
can take a field away. The input listener also returns early during setup,
which additionally stops it spending an org-lookup rate-limit budget that has
nothing to answer.
Two assertions in login-identifier-first.test.js pinned those expressions to
their old address inside login.js. Their intent - "the two drivers must be
combined in one place so they cannot disagree" - is better served by the
extracted module, so they now follow the logic there rather than being
loosened.
Verified: the extracted pre-fix logic returns showPassword=false and
buttonKey=auth.next for setup-plus-one-keystroke, which is the reported
symptom exactly; the new truth table covers it. i18n/login/auth/session/sso
suites go from 108 to 116 passing, none failing.
Not addressed here, and worth its own change: loadAuthConfig() has no error
handling and caches its first result, so a /api/auth/config that fails leaves
needsSetup undefined and the form falls back to the restrictive layout - the
same visible symptom from a different cause.
Claude-Session: https://claude.ai/code/session_014kfhrUPit5MCqxeTQyqr56
Co-authored-by: Dan Walters <dan.walters@bytetinker.net>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Follow-up to #285. Three corrections that rode in with the Japanese locale.
1. The ja key-parity check failed the build whenever en.js had a key ja.js
lacked. i18n.js lookup() is already
`registry[lang]?.[key] ?? fallback[key] ?? key`, so an untranslated string
renders in English and nothing is broken by a gap - the only effect was
that adding any English string blocked CI until a Japanese translation
existed. It also singled out one locale; es/fr/de/pt/hi/it were never held
to it, and hi.js is a deliberate skeleton whose own header explains that
every key falls back to English on purpose.
Replaced with two checks over EVERY locale: a locale may not define a key
that English does not (dead weight after a rename, and fixable by whoever
touched the file, whatever language they speak), and coverage is printed
rather than gated. Help tips still have to exist everywhere - that test is
unchanged and still fails.
Current coverage: ja 100%, es 65.5%, fr/de/pt 63.7%, it 59.5%, hi 0%.
2. Applying the strict half to all locales immediately found
add_display.smart_tv_note living in fr, pt, it and de but not in en.js and
referenced by no view - a string dropped from English that left four
translations behind. Removed.
3. The new timezone test restored process.env.TZ by assigning the saved value
back. When TZ was not set to begin with - which is the case in CI - that
assigns undefined, which writes the STRING "undefined"; Node cannot parse
it and silently falls back to UTC for the rest of the process. Every test
after it in that file is date arithmetic. It now deletes the key when it
was previously unset.
4. package-lock.json removed from .gitignore. server/package-lock.json is
tracked, so the rule was inert, but it would silently prevent a future
lockfile and works against the SBOM and reproducible-install setup added
in #282.
Claude-Session: https://claude.ai/code/session_014kfhrUPit5MCqxeTQyqr56
Co-authored-by: Dan Walters <dan.walters@bytetinker.net>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Three separate defects all showed up as "I saved a schedule and it moved".
1. expandSchedule emitted two different wire formats. A one-off returned
start_time untouched - a naive wall-clock string, which the browser parses
in its own zone - while a recurring instance returned cursor.toISOString(),
an absolute instant derived by reading that same string in the SERVER's
zone. The browser converted it back and the two conversions do not cancel:
an operator in Tokyo saving Wednesday 20:00 got Thursday 10:00 from a
US-Central server. Only recurring schedules were affected, which is why it
looked intermittent.
The calendar was also the odd one out. services/scheduler.js compares
start_time as a string and never builds a Date from it, so expandSchedule
was the only place in the codebase treating a wall-clock time as an
instant - the drawing disagreed with playback as well as with the browser.
2. Saving an edit moved the schedule to today. The save handler rebuilds
start_time from `pendingCreateDate || new Date()`, and editSchedule()
restored only HH:MM - it never recorded the date being edited. Changing a
colour on a block dated 5 Aug rewrote it to this week. No timezone
mismatch required; this one hit everybody and silently altered stored data.
3. A cancelled drag-create leaked its date into the next schedule.
pendingCreateDate was cleared only on a successful save, and the modal's
two dismissers are inline onclick="...display='none'" attributes that
cannot reach that scope. The date is now assigned by every path that OPENS
the modal, so no dismissal path can leave a stale value behind.
Tests assert the property rather than a literal: that the wire value is
wall-clock and that the day survives a server/browser zone mismatch in BOTH
directions. A literal-string assertion would pass just as happily with the
bug present on a differently-configured CI box. Both new files fail on the
parent commit and pass here; the schedule, calendar and timezone suites are
green at 85 tests.
Claude-Session: https://claude.ai/code/session_014kfhrUPit5MCqxeTQyqr56
Co-authored-by: Dan Walters <dan.walters@bytetinker.net>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
An audit of what actually reaches customers turned up three things. None is copyleft — there
is no GPL or AGPL anywhere in the product — but two of them are the kind of detail that
undermines a claim to track licences at all.
org.json WAS BEING PACKAGED INTO THE APK. socket.io-client pulls org.json:json:20090211
transitively, and it was landing in the dex in full: 19 classes, including CDL, XML, JSONML
and the library's own Test class. That release carries the JSON Licence, whose "shall be used
for Good, not Evil" clause is not OSI-approved, is treated as non-free by Debian and Fedora,
and is Category X at Apache. Excluded now, and nothing is lost: Android has provided org.json
in the platform since API 1 and minSdk is 24.
Verified rather than assumed. The whole surface used — by socket.io/engine.io and by our own
Kotlin — is JSONObject, JSONArray and JSONTokener, via get/getString/getLong/getJSONArray/
getJSONObject/has/keys/length/isNull/put/NULL, the opt* family, and JSONTokener.nextValue.
Every one is platform API. The rebuilt APK defines 0 org.json classes (was 19) while still
referencing all three, so they now resolve against the platform; it is 25KB smaller and the
v1 JAR signature is intact. Then it was installed on a real panel, which registered over the
socket, paired, and parsed a playlist with no NoSuchMethodError — the failure mode that would
only ever appear at runtime.
REDOC SHIPPED WITH NO LICENCE NOTICE. frontend/vendor/redoc.standalone.js is in the release
tarball, minified with every header stripped, and the vendor README recorded version and
source but not licence. MIT requires the notice to travel with the software. Added as
redoc.LICENSE, with a note in the README that anything vendored here ships and therefore
needs one.
The server manifest declared no licence at all, though the repo is MIT — tooling and auditors
read package.json, not just the root LICENSE. Set, and the lockfile synced so `npm ci` cannot
disagree.
Audited against production's own installed tree rather than a developer checkout: 365
packages, `COPYLEFT — none`.
1676/1676 server tests, Android build + unit tests, `npm ci` clean.
1.9.35 could not start with TELEMETRY_COLLECTOR=1. It threw before listening:
ReferenceError: Cannot access 'db' before initialization
at server.js:986
and systemd restarted it in a loop. Production was down until it was rolled back.
The mount passed the module-scope `db` to the collector's factory, but that binding is
declared ~275 lines further down. The inline handler this replaced only touched `db`
inside a request callback — which runs long after the binding exists — so moving the same
reference into a factory argument turned a lazy read into an eager one. Now resolved as
`require('./db/database').db`, the way every neighbouring call site in that region does it.
WHY NOTHING CAUGHT IT. The block is gated on a flag that only the statistics-collecting
deployment sets. It had therefore never executed in CI, on alpha, or in any test — 1676
tests, four green jobs, a clean alpha deploy, and the crashing line had still never run.
The unit tests mount the router directly and pass a db, which is precisely the part that
was fine.
So the boot smoke now boots WITH the collector enabled and asserts its routes answer:
/api/public/stats returns the expected shape, and a malformed report is refused with 400.
Booting alone would not be enough — the collector could mount and be broken.
Confirmed by reproduction: the released code fails to boot under that flag, and this does
not. 1676/1676 pass.
The number exists — every install that opts into sharing reports its screen count, and
the collector has been keeping them since it went live. Nothing read them back out.
GET /api/public/stats returns the aggregate: total screens and how many installs they
came from. The landing page shows it under the hero and stays silent otherwise — hidden
until a number arrives, so a self-hosted instance (where the route does not exist) and a
brand-new one (where the count is zero) show nothing rather than an empty frame or a "0".
Gated on TELEMETRY_COLLECTOR, the same flag as the collector, and the gate is doing real
work here: without it, any anonymous visitor could read a private instance's screen count
off its own landing page. Only the deployment that gathers the numbers may state them, and
there the figure is a sum across every reporting install, so it discloses nothing about any
one of them. Verified with the flag unset: both routes 404.
Cached for five minutes. This sits on a public page and the number moves in hours, so a
scraper in a loop costs one query per interval rather than one per request.
Both endpoints moved out of server.js into routes/telemetry-collector.js as an injectable
factory. They were inline and therefore untestable — an unauthenticated endpoint anyone on
the internet can POST to, and the one that decides what a public page claims, with no test
between them. Now covered: the upsert really updates (an install reporting daily must not
become 365 rows and get counted 365 times), malformed and hostile bodies are refused
without reaching the table, the aggregate carries no per-install detail, and the cache
holds.
1676/1676 pass.
npm audit reports 8 high findings. Four of them reach production; the other four are
dev-only and cannot, because prod installs with --omit=dev. Verified rather than
assumed: puppeteer-core, extract-zip, @puppeteer/browsers and js-yaml are all absent
from prod's node_modules.
Three of the four are transitive, and the fix is a patch or minor inside the range
package.json already declares — no API moves, and package.json is untouched by them:
brace-expansion 2.1.2 -> 2.1.4 (archiver -> glob/minimatch)
ip-address 10.2.0 -> 10.5.0 (express-rate-limit)
socket.io-parser 4.2.6 -> 4.2.7 (socket.io)
socket.io-parser was the one worth checking, because a parser change that altered the
wire format would break every deployed player at once rather than fail a test. It does
not: socket.io stays at 4.8.3, engine.io at 6.6.9, and the parser's protocol constant
is still 5. Nothing a player speaks changes.
The fourth is a real bump — nodemailer 6.10.1 -> 9.0.5, across three majors, closing
eight advisories including SMTP command injection and header injection. Our surface is
about as small as it gets: createTransport({host, port, secure, auth}) and sendMail with
from/to/subject/text/html. Engine requirements are unchanged (>=6.0.0), and the entry
point is the same.
The existing email tests mock nodemailer through require.cache, so they would have
stayed green through any breaking change in the library itself — proven, not guessed:
with sendMail patched to throw, those 15 tests still pass. So this adds a test that
drives the REAL library over a loopback SMTP server and asserts on the conversation,
using messages built by our own buildSmtpMessage rather than hand-written ones. That
test does fail against the broken build.
Left alone: extract-zip under puppeteer-core, now with no fix available. It is a
devDependency used only by smoke-ui.js, which already no-ops when it is missing, and the
advisory is symlink traversal while unpacking a downloaded browser — puppeteer-core with
an explicit executablePath never downloads or extracts one.
Production audit goes from 4 high to 0. 1671/1671 pass.
The directory-search widget draws its own on-screen keyboard, on by default, sized
and themed to the panel. On Android it was never visible: the page autofocuses a
real <input>, which is the signal to raise the system IME, and that lands over the
bottom of the screen — exactly where our keyboard is.
So a directory panel showed Google's keyboard instead of the one the widget ships:
split across a 1920x1080 screen, with mic, GIF, emoji, clipboard and a settings key
that opens Google's own UI on a kiosk. On the panel that turned this up, the system
keyboard WAS voice input — the only enabled IME was Google's voice IME, so touching
the search box opened a microphone, on a wall-mounted tenant directory.
When we draw a keyboard, the input now carries inputmode="none", so the platform
leaves its keyboard down. The buttons write input.value directly, so nothing about
typing changes. Browsers that don't know inputmode ignore it, which is the right
fallback — a desktop preview behaves exactly as before.
Gated on the flag, not applied to the markup: with show_onscreen_keyboard off there
is nothing to cover, and the platform keyboard is the only way left to type.
Verified by removing the line and watching the new guard fail. 1669/1669 pass.
A panel on prod looped on an update it could never apply, reporting a download
failure that was not one.
The staged-APK cache is keyed by FILENAME, and the filename is built from the
version the SERVER advertised. Prod advertised 1.9.34 while still serving the
1.9.33 file, so the panel saved 1.9.33 as `ScreenTinker-1.9.34.apk`. On every
retry it found that file, verified the signature — which passed, same key — reused
it, and installed a no-op. The version never changed, so the update was offered
again. Fixing the server did not help: the poisoned file is reused before anything
is fetched. It took `adb rm` to break the loop.
Two changes:
CHECK THE VERSION INSIDE. A cached APK is reused only when the versionName in the
file matches the version being installed, and a fresh download is checked the same
way before install. A server serving stale bytes now fails with what actually
happened — "server served 1.9.33 but advertised 1.9.34 — the update on the server
is stale" — instead of a download error, and the bad file is deleted rather than
kept to poison the next attempt. That makes this class self-healing: the panel
recovers on its own once the server is fixed.
A WAY TO CLEAR IT. `clear_update_cache` deletes every staged APK across all three
staging directories, with a button on the device page next to Force Update. Gated
on `system.self_update` — a player that can update itself is one that can hold a
bad download. Only caches are deleted; they are re-fetched on demand.
The version check should make the button rarely necessary. It exists because it
would have turned tonight's hands-on ADB recovery into one click, and because a
panel already holding a bad file predates the fix and cannot benefit from it.
1668/1668 pass; Android unit tests and lint clean.
A plain string compare on the prerelease tag put "alpha11" below "alpha8", because
'1' < '8'. The OTA check therefore answered client-newer and refused to offer the
update — while reporting the newer build as `latest` in the same response:
latest_version: 1.9.34-alpha11 current_version: 1.9.34-alpha8
update_available: false reason: "client-newer"
So a fleet on alpha8 or alpha9 could not be moved forward at all, silently, and
nothing about the symptom pointed at version ordering. alpha10 was never really on
offer either; the last update that genuinely worked was alpha6 -> alpha8, where the
lexical order happens to be right by luck.
This is what semver specifies for a single alphanumeric identifier, and it is
simply not what the naming means. lib/version-precedence.js compares digit runs
NUMERICALLY, so alpha8 < alpha9 < alpha10 < alpha11, while leaving everything else
alphabetical — beta still outranks alpha, rc still outranks beta, and a release
still outranks any prerelease of the same core.
Dot-separated identifiers are compared per semver and a shorter run loses, so
moving the naming to the semver-correct `-alpha.11` form later needs no further
change here.
TWO comparators carried the assumption, each with a comment asserting lexical was
fine "for our naming" — true only while the counter stayed below 10. Both now use
the shared helper rather than a third copy drifting into the same trap:
- lib/ota-breaker.js the Android OTA path
- lib/brightsign-update.js the BrightSign host package, where a wrong-way
comparison replaces the script that boots the player
lib/ghcr-check.js was checked and is unaffected: it rejects prerelease strings
outright rather than ordering them.
Tests pin the exact stranding case end to end — decide('1.9.34-alpha8',
'1.9.34-alpha11') must be an offer, not client-newer — plus the reverse direction,
so a future change cannot merely invert it.
1668/1668 pass.
Three changes, all about the same failure: sharing appears to be on while nothing
actually arrives.
SEND ON OPT-IN. Turning sharing on now reports immediately instead of waiting for
the next daily tick. Two reasons: the operator is standing right there, and
"nothing has been sent" for the next 24h reads as broken at exactly the moment
someone is checking whether it works. It also means an egress-filtered network
fails HERE, where we can name the host to unblock, rather than silently tonight
where nobody is watching.
NAME THE FAILURE. Failed attempts are now recorded separately from successes, so
Settings can say which address did not answer and why, instead of showing an empty
"nothing sent yet". A blocked outbound connection is the normal failure on a
self-hosted box and is otherwise completely invisible — the operator cannot tell a
firewall from a broken feature. A later success clears the complaint, so a stale
warning never outlives the problem it describes. Docs gained a section on it, and
the UI states plainly that nothing needs opening inbound.
OPERATOR COLLECTOR, ADDITIVE. TELEMETRY_EXTRA_ENDPOINT lets an operator post the
same three fields to their own collector.
The naming is the point. It replaces TELEMETRY_ENDPOINT, which was a true override
— and an override is the wrong shape here, because a variable called "endpoint"
that silently redirected the report someone agreed to SHARE would make the opt-in
mean something other than what the UI says. Our address is hard-wired and not
overridable; theirs is explicitly additional and named so it cannot be mistaken for
a replacement. Settings lists every destination a report goes to.
The operator collector is independent of the sharing switch, because it is their
server posting to their host and our opt-in has no business gating it. So an
operator who wants internal fleet numbers with nothing leaving for us sets it and
leaves sharing off — supported on purpose, and tested.
Destinations are attempted separately: one unreachable collector must not cost the
other its report.
1662/1662 pass. Tests pin the properties that matter: an operator collector never
replaces the shared report, sharing-off still sends nothing to us whatever else is
configured, one dead destination does not stop the other, and a failure records the
address actually tried.
There is no way to answer "how many screens run ScreenTinker?". The product is
self-hostable by design, so most installs are invisible to us on purpose — and
should stay that way. This asks once, and reports only if the operator says yes.
The entire payload is three fields:
{ instance_id, version, screen_count }
instance_id is a random UUID minted on first use and kept in app_settings. It
carries nothing about the install; its only job is to let two reports from the
same server be recognised as one server, so a count is a count rather than a sum
of duplicates. That makes a report pseudonymous rather than anonymous, and the
wording shown to operators says so rather than claiming otherwise.
The payload is short on purpose. Every field added costs participation, and
participation is the only thing that makes the resulting number worth quoting.
Player-platform counts were considered and left out: release assets are already
published per platform, so GitHub's per-asset download counts answer "where should
effort go" at zero privacy cost and without asking anyone for anything.
Verifiability is the feature, not the copy. Settings shows the ACTUAL payload this
server would send, generated live from its own data, plus what it last really sent
and when. The payload is built in one function so a reviewer can check it at a
glance, and the test fails if a field is ever added.
Both answers persist. Declining is remembered as 'off' rather than falling back to
'unasked', so the prompt cannot return after an update — re-prompting is how
telemetry earns its reputation and gets patched out.
Collector side is inert unless TELEMETRY_COLLECTOR=1, so a normal install never
exposes the endpoint. Reports upsert on instance_id rather than appending, so an
install reporting daily occupies one row rather than 365 a year. The source IP is
never read or stored — receiving one is unavoidable, logging it would quietly turn
a pseudonymous report into an identifiable one.
Tests pin the negative promises, which are the ones that rot silently: sends
nothing before consent, sends nothing after a decline, payload is exactly three
keys, id survives a restart, a failed send never records a phantom report. Screen
count excludes unpaired provisioning rows, which would otherwise overstate the one
number this exists to state honestly.
docs/telemetry.md documents the payload, what is not sent, how to verify it, and
that any published total is a floor rather than a basis for extrapolation.
1657/1657 pass.
* Document the Node.js upgrade procedure and this build's reinstall requirement
Upgrading the runtime does not go through scripts/upgrade.sh, so nothing
reinstalls dependencies — which is precisely when the one remaining native
module goes stale. The runbook now covers the version floor imposed by
--env-file-if-exists, why the better-sqlite3 pin is exact, and why a version
without a matching prebuild can turn Restart=always into a boot loop.
Also records that this build changes dependencies in both directions: rolling
back past it needs the reinstall too, because earlier builds import sharp at
runtime and this one drops it from production dependencies.
Kept deployment-neutral — no hostnames, addresses, or environment specifics.
* Fix the only test that fails on Node 22
Node 22 added a built-in `navigator` global, defined as a getter with no
setter. The test's shim assigned to it, which throws "only a getter" under
'use strict' on 22 while being a normal assignment on Node 20, where the global
does not exist at all. It is configurable, so define it instead of assigning.
Defining it unconditionally is also the better fixture: Node 22's own navigator
reports the HOST locale, so a test reading its language would otherwise depend
on the machine or CI runner it happens to run on.
This was the single failure in an otherwise clean Node 22 run (1639/1640 with
better-sqlite3 12.9.0), and it is confined to test code — no production server
or frontend file assigns to globalThis.navigator.
1649/1649 on Node 20.
Prepares for the Node 22 move by decoupling it from the database driver, so the
two upgrades land as independently reversible steps rather than one flag day.
9.6.0 cannot work on Node 22. It uses the raw V8 API (220 v8:: references, zero
napi_), so it is ABI-locked per Node major, and its GitHub release assets carry
prebuilds for ABI 108/115/120 only — nothing for Node 22's 127. Its install script
is `prebuild-install || node-gyp rebuild --release`, so on Node 22 it silently
falls through to compiling raw-V8 code against Node 22 headers. That is not just
slow: lib/preflight-deps.js rebuilds synchronously before the server listens, and
prod's systemd unit is TimeoutStartSec=90 with Restart=always, so a slow or failing
compile is an unbootable loop rather than the intended self-heal.
12.9.0 ships prebuilds for BOTH Node 20 (ABI 115) and Node 22 (127), so neither the
current runtime nor the target has to compile anything.
THE PIN IS EXACT ON PURPOSE — ^12.9.0 would defeat it. 12.10.0 dropped the Node 20
prebuild while still advertising "20.x" in engines, so a caret resolves to 12.11.x
and reintroduces the from-source compile on today's runtime. Verified against the
release assets per version:
12.0.0 / 12.2.0 / 12.4.5 / 12.6.2 / 12.9.0 ABIs 115,127,...
12.10.0 / 12.10.1 / 12.11.1 ABIs 127,137,141,147 — no 115
The reasoning is recorded in preflight-deps.js, which is where anyone hitting the
matching failure will already be reading.
13.x was considered and rejected FOR NOW: it is the first N-API release, which ends
the per-major ABI problem for good (8 prebuilds keyed by platform, not ABI) and is
where we should eventually land — but engines is ">=22", so it cannot be adopted
while prod, alpha and CI all run Node 20. It is also three weeks old with three
patch releases, which is young for the one component that owns all the data.
No API changes to absorb: every major from 10 to 13 bumped only for dropping EOL
Node/Electron versions, so the ~1486 .prepare(), 46 .transaction() and 59 .pragma()
call sites are untouched.
Verified on Node 20: installed from a PREBUILT binary (no obj.target, so no
compilation), opens a real database, and the WAL path the #149 checkpointer depends
on still works — journal_mode=WAL engages on a file DB, pragma(...,{simple:false})
returns the expected shape, and a second connection from another handle reads and
runs wal_checkpoint(TRUNCATE). 1649/1649 tests pass.
* spike: replace sharp with pure-JS image ops (jimp + jsquash WASM)
Removes the last native dependency from the ingest path, so the server no longer
needs a per-platform/per-ABI prebuilt to thumbnail an image. Motivated by getting
the server onto hardware with no toolchain, but the ABI tax is paid on every
install — it is the same failure class lib/preflight-deps.js exists to explain.
lib/image-ops.js is the whole surface: metadata() and writeThumbnail(), which are
the only two things ingest ever asked sharp for.
Format parity holds. jpeg/png/gif/tiff/bmp are native to Jimp; webp and avif go
through @jsquash WASM, whose bundled .wasm must be compiled by hand because the
packages locate it with fetch(file://) and Node has no file:// fetch — the only
symptom otherwise is a bare "fetch failed". heic is unsupported, as it already
was: sharp advertises heif but its prebuilt libvips refuses HEVC.
#170 is preserved by a different mechanism. Jimp applies EXIF orientation at
decode and rewrites the tag to 1, so metadata() reports display dimensions and
imageDisplayDims() runs as a no-op instead of swapping W/H a second time. The
helper stays in the path so the rule keeps living in one place.
Verified: 1643/1643 tests pass, and ingest was exercised in a child process with
node_modules/sharp moved aside — jpeg, EXIF-rotated jpeg, png, webp, avif, gif
all measured and thumbnailed correctly, corrupt input still yields nulls with no
phantom thumbnail_path.
KNOWN BLOCKER, do not ship as-is: Jimp is pure JS on the main thread, where sharp
handed work to a libvips threadpool. A 12MP photo goes 65ms -> 1079ms, and the
event loop stalls for 1003ms of it (sharp: zero stalls). thumbnail-backfill.js
walks a whole library at boot, so this reproduces #240 exactly — blocked loop,
missed heartbeats, panels marked offline, reconnect churn. Needs a worker_thread
offload before this is viable; image-ops.js is the seam for it.
* Run image decoding on a worker thread
Fixes the blocker the previous commit shipped with. Pure-JS decoding costs ~1s of
solid CPU for a 12MP photo, and in-process that is not a slow upload but a stalled
event loop — no heartbeats, no socket traffic. thumbnail-backfill.js walks a whole
library at boot, so it reproduced #240 (blocked loop -> missed heartbeats -> panels
offline -> reconnect churn) from our own maintenance. sharp never did this because
libvips works on a threadpool.
image-ops.js is now a dispatcher over image-ops-worker.js; the work moved unchanged
to image-ops-core.js, so callers and their failure contract are untouched.
Measured on a 12MP photo: 1079ms wall with the loop stalled 1003ms, to 1881ms wall
for two ops with ZERO stalls and 185 timer ticks serviced. Wall time is worse and
that is fine — it is off the main thread now.
Design notes, all load-bearing:
- ONE JOB AT A TIME. A decoded 12MP bitmap is ~48MB of RGBA; overlapping jobs
multiply peak memory by queue depth, which is the wrong failure on the small
targets this change exists to reach. Costs no throughput — the work is CPU-bound
and one busy worker already saturates its core.
- unref'd while idle, ref'd only in flight. Otherwise scripts/backfill-rotation-
dims.js never exits and `node --test` hangs forever. Verified: a CLI-style run
exits in 104ms, code 0.
- decode failures reply as messages, so one bad upload cannot tear down the worker
and take unrelated queued jobs with it.
- in-process fallback if a thread cannot be had, warned rather than silent.
test/image-ops.test.js pins the loop-liveness property, which no functional test
would catch. Its thresholds were mutation-tested against the inline path: the first
version passed there too (4MP stalls only ~355ms, under a non-flaky threshold), so
the fixture is 12MP and the thresholds sit in the gap between the two behaviours —
worker ~90 ticks/~0ms, inline ~3 ticks/~897ms. It now fails inline, as a guard must.
1647/1647 pass. Ingest re-verified with node_modules/sharp moved aside.
* Measure and thumbnail an image from a single decode
Ingest asked for metadata() then writeThumbnail(), which decoded the file twice.
That pairing was free under sharp, whose .metadata() only parses the header, but
every decode here is a full one — ~1s for a 12MP photo — so the naive translation
doubled the most expensive thing on the ingest path.
image-ops.measureAndThumbnail() returns both from one decode. Full ingest of a
12MP photo: 2 decodes/~1.9s -> 1150ms, still with zero event-loop stalls.
The subtlety is the failure contract. In the two-call version width and height
were assigned BEFORE the thumbnail was attempted, so a failed thumbnail still left
usable dimensions on the row — the player needs them to letterbox. Merging naively
would have turned any thumbnail failure into total metadata loss. So a WRITE
failure is reported ({thumbnailWritten:false, thumbnailError}) with the dimensions
intact, and the caller sets thumbnail_path only when the write succeeded, keeping
the phantom-path discipline. A DECODE failure still throws — there is nothing to
report about an unreadable image.
backfill-rotation-dims.js deliberately keeps the separate calls: it probes every
image row but regenerates a thumbnail only for the few whose dimensions changed,
so pairing them there would decode files it has no reason to thumbnail.
Tests count decodes rather than timing them — an exact property, and a wall-clock
comparison would be flaky under load. The count filters for reads of the file under
test: Node's ESM loader also goes through fs.promises.readFile, so a raw call count
picks up jimp's and the WASM codecs' lazy loading and reads 30 instead of 1.
1649/1649 pass. Ingest re-verified across all 7 formats with sharp moved aside.
* Dockerfile: sharp is no longer a production dependency
--omit=dev now leaves it out entirely; better-sqlite3 is the only native module
the builder stage still needs a toolchain for.
"Authentication required" on every click of Link. The Settings button did
`location.href = /api/auth/oidc/<slug>/link/start`, which is a top-level
navigation -- and this app's session lives in localStorage and travels as an
Authorization header, so the request arrived anonymous and requireAuth refused
it, correctly.
The login /start route works precisely because it needs no session. Copying its
shape for a route that does need one was the mistake.
The client now FETCHES link start with its token and navigates to the URL it
returns. The transaction cookie is still set by that response, because a
same-origin fetch stores Set-Cookie normally, so the callback is unchanged.
beginOidc grew an asJson flag rather than a second copy of the PKCE/state/nonce
setup, so login and link still cannot drift apart.
Both mutations fail the new test: navigating straight at the route, and having
the server redirect instead of answering with JSON.
Two halves of the same problem: an account created with a password could never
use single sign-on, and the login page offered a credential before it knew
which one applied.
LINKING. Signing in with a provider never adopts an account that already has a
password -- that is the takeover the login path exists to refuse. The README
promised the way out ("the owner signs in locally and links from Settings") but
nothing had ever been built, so the refusal was a dead end rather than a
redirection. Settings now has a Sign-in method block: an account with a password
can link an instance-wide provider, and one on a provider can unlink back to a
password.
The account being linked comes from the SIGNED TRANSACTION -- the session that
started it -- never from the email in the returned token. That distinction is
the whole feature: taking it from the token would be the same email-keyed
takeover under a friendlier name. The email must still match the account's own,
because login resolves accounts by the asserted address, and one provider
subject may not be linked to two accounts.
Linking DELETES the password rather than keeping it alongside. One credential at
a time, and the confirmation says so in those words, because a password left
behind is a second way in that the user believes they replaced. Unlink therefore
takes the new password up front and writes it in the SAME statement as the
unlink -- never unlink now and set a password after, which leaves an account
briefly, or on failure permanently, with no way in.
Instance-wide providers only. An organization's provider is chosen by a
customer; letting one attach itself to a platform account would hand that
customer whatever the account can do.
IDENTIFIER-FIRST. The password box now appears only after an address has been
submitted, which is what lets the organization lookup happen before a credential
is offered: someone whose company requires its own provider is shown that,
rather than a password box that will be refused. Editing the address returns to
the identifier step so a corrected domain gets a fresh answer.
The per-keystroke lookup is gone with it. It answered for half-typed domains,
changed the form under someone mid-address, and spent a 10/min per-IP budget on
people who had not finished typing -- an office behind one address could exhaust
it without a single sign-in attempt.
Instance-wide providers stay visible at all times now, by decision: the server
refuses them for an SSO-only organization anyway, and hiding them made the page
change shape while typing.
Verified in a real browser, not only by rendering: password hidden -> submit ->
visible and focused -> edit the address -> hidden again, with no page errors.
Four mutations of the linking rules fail the tests (account from the email
instead of the session, keeping the password, allowing org providers, dropping
requireAuth).
The previous fix let the instance-wide Microsoft button work and left the
customer-facing path broken, which is the worst way round. An organization that
brings its own Entra tenant would publish the TXT record, watch its domain go
green, and still be refused at login with `email_unverified` -- because Entra
sends no such claim and rowToProvider pinned the assumption off for every org
provider.
Requiring a claim Microsoft does not emit is not a security control, it is an
outage. What makes it safe to stop requiring it is the proof that already gates
these providers: the callback confines an org provider to its DNS-verified
domains, and an address only reaches the check after passing that. Whoever
controls a domain's DNS controls its mail, which is the same trust that makes a
verification link meaningful.
So the assumption is DERIVED from proof -- `verified.length > 0` -- rather than
pinned off. A provider that has verified nothing still assumes nothing, which is
belt and braces: emailAllowedForProvider already refuses it, since an empty
allow-list matches no domain, but deriving it here means a future reordering of
those checks cannot silently widen it.
It is never a column, and there is no column for it to be read from. An
organization must not be able to switch this on for itself; it is a consequence
of DNS proof, not a setting. A test asserts both -- that the value is derived
next to `source: 'org'`, and that no `assume_email_verified` exists in the
schema.
Domain confinement is untouched. An explicit `email_verified: false` is still
refused from anyone.
Mutations all fail the tests: assuming unconditionally, never assuming, and
reading it from the row.
The OIDC callback required `claims.email_verified === true`. Entra ID v2 does
not send that claim at all, so every Microsoft login authenticated correctly
against the tenant and was then refused with `email_unverified` on the way back.
Nothing caught it: the SSO tests assert how the Microsoft issuer string is built
but never put a Microsoft-shaped token through the policy.
The strict check was itself a fix -- `=== false` had been accepting an omitted
claim -- and it is right for a provider a CUSTOMER configured, since such a
provider is chosen by the party it vouches for and its bare assertion is worth
nothing. What was wrong is treating that as a question about the token when it
is a question about who we trusted. `users.email_verified` is our own state; the
claim is the IdP's. An instance-wide provider was chosen by the operator -- the
same trust that already exempts it from domain confinement -- and Microsoft is
additionally pinned to one tenant GUID, so only that directory can issue a token
whose `iss` matches.
So the policy now depends on the provider, in emailIsVerified(), next to the
flag it reads so the two cannot drift:
- explicit true -> believed, from anyone
- claim absent, operator -> believed (Microsoft; opt-in for other IdPs)
- claim absent, org -> refused
- explicit false -> refused, always
Org providers pin the flag false in rowToProvider and never read it from the
row, so the takeover path the strict check existed to close stays closed.
Google is left strict: it does send the claim.
Also documents MICROSOFT_CLIENT_SECRET (supported in code, missing from the
table), that the redirect URI must be registered under Web rather than SPA, and
the email optional claim -- the other two ways an Entra setup fails.
All three mutations of this policy fail the new tests: reinstating the strict
check (3 failures), letting an org provider assume (1), and accepting an
explicit false (2).
#254 lets an organization opt out of widget iframe isolation so that players
can embed origin-strict third-party sites. It applied that opt-out to the
widget editor's Preview as well.
Preview is framed by the dashboard, from the dashboard's own origin, and the
dashboard keeps its session JWT in localStorage. So with the setting on, anyone
who can author a widget -- workspace_editor and up; viewers are refused at the
create route -- could put script in a text widget and read the session of
whichever admin clicked Preview. That is an editor -> admin escalation, and it
is not the risk the confirmation modal asks the admin to accept: a player runs
on a kiosk with a device token, an admin's dashboard session is a different
thing entirely.
The org setting is what makes players able to embed those sites, so the
/render path keeps consulting it. Preview is pinned to allow-scripts in both
places that build it -- the dashboard iframe and the server-side render -- so
neither a frontend change nor a new server caller can re-grant it alone.
Also correct the modal copy, which claimed same-origin would expose the session
of anyone viewing "a display or preview". Preview is now excluded, and the
display case is really the device token, so say that instead.
widget-preview-stays-isolated.test.js fails if either half is reverted; both
mutations were checked to fail before committing.
Four HIGH findings. Two were mine, and one was a composition of two of my own fixes.
ONE EXTRA SLASH DEFEATED EVERY /api/auth LIMITER
`/api/auth//login` still reaches the login handler — Express normalises the mount
boundary for the router — but `app.use('/api/auth/login', rateLimit(...))` does not
match it, so the limiter never runs. A review got a real session after 60 unthrottled
password attempts. Same for //totp/verify (unlimited 6-digit brute force),
//forgot-password (unlimited reset mail to any address) and //sso/discover (the
customer-enumeration cap, gone). Fixing the limiter KEY could never help, because the
middleware was never invoked: the path is now collapsed to one canonical form before
routing. Pre-existing, and it falsified this file's own warning about walking past the
login limiter.
STORED XSS: I ESCAPED ONE COPY OF THE TABLE
My earlier fix patched views/admin.js line 357 and missed line 372 in the same
function — and missed views/settings.js entirely, which renders a SECOND copy of the
platform users table from the same endpoint, including the email in a raw text node.
The write path was `POST /api/admin/users`, whose EMAIL_RE barred only whitespace, so
an org or workspace admin (not a platform admin) could choose an address that executed
in the operator's session. Both tables escaped, both regexes tightened to reject markup
characters, verified against 11 address shapes.
I KILLED THE BREAK-GLASS WHILE CLOSING AN ORACLE
Hoisting the domain check above the account lookup — my fix for the enumeration oracle
— made `user.role !== 'platform_admin'` unreachable for enforced domains. On a
self-host the operator IS the org owner, and my would_lock_out_actor guard GUARANTEES
their address is inside the enforced set, so the recovery loop closed on itself:
approving a removal request needs a signed-in platform admin. Both properties hold now
by letting the operator through on a CORRECT PASSWORD only — every wrong answer is the
identical 403 whether the address exists, does not exist, or is theirs. Verified: 200 /
403 / 403 / 403.
Also fixed: enabling SSO-only locked out every password-holding member including the
admin who pressed the button (password refused by policy, SSO refused by
account_exists_local). An org provider now adopts a password account at a domain it has
PROVED by DNS when the org requires SSO — which is what a verified domain means, and
what every hosted identity product does.
SSO USERS WERE LANDING IN A PERSONAL ORG
The membership write added organization_members but no workspace_members, and
ensureDefaultOrgForUser looks at workspaces — so it minted each SSO user a private
organization and made it their current one. The customer's Members page read
"Members (1)" while their staff signed in successfully and were invisible.
ALSO: bcrypt on a NULL password_hash 500'd with a stack (and was an oracle for accounts
a provider deletion had returned to local); stranded_members was returned by the server
and discarded by the UI; a provider with zero domains was the one useless state with no
warning; two limiter shapes were missing (removal-request shared the garbage bucket —
an unauthenticated flood could deny the SSO break-glass path); doubled mail subject
prefixes; a DELETE that toasted "Saved"; a decided request left in the DOM with live
listeners; and a confirm dialog promising "immediately" when sessions already open
survive.
1609 tests, three clean runs. Limiter, break-glass, oracle parity and null-password all
verified against a running server.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bvjey4FNam49MN7ybjcq6A
THE HEADLINE FEATURE COULD NOT RUN.
"Continue with single sign-on" was a <form method="POST"> that redirected on to the
customer's identity provider. Chrome applies `form-action` across the WHOLE redirect
chain, and the dashboard sets `form-action 'self'`, so the hop to the provider was
aborted — silently. The user clicked and nothing happened: no navigation, no toast, no
spinner, a byte-identical page. Combined with SSO-only it was a total lockout: password
login answers 403 "use the single sign-on button", pointing at a button that cannot
work.
Every test I ran on this feature checked the button RENDERED. None clicked it.
The provider origins cannot be allowlisted — customers supply them at runtime. So the
page now fetches the destination and navigates itself; a script-initiated navigation is
not governed by form-action. The redirect answer is kept for a caller without
JavaScript, where the chain stays same-origin until the provider takes over. The slug
in the JSON is not a disclosure: following the old redirect put it in the address bar
and history anyway.
Verified in Chrome: the provider start endpoint is reached, zero CSP violations, zero
aborted requests — where before it was ERR_ABORTED plus a console violation.
STORED XSS IN THE PLATFORM ADMIN'S SESSION
admin.js interpolated user name, email and auth_provider into innerHTML unescaped, and
/register accepted an address whose local part was an img tag with an onerror handler —
no spaces, so it slipped the asserted-email check too. A reviewer registered
anonymously and got script execution on #/admin: the page operators are now emailed to.
Escaped, and registration refuses addresses that are not addresses. (The render bug
predates this branch; the reachability and the significance of that screen do not.)
ALSO
- the org SSO button is secondary while a password still works; two identical blue
buttons stacked sent people to their IdP by muscle memory after typing a password.
1609 tests, three clean runs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bvjey4FNam49MN7ybjcq6A
From the regression sweep. The first is a genuine regression against main.
A RATE-LIMITED DISCOVERY PERMANENTLY DEAD-ENDED THE LOGIN PAGE
lookupOrgSso checked that a body PARSED, not that the request succeeded — and a 429
body is valid JSON. So `data.sso` came back undefined, the single sign-on button was
hidden, the password box restored, and the domain recorded as answered: permanently,
for the life of the page. On an SSO-only domain that is the worst outcome available —
the password box then returns 403 and the button the user is told to use is not on the
screen. Discover is 10/min per IP and one person filling in the form costs up to four
calls, so a few colleagues behind one office address is enough. The comment above that
code already claimed to prevent exactly this; it only ever covered the 5xx case.
THE SSO-ONLY REFUSAL WAS AN ACCOUNT-EXISTENCE ORACLE
403 for an address that exists, 401 for one that does not — from an endpoint whose own
lockout returns 401 specifically to avoid that. The DOMAIN check now runs BEFORE the
account lookup, so both answer identically; whether a domain uses single sign-on is
already public through /sso/discover, so it reveals nothing new. The membership-level
refusal is deliberately downgraded to the generic 401, because a distinct answer there
would put the oracle back for exactly the accounts worth enumerating.
Verified: existing and invented addresses at an SSO-only domain both 403; and on an
instance with NO SSO configured, register/login/wrong-password/unknown-address are
201/200/401/401 — the hoisted check does not touch them.
BOOT PREFLIGHT
- a cold install ran `npm ci --omit=dev` unconditionally, so a first start on a
developer machine left `npm test` broken: same class of surprise as the prune this
file already warns about, through the other branch of the same if. Now production-
only.
- two servers starting together: the loser died with ENOTEMPTY even though the tree
was complete by then. It re-checks before failing.
- the opt-out accepted only '1', unlike every other boolean the server takes.
THE LIMITER FOLD, DONE PROPERLY
Unmatched paths under /api/organizations still minted a bucket each. My first fix was a
catch-all regex — which put every unknown path in ONE bucket WITH the real endpoints,
so flooding nonsense URLs exhausted the limit for /sso-only. That trades a bypass for a
denial of service. Folding is now by explicit shape: known endpoints keep their own
keys, everything else shares a bucket kept apart from all of them.
Verified: 120 unmatched paths give 60/60 (bypass closed), and after that flood
/sso-only, /sso and /sso/:id/test all still answer 401 rather than 429 (no starvation),
while 70 hits on one real endpoint do trip its own limit. The login trailing-slash
bypass stays closed.
1609 tests, three clean runs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bvjey4FNam49MN7ybjcq6A
A second attack round defeated three of the previous fixes and found a regression I
introduced. Each is reproduced-then-refused against a live server.
MEMBERSHIP: organization_members IS NOT HOW PEOPLE JOIN
Only three places write that table and nothing deletes from it — every INVITED user,
every admin-created account and every workspace assignment lands in workspace_members
and nowhere else. So keying enforcement on organization_members covered org owners and
people who had already used SSO: exactly the set the domain check already caught. A
reviewer invited an outside address into an SSO-only tenant, kept password login, read
the member list and content, and used it to invite more. Enforcement now asks whether
the user is in ANY workspace belonging to an SSO-only organization.
THE INTERLOCK ASKED THE WRONG QUESTION, TWICE
It fired only when a domain list became EMPTY, and it counted PROVIDERS. So:
- replacing acme.test with decoy.test removed every proof and sailed through — two
PUTs, and the customer's domain enforced nothing, with sso_only still reading true;
- with two providers you could disable the one owning your staff's domain, because
the other one, covering a domain nobody signs in at, still "enforced".
The question that matters is per-DOMAIN: after this change, is every domain that
enforces today still enforcing? Losing one needs the operator, whichever route gets you
there. The refusal now names the domain that would stop being covered.
REGRESSION I CAUSED: THE HAPPY PATH LOCKED THE OWNER OUT
Sign up with a personal address, create the org, verify the company domain, turn this
on — and enforcement covers you (you are a member) while your own address is outside
the verified domains, so passwords are refused AND your org's provider will not assert
for you either. No route removes a membership; reset succeeds but login still refuses.
Recovery meant a platform admin turning SSO off for the whole tenant. Enabling now
refuses when the actor's own address is not covered, naming it, and REPORTS everyone
else who will be stranded instead of letting them be discovered by support ticket.
ALSO
- POST /api/admin/users gated only on the target workspace, so you could mint
cfo@theircompany.test into your OWN workspace: login refused, but the row now has a
password_hash and an SSO login will not adopt one — permanently locking a real
person out of their own address. Now gated on the address's domain too.
- `ceo@acme.test.` (trailing root dot) slipped the registration gate.
- two rate-limited sub-paths were still unfolded because the generic org-id fold ate
`sso-only` as an organization id; the specific shapes are matched first now.
1609 tests, three clean runs. Verified live: invited outsider 403, swap refused,
sibling-disable refused, squat 400, self-lockout refused with the address named, and an
on-domain admin gets `stranded_members` back.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bvjey4FNam49MN7ybjcq6A
The approval workflow had no front door. The notification email told the operator to
"review it in ScreenTinker under Admin" and that screen did not exist — the only way to
approve was curl, while the tenant sat locked out of their own product. Admin now leads
with a removal-request section: who asked, for which organization, the reason they
gave, what approving does, and Approve/Reject. It hides itself when the queue is empty.
Approving is confirmed; rejecting is not, because rejecting only leaves the safe state.
REGISTRATION BYPASSED SSO-ONLY AND SQUATTED ADDRESSES
/register had no domain awareness: it issued a working session at an SSO-only domain,
and the account then held that address forever, because an SSO login will not adopt a
row that has a password. Registering ceo@acme.test before the real CEO's first login
left the address dead in both directions with no self-service way out. Refused now, and
"Create Account" is hidden on the login page for those domains — it was the only action
left on the card, so the page was inviting the one thing that cannot work.
THE NEW RATE LIMIT WAS DECORATIVE
/api/organizations carries three caller-chosen segments, and only the OIDC slug was
folded — so every request minted its own bucket. Measured: 120 calls with unique org
ids produced ZERO 429s, unauthenticated, against the limit that exists to bound
outbound discovery and live DNS. Now 60/60. The general problem was named in the
previous commit's own comment and then not applied to the mount it added.
XSS IN THE TOAST
showToast built innerHTML from server strings, including ones that reflect input
verbatim — a reviewer typed `<img src=x onerror=alert(1)>` as an issuer and got script
execution in the admin's session. Escaped.
ALSO
- the org SSO button sat BETWEEN the "Password" label and its input, so the label
described the button and the field had none; moved below the input, with a for=
- the OR divider survived when the providers under it were hidden
- provider action buttons were clipped off-screen at 375px with no way to scroll to
them — "Remove" was unreachable; the row wraps now
1609 tests. Verified in real Chrome: 13/13 on the approval loop and the login states,
including approving a request and watching password login re-open for that org.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bvjey4FNam49MN7ybjcq6A
Three HIGH findings from the QA round. Each was demonstrated end to end against a
running server, and each is now refused there.
ENFORCEMENT PROTECTED A DOMAIN, NOT AN ORGANIZATION
ssoOnlyForEmail answers about an address's domain, so any account in the tenant at an
outside address kept password login — a contractor, an MSP, the one address nobody
remembered. And it could be manufactured: POST /api/admin/users accepts workspace_admin
and creates a LOCAL password account at any address bound to that workspace. A review
created backdoor@notacme.test, logged in with the password, landed in the SSO-only org,
and used it to create another. Enforcement is now keyed on MEMBERSHIP as well as domain
(ssoOnlyForUser), and that route refuses to mint password accounts into an SSO-only
organization at all. platform_admin keeps both, as the operator break-glass.
THE APPROVAL WORKFLOW WAS DECORATIVE
`sso_only` is honoured only while a provider is enabled and a domain is verified, so
`PUT {enabled:false}`, `PUT {email_domains:""}` and `DELETE` each switched enforcement
off — with sso_only still reading true, no request filed and the operator never told.
The delete variant additionally rewrites every federated account to `local`, after
which a password reset takes over accounts the identity provider was supposed to own.
Anyone who could file a request could simply turn the provider off instead. All three
now refuse with sso_only_locked when nothing else would still enforce, and say to ask
for approval.
FRESH INSTALLS FAILED THE MIGRATION AND FAILED OPEN
The ALTER adding organizations.sso_only sat in the column-migration array, which runs
BEFORE the multi-tenancy migration that creates the table: `[migrate] FAILED … no such
table: organizations`, one line among ~85. The instance then ran its whole first boot
with the SSO settings screen 500ing and ssoOnlyForEmail catching `no such column` and
answering "not required" — password login proceeding for an organization that had
switched it off. It self-healed on the second boot, which is what made it easy to miss.
The column is now added after the table exists, and the catch distinguishes "this
instance has no per-org SSO" (null, so single-tenant installs keep working) from drift
on a table that DOES exist (throw). Login treats an undeterminable answer as "required"
rather than letting a 500 escape or letting the login through.
Verified live, all four refused with enforcement intact and the operator still able to
sign in. 1609 tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bvjey4FNam49MN7ybjcq6A
