Archive/screentinker
1
0
Fork 0
mirror of https://github.com/screentinker/screentinker.git synced 2026-08-14 22:33:12 -06:00
Code Issues Actions 10 Packages Projects Releases Wiki Activity

SSO-only: enforce per-domain, cover invited members, and stop the admin locking themselves out

Browse source 
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
This commit is contained in:
ScreenTinker 2026-08-11 09:51:40 -05:00
parent 355b7a2b86
commit 37e22bb773
4 changed files with 159 additions and 23 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

28
server/lib/oidc-providers.js
View file

@ -310,7 +310,9 @@ function ssoOnlyForEmail(email) {
if (!conn) return null;
const at = String(email || '').lastIndexOf('@');
if (at === -1) return null;
const domain = String(email).slice(at + 1).toLowerCase().trim();
// A trailing root dot is the same domain; `acme.test.` slipped the match and let someone
// register at an SSO-only domain (a distinct string, so no squat — but a hole in the gate).
const domain = String(email).slice(at + 1).toLowerCase().trim().replace(/\.+$/, '');
if (!domain) return null;
try {
return conn.prepare(`
@ -365,15 +367,29 @@ function ssoOnlyForUser(user) {
const conn = db();
if (!conn) return null;
try {
/*
* ⚠️ WORKSPACE membership, not just organization_members.
*
* Almost nobody is in `organization_members`: only three places write it (creating an org,
* an org-SSO login, a platform admin creating an org) and nothing ever deletes a row. Every
* INVITED user, every admin-created account and every workspace assignment lands in
* `workspace_members` and nowhere else — so an earlier version of this check covered org
* owners and people who had already used SSO, which is exactly the set the domain check
* already caught. A review invited an outside address into an SSO-only tenant and kept
* password login, then used it to invite more.
*/
return conn.prepare(`
SELECT o.id AS organization_id, o.name AS organization_name
FROM organization_members m
JOIN organizations o ON o.id = m.organization_id
WHERE m.user_id = ? AND o.sso_only = 1
FROM organizations o
WHERE o.sso_only = 1
AND (EXISTS (SELECT 1 FROM organization_members m WHERE m.organization_id = o.id AND m.user_id = ?)
OR EXISTS (SELECT 1 FROM workspace_members wm
JOIN workspaces w ON w.id = wm.workspace_id
WHERE w.organization_id = o.id AND wm.user_id = ?))
LIMIT 1
`).get(user.id) || null;
`).get(user.id, user.id) || null;
} catch (e) {
if (/no such table: (organization_members|organizations)/i.test(e.message)) return null;
if (/no such table: (organization_members|organizations|workspace_members|workspaces)/i.test(e.message)) return null;
throw e; // drift on a table that exists — fail closed; the caller refuses the login
}
}

22
server/routes/admin.js
View file

@ -3,6 +3,7 @@ const router = express.Router();
const bcrypt = require('bcryptjs');
const { v4: uuidv4 } = require('uuid');
const { db } = require('../db/database');
const oidcProviders = require('../lib/oidc-providers');
const { canAdminWorkspace } = require('../lib/permissions');
const { requirePlatformAdmin, requireAdmin } = require('../middleware/auth');
const { logActivity, getClientIp } = require('../services/activity');
@ -81,6 +82,27 @@ router.post('/users', (req, res) => {
}
}
/*
* ⚠️ And the ADDRESS's own domain, wherever it is being created.
*
* Gating only on the target workspace left the squat open through a different door: create your
* own organization, then mint `cfo@theircompany.test` into YOUR workspace. Login is refused, so
* it is not access — but the row now has a password_hash, and an SSO login will not adopt a row
* that has one. The real CFO can then never sign in through their own identity provider, and a
* password reset they CAN complete lands them at a login that refuses them. Permanent, with no
* self-service way out, for any address at any SSO-only customer.
*/
if (req.user.role !== 'platform_admin') {
let ownedBy = null;
try { ownedBy = oidcProviders.ssoOnlyForEmail(email); } catch { ownedBy = { unavailable: true }; }
if (ownedBy) {
return res.status(400).json({
error: 'That email domain uses single sign-on, so a password account cannot be created for it.',
code: 'sso_only_domain',
});
}
}
// Stamp the target workspace so the activityLogger middleware (and our
// explicit audit row) attribute to the right tenant.
req.workspaceId = ws.id;

121
server/routes/org-sso.js
View file

