From d4b8d7dad4ec9823adead57791c3581323baf5d3 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Mon, 10 Aug 2026 19:23:46 -0500 Subject: [PATCH] SSO: prove domain ownership by DNS, and fix what the second review found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second review pass, run against the previous commit, found four blockers — two of them introduced by the fixes in that commit. It also confirmed the original account takeover is closed: a hostile IdP with real TLS, discovery, JWKS and RS256 driving the real routers now stops at domain_not_allowed, and all 16 bypass variants are refused. DOMAIN OWNERSHIP (the root cause, not the symptom) A claimed domain used to mean "nobody else claimed it". It now means the organization published a record in that domain's own DNS — TXT or CNAME, at a dedicated _screentinker-verify name rather than the apex, where an edit would sit beside SPF. - an unverified domain routes NOBODY and cannot be asserted; it reserves the name - an unverified claim LAPSES after 8 hours, so a domain cannot be held against its real owner, and lapsing rotates the token so a record left over from an abandoned attempt cannot satisfy a later claim - a verified domain never expires — re-proving on a timer would log a customer out over a DNS edit made months later - routing and confinement read the VERIFIED set only, never the typed column - configuring SSO now requires a verified email address - platform admins are emailed when a domain is claimed; nothing is ever sent to the claimed domain, which would let any tenant make this product email third parties Instance-wide providers are exempt from all of it: they are the operator's own configuration and keep the trust they have always had. BLOCKERS FROM THE REVIEW - two unauthenticated remote crashes, both one request, both "async handler throws before its try": `Cookie: st_oidc_tx=%` (unguarded decodeURIComponent) and the fail-closed secret added last commit, which turned a JWT_SECRET rotation into a permanent crash loop. Fixed the CLASS with asyncRoute() rather than the instances. - the SSRF guard was bypassable via IPv4-mapped IPv6 ([::ffff:127.0.0.1]) and also refused every host beginning "fc"/"fd" (fcm.googleapis.com). Addresses are now parsed and compared by RANGE. 42 cases verified. - the takeover fix had NO test — the test named after it asserted two struct fields and passed with the guard deleted. The decision is now a pure function and four mutations were confirmed to turn the suite red. - the PUT path never received the TOCTOU fix, so two orgs could end up holding one domain and forEmail handed routing to the attacker's older row. ALSO - linking compared slugs, so an org could never rotate its own IdP, and fell open on an empty auth_provider. It now asks which ORGANIZATION owns the slug. - an account stranded by a deleted provider can be reclaimed by password reset — proof of the mailbox, which is stronger than the IdP assertion that created it. - /sso/claim accepted a pre-TOTP mfa_pending token and returned the full user row; it now takes a purpose-built 120s claim token with a pinned algorithm and typ. - the rate limiter keyed on a caller-controlled path, so a trailing slash bought a fresh bucket — a real login brute-force bypass. - domain_not_allowed and account_exists_other_provider rendered as "please try again", advice that can never work. - malformed asserted addresses are refused rather than trimmed into shape. - dead config (microsoftTenantId defaulted to 'common', which the provider code now refuses) and the orphaned google-auth-library dependency removed. 1591 tests pass. Domain lifecycle verified end to end against a running server. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Bvjey4FNam49MN7ybjcq6A --- README.md | 36 +++- frontend/js/i18n/en.js | 14 ++ frontend/js/views/login.js | 3 +- frontend/js/views/settings.js | 67 +++++++ server/config.js | 13 +- server/db/database.js | 34 ++++ server/lib/domain-verify.js | 138 ++++++++++++++ server/lib/oidc-providers.js | 88 +++++++-- server/lib/oidc.js | 71 ++++++- server/package-lock.json | 174 +---------------- server/package.json | 5 +- server/routes/auth.js | 212 ++++++++++++++++++--- server/routes/org-sso.js | 348 +++++++++++++++++++++++++++++----- server/server.js | 18 +- server/test/oidc-sso.test.js | 183 ++++++++++++++++-- 15 files changed, 1112 insertions(+), 292 deletions(-) create mode 100644 server/lib/domain-verify.js diff --git a/README.md b/README.md index 84c116c..a331c18 100644 --- a/README.md +++ b/README.md @@ -404,10 +404,10 @@ https://yourdomain.com/api/auth/oidc//callback The slug is generated rather than chosen so two customers cannot collide on — or guess — each other's. A domain may be claimed by only one organization; a second claim is refused. -⚠️ **A provider may only authenticate emails inside the domains it registered.** An organization +⚠️ **A provider may only authenticate emails inside the domains it has VERIFIED.** An organization supplies its own issuer and client ID, so it controls that identity provider completely and could otherwise assert any address at all — including another company's, or an administrator's. Confining -assertions to registered domains is what makes customer-configurable SSO safe to offer. +assertions to verified domains is what makes customer-configurable SSO safe to offer. ⚠️ **Public email providers cannot be claimed.** `gmail.com`, `outlook.com`, `yahoo.com`, `icloud.com` and the rest of the consumer mailboxes are refused (`server/lib/public-email-domains.js`). Claiming @@ -415,10 +415,34 @@ one would offer every Gmail user a "sign in with your organization" button point infrastructure — phishing launched from this product's own login page — and would let one account deny a public domain to everyone else. -⚠️ **Domain ownership is not yet verified.** A claimed domain currently means "no other organization -had claimed it", not "this organization owns it". The blocklist above removes the mass-abuse case, -but proof of control — a DNS TXT record, or a challenge to `postmaster@` — is still the missing -control, and until it exists a domain claim should be treated as a support-reviewable action. +### Proving a domain + +A claimed domain **routes nobody and authenticates nobody until DNS proves the organization controls +it.** Typing a domain into a form reserves the name and nothing more. + +Publish either record — whichever the domain's DNS will accept — then press **Verify**: + +``` +_screentinker-verify.example.com. IN TXT "st-verify=" +_screentinker-verify.example.com. IN CNAME .verify.screentinker.com. +``` + +The token is unique per domain, so publishing one proof cannot be replayed to claim a second. A +dedicated `_`-prefixed name is used rather than the apex, where a careless edit would sit alongside +SPF and DMARC and break mail. + +**An unverified claim lapses after 8 hours**, and lapsing rotates the token. This is what stops +squatting: a tenant cannot type a company's domain and hold it against the real owner, and a record +left in DNS from an abandoned attempt cannot satisfy a later claim. A verified domain never expires — +re-proving on a timer would log a customer out over a DNS edit made months afterwards. + +Platform admins are emailed whenever a domain is claimed. Verification is what makes an unowned +claim worthless; the notification is what makes an attempt visible. Nothing is ever sent to the +claimed domain itself — that would let any tenant make this product email third parties. + +⚠️ **Instance-wide providers are exempt from all of the above.** `GOOGLE_CLIENT_ID`, `OIDC_*` and +friends are the operator's own configuration, are not domain-restricted, and require no verification. +Domain proof exists because per-organization providers are supplied by CUSTOMERS. Signing in through an organization's provider makes the user a member of that organization (`org_member`). Existing members keep whatever role they already have — logging in never promotes or diff --git a/frontend/js/i18n/en.js b/frontend/js/i18n/en.js index 24a7d8b..ad9fe56 100644 --- a/frontend/js/i18n/en.js +++ b/frontend/js/i18n/en.js @@ -151,6 +151,16 @@ export default { 'sso.disable': 'Disable', 'sso.disabled': 'disabled', 'sso.domains_label': 'Email domains', + 'sso.domains_heading': 'Sign-in domains', + 'sso.domain_verified': 'verified', + 'sso.domain_pending': 'not verified — routes nobody yet', + 'sso.unverified_warning': 'Some domains are not verified yet, so nobody is routed to this provider by email address.', + 'sso.verify_now': 'Verify', + 'sso.verifying': 'Checking DNS…', + 'sso.verify_failed': 'Could not verify that domain.', + 'sso.domain_verified_toast': '{domain} is verified.', + 'sso.dns_instructions': 'Publish ONE of these records in this domain\u2019s DNS, then click Verify. Claims expire after 8 hours.', + 'sso.dns_or_cname': 'or, if your DNS will not take a TXT record there:', 'sso.callback_label': 'Redirect URI — add this to your provider', 'sso.f_name': 'Display name', 'sso.f_issuer': 'Issuer URL', @@ -187,6 +197,10 @@ export default { 'auth.sso_err_account_exists_local': 'An account with this email already exists. Sign in with your password, then link your provider in Settings.', 'auth.sso_err_subject_mismatch': 'This email is already linked to a different account at your provider.', 'auth.sso_err_server_error': 'Something went wrong completing sign-in.', + // Both of these used to fall through to "please try again", which is advice that can never work: + // retrying is exactly what will not help, and the user needs to be told who to talk to instead. + 'auth.sso_err_domain_not_allowed': 'Your organization has not verified that email domain for sign-in. Ask your administrator to verify it in ScreenTinker.', + 'auth.sso_err_account_exists_other_provider': 'An account with this email already exists and signs in through a different provider. Use that provider, or ask your administrator.', 'auth.signin_microsoft': 'Sign in with Microsoft', 'auth.back_to_signin': 'Back to Sign In', // TOTP 2FA challenge (second login step) diff --git a/frontend/js/views/login.js b/frontend/js/views/login.js index 4665492..0975205 100644 --- a/frontend/js/views/login.js +++ b/frontend/js/views/login.js @@ -581,7 +581,8 @@ function setupHandlers(config, isSetup) { // rather than failing silently, which is how the previous implementation behaved on every click. const known = ['expired', 'bad_state', 'no_code', 'no_email', 'email_unverified', 'verification_failed', 'provider_refused', 'provider_unavailable', 'unknown_provider', - 'registration_disabled', 'account_exists_local', 'subject_mismatch', 'server_error']; + 'registration_disabled', 'account_exists_local', 'subject_mismatch', 'server_error', + 'domain_not_allowed', 'account_exists_other_provider']; const key = known.includes(ssoError) ? `auth.sso_err_${ssoError}` : 'auth.sso_failed'; showToast(t(key), 'error'); } diff --git a/frontend/js/views/settings.js b/frontend/js/views/settings.js index 6da3f79..e92ef60 100644 --- a/frontend/js/views/settings.js +++ b/frontend/js/views/settings.js @@ -702,6 +702,9 @@ export async function render(container) { ${p.enabled ? '' : ` — ${esc(t('sso.disabled'))}`}
${esc(p.issuer)}
${esc(t('sso.domains_label'))}: ${esc(p.email_domains || '—')}
+ ${(p.domains || []).some((d) => !d.verified) + ? `
⚠️ ${esc(t('sso.unverified_warning'))}
` + : ''}
@@ -721,6 +724,32 @@ export async function render(container) { + + ${(p.domains || []).length ? ` +
+
${esc(t('sso.domains_heading'))}
+ ${p.domains.map((d) => ` +
+
+
${esc(d.domain)} + ${d.verified + ? ` — ${esc(t('sso.domain_verified'))}` + : ` — ${esc(t('sso.domain_pending'))}`} +
+ ${d.verified ? '' : ``} +
+ ${d.verified ? '' : ` +
${esc(t('sso.dns_instructions'))}
+ ${esc(d.record_name)} TXT ${esc(d.txt_value)} +
${esc(t('sso.dns_or_cname'))}
+ ${esc(d.record_name)} CNAME ${esc(d.cname_value)} + ${d.last_error ? `
${esc(d.last_error)}
` : ''}`} +
+
`).join('')} +
` : ''} +