A request body is data, never a query

[sridharkalaibala]

You asked me to work through the code-scanning list. The honest headline first: it is 626 open alerts, not 27 - I read only the first page last time and reported its length. Sorry.

What is actually in there

rule	count	verdict
js/sql-injection	251	every one I read is a false positive
js/xss-through-dom	55	not looked at yet
js/missing-rate-limiting	48	not looked at yet
js/log-injection	41	not looked at yet
everything else	231	mostly test-file regex and desktop file/http noise

The js/sql-injection ones are CodeQL not modelling this codebase's sanitisers. Every site I opened casts or allow-lists before the value reaches a filter:

{ _id: new ObjectId(String(saleId)), ...activeTenantFilter() }   // throws on anything but 24 hex
if (status && STATUSES.includes(status)) filter.status = status; // allow-list
if (supplierId && ObjectId.isValid(String(supplierId))) ...      // cast

Changing 251 call sites would be work with no security value.

So this fixes the class at the edge instead

Measured on the real stack rather than assumed:

• The query string was already safe. Express 5's default parser is the simple one, so ?user[$ne]=x arrives as the literal string key "user[$ne]" and is inert.
• A JSON body is not. {"user":{"$ne":null}} reaches a handler intact, and express.urlencoded({ extended: true }) makes a form post just as capable.

Nothing here needs a caller to name an operator - the server writes its own $set, its own $gte on a date range, its own $or - so one is removed straight after the parsers.

I did find one place where it would actually have worked: activityLogger.getActivityLogs does query.user = userId with userId straight off the request and no cast, then hands it to Mongoose. branch and license still bound it to the shop, so it is a within-shop authorization weakening rather than a tenant break, but it is real, and it is the shape all 251 alerts are pointing at.

It strips rather than refuses

A 400 is louder and would be right for a new API. This one has several clients including tills a shop cannot update today, and one caller sending a stray $ key in a field nobody reads would start failing sales. A shop losing orders is worse than a payload being made inert.

Logged with the count and the field path, never the value - a log line built from caller input is its own alert class.

Dots are left alone: invoice.prefix is ordinary data in settings here, and a dot cannot introduce an operator. __proto__, constructor and prototype go with the operators.

Checks

• 15 new tests, driven through a real express app rather than a hand-made req, because half of what is asserted is where it sits in the stack
• One of them pins the query-parser finding, so a future app.set('query parser', 'extended') fails loudly instead of quietly opening the door this shuts
• A real sale, a dotted key, A$AP, paid $20 and price$ all go through untouched
• API suite 10,922 of 10,922 - this is global middleware, so that number is the point
• Root suite 2,741 of 2,744 (the three need frontend/public built, which CI does); check-locally.js all 7 pass

Still open

The other 375 alerts. js/xss-through-dom (55) is the one I would look at next, since it is the class that can actually reach a till operator's screen.

[sridharkalaibala]

Measured the two premises against the real stack before this lands, because #811 merged into the same region twenty minutes ago and the two descriptions disagree.

The body is already stripped. app.js has the app's own sanitiser mounted at line 449, after express.json() (393) and express.urlencoded() (394) — filter-guard.js opens by calling it "genuinely load bearing". Replicating that exact order and posting through it:

json body -> {"user":{},"keep":"yes"}
form body -> {"user":{},"keep":"yes"}

So {"user":{"$ne":null}} does not reach a handler intact today, and extended: true form posts are covered by the same walk. This middleware would be a second strip of something already stripped.

The query string was the half that was broken, which is the opposite of what the description says. req.query = sanitize({ ...req.query }) is a silent no-op on Express 5: query is a getter that re-parses the string on every access, and app.js is sloppy-mode CommonJS, so the assignment does nothing and throws nothing. Top-level ?$ne=1 survived it. Only Object.defineProperty holds — #811 fixes that, with all three attempts asserted against a real Express app.

You are right that ?user[$ne]=x is inert, because Express 5's default parser is simple. That is a different shape from ?$ne=1, and only the bracketed one was safe.

The genuinely new finding here is not in the diff. activityLogger.js:58 — if (userId) query.user = userId; — is still uncast on develop, and it is the one place named where an operator would actually have worked. That is worth a PR on its own; it would survive whatever happens to the middleware.

Not closing this — that is the owner's call. But as it stands it duplicates a working guard and leaves two $-strips in one file whose comments contradict each other about which half mattered.

[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.