mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 13:53:12 -06:00
d26aaebef6
Five reviewers went at the two SSO commits. Three of them independently
demonstrated a full account takeover, and it was the same defect each time.
TAKEOVER. An org admin supplies the issuer and client_id, so they control that
identity provider completely and can mint an id_token asserting ANY email with
email_verified:true — including a platform_admin's. Every cryptographic check
passed honestly, because the attacker IS the issuer. upsertFederatedUser then
re-pointed the existing account at whichever provider spoke last, because the
only guard was `password_hash IS NULL` — and every SSO-created account has a
null password. Sessions were issued as the victim, and the victim's own login
then failed forever with subject_mismatch.
The rule came from the old Google handler, where it was safe: only the operator
could add a provider. Making providers customer-configurable turned it into a
takeover primitive and the assumption was not re-examined. Now an org provider
may only assert emails inside the domains it registered, and may never adopt an
account another provider established.
REMOTE CRASH, unauthenticated. The state comparison guarded on UTF-16 character
length while Buffer.from produces UTF-8 bytes, so a state of 43 characters
containing one multi-byte character reached timingSafeEqual with mismatched
buffers and threw — inside an async handler, which Express does not catch, which
server.js turns into process.exit. One request per restart killed any instance
with SSO enabled. Compared as bytes now, and /api/auth/oidc gained a rate limit.
LOGIN CSRF. The callback returned the session token in the URL fragment, so a
crafted link installed an ATTACKER'S token and silently signed the victim into
their account. The token now goes in a one-shot httpOnly cookie exchanged at
POST /sso/claim, which a link cannot forge.
FRONTEND, dead on arrival twice over. login.js used `await` in a non-async
function — a SyntaxError that takes the WHOLE app down, since app.js imports it
statically and there is no bundler. And `esc` was never imported, so the org-SSO
button could never render; the ReferenceError was swallowed by the catch written
for network failures. Both slipped through because `node --check` parses these
files as CommonJS and exits 0 on a broken module. The correct check is
`node --input-type=module --check`, and all four frontend files now pass it.
PUBLIC EMAIL DOMAINS cannot be claimed. A tenant had claimed gmail.com in
review, after which every Gmail user typing their address was offered "sign in
with your organization" pointing at that tenant's infrastructure — phishing from
this product's own login page. server/lib/public-email-domains.js.
MICROSOFT multi-tenant is refused rather than silently broken. `common` metadata
advertises the literal template {tenantid}, so the issuer never matches and
every login already failed; and loosening that check is nOAuth. A tenant GUID is
now required, with a loud warning at boot.
SSRF: https only, loopback/RFC1918/link-local refused, redirects not followed,
and the test endpoint no longer echoes upstream status for a caller-supplied
jwks_uri (it was a readable internal port scanner).
Also: an omitted email_verified was accepted (the comment already said it should
not be); the domain-uniqueness check raced an 8s network call before its insert
and is now inside the transaction; same-org duplicate domains were allowed and
made routing depend on table-scan order; routing is now ordered; a client secret
that cannot be decrypted fails closed instead of silently downgrading to a public
client; SSO audit rows were writing the org id into the deviceId column; and
/sso/start was capped at 10/min per IP, which would 429 the 11th employee behind
a corporate NAT.
Adds per-provider editing in the org admin UI (replace-only secrets — never
returned, blank means keep, explicit clear) and a Test button that checks
discovery, endpoints and signing keys while stating plainly that it cannot
verify the client ID, the secret, or the redirect URI registration.
⚠️ STILL MISSING: domain-ownership verification. A claimed domain means "nobody
else had claimed it", not "they own it". DNS TXT proof is the remaining control.
1582 tests pass. New regression tests cover the takeover confinement, ordering,
fail-closed secrets, the Microsoft refusal and the public-domain blocklist.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bvjey4FNam49MN7ybjcq6A
|
||
|---|---|---|
| .. | ||
| config | ||
| db | ||
| lib | ||
| middleware | ||
| player | ||
| routes | ||
| scripts | ||
| services | ||
| test | ||
| ws | ||
| .gitignore | ||
| config.js | ||
| package-lock.json | ||
| package.json | ||
| server.js | ||
| smoke-ui.js | ||
| version.js | ||