fix: body-parser shim to allow strict parse failures to reach the app#10415
fix: body-parser shim to allow strict parse failures to reach the app#10415
Conversation
…ach the app Fixes #10404. Malformed application/json could return 500 on Hosting framework SSR while the same route returned 400 under next start; the shim only clears entity.parse.failed so the framework can still handle the request. - Add patchBodyParser, installBodyParserTolerance, and getBodyParserToleranceShim in utils; document PARSE_FAILED_TYPE against body-parser read.js - Prepend the shim to generated server.js for CJS and ESM; ESM path adds createRequire(import.meta.url) before the snapshot so require and require.cache match the shim - Use non-enumerable __bodyParserPassthroughApplied on body-parser exports for idempotency - Add express/supertest tests for #10404-style JSON failures, other parsers, limits, urlencoded depth, and shim string checks without eval under nyc
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| const rawApp = express(); | ||
| rawApp.use(bodyParserExports.raw!({ type: "*/*" }) as express.RequestHandler); | ||
| rawApp.post("/", (req, res) => res.json({ length: (req.body as Buffer).length })); |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a body-parser tolerance shim to address 500 errors on Firebase Hosting caused by malformed JSON payloads. It adds utility functions in src/frameworks/utils.ts to patch body-parser factories, allowing them to ignore parse failures and pass requests to the downstream framework. The shim is injected into the generated server.js file. A review comment suggests that the regex used to identify body-parser in the module cache is overly specific and should be made more flexible to improve robustness across different environments.
| * Serialized next to {@link patchBodyParser} in {@link getBodyParserToleranceShim}. | ||
| */ | ||
| export function installBodyParserTolerance(): void { | ||
| const BODY_PARSER_INDEX_REGEX = /[\\/]node_modules[\\/]body-parser[\\/]index\.js$/; |
There was a problem hiding this comment.
The regex /[\\/]node_modules[\\/]body-parser[\\/]index\.js$/ is quite specific and might fail to match in environments with non-standard directory structures or if body-parser is bundled. While it works for standard npm layouts, consider if a more flexible check (e.g., just checking for the presence of body-parser in the path) would be safer for future-proofing.
|
I see this is still causing some issues for the user for now #10404 please let me know when the fix is confirmed and I will take a closer look then |
4 participants
Description
Fixes #10404. Malformed application/json returns 500 on Hosting framework SSR while the same route returned 400(reached the API handler) locally; the shim only clears
entity.parse.failedso the framework can handle the request.Scenarios Tested
Deployments / runtime (primary)
server.js(same shape as today: shim first, thenfirebase-functions/ dynamicfirebase-frameworksimport). Confirmserver.json disk (or in the uploaded bundle) starts with the body-parser shim, then the function wiring.POSTthe same routes withContent-Type: application/jsonand an invalid body (e.g. truncated{). Expect the app to behave likenext start: route-level handling can run (e.g. 400 + app body), not an opaque 500 from failing inside Expressbody-parserbefore the framework.type: "module"vs CommonJS — One project with"type": "module"and one without; after deploy, confirm both bundles load (no early crash on import), and the ESM path hascreateRequire(import.meta.url)before any shim code that usesrequire/require.cache.Unit tests (supporting)
utils.spec.tsbody-parser tolerance + shim string checksSample Commands
deploy using a build from this PR then test a request: