Skip to content

fix: POST API requests with JSON body no longer time out#7455

Merged
JohnMcLear merged 3 commits intoether:developfrom
JohnMcLear:fix/api-post-timeout-7127
Apr 5, 2026
Merged

fix: POST API requests with JSON body no longer time out#7455
JohnMcLear merged 3 commits intoether:developfrom
JohnMcLear:fix/api-post-timeout-7127

Conversation

@JohnMcLear
Copy link
Copy Markdown
Member

Summary

POST requests to the legacy API (e.g., /api/1.2.14/createAuthorIfNotExistsFor) with application/json content type were timing out because formidable tried to parse a request body that express.json() had already consumed.

Root Cause

express.json() middleware (registered in apicalls.ts) consumes the request body stream and populates req.body. When the OpenAPI handler then tries to parse the same stream with formidable's IncomingForm, 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.body first — if express.json() already parsed the body, use it directly. Only fall back to formidable for multipart/form-data or application/x-www-form-urlencoded requests where req.body is empty.

Also fixed the method check to be case-insensitive (c.request.method may be uppercase depending on openapi-backend version).

Test plan

  • Type check passes
  • Backend tests pass (744/744)

Fixes #7127

🤖 Generated with Claude Code

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>
@JohnMcLear
Copy link
Copy Markdown
Member Author

/review

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects bot commented Apr 4, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (2) 📎 Requirement gaps (0) 🎨 UX Issues (0)

Grey Divider


Action required

1. JSON primitive POST hangs🐞 Bug ⛨ Security
Description
In openapi.ts, POST requests fall back to formidable.parse(req) unless req.body is an object,
so a valid application/json body that parses to a primitive (e.g., "x", 123, true) will
still hit formidable after express.json() has consumed the stream, reintroducing the hang/timeout.
Because parsing happens before auth checks, an unauthenticated request can trigger this and tie up
server resources.
Code

src/node/hooks/express/openapi.ts[R615-623]

+          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') {
+              formData = req.body;
+            } else {
+              const form = new IncomingForm();
+              formData = (await form.parse(req))[0];
Evidence
express.json() is registered globally, so JSON POST bodies are consumed and req.body is
populated. The new logic in openapi.ts only uses req.body if it is an object; otherwise it calls
IncomingForm().parse(req), which will wait on an already-consumed stream for JSON requests. The
/api/2 handler already avoids this by using a content-type check and always using req.body for
JSON, demonstrating the intended safe pattern.

src/node/hooks/express/openapi.ts[610-630]
src/node/hooks/express/apicalls.ts[26-33]
src/node/handler/RestAPI.ts[1440-1455]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`src/node/hooks/express/openapi.ts` falls back to `formidable.parse(req)` unless `req.body` is an object. For `Content-Type: application/json` requests where Express parses the body to a primitive (string/number/boolean), the code will still call formidable on an already-consumed stream and hang.
## Issue Context
`express.json()` is installed globally, so JSON requests will have their streams consumed before this handler runs.
## Fix Focus Areas
- src/node/hooks/express/openapi.ts[613-630]
### What to change
- Detect JSON requests via `req.headers['content-type']` / `req.is('application/json')` (or equivalently: if `req.body` is not `undefined` for POST) and **never** invoke formidable for those.
- For JSON requests where `req.body` is not a plain object (primitive/array), set `formData` to `{}` (or otherwise map it safely) rather than passing the primitive/array into `Object.assign()`.
- Keep formidable parsing only for `multipart/form-data` and `application/x-www-form-urlencoded` (or when the body stream has not been consumed).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Missing v1.2.14 POST regression test 📘 Rule violation ☼ Reliability
Description
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.
Code

src/node/hooks/express/openapi.ts[R615-629]

+          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];
+                }
         }
       }
Evidence
PR Compliance ID 4 requires a regression test for bug fixes. The PR updates POST parsing logic in
openapi.ts, but existing legacy v1.2.14 API tests only cover a GET endpoint (getStats) and
existing createAuthorIfNotExistsFor coverage is via GET with query parameters, not POST with JSON
body.

