mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-15 06:43:27 -06:00
Fix a login-page dead end, an enumeration oracle, and three boot/limiter defects
From the regression sweep. The first is a genuine regression against main.
A RATE-LIMITED DISCOVERY PERMANENTLY DEAD-ENDED THE LOGIN PAGE
lookupOrgSso checked that a body PARSED, not that the request succeeded — and a 429
body is valid JSON. So `data.sso` came back undefined, the single sign-on button was
hidden, the password box restored, and the domain recorded as answered: permanently,
for the life of the page. On an SSO-only domain that is the worst outcome available —
the password box then returns 403 and the button the user is told to use is not on the
screen. Discover is 10/min per IP and one person filling in the form costs up to four
calls, so a few colleagues behind one office address is enough. The comment above that
code already claimed to prevent exactly this; it only ever covered the 5xx case.
THE SSO-ONLY REFUSAL WAS AN ACCOUNT-EXISTENCE ORACLE
403 for an address that exists, 401 for one that does not — from an endpoint whose own
lockout returns 401 specifically to avoid that. The DOMAIN check now runs BEFORE the
account lookup, so both answer identically; whether a domain uses single sign-on is
already public through /sso/discover, so it reveals nothing new. The membership-level
refusal is deliberately downgraded to the generic 401, because a distinct answer there
would put the oracle back for exactly the accounts worth enumerating.
Verified: existing and invented addresses at an SSO-only domain both 403; and on an
instance with NO SSO configured, register/login/wrong-password/unknown-address are
201/200/401/401 — the hoisted check does not touch them.
BOOT PREFLIGHT
- a cold install ran `npm ci --omit=dev` unconditionally, so a first start on a
developer machine left `npm test` broken: same class of surprise as the prune this
file already warns about, through the other branch of the same if. Now production-
only.
- two servers starting together: the loser died with ENOTEMPTY even though the tree
was complete by then. It re-checks before failing.
- the opt-out accepted only '1', unlike every other boolean the server takes.
THE LIMITER FOLD, DONE PROPERLY
Unmatched paths under /api/organizations still minted a bucket each. My first fix was a
catch-all regex — which put every unknown path in ONE bucket WITH the real endpoints,
so flooding nonsense URLs exhausted the limit for /sso-only. That trades a bypass for a
denial of service. Folding is now by explicit shape: known endpoints keep their own
keys, everything else shares a bucket kept apart from all of them.
Verified: 120 unmatched paths give 60/60 (bypass closed), and after that flood
/sso-only, /sso and /sso/:id/test all still answer 401 rather than 429 (no starvation),
while 70 hits on one real endpoint do trip its own limit. The login trailing-slash
bypass stays closed.
1609 tests, three clean runs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bvjey4FNam49MN7ybjcq6A
This commit is contained in:
ScreenTinker
parent
37e22bb773
commit
85febe05c0
|
|
@ -570,9 +570,20 @@ function setupHandlers(config, isSetup) {
|
|||
if (domain === lastDomainAsked) return;
|
||||
try {
|
||||
const res = await fetch(`/api/auth/sso/discover?email=${encodeURIComponent(email)}`);
|
||||
/*
|
||||
* ⚠️ Check the STATUS, not just that a body parsed.
|
||||
*
|
||||
* The comment below has always said a tripped rate limit must not poison the domain — and it
|
||||
* did anyway, because a 429 body is perfectly valid JSON: res.json() resolved, `data.sso`
|
||||
* came back undefined, so the single sign-on button was hidden, the password box restored,
|
||||
* and `lastDomainAsked` recorded — permanently, for the life of the page. On an SSO-only
|
||||
* domain that is the worst possible outcome: the password box the user is then offered gets
|
||||
* 403, and the button they are told to use is not on the screen. Discover is 10/min per IP,
|
||||
* so a handful of colleagues behind one office address is enough to trigger it.
|
||||
*/
|
||||
if (!res.ok) throw new Error(`discover ${res.status}`);
|
||||
const data = await res.json();
|
||||
// Remembered only after a SUCCESSFUL answer. Recording it before the fetch meant a 5xx or a
|
||||
// tripped rate limit poisoned that domain for the rest of the page's life.
|
||||
// Remembered only after a SUCCESSFUL answer.
|
||||
lastDomainAsked = domain;
|
||||
if (!data.sso) { slot.style.display = 'none'; slot.innerHTML = ''; setPasswordVisible(true); return; }
|
||||
/*
|
||||
|
|
|
|||
|
|
@ -92,7 +92,9 @@ function fail(reason, hint) {
|
|||
}
|
||||
|
||||
function preflight() {
|
||||
if (process.env.ST_SKIP_DEP_PREFLIGHT === '1') return;
|
||||
// Same spellings as every other boolean the server accepts, so an operator who writes `true`
|
||||
// does not silently get a boot that reaches for the registry anyway.
|
||||
if (['1', 'true', 'yes'].includes(String(process.env.ST_SKIP_DEP_PREFLIGHT || '').toLowerCase())) return;
|
||||
|
||||
const missing = missingDeps();
|
||||
const nodeModulesAbsent = !fs.existsSync(NODE_MODULES);
|
||||
|
|
@ -110,8 +112,17 @@ function preflight() {
|
|||
*/
|
||||
const hasLock = fs.existsSync(path.join(SERVER_DIR, 'package-lock.json'));
|
||||
if (hasLock && nodeModulesAbsent) {
|
||||
// Nothing installed, so `ci` has nothing to destroy and gives a reproducible tree.
|
||||
run(['ci', '--omit=dev', '--no-audit', '--no-fund'], 'installing');
|
||||
/*
|
||||
* Nothing installed, so `ci` has nothing to destroy and gives a reproducible tree.
|
||||
*
|
||||
* `--omit=dev` ONLY when this is plainly a production boot. Applying it unconditionally
|
||||
* meant a cold start on a developer machine installed 307 packages and left `npm test`
|
||||
* broken — js-yaml, puppeteer-core and socket.io-client absent — which is the same class of
|
||||
* surprise as the prune this file already warns about, arriving through the other branch of
|
||||
* the same `if`.
|
||||
*/
|
||||
const prod = process.env.NODE_ENV === 'production';
|
||||
run(prod ? ['ci', '--omit=dev', '--no-audit', '--no-fund'] : ['ci', '--no-audit', '--no-fund'], 'installing');
|
||||
} else {
|
||||
/*
|
||||
* ⚠️ Install ONLY what is missing, by name, and never `--omit=dev` on a populated tree.
|
||||
|
|
@ -126,8 +137,18 @@ function preflight() {
|
|||
run(['install', '--no-save', '--no-audit', '--no-fund', ...missing], 'installing missing packages');
|
||||
}
|
||||
} catch (e) {
|
||||
fail(`could not install dependencies: ${e && e.message}`,
|
||||
'Run `npm ci --omit=dev` in the server directory, or check network access to the npm registry.');
|
||||
/*
|
||||
* An install can fail because ANOTHER server started at the same moment and won the race —
|
||||
* observed as `ENOTEMPTY … rename node_modules/fs-extra`. The tree is complete by the time we
|
||||
* see the error, so exiting here killed a process that had nothing wrong with it. Re-check
|
||||
* before giving up; only a genuinely incomplete tree is fatal.
|
||||
*/
|
||||
const afterFailure = missingDeps();
|
||||
if (afterFailure.length) {
|
||||
fail(`could not install dependencies: ${e && e.message}`,
|
||||
'Run `npm ci --omit=dev` in the server directory, or check network access to the npm registry.');
|
||||
}
|
||||
console.warn(`[preflight] install reported an error but the tree is complete (${e && e.message}) — continuing.`);
|
||||
}
|
||||
const still = missingDeps();
|
||||
if (still.length) {
|
||||
|
|
|
|||
|
|
@ -188,6 +188,30 @@ router.post('/login', (req, res) => {
|
|||
const { email, password } = req.body;
|
||||
if (!email || !password) return res.status(400).json({ error: 'Email and password required' });
|
||||
|
||||
/*
|
||||
* The DOMAIN check runs BEFORE the account lookup, deliberately.
|
||||
*
|
||||
* Answering `403 sso_required` only for addresses that exist turned this endpoint into an
|
||||
* account-existence oracle: a wrong password got 403 for a real address and 401 for an invented
|
||||
* one. Whether a domain uses single sign-on is already public — /sso/discover answers it for
|
||||
* anyone — so refusing on the domain alone reveals nothing new, and it reveals it identically
|
||||
* for addresses that exist and addresses that do not.
|
||||
*/
|
||||
const domainEnforced = (() => {
|
||||
try { return oidcProviders.ssoOnlyForEmail(email); } catch (e) {
|
||||
console.error('[login] SSO-only status unavailable, refusing password login:', e && e.message);
|
||||
return { unavailable: true };
|
||||
}
|
||||
})();
|
||||
if (domainEnforced) {
|
||||
logFailedLogin(email, getClientIp(req), 'Password login refused: domain requires SSO');
|
||||
return res.status(403).json({
|
||||
error: 'Your organization requires single sign-on. Use the single sign-on button to continue.',
|
||||
code: 'sso_required',
|
||||
sso_start: '/api/auth/sso/start',
|
||||
});
|
||||
}
|
||||
|
||||
const user = db.prepare('SELECT * FROM users WHERE email = ? AND auth_provider = ?').get(email.toLowerCase(), 'local');
|
||||
if (!user) {
|
||||
logFailedLogin(email, getClientIp(req), 'User not found');
|
||||
|
|
@ -226,12 +250,17 @@ router.post('/login', (req, res) => {
|
|||
enforced = { unavailable: true };
|
||||
}
|
||||
if (enforced) {
|
||||
logFailedLogin(email, getClientIp(req), 'Password login refused: organization requires SSO');
|
||||
return res.status(403).json({
|
||||
error: 'Your organization requires single sign-on. Use the single sign-on button to continue.',
|
||||
code: 'sso_required',
|
||||
sso_start: '/api/auth/sso/start',
|
||||
});
|
||||
/*
|
||||
* Reached only when the ADDRESS's domain is not enforced but the user is a MEMBER of an
|
||||
* organization that requires single sign-on — an off-domain contractor, say. The generic 401
|
||||
* is deliberate: a distinct answer here would put the existence oracle back, for exactly the
|
||||
* accounts an attacker would most like to enumerate. These people cannot sign in by any
|
||||
* route (their domain is not verified, so their org's provider will not assert for them
|
||||
* either), which is why enabling SSO-only now names them to the admin up front instead of
|
||||
* leaving them to discover it here.
|
||||
*/
|
||||
logFailedLogin(email, getClientIp(req), 'Password login refused: member of an SSO-only organization');
|
||||
return res.status(401).json({ error: 'Invalid email or password' });
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -533,6 +533,47 @@ app.use('/socket.io-client', express.static(
|
|||
// safe because the callback runs at request time, which is a subtle thing to depend on.
|
||||
const limiterTelemetry = require('./lib/limiter-telemetry');
|
||||
const rateLimits = new Map();
|
||||
/*
|
||||
* The bucket key is the SHAPE of the endpoint, never the spelling the caller chose.
|
||||
*
|
||||
* Two failures drove this. Express routes non-strictly, so `/api/auth/login/` was a different key
|
||||
* and bought a fresh ten password attempts. And `/api/organizations/<id>/...` carries three
|
||||
* caller-chosen segments, so every request minted its own bucket — 120 calls with unique ids gave
|
||||
* zero 429s against the limit that exists to bound outbound OIDC discovery and live DNS lookups.
|
||||
*
|
||||
* ⚠️ Fold by EXPLICIT shape, not with a clever catch-all. A single regex that collapsed "anything
|
||||
* else" put every unknown path in one bucket WITH the real endpoints, so flooding nonsense URLs
|
||||
* exhausted the limit for `/sso-only` — trading a bypass for a denial of service. Known shapes get
|
||||
* their own keys; everything else shares one, separate from all of them.
|
||||
*/
|
||||
const LIMIT_PATH_SHAPES = [
|
||||
[/^\/api\/auth\/oidc\/[^/]+\/(start|callback)$/, (m) => `/api/auth/oidc/:slug/${m[1]}`],
|
||||
[/^\/api\/organizations\/sso-only\/removal-requests\/[^/]+\/[^/]+$/, () => '/api/organizations/sso-only/removal-requests/:id/:decision'],
|
||||
[/^\/api\/organizations\/sso-only\/removal-requests$/, () => '/api/organizations/sso-only/removal-requests'],
|
||||
[/^\/api\/organizations\/[^/]+\/sso-only\/removal-request\/[^/]+$/, () => '/api/organizations/:id/sso-only/removal-request/:id'],
|
||||
[/^\/api\/organizations\/[^/]+\/sso-only$/, () => '/api/organizations/:id/sso-only'],
|
||||
[/^\/api\/organizations\/[^/]+\/sso\/[^/]+\/domains\/[^/]+\/verify$/, () => '/api/organizations/:id/sso/:id/domains/:domain/verify'],
|
||||
[/^\/api\/organizations\/[^/]+\/sso\/[^/]+\/test$/, () => '/api/organizations/:id/sso/:id/test'],
|
||||
[/^\/api\/organizations\/[^/]+\/sso\/[^/]+$/, () => '/api/organizations/:id/sso/:id'],
|
||||
[/^\/api\/organizations\/[^/]+\/sso$/, () => '/api/organizations/:id/sso'],
|
||||
];
|
||||
|
||||
function canonicalLimitPath(rawPath) {
|
||||
const p = rawPath
|
||||
.replace(/\/{2,}/g, '/') // collapse doubled separators
|
||||
.replace(/\/+$/, '') // a trailing slash is the same endpoint
|
||||
.toLowerCase()
|
||||
|| '/';
|
||||
for (const [re, to] of LIMIT_PATH_SHAPES) {
|
||||
const m = p.match(re);
|
||||
if (m) return to(m);
|
||||
}
|
||||
// Unrecognised, but still under a mount whose ids are caller-chosen: one shared bucket, kept
|
||||
// apart from every real endpoint so flooding it cannot starve them.
|
||||
if (p.startsWith('/api/organizations/')) return '/api/organizations/:unmatched';
|
||||
return p;
|
||||
}
|
||||
|
||||
function rateLimit(windowMs, maxRequests) {
|
||||
return (req, res, next) => {
|
||||
// #100: key on the FULL path, not req.path. These limiters are mounted via
|
||||
|
|
@ -550,34 +591,7 @@ function rateLimit(windowMs, maxRequests) {
|
|||
* slug is folded out too: one bucket per IP per ENDPOINT, not per spelling of it.
|
||||
*/
|
||||
const rawPath = (req.originalUrl || req.url || req.path).split('?')[0];
|
||||
const normalisedPath = rawPath
|
||||
.replace(/\/{2,}/g, '/') // collapse doubled separators
|
||||
.replace(/\/+$/, '') // ignore a trailing slash
|
||||
.toLowerCase()
|
||||
.replace(/^(\/api\/auth\/oidc)\/[^/]+/, '$1') // the slug is not a distinct endpoint
|
||||
/*
|
||||
* ⚠️ /api/organizations/<orgId>/... carries THREE caller-chosen segments (org id, provider
|
||||
* id, domain). Folding only the OIDC slug left this mount with a fresh bucket per request:
|
||||
* a review measured 120 unauthenticated calls with unique org ids and got zero 429s, while
|
||||
* the same path 120 times correctly produced 60. The limiter runs before requireAuth, so an
|
||||
* anonymous caller could mint buckets for free — and this limit exists specifically to bound
|
||||
* outbound OIDC discovery and live DNS lookups.
|
||||
*
|
||||
* 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')
|
||||
// Anchored under the organizations mount so it cannot surprise a future limiter elsewhere.
|
||||
.replace(/^(\/api\/organizations\/:id\/sso\/:id\/domains)\/[^/]+/, '$1/:id')
|
||||
|| '/';
|
||||
const normalisedPath = canonicalLimitPath(rawPath);
|
||||
const key = getClientIp(req) + normalisedPath;
|
||||
const now = Date.now();
|
||||
const windowStart = now - windowMs;
|
||||
|
|
|
|||
Loading…
Reference in a new issue