fix: verify route-level middleware req.body modifications reach handler#1057
Closed
fix: verify route-level middleware req.body modifications reach handler#1057
Conversation
bd8ee11 to
b3c6bb1
Compare
3e3f0c1 to
f9830c9
Compare
d614502 to
eda9765
Compare
eda9765 to
ee2e4c1
Compare
…xpress properties
e5b9091 to
220b32d
Compare
Contributor
Author
|
Closing this ticket because it is working as designed. The issue occurs when defining handlers without the routeHandler() wrapper. This legacy path uses: This explicitly ignores middleware results and only returns the original decoded request input. |
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.
Problem
When Express middleware modified
req(e.g.,req.body,req.query,req.params,req.headers), those modifications were not passed to route handlers defined without therouteHandler()wrapper.More confidence on why this is real bug:
Root Cause
req.decodedis created once before middleware runsroute.request.decode()function uses io-ts validation, which creates a new object fromreq.body,req.query,req.params, andreq.headersreq.body.someField = 'value')runMiddlewareChainIgnoringResults()returned only the originalreq.decoded, ignoring any middleware modificationsExample of Broken Behavior
Solution
Modified runMiddlewareChainIgnoringResults() in packages/express-wrapper/src/middleware.ts to merge middleware modifications back into the handler params.
Before
After
This ensures that after middleware runs, any modifications to the four sources of req.decoded are merged back into the result passed to the handler.
Why merge req.body, req.query, req.params, and req.headers specifically?
These four properties are hard-coded because they are the exact sources used to build req.decoded:
api-ts/packages/typed-express-router/src/index.ts
Lines 119 to 124 in c5000de
Since route.request.decode() creates a new object from these four sources, we must merge them back after middleware runs to capture any modifications.
Closes #188