-
Notifications
You must be signed in to change notification settings - Fork 16
feat!: support Express 5 #201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
26bec2e
to
dbe3bd7
Compare
BREAKING CHANGE: Must use expressApp.set('query parser', 'extended') to parse and sanitize nested query params
dbe3bd7
to
61c495e
Compare
@Peppershaker According to the comments in this issue #200 the maintianer is looking for someone to take over this repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for Express 5 by requiring the “extended” query parser and handling req.query
sanitization, updates dependencies and docs, and bumps the major version.
- Introduce explicit
req.query
sanitization withObject.defineProperty
to respect Express 5’s read‐only queries. - Update tests to set the query parser to “extended” for nested query tests.
- Bump to v3.0.0, update dependencies to Express 5, and refresh documentation.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
test.js | Added app.set('query parser', 'extended') in each test setup |
package.json | Bumped version to 3.0.0; moved Express into dependencies; updated dev deps |
index.js | Removed 'query' from the main loop and added separate handling for req.query |
README.md | Updated for Express 5, added notes about setting the query parser and readonly req.query |
Comments suppressed due to low confidence (1)
test.js:23
- Add tests to cover behavior when the query parser is not set to 'extended', ensuring the middleware fails gracefully or documents the expected outcome.
app.set('query parser', 'extended');
"dependencies": { | ||
"express": "^5.1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving Express to peerDependencies instead of dependencies to avoid bundling a separate Express version and let hosting apps control the Express version.
"dependencies": { | |
"express": "^5.1.0" | |
"peerDependencies": { | |
"express": "^5.1.0" | |
}, | |
"devDependencies": { | |
"@types/express": "^5.0.1", | |
"body-parser": "^1.19.1", | |
"chai": "^4.5.0", | |
"eslint": "^8.6.0", | |
"mocha": "^9.2.2", | |
"prettier": "^2.5.1", | |
"supertest": "^7.1.0", | |
"tsd": "^0.19.1", | |
"express": "^5.1.0" |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Peppershaker Is there a reason why you added the dependencies section?
Breaking Change
The middleware now requires that you set the query parser to
"extended"
with:in order to correctly parse and sanitize nested query parameters.
Express 5 Query Parsing Changes
From Express v5 docs.
qs
to Node’s nativequerystring
parser.false
."simple"
or"extended"
parser, or supply a custom query string parsing function:querystring
module.qs
library.req.query
In Express 5, the
req.query
object is now read-only.Design Considerations
1. Injection Protection for Nested Queries
Option 1: Log a warning on application startup.
"simple"
parser is potentially unsafe and could expose users to nested operator injection attacks.Option 2: Require a reference to the Express app object.
"extended"
query parser is in use, and throw an error if it is not.Option 3: Do nothing.
bodyParser.urlencoded({ extended: true })
.2. Handling the Read-Only Nature of
req.query
Option 1: Maintain the behavior of the middleware as a query parser (similar to v2).
req.query
is read-only.req.query
usingObject.defineProperty
.Option 2: Separate the concerns by splitting functionality into a query parser and a sanitization middleware.
qs
query parser for parsing and let this package handle sanitization of query parameters.req.query
, but since Express only allows a single query parser, users must either implement a custom two-step solution (usingqs
for parsing followed by sanitization) or rely on our built-in helper functions. This may increase dependency complexity and blur the boundaries of the package’s responsibility.Recommendation
req.query
Read-Only Nature: Option 2a (Maintain the behavior of the middleware as a query parser).Additionally, explain the security implications in your documentation.