mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 22:03:13 -06:00
13 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
ScreenTinker
|
fbf55f842c |
Close the third QA round: limiter bypass, stored XSS, break-glass, org placement
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
|
||
|
ScreenTinker
|
37e22bb773 |
SSO-only: enforce per-domain, cover invited members, and stop the admin locking themselves out
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
|
||
|
ScreenTinker
|
983bee31b7 |
SSO-only: close the backdoor, the unilateral disable, and the fresh-install fail-open
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 |
||
|
ScreenTinker
|
a25c6827a7 |
Show every plan on the admin tab, with who is on each
The admin plan table read /api/subscription/plans, which filters `active = 1` because that endpoint feeds the public pricing page. So the one screen meant to show the operator what plans exist could not show a hidden one — a comped or beta tier was invisible to us as well as to customers, with no way to see it existed or who was on it. Found immediately after creating exactly such a plan. GET /api/admin/plans (platform-admin only) returns every plan plus, per plan, the number of accounts, organisations and screens on it. Visible plans sort first so the list still reads like the pricing ladder, with hidden ones after and badged. The public endpoint is deliberately untouched: hiding a plan has to keep working, and the test pins BOTH directions because they pull against each other — the admin list must include an inactive plan, and the public list must never leak one. Counts are the point, not decoration: "how many people are on what plan" is the question you actually ask of this screen, and it was answerable only by hand in SQLite. Also carries a warning for accounts whose plan no longer resolves. Both users.plan_id and organizations.plan_id are FK-enforced to plans.id and there is no delete-plan route, so this should be unreachable — but migrations here do rebuild tables with foreign keys off (the tenant-cascade one rebuilt thirteen), and that is exactly how a row would be orphaned. Six lines for a state that would otherwise be silent. Strings added to en/de/es/fr/it/pt. Not hi: it has no admin translations at all, lookup falls back to English, and four Hindi strings among forty English ones would read worse than consistent English. |
||
|
ScreenTinker
|
792013e36c |
Record auth rate-limit rejections so they can be measured
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 |
||
Fabian Mendoza
|
34f1cb9e7c
|
feat(dashboard): version indicator + GHCR update check (#165)
* feat(dashboard): version indicator + GHCR update check with admin panel - Add server/lib/ghcr-check.js: GHCR tag poller (36h cache, semver filter) - Extend /api/version with latest_version and update_available - Add POST /api/admin/check-update (force GHCR poll) - Add POST /api/admin/trigger-update (Docker compose or manual instructions) - Sidebar footer: version label + amber badge when update available - Admin > System: version comparison card with Check/Update buttons - 14 new tests (10 unit + 4 integration), 68/68 passing Closes #163 * fix(dashboard): gate trigger-update to platform-admin + add GHCR fetch timeout Review follow-up on #165 (the two blockers): - trigger-update runs `docker compose up -d` on the HOST via docker.sock (root-equivalent) but was behind requireAdmin, i.e. reachable by any workspace-level admin. On a multi-tenant host that's a customer, not the infra operator. Gate it with requirePlatformAdmin (DOCKER_UPDATE_ENABLED still gates it further). check-update stays requireAdmin — it's a read-only GHCR poll. - ghcr-check.checkNow had no fetch timeout. Node's global fetch has no default timeout, so a hung GHCR connection never settled — leaving `inFlight` set forever (the finally never ran), which wedged the background poller AND hung any awaited checkNow (/api/admin/check-update). Add a 10s AbortController timeout on both requests so the try/catch/finally always fire. All 405 server tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: ScreenTinker <hello@screentinker.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
ScreenTinker
|
9418582de5 |
feat(#146): always-on devices_connected + admin-toggleable /api/status debug block
1. devices_connected (always on, never gated): a top-level /api/status field next to loop_lag = LIVE WS socket count from the heartbeat connection map (getConnectedCount), NOT devices.status='online' (which lags by the offline-timeout). The single most-glanced operational number, so it can't disappear when debug is off. Also dropped 4 dead per-poll COUNT(*) queries the route computed but never returned. 2. debug block behind an admin flag: new minimal app_settings KV table (none existed; ai_settings is per-workspace, white_labels is branding) + lib/app-settings.js (cached, refresh-on-write so status polls read a cached boolean, not a DB row). routes/status.js includes `debug` ONLY when status_debug_enabled is on (persisted value overrides the STATUS_DEBUG_ENABLED env default); when off the key is omitted entirely. 3. Admin toggle: GET/PUT /api/admin/status-debug (requirePlatformAdmin, mirrors the branding endpoints) + a checkbox in the Admin tab "Status endpoint" section (mirrors the branding checkbox). Takes effect on the next poll, no restart. Tests: devices_connected always present+numeric and rises with a live socket (booted + socket.io-client); debug present by default, admin flips OFF -> key omitted (loop_lag + devices_connected remain) -> ON again, no restart; non-admin 403, anon 401; unit coverage for getConnectedCount + app-settings default/override. Suite 289/289. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
ScreenTinker
|
0d14db97a6 |
feat(admin): Delete Organization + Workspace with cascade (#36)
Platform admins can now cleanly remove a customer org (account ends) or a stray workspace from the UI, instead of raw SQL that risks orphaning resources. The tenant cascade isn't pure DB CASCADE - workspace-scoped tables (devices, content, playlists, ...) are NO ACTION and must be purged before the workspace. Extracted that logic out of deleteUserCascade into shared deleteWorkspaceCascade / deleteOrgCascade helpers (one tested implementation; deleteUserCascade now reuses the purgeWorkspaces extraction). Backend (platform-admin only): GET /api/admin/orgs (list + owner + counts + workspaces), DELETE /api/admin/orgs/:id, DELETE /api/admin/workspaces/:id. UI: an Organizations section in Admin listing every org/workspace with a type-the-name confirmation before the irreversible delete. Tests: org/workspace cascade (real FKs) + endpoint gating/404. Suite 53/53. |
||
|
ScreenTinker
|
ae595a208d |
feat(admin): Create Organization for platform admins (#35)
MSPs onboarding customers as separate orgs had no way to create one with AUTO_CREATE_ORG_ON_SIGNUP=false (the only path was signup auto-org). Add a platform-admin 'Create organization' action. POST /api/admin/orgs (requirePlatformAdmin) creates the org + its first 'Default' workspace. organizations.owner_user_id is NOT NULL, so an org can't be ownerless; the creating admin becomes org_owner + workspace_admin (mirrors the signup bootstrap in routes/auth.js) - which also surfaces the org in their switcher. Customer users are then added via the existing Add User / manage-memberships flow. UI: 'Create organization' button + single-field modal in the Admin area (gated). Tests: create (201 + memberships + audit), empty-name 400, non-admin/operator 403. |
||
|
ScreenTinker
|
eb13f716d0 |
feat(branding): instance-level default white-label branding (#15)
White-label is stored per-workspace (white_labels.workspace_id); unbranded and
new workspaces - and the login page - fell back to hardcoded ScreenTinker. Add a
single platform default that everything inherits beneath the per-workspace layer.
Resolution (lib/branding.js): workspace row -> custom-domain match -> platform
default -> hardcoded ScreenTinker. Row-level override: a workspace with its own
row keeps it (current behavior); only row-less workspaces inherit the default,
so editing the default propagates instantly (no row-copying at creation).
The platform default is a white_labels row with a FIXED id ('platform-default'),
not a "workspace_id IS NULL" sentinel - legacy pre-multitenancy rows can also
have a null workspace_id, which would be ambiguous.
- routes/admin.js: GET/PUT /api/admin/branding (requirePlatformAdmin) to read/
upsert the single platform-default row; audit-logged.
- server.js: public GET /api/branding (domain match -> platform default ->
hardcoded) for pre-login/pre-workspace contexts.
- routes/white-label.js: authed GET now falls back to the platform default
(was hardcoded) for row-less workspaces.
- Frontend: login page resolves + applies branding (logo, name, colors, favicon,
custom CSS) pre-auth; Admin page gets a "Default branding" form.
Tests: resolver order incl. legacy null-ws safety; admin GET/PUT (single row,
upsert, platform-admin-only 403). Full suite 37/37. Verified end-to-end:
public + authed + login-page all inherit the platform default; per-workspace
override preserved.
Closes #15.
|
||
|
ScreenTinker
|
2872b883c7 |
feat(admin): manage a user's workspace memberships (multi + per-workspace role)
The Workspace column on the platform Users page could only move a 0/1-workspace user and showed a dead "N workspaces" label for multi-membership users. Replace it with a "Manage workspaces" modal that handles the full picture. Backend (routes/admin.js, requirePlatformAdmin): - GET /api/admin/users/:id/workspaces list memberships (+org/ws names, role) - POST /api/admin/users/:id/workspaces add to a workspace (upsert role) - PUT /api/admin/users/:id/workspaces/:wsId change role in a workspace - DELETE /api/admin/users/:id/workspaces/:wsId remove (last one allowed -> unassigned) Roles validated against WORKSPACE_ROLES; each mutation writes an audit row. Frontend: - Workspace cell is now a summary (Unassigned / <name> / N workspaces / "Platform (all)" for staff) + a Manage button. - New admin-user-workspaces-modal: lists every membership with an inline role dropdown + Remove, plus a type-to-filter "Add to workspace" picker (org-grouped, excludes current memberships) with a role select. Staff get a note that they already have platform-wide access. Refreshes the table on close if changed. - Removed the old single-select inline move control (superseded by the modal). Tests: 6 added (add to multiple workspaces, per-workspace role change, upsert, remove incl. last->unassigned, validation 400/404, non-platform-admin 403). Full suite 33/33. Verified headless: Manage opens, lists memberships, filtered picker, add/role-change/remove round-trips persist (throwaway user, cleaned up). |
||
|
ScreenTinker
|
7615eabdd5 |
feat(admin): Workspace column + inline move/assign on the Users page
Adds a "Workspace" column (after Plan) to the platform Users admin table so a platform_admin can see and reassign a user's workspace inline, alongside the Role/Plan dropdowns. Single-workspace move/assign model. Backend: - GET /api/auth/users (platform branch): one aggregate query adds workspace_count and, for exactly-one membership, the workspace id/name + org name (no N+1). - PUT /api/admin/users/:id/workspace (requirePlatformAdmin - operator excluded): move (1 membership) or assign (0) into the chosen workspace, default role workspace_viewer, in a transaction; no-op if already there; REFUSES (400) a user with >1 membership (manage in the members view). logActivity admin_set_user_workspace. Frontend (admin.js): - Editable <select> only for a 'user' with 0/1 membership; multi-membership -> read-only "N workspaces", platform staff -> read-only "Platform (all)". - Options grouped by org via <optgroup>, built ONCE from /me's accessible_workspaces (same source as the Add User picker) and reused per row. - Picking "Unassigned" or the same workspace is a no-op so a stray pick can't strip a membership. Success -> toast + refresh. EN i18n only. Tests: 4 added (single-membership move 200 + changed, zero-membership assign 200, multi-membership 400 refused, non-platform-admin/operator 403). npm test 16/16. Verified headless: column renders, selected value correct, "Platform (all)" for staff, and a dropdown move persisted (throwaway user, cleaned up). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
ScreenTinker
|
6e31770cee |
feat(admin): admin-provisioned user creation + first-login gate (#10)
Adds POST /api/admin/users so an admin can create a user directly with a known password and assign them to a workspace + role - for self-hosted instances with no outbound email, where invites never deliver. Server (routes/admin.js, mounted /api/admin with requireAuth + activityLogger): - Gated by canAdminWorkspace(db, req.user, targetWorkspace): 404 if the workspace is missing, 403 if not an admin of it. This scopes org_admins to their own org and excludes platform_operator (no user/role mgmt, #13). - Validates email (invite-create regex), role in WORKSPACE_ROLES, password min-8 (the /me rule). 409 on duplicate email - never overwrites. - One transaction: global users row (auth_provider 'local', bcrypt.hashSync(pw,10), must_change_password from the flag) + a workspace_members row written inline (same footprint as an accepted invite; accept-invite left untouched). - Explicit audit row admin_create_user; never logs the password; response excludes password/hash. - HOSTED_INSTANCE: never calls sendSignupEmails and stamps both welcome_email_sent_at / activation_nudge_sent_at, so an admin-created user gets no welcome email and never enters the activation-nudge sweep. must_change_password (frontend-first enforcement, per spec): - Migration adds users.must_change_password INTEGER NOT NULL DEFAULT 0; surfaced via requireAuth + /me + login responses. - route() in app.js forces users with the flag to a #/change-password screen (new force-password-change view, reuses PUT /api/auth/me) and blocks every other view until set. The /me update clears the flag. Frontend: "Add User" button beside "Invite member" in the members view (admin-only) opening a modal (email, name, password + generate, role, must-change checkbox); invite and Add User coexist. api.adminCreateUser; EN i18n only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |