From 1ccf7c3747f41c95a681db902c263abebd44e92c Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 15:03:23 +0000 Subject: [PATCH] fix(security): throttle failed logins; add security headers to the API Login throttling: the local and LDAP credential logins had no limit (50 wrong passwords against admin took under 5 seconds, and the right one still worked straight after). A new in-memory throttle counts failed sign-ins in a 15-minute window and answers 429 with Retry-After: - 5 failures per client IP + username (case-insensitive), and - 50 failures per client IP across usernames. There is deliberately no per-username lock, which would let anyone lock an account (e.g. admin) out by failing to sign in as it. A successful sign-in clears that client's count. The limits and window are configurable with ANGLES_LOGIN_MAX_FAILURES, ANGLES_LOGIN_MAX_FAILURES_PER_IP and ANGLES_LOGIN_LOCKOUT_MINUTES. Behind a reverse proxy, TRUST_PROXY=true is needed for the client's real address to be used. Headers: every API response now carries X-Content-Type-Options: nosniff, X-Frame-Options: DENY, Referrer-Policy: no-referrer and Content-Security-Policy "default-src 'none'; frame-ancestors 'none'", and X-Powered-By is no longer sent. The Swagger UI keeps its scripts (CSP frame-ancestors only). The HTML build report gets its own policy: inline styles, data: images, and only its one script, by a per-request nonce. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KgQXSUjuLVXmLWxobnMfSf --- app/assets/report/index.pug | 3 +- app/controllers/build.controller.js | 14 ++- app/routes/auth.routes.js | 9 +- app/utils/login-throttle.js | 118 ++++++++++++++++++ app/utils/security-headers.js | 37 ++++++ server.js | 4 + test/security-hardening.tests.js | 186 ++++++++++++++++++++++++++++ 7 files changed, 366 insertions(+), 5 deletions(-) create mode 100644 app/utils/login-throttle.js create mode 100644 app/utils/security-headers.js create mode 100644 test/security-hardening.tests.js diff --git a/app/assets/report/index.pug b/app/assets/report/index.pug index 08bacec..9228777 100644 --- a/app/assets/report/index.pug +++ b/app/assets/report/index.pug @@ -277,5 +277,6 @@ html(lang="en") img(alt="") span.viewer-caption - script + //- The nonce matches the Content-Security-Policy the API serves the report with. + script(nonce=nonce) include report.js diff --git a/app/controllers/build.controller.js b/app/controllers/build.controller.js index 62be9bf..e8565f5 100644 --- a/app/controllers/build.controller.js +++ b/app/controllers/build.controller.js @@ -1,4 +1,5 @@ const { validationResult } = require('express-validator'); +const crypto = require('crypto'); const mongoose = require('mongoose'); const debug = require('debug'); @@ -20,6 +21,7 @@ const { handleError, } = require('../exceptions/errors.js'); const authMiddleware = require('../utils/auth-middleware.js'); +const { reportPolicy } = require('../utils/security-headers.js'); const log = debug('build:controller'); @@ -280,8 +282,16 @@ exports.getReport = (req, res) => { const query = { build: mongoose.Types.ObjectId(build._id) }; return Screenshot.find(query).lean(); }) - // eslint-disable-next-line global-require - .then((screenshots) => res.render('index', { build, screenshots, moment: require('moment') })) + .then((screenshots) => { + // A fresh nonce per report: the report's one inline script carries it, so nothing + // else injected into the page could run when it is opened from the API. + const nonce = crypto.randomBytes(16).toString('base64'); + res.set('Content-Security-Policy', reportPolicy(nonce)); + return res.render('index', { + // eslint-disable-next-line global-require + build, screenshots, nonce, moment: require('moment'), + }); + }) .catch((err) => handleError(err, res)); }; diff --git a/app/routes/auth.routes.js b/app/routes/auth.routes.js index 1b1e03b..148271d 100644 --- a/app/routes/auth.routes.js +++ b/app/routes/auth.routes.js @@ -3,6 +3,7 @@ const passport = require('passport'); const debug = require('debug'); const authConfig = require('../../config/auth.config.js'); const featureConfig = require('../../config/feature.config.js'); +const loginThrottle = require('../utils/login-throttle.js'); const { isProviderReady, getReadyProvider, @@ -38,7 +39,7 @@ module.exports = (app, path) => { .exists({ checkFalsy: true }) .isLength({ min: 1, max: 100 }) .withMessage('Password is required.'), - ], (req, res, next) => { + ], loginThrottle.guard, (req, res, next) => { const errors = validationResult(req); if (!errors.isEmpty()) { return res.status(422).json({ errors: errors.array() }); @@ -49,8 +50,10 @@ module.exports = (app, path) => { return passport.authenticate('local', (err, user, info) => { if (err) return next(err); if (!user) { + loginThrottle.recordFailure(req.ip, req.body.username); return res.status(401).json({ error: info.message || 'Login failed' }); } + loginThrottle.recordSuccess(req.ip, req.body.username); return req.logIn(user, (loginErr) => { if (loginErr) return next(loginErr); return res.json({ @@ -95,7 +98,7 @@ module.exports = (app, path) => { .exists({ checkFalsy: true }) .isLength({ min: 1, max: 200 }) .withMessage('Password is required.'), - ], ssoGuard, (req, res, next) => { + ], loginThrottle.guard, ssoGuard, (req, res, next) => { const errors = validationResult(req); if (!errors.isEmpty()) { return res.status(422).json({ errors: errors.array() }); @@ -112,8 +115,10 @@ module.exports = (app, path) => { return res.status(503).json({ error: 'The directory could not be reached.' }); } if (!user) { + loginThrottle.recordFailure(req.ip, req.body.username); return res.status(401).json({ error: (info && info.message) || 'Login failed' }); } + loginThrottle.recordSuccess(req.ip, req.body.username); return req.logIn(user, (loginErr) => { if (loginErr) return next(loginErr); return res.json({ diff --git a/app/utils/login-throttle.js b/app/utils/login-throttle.js new file mode 100644 index 0000000..0789f8a --- /dev/null +++ b/app/utils/login-throttle.js @@ -0,0 +1,118 @@ +const debug = require('debug'); + +const log = debug('auth:throttle'); + +/* + * Slows down password guessing against the credential logins (local and LDAP). + * + * Two limits, both counting failed sign-ins within a sliding window: + * + * - per client IP + username (default 5): stops one client guessing one account's + * password. Reached, that pair is refused until the window passes. + * - per client IP (default 50): stops one client spraying a few guesses at many + * accounts. + * + * There is deliberately no limit on a username alone. It would also stop a distributed + * guess at one account, but it would let anyone lock any user (an admin, say) out simply + * by failing to sign in as them from a few addresses. + * + * A successful sign-in clears that client's count for the username. Counts are kept in + * memory, so each API instance limits on its own and a restart clears them. + * + * Behind a reverse proxy, set TRUST_PROXY=true so the client's address (from + * X-Forwarded-For) is used; otherwise every request appears to come from the proxy and + * the per-IP limit applies to everyone at once. + */ + +const positiveInt = (value, fallback) => { + const parsed = parseInt(value, 10); + return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback; +}; + +const defaults = () => ({ + maxFailuresPerAccount: positiveInt(process.env.ANGLES_LOGIN_MAX_FAILURES, 5), + maxFailuresPerIp: positiveInt(process.env.ANGLES_LOGIN_MAX_FAILURES_PER_IP, 50), + windowMs: positiveInt(process.env.ANGLES_LOGIN_LOCKOUT_MINUTES, 15) * 60 * 1000, +}); + +let settings = defaults(); + +// key -> timestamps (ms) of failures still inside the window +const failures = new Map(); +// Bound the memory a flood of distinct addresses/usernames can take. +const MAX_KEYS = 10000; + +const accountKey = (ip, username) => `account:${ip}:${String(username || '').toLowerCase().trim()}`; +const ipKey = (ip) => `ip:${ip}`; + +const recent = (key, now) => { + const timestamps = (failures.get(key) || []).filter((t) => now - t < settings.windowMs); + if (timestamps.length) failures.set(key, timestamps); + else failures.delete(key); + return timestamps; +}; + +const prune = (now) => { + if (failures.size < MAX_KEYS) return; + [...failures.keys()].forEach((key) => recent(key, now)); + // Still full of live entries: drop the oldest keys (Map keeps insertion order). + const excess = failures.size - MAX_KEYS + 1; + [...failures.keys()].slice(0, Math.max(0, excess)).forEach((key) => failures.delete(key)); +}; + +// Seconds until the oldest counted failure leaves the window. +const retryAfterSeconds = (timestamps, now) => Math.max( + 1, + Math.ceil((timestamps[0] + settings.windowMs - now) / 1000), +); + +/** + * Whether this client may attempt to sign in as `username` now. Returns + * `{ allowed: true }` or `{ allowed: false, retryAfter }` (seconds). + */ +const check = (ip, username, now = Date.now()) => { + const perAccount = recent(accountKey(ip, username), now); + if (perAccount.length >= settings.maxFailuresPerAccount) { + return { allowed: false, retryAfter: retryAfterSeconds(perAccount, now) }; + } + const perIp = recent(ipKey(ip), now); + if (perIp.length >= settings.maxFailuresPerIp) { + return { allowed: false, retryAfter: retryAfterSeconds(perIp, now) }; + } + return { allowed: true }; +}; + +const recordFailure = (ip, username, now = Date.now()) => { + prune(now); + [accountKey(ip, username), ipKey(ip)].forEach((key) => { + failures.set(key, [...recent(key, now), now]); + }); + log('Failed sign-in for %s from %s', username, ip); +}; + +const recordSuccess = (ip, username) => { + failures.delete(accountKey(ip, username)); +}; + +/** + * Express middleware for a credential login route: refuses with 429 (and Retry-After) + * while the client is over a limit for the posted username. + */ +const guard = (req, res, next) => { + const result = check(req.ip, req.body && req.body.username); + if (result.allowed) return next(); + res.set('Retry-After', String(result.retryAfter)); + return res.status(429).json({ + error: `Too many failed sign-in attempts. Try again in ${Math.ceil(result.retryAfter / 60)} minute(s).`, + }); +}; + +module.exports = { + guard, + check, + recordFailure, + recordSuccess, + // Tests only: replace the limits, and forget every recorded failure. + configure: (overrides) => { settings = { ...defaults(), ...overrides }; }, + reset: () => { failures.clear(); settings = defaults(); }, +}; diff --git a/app/utils/security-headers.js b/app/utils/security-headers.js new file mode 100644 index 0000000..e403e86 --- /dev/null +++ b/app/utils/security-headers.js @@ -0,0 +1,37 @@ +/* + * Security headers for every API response. + * + * The API answers with JSON and files, so the default policy forbids loading anything at + * all (`default-src 'none'`): if a response is ever rendered as a page - an error echoing + * input, a file opened directly - nothing in it can run or load. `frame-ancestors 'none'` + * and X-Frame-Options stop any API page being framed. + * + * Two kinds of response render real pages and set their own policy instead: + * - the Swagger UI under /api-docs, which needs its own scripts and styles; + * - the HTML build report (see buildController.getReport), which uses a nonce. + * Attachment files set a sandbox policy of their own as well. + */ +const API_POLICY = "default-src 'none'; frame-ancestors 'none'"; +const SWAGGER_POLICY = "frame-ancestors 'none'"; + +const securityHeaders = (req, res, next) => { + res.set('X-Content-Type-Options', 'nosniff'); + res.set('X-Frame-Options', 'DENY'); + res.set('Referrer-Policy', 'no-referrer'); + res.set('Content-Security-Policy', req.path.startsWith('/api-docs') ? SWAGGER_POLICY : API_POLICY); + next(); +}; + +/* + * The policy for the HTML build report: its own inline styles, its one inline script (by + * nonce) and its embedded data: screenshots, and nothing else - no network requests. + */ +const reportPolicy = (nonce) => [ + "default-src 'none'", + "style-src 'unsafe-inline'", + `script-src 'nonce-${nonce}'`, + 'img-src data:', + "frame-ancestors 'none'", +].join('; '); + +module.exports = { securityHeaders, reportPolicy }; diff --git a/server.js b/server.js index c9f4d90..1264bbc 100644 --- a/server.js +++ b/server.js @@ -18,6 +18,7 @@ const { configureProviders } = require('./app/utils/passport-setup.js'); const authSettingsService = require('./app/utils/auth-settings-service.js'); const featureSettingsService = require('./app/utils/feature-settings-service.js'); const adminSeedService = require('./app/utils/admin-seed-service.js'); +const { securityHeaders } = require('./app/utils/security-headers.js'); // mongo db config const dbConfig = require('./config/database.config.js'); @@ -28,6 +29,8 @@ const mongoURL = process.env.MONGO_URL || dbConfig.url; // create express app const PORT = process.env.PORT || 3000; const app = express(); +// Don't advertise the framework. +app.disable('x-powered-by'); const corsOptionsDelegate = (req, callback) => { const origin = req.header('Origin'); @@ -60,6 +63,7 @@ const corsOptionsDelegate = (req, callback) => { }; app.use(cors(corsOptionsDelegate)); +app.use(securityHeaders); app.use(compression()); // Request instrumentation for the Prometheus endpoint. Registered before the routes so it diff --git a/test/security-hardening.tests.js b/test/security-hardening.tests.js new file mode 100644 index 0000000..c2a7d8c --- /dev/null +++ b/test/security-hardening.tests.js @@ -0,0 +1,186 @@ +/** + * Login throttling and security headers. + * + * Throttling: repeated failed sign-ins from one client are refused with 429 per IP and + * username, and per IP across usernames, without ever locking an account for everyone. + * Headers: every API response forbids framing and loading anything; the HTML report runs + * only its own nonced script. + */ +const request = require('supertest'); +const should = require('should'); +const bcrypt = require('bcryptjs'); +const app = require('../server.js'); +const User = require('../app/models/user.js'); +const Build = require('../app/models/build.js'); +const Environment = require('../app/models/environment.js'); +const { Team } = require('../app/models/team.js'); +const buildUtils = require('../app/utils/build-utils.js'); +const loginThrottle = require('../app/utils/login-throttle.js'); + +const baseUrl = '/rest/api/v1.0/'; +const PASSWORD = 'unit-testing-ShPass1!'; +const USERS = ['unit-testing-sh-alice', 'unit-testing-sh-bob', 'unit-testing-sh-carol', 'unit-testing-sh-dave']; + +const login = (username, password) => new Promise((resolve, reject) => { + request(app).post(`${baseUrl}auth/login`).send({ username, password }) + .end((err, res) => (err ? reject(err) : resolve(res))); +}); + +describe('Security hardening Tests', () => { + let team; + let environment; + let build; + + before(async () => { + await Promise.all([ + User.deleteMany({ username: /^unit-testing-sh-/ }).exec(), + Team.deleteMany({ name: /^unit-testing-sh-/ }).exec(), + Environment.deleteMany({ name: /^unit-testing-sh-/ }).exec(), + ]); + const hash = await bcrypt.hash(PASSWORD, 10); + team = await new Team({ name: 'unit-testing-sh-team', components: [{ name: 'sh-component' }] }).save(); + environment = await new Environment({ name: 'unit-testing-sh-env' }).save(); + await User.create(USERS.map((username) => ({ + username, password: hash, role: 'user', teams: [team._id], + }))); + build = await new Build({ + name: 'unit-testing-sh-build', + team, + environment, + status: buildUtils.executionStates[0], + component: team.components[0]._id, + suites: [], + result: new Map(buildUtils.defaultResultMap), + }).save(); + }); + + after(async () => { + loginThrottle.reset(); + await Promise.all([ + Build.deleteMany({ _id: build._id }).exec(), + User.deleteMany({ username: /^unit-testing-sh-/ }).exec(), + Team.deleteMany({ name: /^unit-testing-sh-/ }).exec(), + Environment.deleteMany({ name: /^unit-testing-sh-/ }).exec(), + ]); + }); + + describe('Login throttling', () => { + beforeEach(() => loginThrottle.reset()); + + it('refuses a client after 5 failed sign-ins for one account, even with the right password', async () => { + for (let attempt = 0; attempt < 5; attempt += 1) { + // eslint-disable-next-line no-await-in-loop + const res = await login(USERS[0], 'wrong-password'); + should(res.status).equal(401); + } + const blocked = await login(USERS[0], PASSWORD); + should(blocked.status).equal(429); + should(Number(blocked.headers['retry-after'])).be.above(0); + should(blocked.body.error).match(/Too many failed sign-in attempts/); + }); + + it('does not lock the account for other clients, nor other accounts for this client', async () => { + for (let attempt = 0; attempt < 5; attempt += 1) { + // eslint-disable-next-line no-await-in-loop + await login(USERS[0], 'wrong-password'); + } + // Same client, another account: still allowed. + should((await login(USERS[1], PASSWORD)).status).equal(200); + // Another client, the same account: still allowed. + should(loginThrottle.check('203.0.113.7', USERS[0]).allowed).equal(true); + }); + + it('counts the username case-insensitively', async () => { + for (let attempt = 0; attempt < 5; attempt += 1) { + // eslint-disable-next-line no-await-in-loop + await login(USERS[0].toUpperCase(), 'wrong-password'); + } + should((await login(USERS[0], PASSWORD)).status).equal(429); + }); + + it('clears the count after a successful sign-in', async () => { + for (let attempt = 0; attempt < 4; attempt += 1) { + // eslint-disable-next-line no-await-in-loop + await login(USERS[2], 'wrong-password'); + } + should((await login(USERS[2], PASSWORD)).status).equal(200); + for (let attempt = 0; attempt < 4; attempt += 1) { + // eslint-disable-next-line no-await-in-loop + should((await login(USERS[2], 'wrong-password')).status).equal(401); + } + should((await login(USERS[2], PASSWORD)).status).equal(200); + }); + + it('refuses a client that fails across many accounts', async () => { + loginThrottle.configure({ maxFailuresPerIp: 3 }); + await login(USERS[0], 'wrong-password'); + await login(USERS[1], 'wrong-password'); + await login(USERS[2], 'wrong-password'); + should((await login(USERS[3], PASSWORD)).status).equal(429); + }); + + it('forgets failures once the window has passed', () => { + loginThrottle.configure({ maxFailuresPerAccount: 2, windowMs: 1000 }); + const start = 1000000; + loginThrottle.recordFailure('198.51.100.1', 'someone', start); + loginThrottle.recordFailure('198.51.100.1', 'someone', start + 10); + should(loginThrottle.check('198.51.100.1', 'someone', start + 20).allowed).equal(false); + should(loginThrottle.check('198.51.100.1', 'someone', start + 1011).allowed).equal(true); + }); + + it('also guards the LDAP credential login', async () => { + loginThrottle.configure({ maxFailuresPerAccount: 1 }); + loginThrottle.recordFailure('::ffff:127.0.0.1', 'directory-user'); + loginThrottle.recordFailure('127.0.0.1', 'directory-user'); + const res = await new Promise((resolve, reject) => { + request(app).post(`${baseUrl}auth/sso/any-provider/login`) + .send({ username: 'directory-user', password: 'x' }) + .end((err, response) => (err ? reject(err) : resolve(response))); + }); + should(res.status).equal(429); + }); + }); + + describe('Security headers', () => { + let agent; + + before(async () => { + loginThrottle.reset(); + agent = request.agent(app); + const res = await agent.post(`${baseUrl}auth/login`).send({ username: USERS[0], password: PASSWORD }); + should(res.status).equal(200); + }); + + it('API responses forbid framing, sniffing and loading anything', async () => { + const res = await request(app).get(`${baseUrl}auth/config`); + should(res.headers['content-security-policy']).equal("default-src 'none'; frame-ancestors 'none'"); + should(res.headers['x-frame-options']).equal('DENY'); + should(res.headers['x-content-type-options']).equal('nosniff'); + should(res.headers['referrer-policy']).equal('no-referrer'); + should(res.headers).not.have.property('x-powered-by'); + }); + + it('error responses carry the same headers', async () => { + const res = await request(app).get(`${baseUrl}build`); + should(res.status).equal(401); + should(res.headers['content-security-policy']).equal("default-src 'none'; frame-ancestors 'none'"); + }); + + it('the Swagger UI can still load its own scripts, but cannot be framed', async () => { + const res = await request(app).get('/api-docs/'); + should(res.headers['content-security-policy']).equal("frame-ancestors 'none'"); + }); + + it('the HTML report only runs its own script, by nonce', async () => { + const res = await agent.get(`${baseUrl}build/${build._id}/report`); + should(res.status).equal(200); + const csp = res.headers['content-security-policy']; + const nonce = /'nonce-([^']+)'/.exec(csp)[1]; + should(csp).match(/default-src 'none'/); + should(csp).match(/img-src data:/); + should(res.text).containEql(`