From 1dd35dd913fab15d111ddda4c0d68b2fc4c89774 Mon Sep 17 00:00:00 2001 From: cmyers-mieweb Date: Thu, 30 Jul 2026 10:29:24 -0400 Subject: [PATCH 1/3] fix(create-a-container): loosen in-app rate limit, defer to load balancer The previous limiter counted 4xx/5xx (excluding 404) as failures with a tiny budget of 10 per 5 minutes. With the SPA + OIDC flow, 401 and other 4xx responses are a normal part of the auth handshake, so legitimate users were throttled. Replace it with a lenient safety-net limiter that counts every request (1000/min per IP) and leave real rate limiting to the load balancer. --- create-a-container/app.js | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/create-a-container/app.js b/create-a-container/app.js index e94018cc..3f8546c3 100644 --- a/create-a-container/app.js +++ b/create-a-container/app.js @@ -98,17 +98,19 @@ function buildApp({ app.use(express.static('public')); - // We rate limit unsuccessful (4xx/5xx statuses, excluding 404) to only 10 per 5 minutes, this - // should allow legitimate users a few tries to login or experiment without - // allowing bad-actors to abuse requests. 404s are excluded because browsers - // (especially Safari) automatically request favicon/apple-touch-icon paths that - // don't exist, and those harmless misses should not burn the rate-limit budget. + // Real rate limiting is expected to be enforced at the load balancer. This + // in-app limiter is only a lenient safety net against a single runaway client + // hammering the server. It intentionally counts every request (successful or + // not) rather than classifying by status code: with the SPA + OIDC flow, 401 + // (and other 4xx) responses are a normal part of the auth handshake, so + // treating them as failures throttled legitimate users. The ceiling is set + // high enough that normal browsing never trips it, so it needs no tuning. if (rateLimit) { app.use(RateLimit({ - windowMs: 5 * 60 * 1000, - max: 10, - skipSuccessfulRequests: true, - requestWasSuccessful: (req, res) => res.statusCode < 400 || res.statusCode === 404, + windowMs: 60 * 1000, + max: 1000, + standardHeaders: true, + legacyHeaders: false, })); } From 512bed5b6b65b2040b935a1391c178b216c50e92 Mon Sep 17 00:00:00 2001 From: cmyers-mieweb Date: Thu, 30 Jul 2026 10:52:09 -0400 Subject: [PATCH 2/3] refactor(create-a-container): remove in-app rate limiting Rate limiting is enforced at the load balancer. The in-app limiter added little value and its status-code classification throttled legitimate SPA + OIDC users on expected 401 responses. Drop express-rate-limit, the buildApp rateLimit option, and its test overrides. --- create-a-container/app.js | 20 +------------ .../middlewares/__tests__/mcp-proxy.test.js | 1 - create-a-container/package-lock.json | 28 ------------------- create-a-container/package.json | 1 - create-a-container/tests/helpers/app.js | 3 -- 5 files changed, 1 insertion(+), 52 deletions(-) diff --git a/create-a-container/app.js b/create-a-container/app.js index 3f8546c3..f9bd886e 100644 --- a/create-a-container/app.js +++ b/create-a-container/app.js @@ -15,7 +15,6 @@ const morgan = require('morgan'); const fs = require('fs'); const SequelizeStore = require('express-session-sequelize')(session.Store); const path = require('path'); -const RateLimit = require('express-rate-limit'); const swaggerUi = require('swagger-ui-express'); const YAML = require('yamljs'); const { sequelize } = require('./models'); @@ -23,8 +22,6 @@ const { sequelize } = require('./models'); /** * @param {object} options * @param {string|string[]} options.sessionSecrets - express-session secret(s); required. - * @param {boolean} [options.rateLimit=true] - disable in tests: assertions on 4xx - * responses must not burn the budget. * @param {boolean} [options.accessLog=true] - morgan; disable in tests for quiet output. * @param {string} [options.mcpServerUrl] - origin of the MCP server to reverse-proxy * at /mcp (default: $MCP_SERVER_URL). Unset @@ -32,7 +29,6 @@ const { sequelize } = require('./models'); */ function buildApp({ sessionSecrets, - rateLimit = true, accessLog = true, mcpServerUrl = process.env.MCP_SERVER_URL, } = {}) { @@ -98,21 +94,7 @@ function buildApp({ app.use(express.static('public')); - // Real rate limiting is expected to be enforced at the load balancer. This - // in-app limiter is only a lenient safety net against a single runaway client - // hammering the server. It intentionally counts every request (successful or - // not) rather than classifying by status code: with the SPA + OIDC flow, 401 - // (and other 4xx) responses are a normal part of the auth handshake, so - // treating them as failures throttled legitimate users. The ceiling is set - // high enough that normal browsing never trips it, so it needs no tuning. - if (rateLimit) { - app.use(RateLimit({ - windowMs: 60 * 1000, - max: 1000, - standardHeaders: true, - legacyHeaders: false, - })); - } + // Rate limiting is enforced at the load balancer, not in the app. // CSRF guard for every handler that can see the session cookie (CodeQL // js/missing-token-validation). Behavior-preserving: csrfGuard skips diff --git a/create-a-container/middlewares/__tests__/mcp-proxy.test.js b/create-a-container/middlewares/__tests__/mcp-proxy.test.js index 177d99d6..00236d8d 100644 --- a/create-a-container/middlewares/__tests__/mcp-proxy.test.js +++ b/create-a-container/middlewares/__tests__/mcp-proxy.test.js @@ -47,7 +47,6 @@ function startUpstream() { function build(mcpServerUrl) { return buildApp({ sessionSecrets: ['test-secret'], - rateLimit: false, accessLog: false, mcpServerUrl, }); diff --git a/create-a-container/package-lock.json b/create-a-container/package-lock.json index 880f51ba..97fe17e6 100644 --- a/create-a-container/package-lock.json +++ b/create-a-container/package-lock.json @@ -12,7 +12,6 @@ "csrf-sync": "^4.2.1", "dotenv": "^17.2.3", "express": "^5.2.1", - "express-rate-limit": "^8.5.1", "express-session": "^1.19.0", "express-session-sequelize": "^2.3.0", "morgan": "^1.11.0", @@ -2961,24 +2960,6 @@ "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/express-session": { "version": "1.19.0", "resolved": "https://registry.npmjs.org/express-session/-/express-session-1.19.0.tgz", @@ -3617,15 +3598,6 @@ "dev": true, "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/create-a-container/package.json b/create-a-container/package.json index ee3a338c..6a856a5e 100644 --- a/create-a-container/package.json +++ b/create-a-container/package.json @@ -26,7 +26,6 @@ "csrf-sync": "^4.2.1", "dotenv": "^17.2.3", "express": "^5.2.1", - "express-rate-limit": "^8.5.1", "express-session": "^1.19.0", "express-session-sequelize": "^2.3.0", "morgan": "^1.11.0", diff --git a/create-a-container/tests/helpers/app.js b/create-a-container/tests/helpers/app.js index cfe20ebb..100182c6 100644 --- a/create-a-container/tests/helpers/app.js +++ b/create-a-container/tests/helpers/app.js @@ -4,8 +4,6 @@ * options: * * - fixed session secret (production reads secrets from the DB in server.js) - * - rate limiting off (tests assert on 4xx responses; the limiter would - * start rejecting them after 10) * - access log off (quiet output) * * Tests normally authenticate with a Bearer API key (apiAuth accepts it and @@ -23,7 +21,6 @@ const { buildApp: buildRealApp } = require('../../app'); function buildApp() { return buildRealApp({ sessionSecrets: ['test-secret'], - rateLimit: false, accessLog: false, }); } From 6fea691c1affd97cd6212196774e24cbdd17eccf Mon Sep 17 00:00:00 2001 From: Robert Gingras Date: Fri, 31 Jul 2026 15:35:38 -0400 Subject: [PATCH 3/3] docs(create-a-container): remove unnessecary comment --- create-a-container/app.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/create-a-container/app.js b/create-a-container/app.js index f9bd886e..77811ee9 100644 --- a/create-a-container/app.js +++ b/create-a-container/app.js @@ -94,8 +94,6 @@ function buildApp({ app.use(express.static('public')); - // Rate limiting is enforced at the load balancer, not in the app. - // CSRF guard for every handler that can see the session cookie (CodeQL // js/missing-token-validation). Behavior-preserving: csrfGuard skips // GET/HEAD/OPTIONS and Bearer-only requests, and every state-changing