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() + }) + }) +})