Skip to content

fix: align Pages API body parsing and res.send(Buffer) #428

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.

Sign up for GitHub

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

Merged
james-elicx merged 5 commits into from
Mar 11, 2026
Merged

fix: align Pages API body parsing and res.send(Buffer) #428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ca97c9
Fix Buffer handling in Pages API res
JaredStowell Mar 10, 2026
09d6224
fix: skip reporting handled Pages API parse errors
JaredStowell Mar 11, 2026
37eb489
Merge remote-tracking branch 'origin/main' into jstowell/fix-pages-ap…
james-elicx Mar 11, 2026
b41e76e
address review: blank line nit, content-length parity for Buffer, fix…
james-elicx Mar 11, 2026
f3f03f7
fix: set content-length for Buffer in production res.send() to match …
james-elicx Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 38 additions & 17 deletions packages/vinext/src/entries/pages-server-entry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ import { runWithPrivateCache } from "vinext/cache-runtime";
import { runWithRouterState } from "vinext/router-state";
import { runWithHeadState } from "vinext/head-state";
import { safeJsonStringify } from "vinext/html";
import { decode as decodeQueryString } from "node:querystring";
import { getSSRFontLinks as _getSSRFontLinks, getSSRFontStyles as _getSSRFontStylesGoogle, getSSRFontPreloads as _getSSRFontPreloadsGoogle } from "next/font/google";
import { getSSRFontStyles as _getSSRFontStylesLocal, getSSRFontPreloads as _getSSRFontPreloadsLocal } from "next/font/local";
import { parseCookies } from ${JSON.stringify(path.resolve(__dirname, "../config/config-matchers.js").replace(/\\/g, "/"))};
Expand All @@ -277,6 +278,14 @@ const buildId = ${buildIdJson};
// Full resolved config for production server (embedded at build time)
export const vinextConfig = ${vinextConfigJson};

