Skip to content

inspector,http: support builtin http request bodies#62915

Open
GrinZero wants to merge 18 commits intonodejs:mainfrom
GrinZero:fix-http-inspector-request-body
Open

inspector,http: support builtin http request bodies#62915
GrinZero wants to merge 18 commits intonodejs:mainfrom
GrinZero:fix-http-inspector-request-body

Conversation

@GrinZero
Copy link
Copy Markdown
Contributor

@GrinZero GrinZero commented Apr 23, 2026

Summary

This PR adds builtin http/https request-body support to network inspection, so Network.getRequestPostData can return text request bodies while preserving the existing rejection behavior for binary request bodies.

It also moves builtin http response-body tracking to a raw-byte hook before IncomingMessage decoding, so response inspection remains correct even when user code calls response.setEncoding(...).

In addition, this PR extends requestWillBeSent to accept initiator data from JS diagnostics payloads and validates structured initiator.stack objects against the inspector protocol schema before forwarding them to DevTools.

Problem

Builtin http/https network inspection currently emits:

  • Network.requestWillBeSent
  • Network.responseReceived
  • Network.loadingFinished

However, the builtin http client path does not currently expose request-body data to the inspector. As a result, Network.getRequestPostData cannot return POST data for builtin http/https requests.

There are two related gaps as well:

  • IncomingMessage 'data' events are not a stable source of raw bytes. If user code calls response.setEncoding('utf8'), the chunks observed by userland become strings, while the inspector protocol expects byte-oriented payloads.
  • requestWillBeSent needs to accept JS-provided initiator data, but structured stack trace input must be validated before it is forwarded as protocol data.

Approach

1. Reuse the existing request buffering pipeline

This change does not alter the CDP schema or the existing buffering behavior in NetworkAgent.

Instead, it reuses the current pipeline already used by other transports:

Network.dataSent(...) -> NetworkAgent::getRequestPostData(...)

2. Add builtin http request-body diagnostics events

The builtin http client now publishes:

  • http.client.request.bodyChunkSent
  • http.client.request.bodySent

These events are emitted from the ClientRequest write path before HTTP framing is applied, so the inspector sees the original user payload rather than chunked-transfer framing bytes.

3. Capture builtin http response bytes before decoding

For responses, this PR avoids relying on IncomingMessage.on('data') in network_http.js.

Instead, it adds:

  • http.client.response.bodyChunkReceived

This hook runs before Readable.setEncoding() transforms chunks for userland, so the inspector always receives raw bytes.

4. Support and validate network initiator payloads

Network.requestWillBeSent can now use JS diagnostics payloads to provide initiator data.

On the C++ side, the incoming JS object is converted into inspector protocol values, and initiator.stack is validated against the Runtime.StackTrace schema before being attached to the event. If the initiator payload is malformed, the event is rejected with a clear error instead of forwarding bad protocol data to the frontend.

If JS does not provide an initiator, the existing behavior remains unchanged: the inspector captures the current stack trace and emits a default script initiator.

5. Document the new diagnostics channels

This PR also updates diagnostics_channel docs to cover the new builtin HTTP client request/response body channels and their message shapes.

Behavior

After this change:

  • builtin http and https POST requests with UTF-8 text bodies are available through Network.getRequestPostData
  • binary request bodies still reject with the existing inspector error behavior
  • builtin http response inspection continues to work even if user code calls response.setEncoding('utf8')
  • Network.requestWillBeSent can carry JS-provided initiator stack data
  • malformed initiator stack input is rejected by C++ schema validation

Tests

This PR adds and extends coverage in:

  • test/parallel/test-diagnostics-channel-http.js
  • test/parallel/test-inspector-network-http.js
  • test/parallel/test-inspector-network-arbitrary-data.js
  • test/parallel/test-inspector-emit-protocol-event-errors.js

The updated tests cover:

  • request body chunk and request body finished diagnostics events
  • response body raw-byte diagnostics events
  • text request bodies split across write() and end()
  • binary request bodies
  • http and https Network.getRequestPostData
  • binary request-body rejection semantics
  • response inspection when the client calls response.setEncoding('utf8')
  • JS-provided initiator stack parsing and forwarding
  • malformed initiator stack error handling

Verification

Verified locally with:

