From 983bee31b75633055cf24ce3ac45b727881d165a Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Tue, 11 Aug 2026 07:23:28 -0500 Subject: [PATCH] SSO-only: close the backdoor, the unilateral disable, and the fresh-install fail-open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01Bvjey4FNam49MN7ybjcq6A --- server/db/database.js | 22 ++++++++++++- server/lib/oidc-providers.js | 57 +++++++++++++++++++++++++++++++-- server/routes/admin.js | 26 +++++++++++++++ server/routes/auth.js | 16 ++++++++- server/routes/org-sso.js | 45 ++++++++++++++++++++++++++ server/test/admin-users.test.js | 6 +++- server/test/oidc-sso.test.js | 4 +-- 7 files changed, 169 insertions(+), 7 deletions(-) diff --git a/server/db/database.js b/server/db/database.js index 1c6ff1c..bd5a087 100644 --- a/server/db/database.js +++ b/server/db/database.js @@ -482,7 +482,6 @@ const migrations = [ * self-service switch gets flipped under pressure. So removal goes through the operator: the * request is recorded here and a platform admin has to approve it. */ - "ALTER TABLE organizations ADD COLUMN sso_only INTEGER NOT NULL DEFAULT 0", `CREATE TABLE IF NOT EXISTS org_sso_only_requests ( id TEXT PRIMARY KEY, organization_id TEXT NOT NULL, @@ -959,6 +958,27 @@ migrateGroupSchedules(); // updates workspace_id. ensureMultitenancyMigration(); +/* + * `organizations.sso_only` — added HERE, not in the migrations array above. + * + * That array runs BEFORE ensureMultitenancyMigration(), which is what creates the organizations + * table, so on a fresh install the ALTER hit a table that did not exist yet: `[migrate] FAILED … + * no such table: organizations`, one console.error among ~85 migration lines. The instance then + * ran its entire first boot with the SSO settings screen 500ing and — far worse — + * ssoOnlyForEmail() catching `no such column` and returning "not SSO-only", which is password + * login proceeding for an organization that had switched it off. It self-healed on the second + * boot, which is exactly what makes it easy to miss. + */ +try { + const orgCols = db.prepare('PRAGMA table_info(organizations)').all().map((c) => c.name); + if (orgCols.length && !orgCols.includes('sso_only')) { + db.exec('ALTER TABLE organizations ADD COLUMN sso_only INTEGER NOT NULL DEFAULT 0'); + console.log('[migrate] added organizations.sso_only'); + } +} catch (e) { + console.error('[migrate] could not add organizations.sso_only:', e.message); +} + // Phase 2.2c migration: backfill content_folders.workspace_id from owner's // default workspace. The ALTER lives in the migrations array above; this // one-shot populates the column for any rows that pre-date it. diff --git a/server/lib/oidc-providers.js b/server/lib/oidc-providers.js index a28a598..e4b51e8 100644 --- a/server/lib/oidc-providers.js +++ b/server/lib/oidc-providers.js @@ -321,11 +321,64 @@ function ssoOnlyForEmail(email) { WHERE d.domain = ? AND d.verified_at IS NOT NULL AND p.enabled = 1 AND o.sso_only = 1 `).get(domain) || null; } catch (e) { - if (/no such table|no such column/i.test(e.message)) return null; + /* + * ⚠️ FAIL CLOSED. This used to swallow `no such column` and return null — and null means "not + * SSO-only", i.e. password login proceeds. It is the single control stopping a password from + * bypassing a customer's identity provider, so a schema problem must never be the thing that + * quietly switches it off. The sibling forEmail() carries the same warning for the same reason. + * + * `no such table` on the DOMAINS table is different and genuinely means "this instance has no + * per-org SSO at all", so it stays a null. + */ + /* + * "The feature is not installed" and "the schema drifted" are different answers. + * + * A missing per-org SSO table, or no organizations table at all, means this instance has no + * per-organization SSO — nothing is being bypassed, so null is correct and a single-tenant + * install must keep working. A missing sso_only COLUMN on a table that does exist is drift, and + * that is the case that must never quietly answer "not required". + */ + if (/no such table: (org_sso_domains|org_sso_providers|organizations|organization_members)/i.test(e.message)) return null; + console.error('[sso] could not determine SSO-only status, refusing password login:', e.message); throw e; } } +/** + * Must THIS USER use single sign-on? + * + * ⚠️ Membership, not just the address. ssoOnlyForEmail() answers about a DOMAIN, and a review used + * that gap to walk straight in: any account in the tenant whose address sits outside the verified + * domains kept password login — a contractor, an MSP, the one address nobody remembered. Worse, it + * could be manufactured on demand, because an org admin can create a local password account at any + * address and bind it to their workspace. Enforcing on the domain alone protects the domain; it + * does not protect the ORGANIZATION, which is what the setting claims to do. + * + * So both are asked: the address's domain (which catches people who are not members yet) and every + * organization the user actually belongs to. + */ +function ssoOnlyForUser(user) { + if (!user) return null; + const byDomain = ssoOnlyForEmail(user.email); + if (byDomain) return byDomain; + + const conn = db(); + if (!conn) return null; + try { + 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 + LIMIT 1 + `).get(user.id) || null; + } catch (e) { + if (/no such table: (organization_members|organizations)/i.test(e.message)) return null; + throw e; // drift on a table that exists — fail closed; the caller refuses the login + } +} + module.exports = { - list, get, publicList, getOrgProvider, ownerOf, forEmail, ssoOnlyForEmail, DEFAULT_SCOPES, SLUG_RE, + list, get, publicList, getOrgProvider, ownerOf, forEmail, + ssoOnlyForEmail, ssoOnlyForUser, DEFAULT_SCOPES, SLUG_RE, }; diff --git a/server/routes/admin.js b/server/routes/admin.js index ebf05a3..7701935 100644 --- a/server/routes/admin.js +++ b/server/routes/admin.js @@ -55,6 +55,32 @@ router.post('/users', (req, res) => { if (!canAdminWorkspace(db, req.user, ws)) { return res.status(403).json({ error: 'Admin access required' }); } + /* + * ⚠️ An SSO-only organization must not have password accounts minted into it. + * + * This route creates a LOCAL account with an admin-chosen password, and it accepts any address — + * so on a tenant that requires single sign-on it was a one-call backdoor: create + * `contractor@somewhere-else.test` bound to the workspace, log in with the password, and every + * control the customer turned SSO-only on for is behind you. A review did exactly that, and the + * account it created could then mint another. + * + * platform_admin keeps the ability, because that is the operator break-glass — the same + * exemption the login gate makes, for the same reason. + */ + if (req.user.role !== 'platform_admin' && ws.organization_id) { + // The table is absent on a single-tenant install; that simply means no organization requires + // single sign-on, so creation proceeds. + let org = null; + try { org = db.prepare('SELECT sso_only, name FROM organizations WHERE id = ?').get(ws.organization_id); } + catch { org = null; } + if (org && org.sso_only) { + return res.status(400).json({ + error: `${org.name || 'This organization'} requires single sign-on, so password accounts cannot be created. Invite the person through your identity provider instead.`, + code: 'sso_only_org', + }); + } + } + // Stamp the target workspace so the activityLogger middleware (and our // explicit audit row) attribute to the right tenant. req.workspaceId = ws.id; diff --git a/server/routes/auth.js b/server/routes/auth.js index 8bf3693..4e2bc46 100644 --- a/server/routes/auth.js +++ b/server/routes/auth.js @@ -188,7 +188,21 @@ router.post('/login', (req, res) => { * already answered `sso: true` publicly, so naming it reveals nothing new. */ if (user.role !== 'platform_admin') { - const enforced = oidcProviders.ssoOnlyForEmail(user.email); + /* + * A throw here means we could not determine the answer (schema drift, a broken read). Treat + * that as "SSO is required" rather than letting a 500 escape or, worse, letting the login + * through: the whole point of this gate is that a password must not be an alternative way in, + * and "we could not check" is not "there is nothing to check". + */ + let enforced = null; + try { + // By MEMBERSHIP as well as by domain — an account inside the tenant at an outside address + // was the demonstrated way around this. + enforced = oidcProviders.ssoOnlyForUser(user); + } catch (e) { + console.error('[login] SSO-only status unavailable, refusing password login:', e && e.message); + enforced = { unavailable: true }; + } if (enforced) { logFailedLogin(email, getClientIp(req), 'Password login refused: organization requires SSO'); return res.status(403).json({ diff --git a/server/routes/org-sso.js b/server/routes/org-sso.js index f6a71b7..fd857b4 100644 --- a/server/routes/org-sso.js +++ b/server/routes/org-sso.js @@ -316,6 +316,36 @@ function notifyOperatorOfRemovalRequest(req, { id, orgId, orgName, reason }) { } } +/* + * An SSO-only organization may not dismantle its own enforcement sideways. + * + * `sso_only` is honoured only while a provider is enabled AND a domain is verified, so disabling + * the provider, clearing its domains, or deleting it all switch enforcement off — with `sso_only` + * still reading `true`, no request filed and the operator never told. A review used each of the + * three, and 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. + * + * That made the approval workflow decorative: anyone who could file a request could instead just + * 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 + 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. ` + + '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. */ function domainsFor(providerId) { return db.prepare('SELECT * FROM org_sso_domains WHERE provider_id = ? ORDER BY domain').all(providerId) @@ -471,6 +501,15 @@ router.put('/:orgId/sso/:id', requireOrgAdmin, requireVerifiedAdmin, asyncRoute( * returns the secret, so a UI that round-trips a form would otherwise blank it on every save — * the classic way a settings page silently breaks the thing it is editing. */ + // 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'); + } catch (e) { + return res.status(e.status || 400).json({ error: e.message, code: e.code }); + } + let newlyClaimed = []; const secretEnc = clientSecret === undefined ? existing.client_secret_enc : (clientSecret === '' ? null : secretbox.encrypt(String(clientSecret))); @@ -785,6 +824,12 @@ router.delete('/:orgId/sso/:id', requireOrgAdmin, requireVerifiedAdmin, (req, re * Both are handled here, at the moment the intent is known, rather than inferred later from the * 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'); + } catch (e) { + return res.status(e.status || 400).json({ error: e.message, code: e.code }); + } + const freed = db.transaction(() => { const domains = db.prepare('DELETE FROM org_sso_domains WHERE provider_id = ?').run(existing.id).changes; // Back to a local account, so the owner can recover it by proving the mailbox — strictly diff --git a/server/test/admin-users.test.js b/server/test/admin-users.test.js index 4a690bf..0abf80d 100644 --- a/server/test/admin-users.test.js +++ b/server/test/admin-users.test.js @@ -72,7 +72,11 @@ db.exec(` ); CREATE TABLE organizations ( id TEXT PRIMARY KEY, name TEXT NOT NULL, - owner_user_id TEXT, plan_id TEXT, subscription_status TEXT + owner_user_id TEXT, plan_id TEXT, subscription_status TEXT, + -- Mirrors the real schema. Login refuses when it cannot determine whether an organization + -- requires single sign-on, so a fixture missing this column fails closed — correctly, but it + -- is the fixture that is wrong, not the guard. + sso_only INTEGER NOT NULL DEFAULT 0 ); CREATE TABLE activity_log ( id INTEGER PRIMARY KEY AUTOINCREMENT, diff --git a/server/test/oidc-sso.test.js b/server/test/oidc-sso.test.js index dd1fe04..d5fd37a 100644 --- a/server/test/oidc-sso.test.js +++ b/server/test/oidc-sso.test.js @@ -768,7 +768,7 @@ test('the login gate exempts platform_admin, and that exemption is deliberate', * must not be "tidied away" by someone who reads it as a convenience. */ const src = fs.readFileSync(require.resolve('../routes/auth.js'), 'utf8'); - assert.match(src, /user\.role !== 'platform_admin'[\s\S]{0,200}ssoOnlyForEmail/, - 'the break-glass exemption must guard the ssoOnlyForEmail check'); + assert.match(src, /user\.role !== 'platform_admin'[\s\S]{0,600}ssoOnlyForUser/, + 'the break-glass exemption must guard the SSO-only check'); assert.match(src, /code: 'sso_required'/, 'and the refusal must be distinguishable from a bad password'); });