Skip to content

Commit 11ea6e1

Browse files
jkyberneeesclaude
andauthored
fix: honor prioRequestsProcessing default and stop sharing cached params (#52)
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 <noreply@anthropic.com>
1 parent 8e0277c commit 11ea6e1

3 files changed

Lines changed: 83 additions & 4 deletions

File tree

index.js

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,15 @@ const http = require('http')
22
const httpServer = http.Server
33
const httpsServer = require('https').Server
44

5-
module.exports = (config = { prioRequestsProcessing: true }) => {
5+
module.exports = (config = {}) => {
66
const router = config.router || require('./lib/router/sequential')(config)
77
const server = config.server || http.createServer()
88

9+
// Default to true even when a partial config is provided. Keeping the default
10+
// in the parameter alone would silently flip it off for any `zero({ ... })` call.
11+
const prioRequestsProcessing = config.prioRequestsProcessing ?? true
912
server.prioRequestsProcessing =
10-
config.prioRequestsProcessing && (server instanceof httpServer || server instanceof httpsServer)
13+
prioRequestsProcessing && (server instanceof httpServer || server instanceof httpsServer)
1114

1215
if (server.prioRequestsProcessing) {
1316
server.on('request', (req, res) => {

lib/router/sequential.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,10 @@ module.exports = (config = {}) => {
160160

161161
// Optimized parameter assignment with minimal overhead
162162
if (!req.params) {
163-
// Use pre-created empty object or provided params directly
164-
req.params = params || Object.create(null)
163+
// Shallow-copy: the match (and its params) may be served from the LRU
164+
// cache and shared across all requests to the same method+path.
165+
// Assigning by reference would let a middleware mutation leak between requests.
166+
req.params = params ? { ...params } : Object.create(null)
165167
} else if (params) {
166168
// Manual property copying - optimized for small objects
167169
// Pre-compute keys and length to avoid repeated calls

tests/regression-findings.test.js

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
/* global describe, it */
2+
const expect = require('chai').expect
3+
const request = require('supertest')
4+
5+
/**
6+
* Regression tests for two findings:
7+
*
8+
* #1 prioRequestsProcessing default flips OFF whenever ANY config object is
9+
* passed without the flag, contradicting the documented `default: true`.
10+
*
11+
* #2 The route-match cache stores trouter's `match` object (including its
12+
* `params`). On a cache hit `req.params` is assigned that object BY
13+
* REFERENCE, so all requests to the same method+path share one mutable
14+
* object — a non-idempotent middleware mutation bleeds into later requests.
15+
*/
16+
describe('0http - Regression: findings validation', () => {
17+
describe('#1 prioRequestsProcessing default', () => {
18+
it('should default prioRequestsProcessing to true when omitted but other config is provided', (done) => {
19+
// A real-world call: user customizes the router but does NOT mention the flag.
20+
const { server } = require('../index')({
21+
router: require('../lib/router/sequential')()
22+
})
23+
24+
// Documented default is `true`; passing unrelated config must not flip it off.
25+
expect(server.prioRequestsProcessing).equals(true)
26+
done()
27+
})
28+
29+
it('should still honor an explicit false', (done) => {
30+
const { server } = require('../index')({
31+
prioRequestsProcessing: false,
32+
router: require('../lib/router/sequential')()
33+
})
34+
35+
expect(server.prioRequestsProcessing).equals(false)
36+
done()
37+
})
38+
})
39+
40+
describe('#2 cached params are not shared across requests', () => {
41+
const baseUrl = `http://localhost:${~~process.env.PORT + 1}`
42+
const { router, server } = require('../index')({
43+
router: require('../lib/router/sequential')()
44+
})
45+
46+
it('should start the service', (done) => {
47+
// Non-idempotent mutation: append a marker to req.params.id on every request.
48+
router.get('/item/:id', (req, res) => {
49+
req.params.id = req.params.id + '!'
50+
res.end(req.params.id)
51+
})
52+
53+
server.listen(~~process.env.PORT + 1, () => done())
54+
})
55+
56+
it('should not leak a mutated param into the next request to the same path', async () => {
57+
// First hit: 'x' -> 'x!'
58+
const first = await request(baseUrl).get('/item/x').expect(200)
59+
expect(first.text).to.equal('x!')
60+
61+
// Second hit to the SAME path: must also be 'x!', NOT 'x!!'.
62+
const second = await request(baseUrl).get('/item/x').expect(200)
63+
expect(second.text).to.equal('x!')
64+
65+
// Third hit, to be sure it does not keep growing.
66+
const third = await request(baseUrl).get('/item/x').expect(200)
67+
expect(third.text).to.equal('x!')
68+
})
69+
70+
it('should stop the service', async () => {
71+
server.close()
72+
})
73+
})
74+
})

0 commit comments

Comments
 (0)