Skip to content

fix(ext/node): work correctly with wrapper Response objects, use correct rawHeaders structure #29056

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.

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
merged 3 commits into from
Apr 30, 2025

Conversation

nathanwhit
Copy link
Member

@nathanwhit nathanwhit commented Apr 26, 2025

Fixes #28022

Will circle back to add a test + clean up a bit.

Basically drizzle-kit studio uses hono with the node-server adapter. That creates wrapper objects for responses that forward property getters to the underlying response (the one we provided). However, in deno.serve we were assuming that the response was actually the same response we initially gave and crashed when it wasn't. instead, just call the property getters if we can't find the inner response.

The raw headers bug is that we were exposing the rawHeaders field on Incoming as a Headers object, instead it's supposed to be a flat array of the header keys + values. I.e. ["Content-Type:", "application/json", "Host:", "http://localhost"]

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue with Node’s HTTP polyfills to work correctly with wrapper Response objects by replacing direct access to the rawHeaders array with a new private symbol and updating the corresponding header extraction logic.

  • Replaces usage of the public rawHeaders property with a private symbol (kRawHeaders) to encapsulate header data.
  • Updates getter and assignment logic for raw headers in IncomingMessageForServer.
  • Adjusts response body extraction in 00_serve.ts to handle both stream and static responses appropriately.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
ext/node/polyfills/http.ts Uses a private symbol for raw headers and updates getter logic.
ext/http/00_serve.ts Improves response body extraction and header handling.

Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM with a test

@nathanwhit nathanwhit enabled auto-merge (squash) April 29, 2025 23:28
@nathanwhit nathanwhit merged commit 0557466 into denoland:main Apr 30, 2025
18 checks passed
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

I understand what you are trying to solve, but I don't think this is the right approach. If I am understanding right, the problem is twofold:

  • Deno.serve only accepts "real" Response objects created by the original global new Response() constructor. This is enforced through an internal slot check, but this internal slot check happens too late (it should happen in place of the prototype check).
  • The node:http code is constructing a globalThis.Response, which could be manipulated by a user.

I think the correct fix would have been to keep the behaviour of Deno.serve, but move the internal slot check further up (replacing the prototype check), and then updating the node:http code to capture the Response object at the global scope instead of relying on whatever the user may have put there.

See #29105 and follow up #29106

lucacasonato added a commit that referenced this pull request Apr 30, 2025
…use correct `rawHeaders` structure (#29056)"

This reverts commit 0557466.
lucacasonato added a commit that referenced this pull request Apr 30, 2025
…use correct `rawHeaders` structure (#29056)"

This reverts commit 0557466.
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.

TypeError when running Drizzle Kit Studio with Deno
3 participants