One body sanitiser, not two

[sridharkalaibala]

Correcting myself. api/app.js has had a working NoSQL guard on the body all along. req.body is an ordinary property, unlike the query getter that #811 found had been a silent no-op since Express 5.

I did not check that before adding src/middleware/no-mongo-operators.js in #812. I built a bare Express app inside my own test, watched an operator reach the handler there, and reported it as this application's behaviour. It was not. The scaffold had no sanitiser because I had not put one in it, and #811's own comment says the body half was never affected.

The middleware still earns its place

It does three things the old walk does not:

• removes __proto__, constructor and prototype
• bounds its walk - depth limit, node budget, cycle guard - where sanitize is unbounded recursion over caller-supplied structure
• logs a count and a field path rather than a string the caller chose, which is its own alert class

What it should not do is strip the body a second time. Two implementations of one promise is how the two drift: a rule added to one and not the other reads as covered and is not. That is the argument I made about the unanswered-orders sweeper in #807, and it applies to me here.

So the duplicate goes, the dedicated one stays, and sanitize keeps the query string, which is its own problem and was not running at all until #811.

I changed a test from another session

#811's wiring check pinned the removed line by its exact text:

/* The body half never needed it and still must not be broken. */
expect(app).toMatch(/req\.body = sanitize\(\{ \.\.\.req\.body \}\);/);

Its comment says what it was defending, and that guarantee is unchanged - the body is still stripped, before any route, by the dedicated middleware. So the assertion now checks the guarantee instead of the line, and fails if the middleware is ever removed. The change is explained in the test itself so whoever wrote it sees why.

And the new test uses the real app

require('../../../app.js') and walk the actual middleware chain, rather than a scaffold built in the test. That is what the earlier one should have done, and it is the whole lesson here.

Checks

• 5 new tests against the real app; the middleware's own 15 still pass
• API suite 10,954 of 10,954; check-locally.js all 7 pass

Ordering

Release #816 is already open and carries #812 as it was. This one is a follow-up, so it needs its own release after. Nothing is broken in between - the body is sanitised twice, which is wasteful rather than wrong.

[github-actions]

Merged to develop. Anyone can test this - you do not need write access.

Try it at https://develop.posnic.io, or run it yourself:

git fetch origin develop && git checkout develop
npm install && npm --prefix api install
npm run dev   # then http://localhost:3000

When you have tested it, say what you did and what happened, and set
QA passed or QA failed. If you cannot set labels, just comment -
a maintainer will.

Reporting that something is broken is as useful as fixing it. It is
better found here than by a shopkeeper.