python3 tools/test.py \
  parallel/test-diagnostics-channel-http \
  parallel/test-inspector-network-http \
  parallel/test-inspector-network-arbitrary-data \
  parallel/test-inspector-emit-protocol-event-errors

Refs

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/inspector
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 23, 2026
@GrinZero
Copy link
Copy Markdown
Contributor Author

GrinZero commented Apr 23, 2026

image E2E Test:
const http = require("http");

const DEFAULT_TARGET_URL = "http://jsonplaceholder.typicode.com/posts";
const DEFAULT_PORT = Number(process.env.PORT || 3000);
const DEFAULT_HOST = "127.0.0.1";
const targetUrl = process.argv[2] || process.env.TARGET_URL || DEFAULT_TARGET_URL;
const defaultPayload = {
  title: "node inspect demo",
  body: "post payload from local trigger service",
  userId: 123,
};

if (!targetUrl.startsWith("http://")) {
  console.error(
    `[config] This script only uses the http module for outbound requests. Use an http:// URL, got: ${targetUrl}`
  );
  process.exit(1);
}

function readRequestBody(req) {
  return new Promise((resolve, reject) => {
    let body = "";

    req.setEncoding("utf8");
    req.on("data", (chunk) => {
      body += chunk;
    });
    req.on("end", () => {
      resolve(body);
    });
    req.on("error", reject);
  });
}

function buildPayload(rawBody) {
  if (!rawBody || !rawBody.trim()) {
    return defaultPayload;
  }

  const parsed = JSON.parse(rawBody);
  if (!parsed || Array.isArray(parsed) || typeof parsed !== "object") {
    throw new Error("payload must be a JSON object");
  }

  return {
    ...defaultPayload,
    ...parsed,
  };
}

function sendOutboundPost(url, payload) {
  return new Promise((resolve, reject) => {
    const body = JSON.stringify(payload);

    console.log(`\n[outbound] request -> POST ${url}`);
    console.log(`[outbound] request payload: ${body}`);

    const outboundReq = http.request(
      url,
      {
        method: "POST",
        headers: {
          "user-agent": "node-inspect-http-demo",
          accept: "application/json,text/plain,*/*",
          "content-type": "application/json; charset=utf-8",
          "content-length": Buffer.byteLength(body),
        },
      },
      (outboundRes) => {
        let responseBody = "";
        outboundRes.setEncoding("utf8");

        console.log(
          `[outbound] response <- ${outboundRes.statusCode} ${outboundRes.statusMessage || ""}`.trim()
        );
        console.log("[outbound] response headers:", outboundRes.headers);

        outboundRes.on("data", (chunk) => {
          responseBody += chunk;
        });

        outboundRes.on("end", () => {
          console.log("[outbound] body preview:");
          console.log(responseBody.slice(0, 300) || "<empty>");

          resolve({
            statusCode: outboundRes.statusCode,
            statusMessage: outboundRes.statusMessage,
            headers: outboundRes.headers,
            bodyPreview: responseBody.slice(0, 300),
          });
        });
      }
    );

    outboundReq.on("socket", (socket) => {
      socket.on("connect", () => {
        console.log(
          `[outbound] socket connected -> ${socket.remoteAddress}:${socket.remotePort}`
        );
      });
    });

    outboundReq.on("finish", () => {
      console.log("[outbound] request finished");
    });

    outboundReq.on("error", (error) => {
      console.error("[outbound] request error:", error.message);
      reject(error);
    });

    outboundReq.write(body);
    outboundReq.end();
  });
}

function writeJson(res, statusCode, payload) {
  const body = JSON.stringify(payload, null, 2);
  res.writeHead(statusCode, {
    "content-type": "application/json; charset=utf-8",
    "content-length": Buffer.byteLength(body),
  });
  res.end(body);
}

