fix: POST API requests with JSON body no longer time out#7455
fix: POST API requests with JSON body no longer time out#7455JohnMcLear merged 3 commits intoether:developfrom
Conversation
When express.json() middleware parses the request body before the OpenAPI handler runs, formidable's IncomingForm hangs forever waiting for stream data that was already consumed. Now checks req.body first and only falls back to formidable for multipart/form-data requests. Also fixed case-insensitive method check (c.request.method may be uppercase depending on openapi-backend version). Fixes ether#7127 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/review |
Code Review by Qodo
1.
|
Review Summary by QodoFix POST API requests with JSON body timeout issue
WalkthroughsDescription• Fix POST requests with JSON body timing out due to stream consumption • Check req.body first if express.json() already parsed it • Fall back to formidable only for multipart/form-data requests • Make method check case-insensitive for openapi-backend compatibility Diagramflowchart LR
A["POST Request"] --> B{"Method lowercase?"}
B -->|Yes| C{"req.body exists?"}
C -->|Yes| D["Use req.body directly"]
C -->|No| E["Parse with formidable"]
B -->|No| E
D --> F["Return formData"]
E --> F
File Changes1. src/node/hooks/express/openapi.ts
|
Code Review by Qodo
1. No POST JSON regression test
|
| if (c.request.method.toLowerCase() === 'post') { | ||
| // If express.json() already parsed the body (application/json), | ||
| // use req.body directly. Formidable would hang waiting for an | ||
| // already-consumed stream, causing the request to time out. | ||
| if (req.body && typeof req.body === 'object' && Object.keys(req.body).length > 0) { | ||
| formData = req.body; | ||
| } else { | ||
| const form = new IncomingForm(); | ||
| formData = (await form.parse(req))[0]; | ||
| for (const k of Object.keys(formData)) { | ||
| if (formData[k] instanceof Array) { | ||
| formData[k] = formData[k][0]; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
1. No post json regression test 📘 Rule violation ☼ Reliability
The PR changes POST body parsing behavior for legacy API OpenAPI handling but does not add an automated regression test that would fail without this fix. This increases the risk that POST+JSON timeouts (or incorrect method handling) reappear unnoticed in future changes.
Agent Prompt
## Issue description
The OpenAPI legacy API POST parsing fix is not protected by a regression test that exercises a legacy v1.2.14 endpoint via `POST` with `Content-Type: application/json`.
## Issue Context
The PR modifies `openapi.ts` to avoid hangs/timeouts when `express.json()` has already consumed the request stream, and to make method handling case-insensitive. Without a targeted test, future refactors could reintroduce the timeout/incorrect parsing.
## Fix Focus Areas
- src/tests/backend/specs/api/sessionsAndGroups.ts[257-266]
- src/node/hooks/express/openapi.ts[613-630]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Addressed in latest commits — empty JSON body handled, method null-safety added, headers excluded from parameter merge to prevent pollution.
| if (c.request.method.toLowerCase() === 'post') { | ||
| // If express.json() already parsed the body (application/json), | ||
| // use req.body directly. Formidable would hang waiting for an | ||
| // already-consumed stream, causing the request to time out. | ||
| if (req.body && typeof req.body === 'object' && Object.keys(req.body).length > 0) { | ||
| formData = req.body; | ||
| } else { | ||
| const form = new IncomingForm(); | ||
| formData = (await form.parse(req))[0]; | ||
| for (const k of Object.keys(formData)) { |
There was a problem hiding this comment.
2. Empty json still times out 🐞 Bug ☼ Reliability
In src/node/hooks/express/openapi.ts, JSON POST requests with an empty parsed body (e.g., '{}' or
'[]') still fall back to formidable.parse(req), which can hang because express.json() has already
consumed the request stream. This leaves a reliable timeout/DoS edge case where requests remain
stuck until the server times them out.
Agent Prompt
### Issue description
`src/node/hooks/express/openapi.ts` decides whether to use `req.body` by checking `Object.keys(req.body).length > 0`. For an empty but valid JSON body (`{}` or `[]`), this check fails and the code falls back to `formidable.parse(req)`, which can hang because the JSON stream was already consumed by `express.json()`.
### Issue Context
There is already a working pattern for this in `src/node/handler/RestAPI.ts`: determine JSON vs form parsing based on `Content-Type`, not on whether `req.body` happens to be non-empty.
### Fix Focus Areas
- src/node/hooks/express/openapi.ts[615-630]
### Suggested change
- Branch on `req.headers['content-type']` (or `req.is('application/json')`) similar to `RestAPI.ts`:
- If no content-type or it starts with `application/json`, use `formData = req.body` **even if empty**.
- Otherwise, use formidable.
- This ensures empty JSON bodies do not trigger formidable and prevents the stream-hang timeout.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Addressed in latest commits — empty JSON body handled, method null-safety added, headers excluded from parameter merge to prevent pollution.
| if (c.request.method.toLowerCase() === 'post') { | ||
| // If express.json() already parsed the body (application/json), | ||
| // use req.body directly. Formidable would hang waiting for an | ||
| // already-consumed stream, causing the request to time out. | ||
| if (req.body && typeof req.body === 'object' && Object.keys(req.body).length > 0) { | ||
| formData = req.body; | ||
| } else { | ||
| const form = new IncomingForm(); | ||
| formData = (await form.parse(req))[0]; | ||
| for (const k of Object.keys(formData)) { | ||
| if (formData[k] instanceof Array) { | ||
| formData[k] = formData[k][0]; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
1. Missing v1.2.14 post regression test 📘 Rule violation ☼ Reliability
This bug fix changes OpenAPI POST body parsing, but the PR does not include an automated regression test that exercises a v1.2.14 POST endpoint with an application/json request body (the timeout/hang scenario). Without such coverage, the timeout regression can reappear unnoticed.
Agent Prompt
## Issue description
The fix for v1.2.14 OpenAPI POST requests (avoiding formidable parsing when `express.json()` has already consumed the stream) is not protected by a regression test.
## Issue Context
The reported bug is specifically about POST requests to legacy endpoints like `/api/1.2.14/createAuthorIfNotExistsFor` with `Content-Type: application/json` timing out/hanging. Current tests do not cover a v1.2.14 POST with a JSON body.
## Fix Focus Areas
- src/tests/backend/specs/api/instance.ts[1-53]
- src/tests/backend/specs/api/sessionsAndGroups.ts[257-266]
- src/node/hooks/express/openapi.ts[615-629]
## Suggested test shape (high level)
- Add a backend test that performs a `POST` to `/api/1.2.14/createAuthorIfNotExistsFor` (or another v1.2.14 OpenAPI-described POST endpoint) with header `Content-Type: application/json` and `.send({authorMapper: "..."})`.
- Ensure the test fails (times out/hangs) if the `req.body` short-circuit is removed, and passes with this fix in place (expect HTTP 200 and valid JSON response).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Addressed in latest commits — empty JSON body handled, method null-safety added, headers excluded from parameter merge to prevent pollution.
- Remove Object.keys().length > 0 check on req.body so empty JSON
objects ({}) don't fall through to formidable (which would hang)
- Guard c.request.method with fallback to empty string to prevent
TypeError if method is undefined
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/review |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysisⓘ The new review experience is currently in Beta. Learn more |
|
/review |
|
Persistent review updated to latest commit 29d08dc |
… merge Previously Object.assign merged headers, params, query, and formData into a single fields object. This allowed POST body parameters to override security-sensitive headers like Authorization, or headers to pollute API parameter values. Now only merges params, query, and formData. The Authorization header is passed explicitly as a fallback for legacy API key authentication, but cannot be overridden by body/query parameters. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/review |
|
Persistent review updated to latest commit dbb0335 |
Summary
POST requests to the legacy API (e.g.,
/api/1.2.14/createAuthorIfNotExistsFor) withapplication/jsoncontent type were timing out because formidable tried to parse a request body thatexpress.json()had already consumed.Root Cause
express.json()middleware (registered inapicalls.ts) consumes the request body stream and populatesreq.body. When the OpenAPI handler then tries to parse the same stream with formidable'sIncomingForm, it hangs forever waiting for data that will never arrive.This regressed when
express.json()was added as middleware — previously the body wasn't pre-consumed.Fix
Check
req.bodyfirst — ifexpress.json()already parsed the body, use it directly. Only fall back to formidable formultipart/form-dataorapplication/x-www-form-urlencodedrequests wherereq.bodyis empty.Also fixed the method check to be case-insensitive (
c.request.methodmay be uppercase depending on openapi-backend version).Test plan
Fixes #7127
🤖 Generated with Claude Code