diff --git a/nodejs/app.js b/nodejs/app.js index b64b5d4..a829835 100755 --- a/nodejs/app.js +++ b/nodejs/app.js @@ -7,6 +7,11 @@ const express = require('express'); // Set up the express app. const app = express(); +// The app always runs behind the OpenResty reverse proxy (a single hop) which +// sets X-Real-IP / X-Forwarded-For. Trust that one proxy so req.ip reflects the +// real client — needed for correct per-client rate limiting on /api/auth. +app.set('trust proxy', 1); + // Hold list of functions to run when the server is ready app.onListen = []; diff --git a/nodejs/package-lock.json b/nodejs/package-lock.json index 8c88a71..a30d2d1 100644 --- a/nodejs/package-lock.json +++ b/nodejs/package-lock.json @@ -18,6 +18,7 @@ "bootstrap": "^5.3.8", "ejs": "^6.0.1", "express": "^5.2.1", + "express-rate-limit": "^8.5.2", "extend": "^3.0.2", "jq-repeat": "^2.0.1", "jquery": "^4.0.0", @@ -917,6 +918,24 @@ "url": "https://opencollective.com/express" } }, + "node_modules/express-rate-limit": { + "version": "8.5.2", + "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-8.5.2.tgz", + "integrity": "sha512-5Kb34ipNX694DH48vN9irak1Qx30nb0PLYHXfJgw4YEjiC3ZEmZJhwOp+VfiCYwFzvFTdB9QkArYS5kXa2cx2A==", + "license": "MIT", + "dependencies": { + "ip-address": "^10.2.0" + }, + "engines": { + "node": ">= 16" + }, + "funding": { + "url": "https://github.com/sponsors/express-rate-limit" + }, + "peerDependencies": { + "express": ">= 4.11" + } + }, "node_modules/extend": { "version": "3.0.2", "resolved": "https://registry.npmjs.org/extend/-/extend-3.0.2.tgz", @@ -1229,6 +1248,15 @@ "integrity": "sha512-k/vGaX4/Yla3WzyMCvTQOXYeIHvqOKtnqBduzTHpzpQZzAskKMhZ2K+EnBiSM9zGSoIFeMpXKxa4dYeZIQqewQ==", "license": "ISC" }, + "node_modules/ip-address": { + "version": "10.2.0", + "resolved": "https://registry.npmjs.org/ip-address/-/ip-address-10.2.0.tgz", + "integrity": "sha512-/+S6j4E9AHvW9SWMSEY9Xfy66O5PWvVEJ08O0y5JGyEKQpojb0K0GKpz/v5HJ/G0vi3D2sjGK78119oXZeE0qA==", + "license": "MIT", + "engines": { + "node": ">= 12" + } + }, "node_modules/ipaddr.js": { "version": "1.9.1", "resolved": "https://registry.npmjs.org/ipaddr.js/-/ipaddr.js-1.9.1.tgz", diff --git a/nodejs/package.json b/nodejs/package.json index c2de640..88669bd 100644 --- a/nodejs/package.json +++ b/nodejs/package.json @@ -11,10 +11,10 @@ "scripts": { "start": "node ./bin/www", "dev": "npx nodemon --ignore public/ ./bin/www", - "test": "node --test test/unit/callback_queue.test.js test/unit/host_lookup.test.js test/unit/wildcard_matchany.test.js test/unit/roles.test.js test/unit/oidc.test.js test/unit/unix_socket.test.js test/integration/dns_provider.test.js", - "test:unit": "node --test test/unit/callback_queue.test.js test/unit/host_lookup.test.js test/unit/wildcard_matchany.test.js test/unit/roles.test.js test/unit/oidc.test.js test/unit/unix_socket.test.js", + "test": "node --test test/unit/callback_queue.test.js test/unit/host_lookup.test.js test/unit/wildcard_matchany.test.js test/unit/roles.test.js test/unit/oidc.test.js test/unit/safe_redirect.test.js test/unit/unix_socket.test.js test/integration/dns_provider.test.js", + "test:unit": "node --test test/unit/callback_queue.test.js test/unit/host_lookup.test.js test/unit/wildcard_matchany.test.js test/unit/roles.test.js test/unit/oidc.test.js test/unit/safe_redirect.test.js test/unit/unix_socket.test.js", "test:integration": "node --test test/integration/dns_provider.test.js", - "test:watch": "node --test --watch test/unit/callback_queue.test.js test/unit/host_lookup.test.js test/unit/wildcard_matchany.test.js test/unit/roles.test.js test/unit/oidc.test.js test/unit/unix_socket.test.js test/integration/dns_provider.test.js" + "test:watch": "node --test --watch test/unit/callback_queue.test.js test/unit/host_lookup.test.js test/unit/wildcard_matchany.test.js test/unit/roles.test.js test/unit/oidc.test.js test/unit/safe_redirect.test.js test/unit/unix_socket.test.js test/integration/dns_provider.test.js" }, "engines": { "node": ">=18.0.0" @@ -29,6 +29,7 @@ "bootstrap": "^5.3.8", "ejs": "^6.0.1", "express": "^5.2.1", + "express-rate-limit": "^8.5.2", "extend": "^3.0.2", "jq-repeat": "^2.0.1", "jquery": "^4.0.0", diff --git a/nodejs/public/lib/js/app-base.js b/nodejs/public/lib/js/app-base.js index 1ba6b28..3a7b153 100644 --- a/nodejs/public/lib/js/app-base.js +++ b/nodejs/public/lib/js/app-base.js @@ -200,6 +200,17 @@ app.auth = (function(app){ } } + // Constrain a redirect target to a same-origin absolute path. Rejects + // absolute URLs (open redirect), protocol-relative "//host" and "/\host", + // and non-path schemes like "javascript:" (XSS). Falls back to "/". + function safeInternalPath(path){ + if(typeof path !== 'string' || path.charAt(0) !== '/' + || path.charAt(1) === '/' || path.charAt(1) === '\\'){ + return '/'; + } + return path; + } + // Consume an app token handed back by the OIDC callback via the URL // fragment (#token=…&redirect=…). Stores it, strips the fragment, and // forwards to the intended page. Returns true if a token was consumed. @@ -210,7 +221,9 @@ app.auth = (function(app){ if(!token) return false; setToken(token); - var redirect = params.get('redirect') || '/'; + // redirect comes from the URL fragment (attacker-controllable); only + // allow a same-origin path so it can't become an open redirect / XSS. + var redirect = safeInternalPath(params.get('redirect') || '/'); // Drop the token from the address bar before navigating on. history.replaceState(null, '', location.pathname + location.search); window.location.href = redirect; @@ -248,7 +261,7 @@ app.auth = (function(app){ } function logInRedirect(){ - window.location.href = location.href.replace(location.origin+'/login', '') || '/' + window.location.href = safeInternalPath(location.href.replace(location.origin+'/login', '') || '/') } return { diff --git a/nodejs/routes/auth.js b/nodejs/routes/auth.js index dbffa44..9e40523 100755 --- a/nodejs/routes/auth.js +++ b/nodejs/routes/auth.js @@ -1,13 +1,25 @@ 'use strict'; const router = require('express').Router(); +const { rateLimit } = require('express-rate-limit'); const conf = require('@simpleworkjs/conf'); const { Auth } = require('../models/auth'); const { OidcState } = require('../models/oidc_state'); const oidc = require('../utils/oidc'); +const { safeInternalPath } = require('../utils/safe_redirect'); + +// Throttle unauthenticated auth endpoints (credential login + the OIDC +// handshake) to blunt brute-force / callback abuse. Keyed per IP. +const authLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, // 15 minutes + max: 60, // 60 attempts per IP per window + standardHeaders: true, + legacyHeaders: false, + message: {name: 'TooManyRequests', message: 'Too many attempts, please try again later.'}, +}); -router.post('/login', async function(req, res, next){ +router.post('/login', authLimiter, async function(req, res, next){ try{ let auth = await Auth.login(req.body); return res.json({ @@ -36,7 +48,7 @@ router.all('/logout', async function(req, res, next){ * OIDC login start: create a PKCE + state challenge, persist it (auto-expiring * via OidcState TTL), and redirect the browser to the SSO authorize endpoint. */ -router.get('/oidc/start', async function(req, res, next){ +router.get('/oidc/start', authLimiter, async function(req, res, next){ try{ if(!conf.oidc || !conf.oidc.enabled){ let error = new Error('OidcDisabled'); @@ -49,7 +61,9 @@ router.get('/oidc/start', async function(req, res, next){ await OidcState.create({ state, codeVerifier, - redirect: req.query.redirect || '/', + // Sanitize now so a hostile ?redirect= can't be stored and later + // reflected into the login page's navigation. + redirect: safeInternalPath(req.query.redirect || '/'), }); return res.redirect(oidc.buildAuthUrl(state, codeChallenge)); @@ -64,7 +78,7 @@ router.get('/oidc/start', async function(req, res, next){ * the app token back to the browser via a URL fragment for the login page to * store in localStorage. */ -router.get('/oidc/callback', async function(req, res, next){ +router.get('/oidc/callback', authLimiter, async function(req, res, next){ try{ let {code, state} = req.query; if(!code || !state){ @@ -85,7 +99,7 @@ router.get('/oidc/callback', async function(req, res, next){ let {token} = await Auth.oidcSession(identity); - let redirect = saved.redirect || '/'; + let redirect = safeInternalPath(saved.redirect || '/'); return res.redirect( `/login#token=${encodeURIComponent(token.token)}&redirect=${encodeURIComponent(redirect)}` ); diff --git a/nodejs/test/unit/safe_redirect.test.js b/nodejs/test/unit/safe_redirect.test.js new file mode 100644 index 0000000..5a449e0 --- /dev/null +++ b/nodejs/test/unit/safe_redirect.test.js @@ -0,0 +1,43 @@ +'use strict'; + +const {describe, test} = require('node:test'); +const assert = require('node:assert'); + +const {safeInternalPath} = require('../../utils/safe_redirect'); + +/** + * safeInternalPath guards the OIDC post-login redirect against open-redirect + * and script-scheme (XSS) targets. Only same-origin "/path" values pass. + */ +describe('safeInternalPath', () => { + + test('allows plain same-origin paths', () => { + assert.strictEqual(safeInternalPath('/'), '/'); + assert.strictEqual(safeInternalPath('/hosts'), '/hosts'); + assert.strictEqual(safeInternalPath('/dns?x=1'), '/dns?x=1'); + assert.strictEqual(safeInternalPath('/a/b/c#frag'), '/a/b/c#frag'); + }); + + test('rejects absolute URLs', () => { + assert.strictEqual(safeInternalPath('https://evil.com'), '/'); + assert.strictEqual(safeInternalPath('http://evil.com/x'), '/'); + }); + + test('rejects protocol-relative and backslash host tricks', () => { + assert.strictEqual(safeInternalPath('//evil.com'), '/'); + assert.strictEqual(safeInternalPath('/\\evil.com'), '/'); + }); + + test('rejects script / data schemes', () => { + assert.strictEqual(safeInternalPath('javascript:alert(1)'), '/'); + assert.strictEqual(safeInternalPath('data:text/html,