const server = http.createServer(async (req, res) => {
  const url = new URL(req.url || "/", `http://${req.headers.host || "127.0.0.1"}`);

  if (req.method === "GET" && url.pathname === "/") {
    return writeJson(res, 200, {
      ok: true,
      message: "local trigger service is running",
      endpoints: {
        trigger: "POST /trigger",
      },
      targetUrl,
      defaultPayload,
    });
  }

  if (req.method === "POST" && url.pathname === "/trigger") {
    console.log(`\n[inbound] trigger <- ${req.method} ${url.pathname}`);

    try {
      const rawBody = await readRequestBody(req);
      const payload = buildPayload(rawBody);
      const outbound = await sendOutboundPost(targetUrl, payload);

      return writeJson(res, 200, {
        ok: true,
        targetUrl,
        payload,
        outbound,
      });
    } catch (error) {
      return writeJson(res, 500, {
        ok: false,
        error: error.message,
      });
    }
  }

  return writeJson(res, 404, {
    ok: false,
    error: "not found",
  });
});

server.listen(DEFAULT_PORT, DEFAULT_HOST, () => {
  console.log(`[server] listening on http://${DEFAULT_HOST}:${DEFAULT_PORT}`);
  console.log(`[server] outbound target: ${targetUrl}`);
  console.log("[usage] node --inspect-wait --experimental-network-inspection inspect-http.js");
  console.log(
    `[usage] curl -X POST http://${DEFAULT_HOST}:${DEFAULT_PORT}/trigger -H 'content-type: application/json' -d '{"title":"manual trigger"}'`
  );
});

Copy link
Copy Markdown
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

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

lgtm, it just seems that is gonna need a documentation for the new dcs

@GrinZero
Copy link
Copy Markdown
Contributor Author

Thanks for the feedback. I’ve updated the documentation in doc/api/diagnostics_channel.md to address the review comments and include the new HTTP diagnostics channel events.

Specifically, I added/updated entries for:

  • http.client.request.bodyChunkSent
  • http.client.request.bodySent
  • http.client.response.bodyChunkReceived

I also aligned the wording and structure with the existing Node.js docs style for built-in channels.

@GrinZero
Copy link
Copy Markdown
Contributor Author

A slight push, hoping that a maintainer will notice.

@GrinZero
Copy link
Copy Markdown
Contributor Author

Sorry for the ping, @legendecas

If you have some time, could you please help review this PR and trigger CI?

This PR fixes an issue where the network inspector does not show postData for http / https requests.

Thanks a lot!

@GrinZero
Copy link
Copy Markdown
Contributor Author

push, request-ci plz

@marco-ippolito
Copy link
Copy Markdown
Member

Ill try to review the PR later, I think it's important we make sure it doesn't leak memory or keep the body buffer reference alive, which can exhaust resource quickly @ShogunPanda ptal

@GrinZero
Copy link
Copy Markdown
Contributor Author

Ill try to review the PR later, I think it's important we make sure it doesn't leak memory or keep the body buffer reference alive, which can exhaust resource quickly @ShogunPanda ptal

I'll go explore the issue you mentioned

@GrinZero
Copy link
Copy Markdown
Contributor Author

Thanks for raising this concern.

I did a focused memory/lifetime check for this PR, mainly around two questions:

  1. whether the inspector keeps the original userland request/response body buffers alive;
  2. whether the new body buffering path can grow without bound.

Based on the current implementation and local testing, I do not see evidence that this PR keeps the original JS buffers alive.

For both request and response bodies, the payload bytes are copied into inspector-owned storage before being retained by NetworkAgent. I also verified this with a WeakRef/GC test: the original 4 MiB request body buffer and the observed response chunk both became collectible after the request completed, while Network.getRequestPostData() / Network.getResponseBody() could still return the payloads.

So the inspector is retaining copied payload bytes, not the original userland Buffer objects.

I also ran a repeated-request memory test with explicit Network.enable() limits. Memory increased initially, as expected from inspector-side buffering, but then reached a plateau and older requests were evicted. That behavior looks consistent with bounded retention by the existing request buffer manager, not an unbounded leak.

@GrinZero
Copy link
Copy Markdown
Contributor Author

The test plan above was proposed by AI, and I reviewed it.

For the first test, it uses WeakRef, manual GC, and process.memoryUsage() to verify object lifetime. I think the underlying testing principle is reasonably credible.

The second test is a stress test. It lowers maxTotalBufferSize and maxResourceBufferSize, then sends multiple requests with relatively large bodies. I’m less familiar with this approach.

@GrinZero GrinZero force-pushed the fix-http-inspector-request-body branch from ab906d5 to 194a5fd Compare April 29, 2026 05:28
@GrinZero
Copy link
Copy Markdown
Contributor Author

