Accept cross-realm Response objects from plugin HTTP routes - #1693
Conversation
Fixes #1661 Co-Authored-By: Claude <noreply@anthropic.com>
|
🚨 SLOP COP 🚨 · I am SlopCop. I started a review of this pull request. I will check security, code quality, performance, architecture, duplicate code, and the HTTP route behavior. I will post one final review comment. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| if (!isResponseLike(value)) { | ||
| throw new Error("http route handler must return a Response"); | ||
| } | ||
| const body = await value.arrayBuffer(); |
There was a problem hiding this comment.
🚨 slopcop/review — Foreign responses lose streaming and can buffer without a limit.
await value.arrayBuffer() waits for the complete body before this function returns. It removes backpressure and creates a full-size memory copy.
I confirmed this with the development server and a browser. A structural foreign route returned headers after 1,505 ms. It also delivered both delayed chunks together. The equivalent native route returned headers in 3 ms.
Please preserve the response stream through a local stream adapter. The adapter should also pass request cancellation to the source stream.
There was a problem hiding this comment.
Fixed in the follow-up commit. The adapter now pipes the foreign body stream chunk by chunk through a this-realm ReadableStream and forwards cancel to the source reader. It only falls back to arrayBuffer() when the foreign value has no usable stream. A new server test streams two gated chunks and asserts the first chunk arrives before the second is produced.
| expect(entry?.statusDetail).toContain("http GET /boom failed"); | ||
| }); | ||
|
|
||
| it("adopts a structurally valid Response from another realm (#1661)", async () => { |
There was a problem hiding this comment.
🚨 slopcop/review — The server and the SDK test host now use different response rules.
This test covers the server dispatcher only. packages/plugin-sdk/src/testing/fake-plugin-host.ts still uses response instanceof Response in fetchHttp().
A direct test-host check returns a 500 response for the same foreign response shape that this test accepts. Please share the adapter or update the test host with the same contract.
There was a problem hiding this comment.
Fixed in the follow-up commit. The adapter now lives in @get-bb/plugin-sdk/internal/host-policy as adoptHttpRouteResponse, and both the server invokeHttpRoute and the SDK fake host fetchHttp() call it. Added a fake-host test for the foreign shape and the malformed shape. Bumped the SDK patch version because bundled types changed.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This change lets plugin HTTP routes return responses from a different JavaScript environment. It converts those responses into server-native responses before Hono uses them.
I found two issues.
-
P1: The conversion removes streaming. The code reads the complete body into memory before it returns the response. This delays data, removes backpressure, and permits large memory copies. A browser test showed the foreign path waited 1,505 ms and combined two delayed chunks. Please use a streaming adapter with cancellation support.
-
P2: The SDK test host still rejects foreign responses. Its
fetchHttp()dispatcher keeps the oldinstanceof Responsecheck. Plugin tests can fail for responses that now work in the server. Please reuse the conversion contract in both dispatchers.
The architecture search found no existing shared response adapter. A shared internal helper would prevent these two dispatchers from drifting again.
Security review found no separate privilege issue. Plugins already run as trusted server code. The unbounded body read still creates a denial-of-service path through memory use or an endless stream.
Verification:
- The focused server wire suite passed all 21 tests.
- The
@bb/servertype check passed. - A real development server and browser returned the foreign JSON route with status 201 and the expected headers and body.
- The browser test reproduced the stream delay and body combination on the structural foreign path.
- A direct SDK fake-host check reproduced the old 500 response.
I used a comment review. I did not approve the pull request or request changes.
Address SlopCop review on #1693: move the cross-realm Response adapter into plugin-sdk internal/host-policy so the server host and the SDK fake host use one contract, and pipe a foreign body stream through with cancellation instead of buffering it. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Summary
Plugin HTTP route handlers can run in a different realm, so a valid `Response` from a handler failed the `instanceof Response` check in `invokeHttpRoute`. Every plugin HTTP route then returned a 500 with `http route handler must return a Response`.
This change:
Tests
Fixes #1661