theta42/proxy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Address CodeQL findings on the OIDC auth flow

Browse Source 
- Open redirect / client-side XSS (app-base.js): the post-login `redirect`
  read from the URL fragment was assigned straight to window.location. Add a
  same-origin guard (safeInternalPath) that rejects absolute URLs,
  protocol-relative "//host"/"/\\host", and scheme targets like
  "javascript:". Apply it in consumeTokenFragment and logInRedirect.
- Server-side defense in depth: sanitize `redirect` when storing OidcState
  and when building the callback fragment (utils/safe_redirect.js, shared +
  unit-tested).
- Missing rate limiting: throttle the unauthenticated auth endpoints
  (/login, /oidc/start, /oidc/callback) with express-rate-limit (60/IP/15m).
  Set `trust proxy: 1` so req.ip reflects the real client behind OpenResty.

Adds test/unit/safe_redirect.test.js; unit suite 77 pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
William Mantly
2026-07-10 13:22:12 -04:00
parent 08bef1bd00
commit cff816fa06
7 changed files with 137 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
nodejs/app.js
+5
View File
@@ -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 = [];
nodejs/package-lock.json
Generated
+28
View File
@@ -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",
nodejs/package.json
+4 -3
View File
@@ -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",
nodejs/public/lib/js/app-base.js
+15 -2
View File
@@ -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 {
nodejs/routes/auth.js
+19 -5
View File
@@ -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)}`
);
nodejs/test/unit/safe_redirect.test.js
+43
View File
@@ -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,<script>'), '/');
});
test('rejects non-path and non-string input', () => {
assert.strictEqual(safeInternalPath('hosts'), '/'); // no leading slash
assert.strictEqual(safeInternalPath(''), '/');
assert.strictEqual(safeInternalPath(undefined), '/');
assert.strictEqual(safeInternalPath(null), '/');
assert.strictEqual(safeInternalPath({}), '/');
});
});
nodejs/utils/safe_redirect.js
+23
View File
@@ -0,0 +1,23 @@
'use strict';
/**
* Constrain a post-login redirect target to a same-origin path.
*
* Rejects anything that could leave the site or execute script:
* - absolute URLs ("https://evil.com") -> not a "/" path
* - protocol-relative ("//evil.com", "/\\evil.com") -> host takeover
* - scheme targets ("javascript:...", "data:...") -> XSS
* Anything not a plain "/path" falls back to "/".
*
* The browser has its own copy of this in public/lib/js/app-base.js; keep the
* two in sync.
*/
function safeInternalPath(path){
if(typeof path !== 'string' || path.charAt(0) !== '/'
|| path.charAt(1) === '/' || path.charAt(1) === '\\'){
return '/';
}
return path;
}
module.exports = {safeInternalPath};