diff --git a/frontend/js/views/login.js b/frontend/js/views/login.js index 35f330e..e31965d 100644 --- a/frontend/js/views/login.js +++ b/frontend/js/views/login.js @@ -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; } /* diff --git a/server/lib/preflight-deps.js b/server/lib/preflight-deps.js index 12d5b82..9be7249 100644 --- a/server/lib/preflight-deps.js +++ b/server/lib/preflight-deps.js @@ -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) { diff --git a/server/routes/auth.js b/server/routes/auth.js index 514873f..27338e8 100644 --- a/server/routes/auth.js +++ b/server/routes/auth.js @@ -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' }); } } diff --git a/server/server.js b/server/server.js index 9672acc..b3cf602 100644 --- a/server/server.js +++ b/server/server.js @@ -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//...` 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//... 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;