@ -329,24 +329,54 @@ function notifyOperatorOfRemovalRequest(req, { id, orgId, orgName, reason }) {
* turn the provider off. So the same interlock guards every route that would leave the tenant with
* nothing enforcing, and points at the request as the way through.
*/
function assertNotLastEnforcingProvider(orgId, providerId, what) {
const org = db.prepare('SELECT sso_only FROM organizations WHERE id = ?').get(orgId);
if (!org || !org.sso_only) return;
const remaining = db.prepare(`
SELECT COUNT(*) AS n FROM org_sso_domains d
/** The domains that currently ENFORCE for an organization: verified, on an enabled provider. */
function enforcingDomains(orgId, excludeProviderId = null) {
return db.prepare(`
SELECT d.domain FROM org_sso_domains d
JOIN org_sso_providers p ON p.id = d.provider_id
WHERE d.organization_id = ? AND d.verified_at IS NOT NULL AND p.enabled = 1
AND p.id != ?
`).get(orgId, providerId).n;
if (remaining > 0) return; // another provider still enforces; this one may go
const e = new Error(`Your organization requires single sign-on, so ${what} would leave nobody able to sign in. `
AND (? IS NULL OR p.id != ?)
`).all(orgId, excludeProviderId, excludeProviderId).map((r) => r.domain);
}
/*
* An SSO-only organization may not shrink the set of domains that enforce.
*
* The first version of this asked "would ANY provider still enforce?", which was the wrong
* question twice over, and a review defeated it both ways:
*
* SWAP it only fired when the resulting domain list was EMPTY, so replacing
* `acme.test` with `decoy.test` removed every proof and sailed through — two PUTs
* and the customer's domain no longer required anything.
* SIBLING it counted PROVIDERS, so with two configured you could disable the one that owns
* your staff's domain while the other, covering a domain nobody signs in at, kept the
* answer "yes, something still enforces".
*
* The question that matters is per-DOMAIN: after this change, is every domain that enforces today
* still enforcing? Losing one is exactly what needs the operator, whichever route gets you there.
*/
function assertEnforcementNotReduced(orgId, providerId, nextDomainsFor, what) {
const org = db.prepare('SELECT sso_only FROM organizations WHERE id = ?').get(orgId);
if (!org || !org.sso_only) return;
const before = new Set(enforcingDomains(orgId));
const after = new Set(enforcingDomains(orgId, providerId));
// Whatever this provider will still contribute afterwards, as VERIFIED domains only — a domain
// being re-added is unverified, so it does not count as still enforcing.
for (const d of nextDomainsFor) after.add(d);
const lost = [...before].filter((d) => !after.has(d));
if (!lost.length) return;
const e = new Error(`Your organization requires single sign-on, so ${what} would stop `
+ `${lost.join(', ')} from being covered and leave those people unable to sign in. `
+ 'Ask the people who run this server to approve stopping the requirement first.');
e.status = 409;
e.code = 'sso_only_locked';
throw e;
}
/** A provider's domains, with the DNS record each unverified one still needs. */
/** A provider's domains, with the DNS record each unverified one still needs. *//** A provider's domains, with the DNS record each unverified one still needs. */
function domainsFor(providerId) {
return db.prepare('SELECT * FROM org_sso_domains WHERE provider_id = ? ORDER BY domain').all(providerId)
.map((r) => ({
@ -504,8 +534,21 @@ router.put('/:orgId/sso/:id', requireOrgAdmin, requireVerifiedAdmin, asyncRoute(
// Disabling this provider, or removing the domains it enforces through, is the same act as
// turning the requirement off — and that needs the operator.
try {
if (enabled !== undefined && !enabled) assertNotLastEnforcingProvider(req.orgId, existing.id, 'disabling this provider');
if (domainsSupplied && !cleanDomains) assertNotLastEnforcingProvider(req.orgId, existing.id, 'removing every sign-in domain');
const willBeEnabled = enabled === undefined ? !!existing.enabled : !!enabled;
/*
* What this provider still covers afterwards: nothing if it is being disabled, otherwise the
* domains it keeps that are ALREADY verified. A domain typed back in arrives unverified and
* enforces nobody, which is precisely how the swap bypass worked.
*/
let keeps = [];
if (willBeEnabled) {
const kept = domainsSupplied ? new Set(cleanDomains.split(',').filter(Boolean)) : null;
keeps = db.prepare("SELECT domain FROM org_sso_domains WHERE provider_id = ? AND verified_at IS NOT NULL")
.all(existing.id).map((r) => r.domain)
.filter((d) => (kept ? kept.has(d) : true));
}
const what = !willBeEnabled ? 'disabling this provider' : 'changing its sign-in domains';
assertEnforcementNotReduced(req.orgId, existing.id, keeps, what);
} catch (e) {
return res.status(e.status || 400).json({ error: e.message, code: e.code });
}
@ -719,10 +762,56 @@ router.post('/:orgId/sso-only', requireOrgAdmin, requireVerifiedAdmin, (req, res
code: 'no_verified_domain',
});
}
/*
* ⚠️ Do not let the person pressing this button lock themselves out.
*
* The commonest onboarding shape is: sign up with a personal or consultancy address, create the
* organization, verify the company domain, turn this on. Enforcement then covers them (they are
* a member) while their own address is outside the verified domains — so passwords are refused
* AND their org's provider will not assert for them either, because assertions are confined to
* verified domains. There is no self-service way back: no route removes a membership, and
* password reset succeeds but login still refuses. A review walked into it on the happy path.
*
* They are told exactly which address is the problem, and what to do about it.
*/
const domains = enforcingDomains(req.orgId);
const at = String(req.user.email || '').lastIndexOf('@');
const ownDomain = at === -1 ? '' : String(req.user.email).slice(at + 1).toLowerCase().replace(/\.+$/, '');
if (!domains.includes(ownDomain)) {
return res.status(400).json({
error: `Your own address (${req.user.email}) is not at a verified domain (${domains.join(', ')}), `
+ 'so requiring single sign-on would lock you out with no way back. Verify that domain first, '
+ 'or hand ownership to someone whose address is covered.',
code: 'would_lock_out_actor',
});
}
/*
* Everyone ELSE in the same position is reported rather than refused — they may be exactly the
* contractors this is meant to shut out. But the admin must find out here, not from a support
* ticket after the fact.
*/
const stranded = db.prepare(`
SELECT DISTINCT u.email FROM users u
WHERE u.id IN (
SELECT m.user_id FROM organization_members m WHERE m.organization_id = ?
UNION
SELECT wm.user_id FROM workspace_members wm JOIN workspaces w ON w.id = wm.workspace_id
WHERE w.organization_id = ?
)
`).all(req.orgId, req.orgId)
.map((r) => r.email)
.filter((e) => {
const i = String(e).lastIndexOf('@');
return i === -1 || !domains.includes(String(e).slice(i + 1).toLowerCase().replace(/\.+$/, ''));
});
db.prepare('UPDATE organizations SET sso_only = 1 WHERE id = ?').run(req.orgId);
logActivity(req.user.id, 'org_sso_only_enabled', `org=${req.orgId}`, null, getClientIp(req));
console.log(`[org-sso] SSO-only ENABLED for org ${req.orgId} by ${req.user.email}`);
res.json({ sso_only: true });
logActivity(req.user.id, 'org_sso_only_enabled',
`org=${req.orgId} stranded=${stranded.length}`, null, getClientIp(req));
console.log(`[org-sso] SSO-only ENABLED for org ${req.orgId} by ${req.user.email}`
+ (stranded.length ? ` — ${stranded.length} member(s) outside the verified domains: ${stranded.join(', ')}` : ''));
res.json({ sso_only: true, stranded_members: stranded });
});
/*
@ -825,7 +914,7 @@ router.delete('/:orgId/sso/:id', requireOrgAdmin, requireVerifiedAdmin, (req, re
* absence of configuration — which is what made an unset GOOGLE_CLIENT_ID look like a deletion.
*/
try {
assertNotLastEnforcingProvider(req.orgId, existing.id, 'deleting this provider');
assertEnforcementNotReduced(req.orgId, existing.id, [], 'deleting this provider');
} catch (e) {
return res.status(e.status || 400).json({ error: e.message, code: e.code });
}

11
server/server.js
View file

@ -565,9 +565,18 @@ function rateLimit(windowMs, maxRequests) {
*
* Ids are collapsed to a placeholder so the SHAPE of the endpoint is the key.
*/
/*
* Order matters: the most specific shapes first, because the generic org-id fold would
* otherwise eat `sso-only` as an organization id and leave the request id free — which is
* how two of these stayed unlimited after the first attempt.
*/
.replace(/^\/api\/organizations\/sso-only\/removal-requests\/[^/]+\/[^/]+/, '/api/organizations/sso-only/removal-requests/:id/:decision')
.replace(/^\/api\/organizations\/sso-only\/removal-requests/, '/api/organizations/sso-only/removal-requests')
.replace(/^(\/api\/organizations)\/[^/]+/, '$1/:id')
.replace(/^(\/api\/organizations\/:id\/sso-only\/removal-request)\/[^/]+/, '$1/:id')
.replace(/^(\/api\/organizations\/:id\/sso)\/[^/]+/, '$1/:id')
.replace(/(\/domains)\/[^/]+/, '$1/:id')
// Anchored under the organizations mount so it cannot surprise a future limiter elsewhere.
.replace(/^(\/api\/organizations\/:id\/sso\/:id\/domains)\/[^/]+/, '$1/:id')
|| '/';
const key = getClientIp(req) + normalisedPath;
const now = Date.now();