I merged the main branch code to avoid CI errors.

@legendecas legendecas added inspector Issues and PRs related to the V8 inspector protocol http Issues or PRs related to the http subsystem. labels Apr 29, 2026
Copy link
Copy Markdown
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@GrinZero GrinZero force-pushed the fix-http-inspector-request-body branch from 194a5fd to ba8f093 Compare April 30, 2026 17:33
@GrinZero
Copy link
Copy Markdown
Contributor Author

LGTM!

Thanks πŸ™ I think we should add the request-ci tag to trigger the next steps. I have synced the code of the main branch

@GrinZero GrinZero force-pushed the fix-http-inspector-request-body branch from aace601 to 16da9f4 Compare May 3, 2026 04:23
@codecov
Copy link
Copy Markdown

codecov Bot commented May 3, 2026

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 89.70%. Comparing base (480ee08) to head (7edffdf).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62915      +/-   ##
==========================================
+ Coverage   89.67%   89.70%   +0.03%     
==========================================
  Files         712      712              
  Lines      221256   221492     +236     
  Branches    42397    42443      +46     
==========================================
+ Hits       198404   198694     +290     
+ Misses      14676    14628      -48     
+ Partials     8176     8170       -6     
Files with missing lines Coverage Ξ”
lib/_http_client.js 97.44% <100.00%> (+<0.01%) ⬆️
lib/_http_common.js 100.00% <100.00%> (ΓΈ)
lib/_http_outgoing.js 96.20% <100.00%> (+0.07%) ⬆️
lib/internal/http.js 92.14% <100.00%> (+0.02%) ⬆️
lib/internal/inspector/network_http.js 97.35% <100.00%> (+1.67%) ⬆️
src/inspector/network_agent.cc 90.50% <100.00%> (+2.46%) ⬆️

... and 30 files with indirect coverage changes

πŸš€ New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • πŸ“¦ JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@GrinZero GrinZero force-pushed the fix-http-inspector-request-body branch from c0db72d to 7edffdf Compare May 5, 2026 10:18
@GrinZero
Copy link
Copy Markdown
Contributor Author

GrinZero commented May 5, 2026

What are the commits after 98830e2 doing?:

Root cause

onClientRequestCreated was emitting Network.requestWillBeSent at request-creation time and computing:

hasPostData: !request.writableEnded

At that moment, writableEnded is always false, so DevTools always saw hasPostData: true β€” even for body-less calls like http.get().

The fix

Defer the requestWillBeSent emission until body state is actually known:

  • bodyChunkSent arrives β†’ hasPostData = true
  • request.start / error / responseFinish reached without any chunk β†’ hasPostData = false

A kRequestWillBeSent symbol guards against double-emission.

Why the C++ side has to change

The previous network_agent.cc::requestWillBeSent filled in the initiator like this:

.setStack(v8_inspector_->captureStackTrace(true)->buildInspectorObject(0))

That relied on an implicit invariant: when JS calls Network.requestWillBeSent, the JS stack is the user's stack, because the emission happens synchronously inside the http.get() call chain.

Once the emission is deferred, the trigger point becomes a diagnostics_channel or socket callback, and a stack captured in C++ at that moment points into Node.js internals β€” the initiator becomes useless.

The only correct fix is to capture the stack on the JS side at request-creation time via createInitiator() + getStructuredStack, stash it on the request, and pass it through when the actual emission happens.

That in turn requires the C++ backend to accept a JS-supplied initiator object, which is why:

  1. createInitiatorFromObject was added to parse { type, stack, url, lineNumber, ... }.
  2. A generic V8ToProtocolValue was added to convert the V8 stack object to protocol::Value, then rehydrated into a protocol StackTrace via ValueConversions<Runtime::API::StackTrace>::fromValue.
  3. requestWillBeSent now uses the JS-supplied initiator when present and falls back to captureStackTrace otherwise, preserving backward compatibility.

How the follow-up commits fit in

reject malformed initiator stack traces, document network initiator validation, drop unreachable initiator.stack schema, tighten createInitiator coverage, etc., are all hardening and documentation for the new JS β†’ C++ initiator path introduced above β€” not independent refactors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues or PRs related to the http subsystem. inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants