mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 22:03:13 -06:00
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bvjey4FNam49MN7ybjcq6A
139 lines
6 KiB
JavaScript
139 lines
6 KiB
JavaScript
'use strict';
|
|
|
|
/*
|
|
* Proving that a tenant controls a sign-in domain.
|
|
*
|
|
* Per-organization SSO routes everyone at a domain to that organization's identity provider. That
|
|
* is exactly right when the organization owns the domain and an account-takeover primitive when it
|
|
* does not — and typing a domain into a form is not ownership. A review demonstrated the whole
|
|
* chain: claim a company's domain, sign in as a named address there, and the real owner is left
|
|
* unable to reach an account bearing their own address.
|
|
*
|
|
* DNS is the check, because control of a domain's DNS is what "owning a domain" means in the only
|
|
* sense that matters here. It is also the mechanism every other vendor uses, so the instructions
|
|
* are already familiar to the person who has to follow them.
|
|
*
|
|
* TWO RECORD FORMS, both at the same name, because organizations differ in what their DNS lets
|
|
* them add — some providers refuse TXT at a subdomain, some refuse CNAME anywhere useful:
|
|
*
|
|
* _screentinker-verify.example.com. IN TXT "st-verify=<token>"
|
|
* _screentinker-verify.example.com. IN CNAME <token>.verify.screentinker.com.
|
|
*
|
|
* A dedicated `_`-prefixed name is used rather than the apex on purpose: an apex TXT record sits
|
|
* alongside SPF and DMARC, where a careless edit breaks mail, and it is the one record set an
|
|
* administrator is most reluctant to touch.
|
|
*/
|
|
|
|
const dns = require('dns').promises;
|
|
const crypto = require('crypto');
|
|
|
|
const RECORD_PREFIX = '_screentinker-verify';
|
|
const TXT_PREFIX = 'st-verify=';
|
|
const CNAME_SUFFIX = '.verify.screentinker.com';
|
|
|
|
// A DNS answer that never arrives must not hold an HTTP request open. The resolver's own retries
|
|
// sit under this, so it is a ceiling on the whole lookup rather than on one query.
|
|
const LOOKUP_TIMEOUT_MS = 5000;
|
|
|
|
/*
|
|
* How long an UNVERIFIED claim is worth anything.
|
|
*
|
|
* A claim reserves the domain so two tenants cannot race it — but a reservation that never lapses
|
|
* is squatting with extra steps: type a company's domain, prove nothing, and hold it against its
|
|
* real owner forever. Eight hours is comfortably longer than a DNS change takes to publish and
|
|
* propagate, and short enough that an unprovable claim is gone by the next working day.
|
|
*
|
|
* The token dies with the claim. Trying again mints a NEW token, so an old record left in DNS from
|
|
* a lapsed attempt proves nothing, and a domain that changed hands cannot be verified with the
|
|
* previous holder's value.
|
|
*
|
|
* A VERIFIED domain is not affected — proof already happened, and re-proving on a timer would log
|
|
* out a customer over a DNS edit made months later.
|
|
*/
|
|
const CLAIM_TTL_S = 8 * 60 * 60;
|
|
|
|
/** True when an unverified claim has run out of time and no longer reserves anything. */
|
|
function isClaimExpired(row, nowS = Math.floor(Date.now() / 1000)) {
|
|
if (!row || row.verified_at) return false;
|
|
return (Number(row.token_issued_at) || 0) + CLAIM_TTL_S <= nowS;
|
|
}
|
|
|
|
/** Tokens are compared, so they are random and long enough that guessing is not a strategy. */
|
|
const newToken = () => crypto.randomBytes(16).toString('hex');
|
|
|
|
const recordName = (domain) => `${RECORD_PREFIX}.${domain}`;
|
|
|
|
/** Exactly what the admin has to publish — shown in the UI, so it is built in one place. */
|
|
function instructions(domain, token) {
|
|
return {
|
|
record_name: recordName(domain),
|
|
txt_value: `${TXT_PREFIX}${token}`,
|
|
cname_value: `${token}${CNAME_SUFFIX}`,
|
|
};
|
|
}
|
|
|
|
function withTimeout(promise, ms) {
|
|
let timer;
|
|
const timeout = new Promise((_, reject) => {
|
|
timer = setTimeout(() => reject(new Error('DNS lookup timed out')), ms);
|
|
});
|
|
return Promise.race([promise, timeout]).finally(() => clearTimeout(timer));
|
|
}
|
|
|
|
/*
|
|
* Look for the proof.
|
|
*
|
|
* Both record types are queried together and either one is enough. NXDOMAIN and "no such record"
|
|
* are ordinary answers here — the overwhelmingly common case is an admin checking before the record
|
|
* has propagated — so they are reported as "not found yet", never as an error to be alarmed by.
|
|
*
|
|
* ⚠️ Resolution uses the system resolver, which is the same view of DNS the operator already
|
|
* trusts. A tenant that can poison that resolver can forge a proof, but a tenant that can do that
|
|
* has already won something larger.
|
|
*/
|
|
async function check(domain, token) {
|
|
const name = recordName(domain);
|
|
const wantTxt = `${TXT_PREFIX}${token}`;
|
|
const wantCname = `${token}${CNAME_SUFFIX}`;
|
|
|
|
const results = await Promise.allSettled([
|
|
withTimeout(dns.resolveTxt(name), LOOKUP_TIMEOUT_MS),
|
|
withTimeout(dns.resolveCname(name), LOOKUP_TIMEOUT_MS),
|
|
]);
|
|
|
|
const [txtRes, cnameRes] = results;
|
|
|
|
if (txtRes.status === 'fulfilled') {
|
|
// resolveTxt returns arrays of string chunks — a long value is split, so join before comparing.
|
|
for (const chunks of txtRes.value) {
|
|
if (chunks.join('').trim() === wantTxt) return { ok: true, via: 'TXT' };
|
|
}
|
|
}
|
|
if (cnameRes.status === 'fulfilled') {
|
|
for (const target of cnameRes.value) {
|
|
// DNS names are case-insensitive and may or may not carry the root dot.
|
|
if (target.replace(/\.$/, '').toLowerCase() === wantCname.toLowerCase()) return { ok: true, via: 'CNAME' };
|
|
}
|
|
}
|
|
|
|
// Nothing matched. Say which of the two failure shapes it is, because the fixes differ: a record
|
|
// that is absent needs publishing, a record that is present but wrong needs correcting.
|
|
const found = [];
|
|
if (txtRes.status === 'fulfilled') found.push(...txtRes.value.map((c) => `TXT ${c.join('')}`));
|
|
if (cnameRes.status === 'fulfilled') found.push(...cnameRes.value.map((c) => `CNAME ${c}`));
|
|
|
|
if (found.length) {
|
|
return { ok: false, error: `${name} exists but does not match. Found: ${found.join('; ')}` };
|
|
}
|
|
|
|
const timedOut = results.some((r) => r.status === 'rejected' && /timed out/i.test(r.reason && r.reason.message));
|
|
if (timedOut) return { ok: false, error: 'the DNS lookup timed out — try again shortly' };
|
|
|
|
return { ok: false, error: `no ${RECORD_PREFIX} record found for ${domain} yet (DNS can take a few minutes)` };
|
|
}
|
|
|
|
module.exports = {
|
|
check, instructions, newToken, recordName, isClaimExpired,
|
|
CLAIM_TTL_S, RECORD_PREFIX, TXT_PREFIX, CNAME_SUFFIX,
|
|
};
|