fix: persist sanitized query and route params - #610
Open
Nwoyemartha wants to merge 2 commits into
Open
Conversation
sanitizeInputs() validated query and route params but discarded the value returned by sanitizeString(), so HTML tags were detected yet never stripped from req.query/req.params (issue Heliobond#518). - Persist sanitized query values by defining the sanitized copy as the request's own `query` property, shadowing Express 5's getter-only req.query (whole-object assignment throws in strict mode and per-key writes to the re-parsed object are lost). - Persist sanitized route param values via per-key assignment, which works wherever params are already populated. - Add regression coverage: body/query/route-param sanitization at the middleware integration boundary, rejection of SQL injection, command injection, and path traversal patterns preserved, and non-string query values unchanged. Fixes Heliobond#518 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
@Nwoyemartha Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Lockfile had drifted from package.json: it still contained semantic-release, @octokit, @actions, and pnpm helper packages that are no longer declared, and was missing the root "license" field. Regenerated with npm to restore a consistent install state. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sanitizeInputs()validatedreq.queryandreq.paramsbut discarded the value returned bysanitizeString(): HTML tags were detected/processed, yet the stripped value was never persisted for query and route parameters (onlyreq.bodyreceived its sanitized result). This fix writes the sanitized value back for all three input sources.Implementation notes (verified against Express 5.2.1 /
@types/express@5):req.queryas a getter-only prototype property that re-parses the URL on every access, so per-key writes to the returned object are lost and a whole-objectreq.query = ...assignment throwsTypeErrorin strict mode. The middleware now builds the sanitized copy and defines it as the request's ownqueryproperty (Object.defineProperty), which cleanly shadows the getter — no unsafe casts, and the value persists for all downstream handlers.req.paramsis a plain own property, so per-key assignment (req.params[key] = sanitized) is type-safe and persists wherever params are populated when the middleware runs. Note that Express 5 assignsreq.paramsat route-layer dispatch, so at a pre-routeapp.useposition params are empty (this is a runtime lifecycle fact of Express, not of this change); when attached on a parameterised route, sanitized values persist.Changes
req, shadowing the Express 5 getter)src/__tests__/sanitize.test.tsApiErrors; non-string query values untouched)Tests
Added
src/__tests__/sanitize.test.ts(11 tests, following the supertest conventions ofcsrf.test.ts):sanitizes req.body as before (strips tags, keeps inner text)sanitizes a query parameter containing HTML/script tags and writes the stripped result back(asserts the handler sees the stripped value — i.e. persistence, not just validation)sanitizes a route parameter containing HTML/script tags and writes the stripped result back(route-attached middleware, where Express has populated params)persists the sanitized param value on req.params via direct middleware invocationstill rejects 1' OR '1'='1 / \whoami` / ../../etc/passwd with a 400 for dangerous input in a query parameter` (SQL injection, command injection, path traversal)still rejects a route parameter containing an SQL injection patternleaves non-string query values (arrays) unchanged,leaves a plain query value with no HTML unchanged,does not mutate req.body when it is absentCommands run (actual results):
npx jest src/__tests__/sanitize.test.ts— ✅ 11 passednpx prettier --checkon both changed files — ✅ pass (repo'slint-stagedpre-commit hook also ranprettier --write+eslint --fixon commit)npx eslint src/middleware/sanitize.ts src/__tests__/sanitize.test.ts— ✅ cleannpx eslint src/— 2 errors / 173 warnings; identical count onmain(pre-existing, in unrelated filesconfig.test.ts,process-exit-codes.test.ts)npx tsc --noEmit— fails only in pre-existing, unrelated files (registry.ts,admin.ts,batch.ts); verified identical error set with changes stashed onmainnpx tsc -p tsconfig.test.json --noEmit— 21 error lines on bothmainand this branch; none in the changed filesnpx jest(full suite) — 23 failing suites; verified the failure set is identical onmain(diff of FAIL lists matches, apart from timing suffixes), so all failures are pre-existing and unrelated to this changeSecurity / correctness
The change ensures all three input sources — body, query, and route params — receive the same sanitization transformation. HTML/script tags are now actually stripped from query and route parameter values instead of only being validated, while existing rejection of dangerous input patterns (SQL injection, command injection, path traversal) and non-string handling are unchanged.
Compatibility
Behavior changes only in the intended security direction: query/route values that previously kept their HTML content after passing through
sanitizeInputsare now stripped of tags (e.g.?search=<b>x</b>y→xy), matching what already happened forreq.body. No API shape changes: response bodies, routes, and status codes are unaffected; validation rejections behave exactly as before.Fixes #518