diff --git a/README.md b/README.md index 4855d0a..c249b33 100644 --- a/README.md +++ b/README.md @@ -47,5 +47,24 @@ server.listen(3000) You can support the maintenance of this project: - PayPal: https://www.paypal.me/kyberneees +# Security notes + +- **Errors never crash the process.** A route handler that throws (or rejects) + after response headers are already sent is contained: the response is ended or + the socket closed, and the server keeps serving. A custom `errorHandler` that + itself throws or rejects is also contained with a bare `500`. +- **Route matching is case-insensitive.** `/Admin`, `/admin` and `/ADMIN` all + match a route registered as `/admin`. Reverse proxies, WAFs and auth layers + that classify paths case-sensitively must be aligned accordingly. +- **Mount nested routers with `use()`.** Only `router.use(prefix, subRouter)` + rewrites the URL when entering a nested router. A router passed to `get()` or + `on()` is treated as a plain middleware and will not match its sub-routes. +- **Nested routers own unmatched requests.** If a mounted router finds no route + for the rewritten URL, it responds `404` itself and the parent chain is not + resumed (unlike Express mounted apps, which fall through). +- **The route cache is memory-bounded.** Matches with parameters, `404`s, and + paths longer than 4KB are never cached, and the cache is additionally capped + by total size — so attacker-controlled unique paths cannot pin memory. + # More - Website and documentation: https://0http.21no.de diff --git a/lib/next.js b/lib/next.js index 2c2ff90..005884d 100644 --- a/lib/next.js +++ b/lib/next.js @@ -24,6 +24,41 @@ function restoreNestedUrl (req) { } } +/** + * Contain every error-handler invocation so a routing error can never take the + * process down. A handler error after headers are sent makes res.setHeader + * throw ERR_HTTP_HEADERS_SENT; a user-supplied errorHandler may throw (sync) + * or reject (async) for any reason. All of these previously escaped as + * uncaughtException / unhandledRejection and killed the server. + */ +function failSafe (res) { + if (res.headersSent) { + // Response already started: a fresh status is impossible. End it as-is, + // falling back to destroying the socket when the stream already finished. + try { res.end() } catch (_) { res.destroy() } + return undefined + } + // Last-resort bare 500 for a broken error handler. + res.statusCode = 500 + try { res.setHeader('Content-Type', 'text/plain') } catch (_) {} + try { res.end('Internal Server Error') } catch (_) {} + return undefined +} + +function dispatchError (errorHandler, err, req, res) { + let result + try { + result = errorHandler(err, req, res) + } catch (_) { + return failSafe(res) + } + // An async error handler can reject after returning: contain that too. + // Promise.resolve() normalises arbitrary thenables (which may lack .catch). + return result && typeof result.then === 'function' + ? Promise.resolve(result).catch(() => failSafe(res)) + : result +} + function next (middlewares, req, res, index, routers, defaultRoute, errorHandler) { // Fast path for end of middleware chain if (index >= middlewares.length) { @@ -37,7 +72,7 @@ function next (middlewares, req, res, index, routers, defaultRoute, errorHandler // Create step function - this is called by middleware to continue the chain const step = function (err) { return err - ? errorHandler(err, req, res) + ? dispatchError(errorHandler, err, req, res) : next(middlewares, req, res, index + 1, routers, defaultRoute, errorHandler) } // Expose the error handler so nested routers can bubble errors to the parent @@ -74,25 +109,26 @@ function next (middlewares, req, res, index, routers, defaultRoute, errorHandler return result && typeof result.then === 'function' ? result.catch(err => { restoreNestedUrl(req) - return errorHandler(err, req, res) + return dispatchError(errorHandler, err, req, res) }) : result } catch (err) { // Sync error that escaped the nested router's own handling. // Restore the parent URL context before invoking the error handler. restoreNestedUrl(req) - return errorHandler(err, req, res) + return dispatchError(errorHandler, err, req, res) } } // Regular middleware function const result = middleware(req, res, step) return result && typeof result.then === 'function' - ? result.catch(err => errorHandler(err, req, res)) + ? result.catch(err => dispatchError(errorHandler, err, req, res)) : result } catch (err) { - return errorHandler(err, req, res) + return dispatchError(errorHandler, err, req, res) } } module.exports = next +module.exports.dispatchError = dispatchError diff --git a/lib/router/sequential.js b/lib/router/sequential.js index 283e8a0..286d569 100644 --- a/lib/router/sequential.js +++ b/lib/router/sequential.js @@ -3,6 +3,7 @@ const next = require('./../next') const { parse } = require('regexparam') const { LRUCache: Cache } = require('lru-cache') const queryparams = require('./../utils/queryparams') +const dispatchError = require('./../next').dispatchError /** * Default handlers as constants to avoid creating functions on each router instance. @@ -14,15 +15,18 @@ const DEFAULT_ROUTE = (req, res) => { } const DEFAULT_ERROR_HANDLER = (err, req, res) => { - res.statusCode = 500 - res.setHeader('Content-Type', 'text/plain') // Safe by default: only expose error details in explicit development mode. // Production, staging, testing, and unset NODE_ENV all receive sanitized response. - if (process.env.NODE_ENV === 'development') { - res.end(err.message) - } else { - res.end('Internal Server Error') + if (res.headersSent) { + // Headers already flushed (mid-stream failure): a status code can no longer + // be applied. End the response as-is, falling back to destroying the socket + // when the stream has already finished. + try { res.end() } catch (_) { res.destroy() } + return } + res.statusCode = 500 + res.setHeader('Content-Type', 'text/plain') + res.end(process.env.NODE_ENV === 'development' ? err.message : 'Internal Server Error') } /** @@ -58,13 +62,21 @@ module.exports = (config = {}) => { if (cacheSize > 0) { cache = new Cache({ max: cacheSize, + maxSize: Math.max(cacheSize * 1024, 1 << 20), // hard byte cap (≥1MB) + maxEntrySize: 4096, // never cache absurdly long paths + sizeCalculation: (value, key) => key.length + 64, updateAgeOnGet: false, // Disable age updates for better performance updateAgeOnHas: false }) } else if (cacheSize < 0) { - // Reduced from 100k to 50k for better memory efficiency while maintaining performance + // Reduced from 100k to 50k for better memory efficiency while maintaining performance. + // Byte-bounded so attacker-controlled long paths cannot pin memory even when + // they match global middleware or catch-all regex routes (empty params). cache = new Cache({ max: 50000, + maxSize: 16 * 1024 * 1024, // 16MB hard cap + maxEntrySize: 4096, + sizeCalculation: (value, key) => key.length + 64, updateAgeOnGet: false, updateAgeOnHas: false }) @@ -132,16 +144,27 @@ module.exports = (config = {}) => { * Uses property deletion instead of undefined assignment for better performance. * Optimized to minimize closure creation overhead. */ - const createCleanupMiddleware = (step) => { - // Pre-create the cleanup function to avoid repeated function creation - return (req, res, next) => { - req.url = req.preRouterUrl - req.path = req.preRouterPath - - // Use delete for better performance than setting undefined + // Restore the request state a nested lookup changed, using values snapshotted + // BEFORE the lookup ran. Closure snapshots give correct stack semantics: a + // grandchild cannot corrupt a parent's restore (preRouterUrl is a single slot, + // so re-reading req after deeper levels run yields undefined). + const restoreNestedUrlContext = (req, context) => { + if (context.hasUrlContext) { + req.url = context.url + req.path = context.path delete req.preRouterUrl delete req.preRouterPath + } + } + const restoreNestedContext = (req, context) => { + restoreNestedUrlContext(req, context) + req.params = context.params + } + + const createCleanupMiddleware = (step, context) => { + return (req, res, next) => { + restoreNestedContext(req, context) return step() } } @@ -166,7 +189,14 @@ module.exports = (config = {}) => { if (!match) { match = router.find(req.method, req.path) - cache.set(reqCacheKey, match) + // Parametrized matches have unbounded distinct keys (one per param + // value): caching them pins attacker-growable memory and churns hot + // static entries out of the LRU. Never cache them, and never cache + // 404s either (empty handlers) — junk unmatched paths are the same + // memory-pressure vector. + if (match.handlers.length && Object.keys(match.params).length === 0) { + cache.set(reqCacheKey, match) + } } } else { match = router.find(req.method, req.path) @@ -175,12 +205,21 @@ module.exports = (config = {}) => { const { handlers, params } = match if (handlers.length) { - // Optimized middleware array handling + // Snapshot the request state this lookup is about to change (URL rewrite + + // params). Cleanup and error paths restore from the snapshot — closure + // semantics, so deeper nesting levels cannot corrupt an outer restore. + const context = { + hasUrlContext: req.preRouterUrl !== undefined, + url: req.preRouterUrl, + path: req.preRouterPath, + params: req.params + } + let middlewares if (step !== undefined) { // Create new array only when step middleware is needed middlewares = handlers.slice() - middlewares.push(createCleanupMiddleware(step)) + middlewares.push(createCleanupMiddleware(step, context)) } else { middlewares = handlers } @@ -191,29 +230,25 @@ module.exports = (config = {}) => { // nested router's own default error handler. const activeErrorHandler = step?.errorHandler || errorHandler - // Wrap the active error handler so URL restoration happens before the - // handler is invoked. This fixes state corruption when a nested router - // handler throws, calls next(err), or rejects asynchronously. + // Wrap the active error handler so request-state restoration happens + // before the handler is invoked, and so a throwing handler can never + // take the process down. const errorHandlerWithCleanup = (err, req, res) => { - if (req.preRouterUrl !== undefined) { - req.url = req.preRouterUrl - req.path = req.preRouterPath - delete req.preRouterUrl - delete req.preRouterPath - } - return activeErrorHandler(err, req, res) + // Restore URL context only: error handlers are terminal, so keep the + // matched route's params available to the handler. + restoreNestedUrlContext(req, context) + return dispatchError(activeErrorHandler, err, req, res) } - // Optimized parameter assignment with minimal overhead - if (!req.params) { - // Shallow-copy: the match (and its params) may be served from the LRU - // cache and shared across all requests to the same method+path. - // Assigning by reference would let a middleware mutation leak between requests. - req.params = params ? { ...params } : Object.create(null) - } else if (params) { - // Manual property copying - optimized for small objects - // Pre-compute keys and length to avoid repeated calls - Object.assign(req.params, params) + // Per-level params: never mutate an upstream params object in place. + // Nested routers inherit parent params via a shallow copy, so existing + // consumers keep seeing merged params while mutations stay confined to + // this level's own object (restored on cleanup/error). + const inherited = req.params + if (inherited === undefined && Object.keys(params).length === 0) { + req.params = Object.create(null) + } else { + req.params = { ...(inherited || {}), ...(params || {}) } } return next(middlewares, req, res, 0, routers, defaultRoute, errorHandlerWithCleanup) diff --git a/tests/security-hardening.test.js b/tests/security-hardening.test.js new file mode 100644 index 0000000..0266b13 --- /dev/null +++ b/tests/security-hardening.test.js @@ -0,0 +1,262 @@ +/* global describe, it, before, after */ +const expect = require('chai').expect +const cero = require('../index') +const sequential = require('../lib/router/sequential') + +// Runs `fn` while collecting process-level failures (uncaughtException / +// unhandledRejection). Returns them so tests can assert the process survived. +async function withoutProcessCrash (fn) { + const crashes = [] + const onUncaught = (e) => crashes.push(['uncaughtException', e]) + const onRejection = (e) => crashes.push(['unhandledRejection', e]) + process.on('uncaughtException', onUncaught) + process.on('unhandledRejection', onRejection) + try { + await fn() + } finally { + process.off('uncaughtException', onUncaught) + process.off('unhandledRejection', onRejection) + } + return crashes +} + +function startServer (setup) { + const { router, server } = cero({ router: sequential() }) + setup(router) + return new Promise((resolve) => { + server.listen(0, () => resolve({ server, base: `http://127.0.0.1:${server.address().port}` })) + }) +} + +function stopServer (server) { + if (server.closeAllConnections) server.closeAllConnections() + server.close() +} + +describe('0http - security hardening (adversarial review 20260906)', () => { + describe('F1: errors after headers sent must never crash the process', () => { + let base, server + + before(async () => { + ({ base, server } = await startServer((router) => { + router.get('/boom-sync', (req, res) => { + res.write('partial') + throw new Error('sync after headers') + }) + router.get('/boom-async', async (req, res) => { + res.write('partial') + await new Promise((resolve) => setTimeout(resolve, 10)) + throw new Error('async after headers') + }) + router.get('/ok', (req, res) => res.end('ok')) + })) + }) + + after(() => stopServer(server)) + + it('contains sync handler errors thrown after headers were flushed', async () => { + const crashes = await withoutProcessCrash(async () => { + const res = await fetch(`${base}/boom-sync`) + expect(res.status).to.equal(200) + await res.text() + }) + expect(crashes, 'process-level failure').to.deep.equal([]) + // The process must survive: a follow-up request must still be served. + const ok = await fetch(`${base}/ok`) + expect(ok.status).to.equal(200) + expect(await ok.text()).to.equal('ok') + }) + + it('contains async handler rejections thrown after headers were flushed', async () => { + const crashes = await withoutProcessCrash(async () => { + const res = await fetch(`${base}/boom-async`) + expect(res.status).to.equal(200) + await res.text() + }) + expect(crashes, 'process-level failure').to.deep.equal([]) + const ok = await fetch(`${base}/ok`) + expect(ok.status).to.equal(200) + }) + }) + + describe('F1b: a throwing custom error handler must never crash the process', () => { + let base, server + + before(async () => { + const { router, server: srv } = cero({ + router: sequential({ + errorHandler: () => { throw new Error('broken custom handler') } + }) + }) + router.get('/boom', () => { throw new Error('route error') }) + router.get('/ok', (req, res) => res.end('ok')) + server = srv + await new Promise((resolve) => server.listen(0, resolve)) + base = `http://127.0.0.1:${server.address().port}` + }) + + after(() => stopServer(server)) + + it('responds a bare 500 and keeps serving', async () => { + const crashes = await withoutProcessCrash(async () => { + const res = await fetch(`${base}/boom`) + expect(res.status).to.equal(500) + }) + expect(crashes, 'process-level failure').to.deep.equal([]) + const ok = await fetch(`${base}/ok`) + expect(ok.status).to.equal(200) + }) + }) + + describe('F1c: an async custom error handler that rejects must never crash the process', () => { + let base, server + + before(async () => { + const { router, server: srv } = cero({ + router: sequential({ + errorHandler: async () => { throw new Error('broken async custom handler') } + }) + }) + router.get('/boom', () => { throw new Error('route error') }) + router.get('/ok', (req, res) => res.end('ok')) + server = srv + await new Promise((resolve) => server.listen(0, resolve)) + base = `http://127.0.0.1:${server.address().port}` + }) + + after(() => stopServer(server)) + + it('responds a bare 500 and keeps serving', async () => { + const crashes = await withoutProcessCrash(async () => { + const res = await fetch(`${base}/boom`) + expect(res.status).to.equal(500) + }) + expect(crashes, 'process-level failure').to.deep.equal([]) + const ok = await fetch(`${base}/ok`) + expect(ok.status).to.equal(200) + }) + }) + + describe('F2: deep nesting restores the parent URL context', () => { + let base, server + const seen = {} + + before(async () => { + ({ base, server } = await startServer((router) => { + const child = sequential({ id: 'CHILD' }) + const grand = sequential({ id: 'GRAND' }) + grand.get('/c', (req, res, next) => next()) + child.use('/b', grand, (req, res, next) => next()) + router.use('/a', child, (req, res) => { + seen.url = req.url + seen.path = req.path + res.end('done') + }) + })) + }) + + after(() => stopServer(server)) + + it('parent middleware after a grandchild sees the original url/path', async () => { + const res = await fetch(`${base}/a/b/c`) + expect(res.status).to.equal(200) + expect(seen.url).to.equal('/a/b/c') + expect(seen.path).to.equal('/a/b/c') + }) + }) + + describe('F3: nested router params do not leak into the parent scope', () => { + let base, server + const seen = {} + + before(async () => { + ({ base, server } = await startServer((router) => { + const child = sequential({ id: 'CHILD' }) + const leaf = sequential({ id: 'LEAF' }) + leaf.get('/:sid', (req, res, next) => { + seen.leafParams = { ...req.params } + next() + }) + child.use('/s', leaf) + router.use('/p/:pid', child, (req, res) => { + seen.parentParams = { ...req.params } + res.end('done') + }) + })) + }) + + after(() => stopServer(server)) + + it('child keeps seeing inherited params (existing behavior)', async () => { + const res = await fetch(`${base}/p/1/s/2`) + expect(res.status).to.equal(200) + expect(seen.leafParams).to.deep.equal({ pid: '1', sid: '2' }) + }) + + it('parent middleware after the nested router sees only its own params', async () => { + expect(seen.parentParams).to.deep.equal({ pid: '1' }) + }) + }) + + describe('F4: parametrized matches are never cached', () => { + it('param routes hit the matcher on every request; static routes stay cached', () => { + const router = sequential({ cacheSize: 100 }) + router.get('/static', () => {}) + router.get('/user/:id', () => {}) + + const findCalls = [] + const origFind = router.find.bind(router) + router.find = (method, path) => { + findCalls.push(path) + return origFind(method, path) + } + + const fakeRes = { end () {}, write () {} } + const hit = (p) => router.lookup({ method: 'GET', url: p }, fakeRes) + + hit('/static') + hit('/static') + hit('/user/1') + hit('/user/1') + hit('/no-such-route') + hit('/no-such-route') + + expect(findCalls.filter((p) => p === '/static')).to.have.lengthOf(1) + expect(findCalls.filter((p) => p === '/user/1')).to.have.lengthOf(2) + expect(findCalls.filter((p) => p === '/no-such-route')).to.have.lengthOf(2) + }) + + it('does not cache long paths matched only by global middleware', () => { + const router = sequential({ cacheSize: 100 }) + router.use(() => {}) // global middleware matches every path (empty params) + const findCalls = [] + const origFind = router.find.bind(router) + router.find = (method, path) => { + findCalls.push(path) + return origFind(method, path) + } + const fakeRes = { end () {}, write () {} } + const longPath = '/junk/' + 'x'.repeat(5000) + router.lookup({ method: 'GET', url: longPath }, fakeRes) + router.lookup({ method: 'GET', url: longPath }, fakeRes) + expect(findCalls.filter((p) => p === longPath)).to.have.lengthOf(2) + }) + + it('unique param traffic does not pin memory in the cache', function () { + if (!global.gc) return this.skip() + global.gc() + const before = process.memoryUsage().heapUsed + + const router = sequential() + router.get('/user/:id', () => {}) + const fakeRes = { end () {}, write () {} } + for (let i = 0; i < 50000; i++) { + router.lookup({ method: 'GET', url: `/user/${i}` }, fakeRes) + } + + global.gc() + const delta = process.memoryUsage().heapUsed - before + expect(delta).to.be.below(32 * 1024 * 1024) + }) + }) +})