src/node/hooks/express/openapi.ts[615-629]
src/tests/backend/specs/api/instance.ts[11-53]
src/tests/backend/specs/api/sessionsAndGroups.ts[257-266]
Best Practice: Repository guidelines

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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


3. Empty JSON still hangs🐞 Bug ≡ Correctness
Description
In src/node/hooks/express/openapi.ts, POST requests with an already-parsed JSON body will still
fall back to formidable when req.body is {}, re-triggering the original timeout/hang because
formidable tries to parse an already-consumed request stream. This breaks POST endpoints that
legitimately send an empty JSON object or otherwise result in an empty parsed body.
Code

src/node/hooks/express/openapi.ts[R619-624]

+            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)) {
Evidence
The openapi handler only skips formidable if Object.keys(req.body).length > 0, so an empty parsed
JSON object causes the code to call form.parse(req) again. The repo registers express.json()
globally, which can populate req.body and consume the request stream before this handler. A
similar codepath in RestAPI.ts avoids this by keying off the Content-Type header instead of body
key count.

src/node/hooks/express/openapi.ts[613-630]
src/node/hooks/express/apicalls.ts[26-28]
src/node/handler/RestAPI.ts[1440-1455]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`openapi.ts` falls back to formidable whenever `Object.keys(req.body).length === 0`. If `express.json()` already consumed the stream and produced `{}`, formidable will attempt to parse an already-consumed stream, risking the original hang/timeout.
## Issue Context
`express.json()` is registered globally, and `RestAPI.ts` already uses a safer `Content-Type` based decision.
## Fix Focus Areas
- src/node/hooks/express/openapi.ts[615-630]
## Suggested change
- Decide whether to use `req.body` vs formidable based on `req.headers['content-type']` (similar to `src/node/handler/RestAPI.ts`).
- For `application/json` (or missing content-type if you want to preserve existing behavior), always use `req.body` even if it is `{}`.
- Only invoke formidable for `multipart/form-data` / `application/x-www-form-urlencoded` (or other non-JSON content types).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (4)
4. No POST JSON regression test📘 Rule violation ☼ Reliability
Description
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.
Code

src/node/hooks/express/openapi.ts[R615-630]

+          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];
+                }
        }
      }
    }
Evidence
PR Compliance ID 4 requires a regression test for bug fixes. The changed code introduces new POST
parsing logic in openapi.ts, but the existing test for createAuthorIfNotExistsFor uses GET
(not POST with a JSON body), so it does not protect the fixed behavior.

src/node/hooks/express/openapi.ts[613-630]
src/tests/backend/specs/api/sessionsAndGroups.ts[257-266]
Best Practice: Repository guidelines

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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


5. Empty JSON still times out🐞 Bug ☼ Reliability
Description
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.
Code

src/node/hooks/express/openapi.ts[R615-624]

+          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)) {
Evidence
The OpenAPI handler only uses req.body when it is a non-empty object; otherwise it calls
formidable.parse(req). Because express.json() is registered globally, it will consume
application/json request streams before this handler runs, so formidable will wait forever for bytes
that will never arrive (same root cause as the original regression) for empty JSON payloads. The
newer /api/2 handler avoids this by branching on Content-Type rather than Object.keys(req.body).

src/node/hooks/express/openapi.ts[613-632]
src/node/hooks/express/apicalls.ts[26-33]
src/node/handler/RestAPI.ts[1440-1455]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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


6. No POST JSON regression test 📘 Rule violation ☼ Reliability
Description
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.
Code

src/node/hooks/express/openapi.ts[R615-630]

+          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];
+                }
             }
           }
         }
Evidence
PR Compliance ID 4 requires a regression test for bug fixes. The changed code introduces new POST
parsing logic in openapi.ts, but the existing test for createAuthorIfNotExistsFor uses GET
(not POST with a JSON body), so it does not protect the fixed behavior.

src/node/hooks/express/openapi.ts[613-630]
src/tests/backend/specs/api/sessionsAndGroups.ts[257-266]
Best Practice: Repository guidelines

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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