class ApiBodyParseError extends Error {
constructor(message, statusCode) {
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 10, 2026

Choose a reason for hiding this comment

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

Nit: in the generated (untyped) code, statusCode is assigned as a property on the instance but never declared. This works fine at runtime, but it might be clearer to assign it explicitly:

Suggested change
constructor(message, statusCode) {
constructor(message, statusCode) {
super(message);
this.statusCode = statusCode;
this.name = 'ApiBodyParseError';
}

Setting name makes stack traces more readable when this error does get logged (e.g., during debugging). Optional.

super(message);
this.statusCode = statusCode;
this.name = "ApiBodyParseError";
}
}

// ISR cache helpers (inlined for the server entry)
async function isrGet(key) {
const handler = getCacheHandler();
Expand Down Expand Up @@ -631,8 +640,16 @@ function createReqRes(request, url, query, body) {
res.end(JSON.stringify(data));
},
send: function(data) {
if (typeof data === "object" && data !== null) { res.json(data); }
else { if (!resHeaders["content-type"]) resHeaders["content-type"] = "text/plain"; res.end(String(data)); }
if (Buffer.isBuffer(data)) {
if (!resHeaders["content-type"]) resHeaders["content-type"] = "application/octet-stream";
resHeaders["content-length"] = String(data.length);
res.end(data);
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

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

Dev/prod parity gap: the dev-mode api-handler.ts sets Content-Length for Buffer payloads (line 148), but this production path does not. The integration test for production (sends Buffer payloads from res.send() as raw bytes in production in pages-router.test.ts) also doesn't assert content-length, while the dev test does.

This means the same API handler will return Content-Length in dev but not in production — exactly the kind of divergence AGENTS.md warns against. Should be a one-liner:

Suggested change
res.end(data);
if (!resHeaders["content-type"]) resHeaders["content-type"] = "application/octet-stream";
resHeaders["content-length"] = String(data.length);

Copy link
Contributor

@ask-bonk ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

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

Dev/prod parity gap: the dev-mode api-handler.ts sets Content-Length for Buffer payloads (line 148), but this production path does not. In practice this works today because sendCompressed in prod-server.ts sets Content-Length for uncompressed responses — but this is an accidental layering dependency. The production res.send() should behave identically to the dev one so the parity is explicit rather than relying on the outer transport layer.

Suggested change
res.end(data);
if (!resHeaders["content-type"]) resHeaders["content-type"] = "application/octet-stream";
resHeaders["content-length"] = String(data.length);
res.end(data);

} else if (typeof data === "object" && data !== null) {
res.json(data);
} else {
if (!resHeaders["content-type"]) resHeaders["content-type"] = "text/plain";
res.end(String(data));
}
},
redirect: function(statusOrUrl, url2) {
if (typeof statusOrUrl === "string") { res.writeHead(307, { Location: statusOrUrl }); }
Expand Down Expand Up @@ -1038,28 +1055,32 @@ export async function handleApiRoute(request, url) {
if (contentLength > 1 * 1024 * 1024) {
return new Response("Request body too large", { status: 413 });
}
let body;
const ct = request.headers.get("content-type") || "";
let rawBody;
try { rawBody = await readBodyWithLimit(request, 1 * 1024 * 1024); }
catch { return new Response("Request body too large", { status: 413 }); }
if (!rawBody) {
body = undefined;
} else if (ct.includes("application/json")) {
try { body = JSON.parse(rawBody); } catch { body = rawBody; }
} else {
body = rawBody;
}

const { req, res, responsePromise } = createReqRes(request, url, query, body);

try {
let body;
const ct = request.headers.get("content-type") || "";
let rawBody;
try { rawBody = await readBodyWithLimit(request, 1 * 1024 * 1024); }
catch { return new Response("Request body too large", { status: 413 }); }
if (!rawBody) {
body = undefined;
} else if (ct.includes("application/json")) {
try { body = JSON.parse(rawBody); } catch { throw new ApiBodyParseError("Invalid JSON", 400); }
} else if (ct.includes("application/x-www-form-urlencoded")) {
body = decodeQueryString(rawBody);
} else {
body = rawBody;
}

const { req, res, responsePromise } = createReqRes(request, url, query, body);
await handler(req, res);
// If handler didn't call res.end(), end it now.
// The end() method is idempotent — safe to call twice.
res.end();
return await responsePromise;
} catch (e) {
if (e instanceof ApiBodyParseError) {
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 10, 2026

Choose a reason for hiding this comment

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

Same observation as in api-handler.ts — in the production entry, the ApiBodyParseError check happens after the generic console.error("[vinext] API error:", e) line. A 400 from malformed JSON will get logged as [vinext] API error: which is misleading since it's a client error. Consider reordering the check before the console.error.

return new Response(e.message, { status: e.statusCode });
}
console.error("[vinext] API error:", e);
return new Response("Internal Server Error", { status: 500 });
}
Expand Down
35 changes: 28 additions & 7 deletions packages/vinext/src/server/api-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
*/
import type { ViteDevServer } from "vite";
import type { IncomingMessage, ServerResponse } from "node:http";
import { decode as decodeQueryString } from "node:querystring";
import { type Route, matchRoute } from "../routing/pages-router.js";
import { reportRequestError } from "./instrumentation.js";
import { addQueryParam } from "../utils/query.js";
Expand Down Expand Up @@ -39,6 +40,16 @@ interface NextApiResponse extends ServerResponse {
*/
const MAX_BODY_SIZE = 1 * 1024 * 1024;

class ApiBodyParseError extends Error {
constructor(
message: string,
readonly statusCode: number,
) {
super(message);
this.name = "ApiBodyParseError";
}
}

/**
* Parse the request body based on content-type.
* Enforces a size limit to prevent memory exhaustion attacks.
Expand Down Expand Up @@ -77,15 +88,10 @@ async function parseBody(req: IncomingMessage): Promise<unknown> {
try {
resolve(JSON.parse(raw));
} catch {
resolve(raw);
reject(new ApiBodyParseError("Invalid JSON", 400));
}
} else if (contentType.includes("application/x-www-form-urlencoded")) {
const params = new URLSearchParams(raw);
const obj: Record<string, string> = {};
for (const [key, value] of params) {
obj[key] = value;
}
resolve(obj);
resolve(decodeQueryString(raw));
} else {
resolve(raw);
}
Expand Down Expand Up @@ -135,6 +141,15 @@ function enhanceApiObjects(
};

apiRes.send = function (data: unknown) {
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 10, 2026

Choose a reason for hiding this comment

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

Nit: Next.js's res.send() also handles Buffer-like objects via the content-type header and sets the Content-Length header. This implementation doesn't set Content-Length for the Buffer case. Not a blocker — just noting the difference for future reference.

if (Buffer.isBuffer(data)) {
if (!this.getHeader("Content-Type")) {
this.setHeader("Content-Type", "application/octet-stream");
}
this.setHeader("Content-Length", String(data.length));
this.end(data);
return;
}

if (typeof data === "object" && data !== null) {
this.setHeader("Content-Type", "application/json");
this.end(JSON.stringify(data));
Expand Down Expand Up @@ -206,6 +221,12 @@ export async function handleApiRoute(
await handler(apiReq, apiRes);
return true;
} catch (e) {
if (e instanceof ApiBodyParseError) {
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

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

Nit: the previous reviewer and Codex both flagged this, and the test already asserts ssrFixStacktrace and reportRequestError are NOT called — but that assertion currently passes only because ApiBodyParseError is caught before those calls. The control flow is correct, but it would be slightly clearer to separate the early-return from the logging block with a blank line, so a future reader immediately sees the intent:

Suggested change
if (e instanceof ApiBodyParseError) {
if (e instanceof ApiBodyParseError) {
res.statusCode = e.statusCode;
res.end(e.message);
return true;
}
server.ssrFixStacktrace(e as Error);

Trivial — feel free to ignore.

res.statusCode = e.statusCode;
res.end(e.message);
return true;
}

server.ssrFixStacktrace(e as Error);
console.error(e);
reportRequestError(
Expand Down
5 changes: 4 additions & 1 deletion packages/vinext/src/server/prod-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,11 @@ function sendCompressed(
/* ignore pipeline errors on closed connections */
});
} else {
// Strip any pre-existing content-length (from the Web Response constructor)
// before setting our own — avoids duplicate Content-Length headers.
const { "content-length": _cl, "Content-Length": _CL, ...headersWithoutLength } = extraHeaders;
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

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

Good fix. The Response constructor auto-generates a content-length from the body, which mergeResponseHeaders propagates into extraHeaders. Without this strip, you'd get a duplicate Content-Length when sendCompressed sets its own.

Minor note: the compressed branch (line 190) still spreads ...extraHeaders without stripping content-length. For compressed responses Content-Length is meaningless (the compressed size differs), so a stale content-length could confuse clients. Not introduced by this PR and unlikely to cause issues in practice (browsers ignore it when Content-Encoding is present), but worth being aware of.

res.writeHead(statusCode, {
...extraHeaders,
...headersWithoutLength,
"Content-Type": contentType,
"Content-Length": String(buf.length),
});
Expand Down
83 changes: 54 additions & 29 deletions tests/__snapshots__/entry-templates.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17587,6 +17587,7 @@ import { runWithPrivateCache } from "vinext/cache-runtime";
import { runWithRouterState } from "vinext/router-state";
import { runWithHeadState } from "vinext/head-state";
import { safeJsonStringify } from "vinext/html";
import { decode as decodeQueryString } from "node:querystring";
import { getSSRFontLinks as _getSSRFontLinks, getSSRFontStyles as _getSSRFontStylesGoogle, getSSRFontPreloads as _getSSRFontPreloadsGoogle } from "next/font/google";
import { getSSRFontStyles as _getSSRFontStylesLocal, getSSRFontPreloads as _getSSRFontPreloadsLocal } from "next/font/local";
import { parseCookies } from "<ROOT>/packages/vinext/src/config/config-matchers.js";
Expand Down Expand Up @@ -17616,6 +17617,14 @@ const buildId = "test-build-id";
// Full resolved config for production server (embedded at build time)
export const vinextConfig = {"basePath":"","trailingSlash":false,"redirects":[{"source":"/old-about","destination":"/about","permanent":true},{"source":"/repeat-redirect/:id","destination":"/docs/:id/:id","permanent":false}],"rewrites":{"beforeFiles":[{"source":"/before-rewrite","destination":"/about"},{"source":"/repeat-rewrite/:id","destination":"/docs/:id/:id"},{"source":"/mw-gated-before","has":[{"type":"cookie","key":"mw-before-user"}],"destination":"/about"}],"afterFiles":[{"source":"/after-rewrite","destination":"/about"},{"source":"/mw-gated-rewrite","has":[{"type":"cookie","key":"mw-user"}],"destination":"/about"}],"fallback":[{"source":"/fallback-rewrite","destination":"/about"}]},"headers":[{"source":"/api/(.*)","headers":[{"key":"X-Custom-Header","value":"vinext"}]},{"source":"/about","has":[{"type":"cookie","key":"logged-in"}],"headers":[{"key":"X-Auth-Only-Header","value":"1"}]},{"source":"/about","missing":[{"type":"cookie","key":"logged-in"}],"headers":[{"key":"X-Guest-Only-Header","value":"1"}]},{"source":"/ssr","headers":[{"key":"Vary","value":"Accept-Language"}]}],"i18n":null,"images":{}};

class ApiBodyParseError extends Error {
constructor(message, statusCode) {
super(message);
this.statusCode = statusCode;
this.name = "ApiBodyParseError";
}
}

// ISR cache helpers (inlined for the server entry)
async function isrGet(key) {
const handler = getCacheHandler();
Expand Down Expand Up @@ -17702,12 +17711,14 @@ import * as page_29 from "<ROOT>/tests/fixtures/pages-basic/pages/products/[pid]
import * as page_30 from "<ROOT>/tests/fixtures/pages-basic/pages/docs/[...slug].tsx";
import * as page_31 from "<ROOT>/tests/fixtures/pages-basic/pages/sign-up/[[...sign-up]]/index.tsx";
import * as api_0 from "<ROOT>/tests/fixtures/pages-basic/pages/api/binary.ts";
import * as api_1 from "<ROOT>/tests/fixtures/pages-basic/pages/api/error-route.ts";
import * as api_2 from "<ROOT>/tests/fixtures/pages-basic/pages/api/hello.ts";
import * as api_3 from "<ROOT>/tests/fixtures/pages-basic/pages/api/instrumentation-test.ts";
import * as api_4 from "<ROOT>/tests/fixtures/pages-basic/pages/api/middleware-test.ts";
import * as api_5 from "<ROOT>/tests/fixtures/pages-basic/pages/api/no-content-type.ts";
import * as api_6 from "<ROOT>/tests/fixtures/pages-basic/pages/api/users/[id].ts";
import * as api_1 from "<ROOT>/tests/fixtures/pages-basic/pages/api/echo-body.ts";
import * as api_2 from "<ROOT>/tests/fixtures/pages-basic/pages/api/error-route.ts";
import * as api_3 from "<ROOT>/tests/fixtures/pages-basic/pages/api/hello.ts";
import * as api_4 from "<ROOT>/tests/fixtures/pages-basic/pages/api/instrumentation-test.ts";
import * as api_5 from "<ROOT>/tests/fixtures/pages-basic/pages/api/middleware-test.ts";
import * as api_6 from "<ROOT>/tests/fixtures/pages-basic/pages/api/no-content-type.ts";
import * as api_7 from "<ROOT>/tests/fixtures/pages-basic/pages/api/send-buffer.ts";
import * as api_8 from "<ROOT>/tests/fixtures/pages-basic/pages/api/users/[id].ts";

import { default as AppComponent } from "<ROOT>/tests/fixtures/pages-basic/pages/_app.tsx";
import { default as DocumentComponent } from "<ROOT>/tests/fixtures/pages-basic/pages/_document.tsx";
Expand Down Expand Up @@ -17749,12 +17760,14 @@ const pageRoutes = [

const apiRoutes = [
{ pattern: "/api/binary", patternParts: ["api","binary"], isDynamic: false, params: [], module: api_0 },
{ pattern: "/api/error-route", patternParts: ["api","error-route"], isDynamic: false, params: [], module: api_1 },
{ pattern: "/api/hello", patternParts: ["api","hello"], isDynamic: false, params: [], module: api_2 },
{ pattern: "/api/instrumentation-test", patternParts: ["api","instrumentation-test"], isDynamic: false, params: [], module: api_3 },
{ pattern: "/api/middleware-test", patternParts: ["api","middleware-test"], isDynamic: false, params: [], module: api_4 },
{ pattern: "/api/no-content-type", patternParts: ["api","no-content-type"], isDynamic: false, params: [], module: api_5 },
{ pattern: "/api/users/:id", patternParts: ["api","users",":id"], isDynamic: true, params: ["id"], module: api_6 }
{ pattern: "/api/echo-body", patternParts: ["api","echo-body"], isDynamic: false, params: [], module: api_1 },
{ pattern: "/api/error-route", patternParts: ["api","error-route"], isDynamic: false, params: [], module: api_2 },
{ pattern: "/api/hello", patternParts: ["api","hello"], isDynamic: false, params: [], module: api_3 },
{ pattern: "/api/instrumentation-test", patternParts: ["api","instrumentation-test"], isDynamic: false, params: [], module: api_4 },
{ pattern: "/api/middleware-test", patternParts: ["api","middleware-test"], isDynamic: false, params: [], module: api_5 },
{ pattern: "/api/no-content-type", patternParts: ["api","no-content-type"], isDynamic: false, params: [], module: api_6 },
{ pattern: "/api/send-buffer", patternParts: ["api","send-buffer"], isDynamic: false, params: [], module: api_7 },
{ pattern: "/api/users/:id", patternParts: ["api","users",":id"], isDynamic: true, params: ["id"], module: api_8 }
];

function matchRoute(url, routes) {
Expand Down Expand Up @@ -18044,8 +18057,16 @@ function createReqRes(request, url, query, body) {
res.end(JSON.stringify(data));
},
send: function(data) {
if (typeof data === "object" && data !== null) { res.json(data); }
else { if (!resHeaders["content-type"]) resHeaders["content-type"] = "text/plain"; res.end(String(data)); }
if (Buffer.isBuffer(data)) {
if (!resHeaders["content-type"]) resHeaders["content-type"] = "application/octet-stream";
resHeaders["content-length"] = String(data.length);
res.end(data);
} else if (typeof data === "object" && data !== null) {
res.json(data);
} else {
if (!resHeaders["content-type"]) resHeaders["content-type"] = "text/plain";
res.end(String(data));
}
},
redirect: function(statusOrUrl, url2) {
if (typeof statusOrUrl === "string") { res.writeHead(307, { Location: statusOrUrl }); }
Expand Down Expand Up @@ -18451,28 +18472,32 @@ export async function handleApiRoute(request, url) {
if (contentLength > 1 * 1024 * 1024) {
return new Response("Request body too large", { status: 413 });
}
let body;
const ct = request.headers.get("content-type") || "";
let rawBody;
try { rawBody = await readBodyWithLimit(request, 1 * 1024 * 1024); }
catch { return new Response("Request body too large", { status: 413 }); }
if (!rawBody) {
body = undefined;
} else if (ct.includes("application/json")) {
try { body = JSON.parse(rawBody); } catch { body = rawBody; }
} else {
body = rawBody;
}

const { req, res, responsePromise } = createReqRes(request, url, query, body);

try {
let body;
const ct = request.headers.get("content-type") || "";
let rawBody;
try { rawBody = await readBodyWithLimit(request, 1 * 1024 * 1024); }
catch { return new Response("Request body too large", { status: 413 }); }
if (!rawBody) {
body = undefined;
} else if (ct.includes("application/json")) {
try { body = JSON.parse(rawBody); } catch { throw new ApiBodyParseError("Invalid JSON", 400); }
} else if (ct.includes("application/x-www-form-urlencoded")) {
body = decodeQueryString(rawBody);
} else {
body = rawBody;
}

const { req, res, responsePromise } = createReqRes(request, url, query, body);
await handler(req, res);
// If handler didn't call res.end(), end it now.
// The end() method is idempotent — safe to call twice.
res.end();
return await responsePromise;
} catch (e) {
if (e instanceof ApiBodyParseError) {
return new Response(e.message, { status: e.statusCode });
}
console.error("[vinext] API error:", e);
return new Response("Internal Server Error", { status: 500 });
}
Expand Down
Loading
Loading