From bb2b7153f1c2f8e27dbdcb793b92437db1e53efe Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sun, 31 May 2026 10:37:47 +0200 Subject: [PATCH] fix: honor prioRequestsProcessing default and stop sharing cached params MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two correctness fixes, each validated by a regression test that fails on the prior code: 1. prioRequestsProcessing default — the `true` default lived only in the parameter default `(config = { prioRequestsProcessing: true })`, so any `zero({ ...otherConfig })` call left the flag `undefined` (falsy) and silently disabled it, contradicting the documented `default: true`. Now resolved inside the body via `config.prioRequestsProcessing ?? true`. 2. Shared cached params — `router.lookup` caches trouter's full `match` object (including `params`) in the LRU and assigned `req.params = params` by reference. All requests to the same method+path shared one mutable object, so a non-idempotent middleware mutation (e.g. `req.params.id += x`) bled into later requests. Now shallow-copied on assignment. Regression test: tests/regression-findings.test.js (fails before, passes after). Full suite: 69 passing, coverage 97%. Co-Authored-By: Claude Opus 4.8 --- index.js | 7 ++- lib/router/sequential.js | 6 ++- tests/regression-findings.test.js | 74 +++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 4 deletions(-) create mode 100644 tests/regression-findings.test.js diff --git a/index.js b/index.js index ddd0dbc..08bbc39 100644 --- a/index.js +++ b/index.js @@ -2,12 +2,15 @@ const http = require('http') const httpServer = http.Server const httpsServer = require('https').Server -module.exports = (config = { prioRequestsProcessing: true }) => { +module.exports = (config = {}) => { const router = config.router || require('./lib/router/sequential')(config) const server = config.server || http.createServer() + // Default to true even when a partial config is provided. Keeping the default + // in the parameter alone would silently flip it off for any `zero({ ... })` call. + const prioRequestsProcessing = config.prioRequestsProcessing ?? true server.prioRequestsProcessing = - config.prioRequestsProcessing && (server instanceof httpServer || server instanceof httpsServer) + prioRequestsProcessing && (server instanceof httpServer || server instanceof httpsServer) if (server.prioRequestsProcessing) { server.on('request', (req, res) => { diff --git a/lib/router/sequential.js b/lib/router/sequential.js index ee981e2..864de3a 100644 --- a/lib/router/sequential.js +++ b/lib/router/sequential.js @@ -160,8 +160,10 @@ module.exports = (config = {}) => { // Optimized parameter assignment with minimal overhead if (!req.params) { - // Use pre-created empty object or provided params directly - req.params = params || Object.create(null) + // 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 diff --git a/tests/regression-findings.test.js b/tests/regression-findings.test.js new file mode 100644 index 0000000..3d0c929 --- /dev/null +++ b/tests/regression-findings.test.js @@ -0,0 +1,74 @@ +/* global describe, it */ +const expect = require('chai').expect +const request = require('supertest') + +/** + * Regression tests for two findings: + * + * #1 prioRequestsProcessing default flips OFF whenever ANY config object is + * passed without the flag, contradicting the documented `default: true`. + * + * #2 The route-match cache stores trouter's `match` object (including its + * `params`). On a cache hit `req.params` is assigned that object BY + * REFERENCE, so all requests to the same method+path share one mutable + * object — a non-idempotent middleware mutation bleeds into later requests. + */ +describe('0http - Regression: findings validation', () => { + describe('#1 prioRequestsProcessing default', () => { + it('should default prioRequestsProcessing to true when omitted but other config is provided', (done) => { + // A real-world call: user customizes the router but does NOT mention the flag. + const { server } = require('../index')({ + router: require('../lib/router/sequential')() + }) + + // Documented default is `true`; passing unrelated config must not flip it off. + expect(server.prioRequestsProcessing).equals(true) + done() + }) + + it('should still honor an explicit false', (done) => { + const { server } = require('../index')({ + prioRequestsProcessing: false, + router: require('../lib/router/sequential')() + }) + + expect(server.prioRequestsProcessing).equals(false) + done() + }) + }) + + describe('#2 cached params are not shared across requests', () => { + const baseUrl = `http://localhost:${~~process.env.PORT + 1}` + const { router, server } = require('../index')({ + router: require('../lib/router/sequential')() + }) + + it('should start the service', (done) => { + // Non-idempotent mutation: append a marker to req.params.id on every request. + router.get('/item/:id', (req, res) => { + req.params.id = req.params.id + '!' + res.end(req.params.id) + }) + + server.listen(~~process.env.PORT + 1, () => done()) + }) + + it('should not leak a mutated param into the next request to the same path', async () => { + // First hit: 'x' -> 'x!' + const first = await request(baseUrl).get('/item/x').expect(200) + expect(first.text).to.equal('x!') + + // Second hit to the SAME path: must also be 'x!', NOT 'x!!'. + const second = await request(baseUrl).get('/item/x').expect(200) + expect(second.text).to.equal('x!') + + // Third hit, to be sure it does not keep growing. + const third = await request(baseUrl).get('/item/x').expect(200) + expect(third.text).to.equal('x!') + }) + + it('should stop the service', async () => { + server.close() + }) + }) +})