7. Empty JSON still times out🐞 Bug ☼ Reliability
Description
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.
Code

src/node/hooks/express/openapi.ts[R615-624]

+          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)) {
Evidence
The OpenAPI handler only uses req.body when it is a non-empty object; otherwise it calls
formidable.parse(req). Because express.json() is registered globally, it will consume
application/json request streams before this handler runs, so formidable will wait forever for bytes
that will never arrive (same root cause as the original regression) for empty JSON payloads. The
newer /api/2 handler avoids this by branching on Content-Type rather than Object.keys(req.body).

src/node/hooks/express/openapi.ts[613-632]
src/node/hooks/express/apicalls.ts[26-33]
src/node/handler/RestAPI.ts[1440-1455]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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



Remediation recommended

8. Non-string padID crashes 🐞 Bug ≡ Correctness
Description
openapi.ts now forwards req.body directly as formData for POST requests, so JSON bodies can
provide non-string values (e.g., padID: 123 or padID: {}) that propagate into APIHandler.
APIHandler then calls PadManager.sanitizePadId() which unconditionally uses padId.replace(...),
throwing a TypeError and returning a 500 InternalError instead of a 400.
Code

src/node/hooks/express/openapi.ts[R615-623]

+          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') {
+              formData = req.body;
+            } else {
+              const form = new IncomingForm();
+              formData = (await form.parse(req))[0];
Evidence
The PR change assigns req.body (unvalidated) into formData and merges it into fields, which is
passed to apiHandler.handle(). APIHandler.handle() uses fields.padID/fields.padName without
type checks and passes them to PadManager.sanitizePadId(), which assumes a string and calls
.replace(), so a non-string value will throw at runtime and be converted to an internal error by
the OpenAPI wrapper.

src/node/hooks/express/openapi.ts[613-641]
src/node/handler/APIHandler.ts[202-216]
src/node/db/PadManager.ts[175-187]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`src/node/hooks/express/openapi.ts` now uses `req.body` directly for POST requests when it is an object. This allows non-string values from JSON (numbers/objects/arrays) to flow into `APIHandler.handle()`, which calls `padManager.sanitizePadId()` and assumes a string (`padId.replace(...)`). Non-string values will throw a TypeError and become a 500 response.
### Issue Context
This path is newly reachable for legacy OpenAPI endpoints once JSON POST bodies are accepted. Existing form parsing via formidable yields strings, so this string-assumption was previously protected for these endpoints.
### Fix Focus Areas
- Add type validation (or safe coercion) for `padID`/`padName` (and possibly other known string fields) before calling `sanitizePadId`, returning a 400 on invalid types.
- Prefer fixing centrally in `APIHandler.handle()` so all API entry points (including REST API v2) are protected.
- src/node/hooks/express/openapi.ts[615-632]
- src/node/handler/APIHandler.ts[202-216]
- src/node/db/PadManager.ts[175-187]
### Suggested implementation direction
Option A (central hardening):
- In `APIHandler.handle()`, before `sanitizePadId()` calls:
- If `fields.padID != null && typeof fields.padID !== 'string'`, throw `createHTTPError.BadRequest('padID must be a string')`.
- Same for `fields.padName`.
Option B (OpenAPI entry normalization):
- When using `req.body` in `openapi.ts`, only accept a plain object (not an array) and ensure values are strings (or convert simple primitives via `String(v)`), otherwise throw `createHTTPError.BadRequest(...)`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


9. Method lowercasing can throw🐞 Bug ☼ Reliability
Description
c.request.method.toLowerCase() will throw a TypeError if c.request.method is missing or not a
string, turning requests into 500s. Previously the strict equality check would not crash in that
scenario.
Code

src/node/hooks/express/openapi.ts[615]

+          if (c.request.method.toLowerCase() === 'post') {
Evidence
The new code unconditionally calls .toLowerCase() on an any-typed value (c: any), which is a
runtime hazard if the field is absent or non-string. The old code only compared against a string
literal and would not throw if the method was undefined.

src/node/hooks/express/openapi.ts[609-617]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`c.request.method.toLowerCase()` can throw if `method` is undefined/null/non-string.
## Issue Context
The handler signature uses `c: any`, so runtime type safety matters.
## Fix Focus Areas
- src/node/hooks/express/openapi.ts[615-616]
## Suggested change
- Replace with a guarded normalization:
- `const method = (c.request.method ?? '').toString().toLowerCase();`
- or `if ((c.request.method ?? '').toLowerCase() === 'post') { ... }`
- Keep the case-insensitive behavior without risking a throw.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Fix POST API requests with JSON body timeout issue

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• 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
Diagram
flowchart 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
Loading

Grey Divider

File Changes

1. src/node/hooks/express/openapi.ts 🐞 Bug fix +13/-6

Fix JSON body timeout by checking req.body first

• Changed method check to case-insensitive using .toLowerCase()
• Added check for pre-parsed req.body from express.json() middleware
• Use req.body directly when available to avoid stream consumption issues
• Only fall back to formidable parsing for non-JSON request bodies

src/node/hooks/express/openapi.ts


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects bot commented Apr 4, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX Issues (0)

Grey Divider


Action required

1. No POST JSON regression test 📘 Rule violation ☼ Reliability
Description
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.
Code

src/node/hooks/express/openapi.ts[R615-630]

+          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];
+                }
              }
            }
          }
Evidence
PR Compliance ID 4 requires a regression test for bug fixes. The changed code introduces new POST
parsing logic in openapi.ts, but the existing test for createAuthorIfNotExistsFor uses GET
(not POST with a JSON body), so it does not protect the fixed behavior.

src/node/hooks/express/openapi.ts[613-630]
src/tests/backend/specs/api/sessionsAndGroups.ts[257-266]
Best Practice: Repository guidelines

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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


2. Empty JSON still times out 🐞 Bug ☼ Reliability
Description
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.
Code

src/node/hooks/express/openapi.ts[R615-624]

+          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)) {
Evidence
The OpenAPI handler only uses req.body when it is a non-empty object; otherwise it calls
formidable.parse(req). Because express.json() is registered globally, it will consume
application/json request streams before this handler runs, so formidable will wait forever for bytes
that will never arrive (same root cause as the original regression) for empty JSON payloads. The
newer /api/2 handler avoids this by branching on Content-Type rather than Object.keys(req.body).

src/node/hooks/express/openapi.ts[613-632]
src/node/hooks/express/apicalls.ts[26-33]
src/node/handler/RestAPI.ts[1440-1455]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### 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


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment thread src/node/hooks/express/openapi.ts Outdated
Comment on lines 615 to 630
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];
}
}
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in latest commits — empty JSON body handled, method null-safety added, headers excluded from parameter merge to prevent pollution.

Comment thread src/node/hooks/express/openapi.ts Outdated
Comment on lines +615 to +624
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)) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in latest commits — empty JSON body handled, method null-safety added, headers excluded from parameter merge to prevent pollution.

Comment thread src/node/hooks/express/openapi.ts Outdated
Comment on lines 615 to 629
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];
}
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in latest commits — empty JSON body handled, method null-safety added, headers excluded from parameter merge to prevent pollution.

Comment thread src/node/hooks/express/openapi.ts Outdated
- 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>
@JohnMcLear
Copy link
Copy Markdown
Member Author

/review

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects bot commented Apr 4, 2026

Code Review by Qodo

Grey Divider

New Review Started

This review has been superseded by a new analysis

Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@JohnMcLear
Copy link
Copy Markdown
Member Author

/review

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects bot commented Apr 4, 2026

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>
@JohnMcLear
Copy link
Copy Markdown
Member Author

/review

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects bot commented Apr 4, 2026

Persistent review updated to latest commit dbb0335

Comment thread src/node/hooks/express/openapi.ts
@JohnMcLear JohnMcLear merged commit af03259 into ether:develop Apr 5, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

undocumented api changes or bug when accessing createAuthorIfNotExistsFor via POST

1 participant