Repository navigation
feat(plugins): preserve binary HTTP in sandboxes - #3180
Conversation
🦋 Changeset detectedLatest commit: bfa1de3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
review/needs-review
Scope checkThis PR changes 1,825 lines across 40 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | dd8d1a4 | Sep 20 2026, 11:49 AM |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds a shared, binary-safe HTTP wire for ctx.http.fetch() across the in-process, Cloudflare Worker Loader, and Node/workerd runners. The approach is sound: a single http-wire.ts module buffers request/response bodies, applies Fetch redirect rules, strips credentials on cross-origin hops, and exposes a stable PluginHttpResponseWire for both RPC-based runners. Tests cover binary round-trips, redirect behavior, credential stripping, SSRF literals, and the new @emdash-cms/plugin-test helpers. I did not run the test suite; the review is static.
Headline conclusion: the implementation is mostly clean and well-tested, but there are a few real doc/contract inconsistencies that should be fixed before merge.
- Doc comments still name the deprecated
network:fetchcapability inpackages/core/src/plugins/types.ts. The canonical capability isnetwork:request; the source comments are user-facing API docs and need to match. - Redirect method-rewrite cleanup is missing
content-length/transfer-encoding. When a 301/302/303 rewrites a POST to GET, leaving a caller-suppliedContent-Lengthon a now-body-less GET can break compliant servers or cause timeouts. - Changeset is technically accurate but could be more useful. It names the feature and limits, but a short "what to do" example for plugin authors and an h4 section on the new test helpers would help readers decide if the release affects them.
No blockers around security, data loss, or backwards incompatibility.
Findings
-
[needs fixing]
packages/core/src/plugins/types.ts:521The
HttpAccessinterface docstring still names the deprecatednetwork:fetchcapability. The canonical capability isnetwork:request(used everywhere else in this PR, the manifest schema, the docs, and the generated skill). Plugin authors reading source or generated API docs will be misled./** * HTTP client interface - requires network:request capability */ -
[needs fixing]
packages/core/src/plugins/types.ts:621The
httpfield onPluginContextdocuments the wrong capability name./** HTTP client - only if network:request capability */ -
[needs fixing]
packages/core/src/plugins/types.ts:1546The
allowedHostscomment onPluginDefinitionalso names the deprecated capability./** Allowed hosts for network:request (wildcards supported: *.example.com) */ -
[suggestion]
packages/core/src/plugins/http-wire.ts:3When
rewritePluginHttpRedirectrewrites a POST to GET on 301/302/303, Fetch request-body headers should be removed. The current list dropscontent-encoding,content-language,content-location, andcontent-typebut leavescontent-length(and, less commonly,transfer-encoding). If the caller supplied aContent-Length, a now-body-less GET will carry a stale length, which can confuse compliant servers or cause timeouts.const REDIRECT_BODY_HEADERS = [ "content-encoding", "content-language", "content-location", "content-type", "content-length", "transfer-encoding", ]; -
[suggestion]
.changeset/binary-plugin-http.md:9The changeset is clear, but it could help readers decide whether the release matters to them. Consider adding a short h4 section showing how a plugin author uses the new observable behavior and the new test helpers. For example:
Adds binary-safe `ctx.http.fetch()` behavior to sandboxed plugins on Cloudflare Worker Loader and Node/workerd. Request and response bodies are buffered with an 8 MiB decoded limit, and the returned WHATWG `Response` preserves bytes, status text, headers, final URL, redirect state, and clones across both runners. Redirected requests follow Fetch method and body rules. The Node/workerd runner also applies the installed version's current network capability and host list immediately after a plugin update. #### Testing external HTTP `createPluginRuntimeTestHost()` now adds `http.respond()`, `http.requests()`, and `http.clear()` so tests can queue deterministic external responses and inspect requests made through `ctx.http.fetch()`: ```ts await host.http.respond("https://api.example.com/v1", new Response(bytes)); await host.actions.routes.request("import", { user: admin, headers: { "X-EmDash-Request": "1" } }); expect(host.http.requests()).toContainEqual(expect.objectContaining({ url: "https://api.example.com/v1" }));
github-actions
Bot
added
review/awaiting-author
aa9830d to
33c7b63
Compare
github-actions
Bot
added
review/needs-rereview
|
Addressed the current-head review in
Revalidated the focused tests, full root build, ~ 🤖 Codex |
ascorbic
added
bot:review
There was a problem hiding this comment.
Re-review of the binary-safe sandbox HTTP PR. The previously requested fixes are all in place:
- The deprecated
network:fetchcapability name has been replaced bynetwork:requestin the public API docstrings onHttpAccess,PluginContext.http, andPluginDefinition.allowedHosts. REDIRECT_BODY_HEADERSnow includescontent-lengthandtransfer-encoding, so 301/302/303 GET rewrites no longer send stale request-body headers.- The changeset has been rewritten with concrete h4 sections and example code for reading binary responses and testing external HTTP.
The overall approach remains sound: a single shared wire module, bounded request/response bodies, redirect-method rules, per-hop host/SSRF/credential checks, version-keyed bridge caches for workerd, and a deterministic host.http fixture layer in @emdash-cms/plugin-test.
One stale doc comment survived the rename pass. This is a real user-facing contract error, not a nit, and is flagged below. No security, data-loss, or backwards-incompatibility blockers were found in a static read of the diff.
Findings
-
[needs fixing]
packages/core/src/plugins/context.ts:837The
createUnrestrictedHttpAccessdocstring still refers to the non-existentnetwork:fetch:anycapability. The canonical capability isnetwork:request:unrestricted, which is what the rest of the code, manifest schema, and docs already use. Plugin authors reading source or generated API docs will be misled about which capability enables unrestricted outbound HTTP.* Create unrestricted HTTP access (for plugins with network:request:unrestricted capability).
github-actions
Bot
added
review/awaiting-author
|
Fixed the remaining stale capability reference in ~ 🤖 Codex |
github-actions
Bot
added
the
review/needs-rereview
There was a problem hiding this comment.
This remains the right change for making sandbox ctx.http.fetch() binary-safe and runner-consistent. Centralizing the request/response wire format and redirect logic in packages/core, serializing byte payloads over the workerd bridge with __emdashBytes/__emdashEscapedObject, keying the workerd bridge-handler cache by pluginId:version, and adding deterministic host.http.* fixtures in @emdash-cms/plugin-test all fit EmDash’s architecture.
Prior review findings are resolved:
- The stale
network:fetchreferences inpackages/workerd/src/sandbox/dev-runner.tsandcapnp.tsare nownetwork:request. bridgeJsonReviveris applied to every backing-bridge response viaJSON.parse(..., bridgeJsonReviver), and test coverage now exercises both rawUint8Arrayrehydration and the__emdashEscapedObjectround-trip.- The unreachable form-data marshaling paths in
packages/workerd/src/sandbox/bridge-handler.tsare gone. packages/core/src/plugins/context.tsnow normalizes casing and strips trailing dots inisHostAllowed, matching the Cloudflare bridge and closing the previously noted runner-parity gap.
I re-checked the new/changed authored files in core, cloudflare, workerd, plugin-test, and plugin-cli, the supporting tests, and the docs/skill-reference updates. No blocking security, data-loss, or build regressions were found. Test assertions cover binary round-trips, buffered body limits, redirect method/body rules, redirect modes, concurrent workerd transport, and authority eviction on plugin version changes. Two minor maintainability notes remain.
There was a problem hiding this comment.
This is the right change for the stated goal: centralizing the buffered HTTP wire format and redirect logic in packages/core/src/plugins/http-wire.ts makes ctx.http.fetch() binary-safe and behaviorally consistent across native execution, Cloudflare Worker Loader, and Node/workerd. It also closes the previous authority-staleness gap by keying the workerd bridge-handler cache on pluginId:version.
I re-read the changed authored files in core, cloudflare, workerd, plugin-test, and plugin-cli, the supporting tests, and the changesets/docs. The four issues from the prior review are still resolved:
network:fetchstale references are nownetwork:request.bridgeJsonReviveris applied to every backing-bridge JSON response.- The unreachable form-data marshaling paths in the workerd bridge handler are gone.
isHostAllowedinpackages/core/src/plugins/context.tsnow normalizes case and strips trailing dots, matching the Cloudflare bridge.
No new blocking security, data-loss, or build regressions were found. Test assertions cover binary round-trips, buffered body limits, redirect method/body rules, redirect modes, concurrent workerd transport, and authority eviction on plugin version changes. The two changesets are proportional and user-facing.
One minor maintainability observation remains: both generated wrappers (packages/cloudflare/src/sandbox/wrapper.ts and packages/workerd/src/sandbox/wrapper.ts) still inline byte-buffering, request-buffering, response-decoration, and the 8 MiB limit rather than importing the equivalent helpers from emdash/plugins/http-wire.ts. The duplication is functionally harmless today, but it is exactly the kind of drift that produced the runner-parity gaps this PR is fixing. If those were the two prior notes, they are still present.
There was a problem hiding this comment.
This is the right change for preserving binary HTTP round-trips across sandbox runners. Centralizing the wire format and redirect logic in packages/core/src/plugins/http-wire.ts, gating outbound work through the bridge, and keying the workerd authority cache on pluginId:version all fit EmDash's architecture and close the previous parity/staleness gaps.
I re-read the full diff, the key implementation files in core, cloudflare, workerd, plugin-test, and plugin-cli, and the accompanying tests/docs. The four issues from the prior review are still resolved:
network:fetchreferences are nownetwork:request.bridgeJsonReviveris applied to every backing-bridge JSON response inworkerd.- Unreachable form-data marshaling paths are gone.
isHostAllowednormalizes case and trailing dots consistently.
No new blocking security, data-loss, or build regressions were found. SSRF checks, cross-origin credential stripping, per-hop redirect validation, and the 8 MiB decoded body limits all behave as documented in both runners. Tests cover binary round-trips, buffered limits, redirect method/body rules, redirect modes, concurrent transport, and authority eviction on plugin version changes.
I found one concrete robustness gap in the Cloudflare wrapper: it forwards the entire buffered RequestInit object over the WorkerEntrypoint RPC boundary. Extra RequestInit fields that plugins are allowed to pass — most notably an AbortSignal, but also mode, credentials, integrity, referrer, etc. — are not serializable across RPC and will cause a runtime serialization failure that the workerd path avoids by only marshaling method/redirect/headers/body. Restricting the RPC payload to the same four fields fixes the parity issue and makes timeout signals safe to ignore without crashing.
One unrelated note: the redirect.ts 404-cap fix is a correct bug fix with its own changeset, but it is not part of the binary-HTTP feature; it would be cleaner as a separate PR.
Overall: approve once the Cloudflare wrapper narrows its RPC payload to the fields the bridge actually needs.
There was a problem hiding this comment.
Re-review of PR #3180.
The approach is still the right one: centralizing HTTP body buffering, redirect handling, and wire serialization in packages/core/src/plugins/http-wire.ts, gating all sandbox outbound requests through the bridge, and keying the workerd backing-service handler cache on pluginId:version fixes the binary-HTTP parity/staleness gaps without changing the network:request / network:request:unrestricted capability model or install consent.
I re-read the full diff and the key implementation files in core, cloudflare, workerd, plugin-test, and plugin-cli. The four issues from the previous review remain resolved:
- Remaining
network:fetchreferences were already replaced withnetwork:request. bridgeJsonReviveris applied to every backing-bridge JSON response inworkerd.- Unreachable form-data marshaling paths are gone.
isHostAllowednormalizes case and trailing dots consistently.
The one concrete blocker from the prior review is also fixed: the Cloudflare wrapper now extracts only method, redirect, headers, and body from the buffered request before sending it over the WorkerEntrypoint RPC boundary, so AbortSignal, mode, credentials, integrity, referrer, and keepalive no longer cause a runtime serialization failure. I verified this both in packages/cloudflare/src/sandbox/wrapper.ts and in the updated packages/cloudflare/tests/sandbox/wrapper-context.test.ts coverage.
I also checked:
- The new
http-wire.tshelpers deserialize into a WHATWGResponsethat preservesstatusText,url,redirected, headers, byte bodies, andclone()behavior. - Both runners apply the 8 MiB decoded request/response body limits, Fetch redirect method/body rules, per-hop host checks, credential-header stripping, and the
manual/errorredirect modes. - The workerd runner evicts versioned bridge handlers on plugin unload and guardrails
stopWorkerdagainst phantom crash-restart cycles. - Tests cover binary round-trips, limit enforcement, redirect rules, concurrent transport, and authority eviction on version changes.
- The documentation and skill instructions were reviewed against the implementation; the claims about buffered binary bodies, the 8 MiB limit, and the
host.http.respond(...)test helper are accurate.
The only non-blocking note is the same one from the previous review: the quiet-owls-count 404-cap fix is a correct, well-tested bug fix with its own changeset, but it is logically separate from the binary-HTTP feature and would be cleaner as its own PR.
No blocking issues remain; this is ready to merge.
There was a problem hiding this comment.
Re-review of PR #3180.
The approach is still the right one: centralizing HTTP body buffering, redirect handling, and wire serialization in packages/core/src/plugins/http-wire.ts, routing all sandbox outbound requests through the bridge, and keying the workerd backing-service handler cache on pluginId:version fixes the binary-HTTP parity/staleness gaps without touching the network:request / network:request:unrestricted capability or install-consent model.
I re-read the full diff and the key implementation files in core, cloudflare, workerd, plugin-test, and plugin-cli. The prior-review blockers remain fixed (the Cloudflare wrapper now extracts only serializable RequestInit fields before RPC, and the workerd bridge uses a byte reviver everywhere).
This pass turned up two related issues and one lingering scope note:
-
Capability implications are not applied in the generated wrappers. Both the Cloudflare and workerd wrappers still call
normalizeCapabilities()instead of the newnormalizePluginCapabilities(). The bridge now treatsredirects:writeas implyingredirects:read, but the wrapper won’t exposectx.redirects.list()/ctx.redirects.get()for a write-only manifest, so the sandbox-visible API is inconsistent with the bridge and with in-process plugins. The fix is a one-line import/use change in each wrapper. -
The
quiet-owls-count404-cap fix remains a correct, well-tested bug fix with its own changeset, but it is logically separate from the binary-HTTP feature. Per AGENTS.md scope discipline it would be cleaner as its own PR.
Docs and skill instructions accurately describe the new buffered-binary behavior, 8 MiB limits, and host.http.respond(...) helper. No security, data-integrity, or logged-out hot-path concerns were introduced.
Findings
-
[needs fixing]
packages/cloudflare/src/sandbox/wrapper.ts:44The wrapper still derives its capability flags with
normalizeCapabilities(), while the bridge now usesnormalizePluginCapabilities(). That means implied capabilities such asredirects:write → redirects:readare honored at the bridge but not in the generatedctx, so a plugin that declares onlyredirects:writewill not seectx.redirects.list()/ctx.redirects.get()even though the bridge would allow those calls.Use the same normalizer that the runner passes to the bridge:
import { normalizePluginCapabilities, type PluginManifest } from "emdash";const capabilities = normalizePluginCapabilities(manifest.capabilities ?? []); -
[needs fixing]
packages/workerd/src/sandbox/wrapper.ts:42Same inconsistency as the Cloudflare wrapper: the generated context uses
normalizeCapabilities()instead ofnormalizePluginCapabilities(), so implied capabilities likeredirects:write → redirects:readare invisible to the plugin proxy even though the workerd bridge now enforces them.import { normalizePluginCapabilities, type PluginManifest } from "emdash";const capabilities = normalizePluginCapabilities(manifest.capabilities); -
[suggestion]
packages/core/src/database/repositories/redirect.ts:643The 404-cap fix is correct and well-tested (using the returned
idto detect a true insert avoids the timestamp-collision bug), but it is logically separate from the binary-HTTP feature. Per the AGENTS.md scope discipline rule against drive-by changes, it should land in its own PR with its own changeset rather than being bundled here.
There was a problem hiding this comment.
Re-review of #3180 feat/sandbox-binary-http.
The binary-HTTP plumbing is solid: the new http-wire helpers are shared by both runners, the Cloudflare bridge now transports PluginHttpResponseWire (preserving bytes, status text, headers, final URL, and redirect state), the workerd bridge/base64 round-trip for Uint8Array is implemented and escaped, the test host gained http.respond/requests/clear, the backing-service handler cache is now keyed by pluginId:version, and the docs/changesets describe the new behavior accurately.
However, the two issues I flagged in the previous review are still unresolved:
-
Generated wrappers still use the wrong capability normalizer. Both
packages/cloudflare/src/sandbox/wrapper.tsandpackages/workerd/src/sandbox/wrapper.tsimport and callnormalizeCapabilities()instead of the newnormalizePluginCapabilities(). BecausenormalizeCapabilities()does not apply capability implications, a plugin that declares onlyredirects:write(or any other write-like capability) will not see the impliedredirects:readin its sandbox-visiblectx, even though the bridge binding now grants it. This contradicts the capability implications documented incapabilities.mdx. The manualcomments:moderate → comments:readpush in the wrappers also becomes redundant once the shared normalizer is used. -
The 404 row-cap fix is still bundled into this PR.
.changeset/quiet-owls-count.mdand theRedirectRepository.log404change are unrelated to binary HTTP and should be split into a separate PR.
No new blocking bugs surfaced in the cross-cutting checks (binary-body round-trips, redirect method/body rewrite, host allowlist normalization, bridge JSON escape/reviver, handler-cache invalidation, changeset prose).
Findings
-
[needs fixing]
packages/cloudflare/src/sandbox/wrapper.ts:14The generated Worker Loader wrapper still imports
normalizeCapabilities, which does not apply capability implications. The bridge binding already usesnormalizePluginCapabilities, so a plugin that declares onlyredirects:writewill be grantedredirects:readat the bridge but will not seectx.redirects.list/getin the sandbox. This contradicts the documented capability implications.import { normalizePluginCapabilities, type PluginManifest } from "emdash"; -
[needs fixing]
packages/cloudflare/src/sandbox/wrapper.ts:44-47Use
normalizePluginCapabilitieshere and remove the redundant manualcomments:moderate → comments:readpush (the shared normalizer already covers it, plusredirects:write → redirects:readand the rest).const capabilities = normalizePluginCapabilities(manifest.capabilities ?? []); const hasContentAccess = -
[needs fixing]
packages/workerd/src/sandbox/wrapper.ts:14The generated workerd wrapper still imports
normalizeCapabilities, which does not apply capability implications. The bridge handler already usesnormalizePluginCapabilities, so the sandbox-visiblectxcan fall out of sync with what the bridge actually allows.import { normalizePluginCapabilities, type PluginManifest } from "emdash"; -
[needs fixing]
packages/workerd/src/sandbox/wrapper.ts:42-45Use
normalizePluginCapabilitieshere and remove the redundant manualcomments:moderate → comments:readpush (the shared normalizer already covers it, plusredirects:write → redirects:readand the rest).const capabilities = normalizePluginCapabilities(manifest.capabilities ?? []); const hasContentAccess = capabilities.includes("content:read") || -
[suggestion]
.changeset/quiet-owls-count.md:1The 404 row-cap fix is a separate bug fix unrelated to the binary-HTTP feature. It still belongs in its own pull request rather than being a drive-by change under
feat/sandbox-binary-http.
There was a problem hiding this comment.
The binary-HTTP feature is the right change for the stated problem: it centralizes the request/response buffering in a shared http-wire module, transports a single wire representation through both sandbox runners, preserves bytes/status text/headers/URL/redirect state/clone() behavior, and keeps SSRF, host-allowlist, and capability checks in place. The Cloudflare bridge, Worker Loader wrapper, workerd wrapper/backing service, and test host are all wired consistently, and the docs/changeset describe the behavior accurately.
The previously unresolved capability-normalizer issue is fixed: both generated wrappers now import and call normalizePluginCapabilities, so implied capabilities line up with the bridge binding.
Two issues remain:
-
packages/core/src/emdash-runtime.tsstill applies a hand-rolled subset of capability implications afternormalizeCapabilities(lines 2503–2518). It missestaxonomies:write → taxonomies:read,comments:moderate → comments:read,redirects:write → redirects:read, andcontent:revisions:read → content:read, which are already covered by the new sharednormalizePluginCapabilities. This leaves the runtime-produced manifest out of sync with the wrappers and the bridge, and duplicates logic that the PR just centralized. It should be replaced with the shared normalizer. -
The 404 row-cap fix (
.changeset/quiet-owls-count.mdand theRedirectRepository.log404change) is unrelated to binary HTTP and is still bundled here. Per the repo’s scope discipline, it belongs in its own PR.
No security, data-loss, or build-breaking blockers surfaced in this pass. Addressing the two items above would make this approveable.
Findings
-
[needs fixing]
packages/core/src/emdash-runtime.ts:2503The runtime manifest builder still hand-rolls a partial set of capability implications using the old
normalizeCapabilities. It duplicates logic that this PR just centralized innormalizePluginCapabilitiesand misses four implications that the wrappers and bridge now apply:taxonomies:write → taxonomies:read,comments:moderate → comments:read,redirects:write → redirects:read, andcontent:revisions:read → content:read. That leaves the manifest cached by the runtime out of sync with what the sandbox actually grants.const capabilities = normalizePluginCapabilities(entry.capabilities ?? []);Also update the import on line 124 from
normalizeCapabilitiestonormalizePluginCapabilities. -
[needs fixing]
.changeset/quiet-owls-count.md:1This changeset and the
RedirectRepository.log404change are an unrelated bug fix; they are still bundled into the binary-HTTP feature PR. They should be split into a separate PR per the repo’s scope discipline.
There was a problem hiding this comment.
Re-review of the binary-HTTP sandbox feature. The approach is sound: centralizing request/response buffering in packages/core/src/plugins/http-wire.ts and carrying a single wire representation through both the Cloudflare Worker Loader bridge and the Node/workerd runner keeps binary bodies, status text, headers, final URL, redirect state, and clone() behavior consistent. SSRF, host-allowlist, capability, and cross-origin credential checks are preserved on every redirect hop in both runners, and the workerd backing-service cache invalidation prevents stale network authority from surviving plugin updates.
From the previous review:
- Fixed:
packages/core/src/emdash-runtime.tsnow imports and callsnormalizePluginCapabilities()instead of hand-rolling a partial subset of implications, so the runtime-produced manifest matches the wrappers and the bridge. - Still open: the 404 row-cap fix (
.changeset/quiet-owls-count.mdand theRedirectRepository.log404change) remains bundled in this feature PR. It is unrelated to binary HTTP and should be split out per the repo’s scope discipline.
I spot-checked the new http-wire module, both generated wrappers, the Cloudflare and workerd bridge paths, the docs changes, and the plugin-test runtime host fixtures. No new logic bugs, security regressions, or build-breaking issues surfaced in this pass. Once the unrelated 404 fix is moved to its own PR, this will be approvable.
| @@ -648,7 +649,7 @@ export class RedirectRepository { | |||
| const result = await this.db | |||
| .insertInto("_emdash_404_log") | |||
| .values({ | |||
| id: ulid(), | |||
| id, | |||
| path: entry.path, | |||
| referrer, | |||
| user_agent: userAgent, | |||
| @@ -666,13 +667,13 @@ export class RedirectRepository { | |||
| ip, | |||
| }), | |||
| ) | |||
| .returning("created_at") | |||
| .returning("id") | |||
| .executeTakeFirst(); | |||
|
|
|||
| // The conflict branch only updates existing rows, so repeat hits | |||
| // cannot grow the table. Only enforce the row cap when we actually | |||
| // inserted a new path. | |||
| if (result?.created_at !== now) return; | |||
| if (result?.id !== id) return; | |||
There was a problem hiding this comment.
[needs fixing] This 404 row-cap optimization is unrelated to the binary-HTTP sandbox feature. It changes RedirectRepository.log404() to generate a per-call ULID and skip enforce404Cap() when the upsert hit an existing path, which is a separate bug fix.
Bundling it here violates scope discipline and makes the binary-HTTP changeset/release carry a behavior change that has nothing to do with ctx.http.fetch(). The 404 fix should move to its own PR with its own changeset.
| // (remove the unrelated 404 row-cap optimization from this PR) |
| @@ -0,0 +1,5 @@ | |||
| --- | |||
There was a problem hiding this comment.
[needs fixing] This changeset documents a 404 row-cap fix that is unrelated to the binary-HTTP feature described by the PR and the binary-plugin-http.md changeset. It should be removed from this PR and included with the separate PR that contains the RedirectRepository.log404 change.
There was a problem hiding this comment.
The PR’s core approach—buffering binary request/response bodies in a shared HTTP wire and using it in both sandbox runners—is sound, and the earlier capability-normalization duplication has been fixed. I still see three issues worth raising: an unrelated 404 fix bundled into the same PR, a logged-out query-count regression on the 404 path, and a security-parity gap between the Cloudflare and workerd/Node SSRF enforcement.
Findings
-
[needs fixing]
.changeset/quiet-owls-count.md:1This changeset, plus the
RedirectRepository.log404change andpackages/core/tests/integration/redirects/log404-bounded.test.ts, is unrelated to preserving binary HTTP in plugin sandboxes. AGENTS.md scope discipline requires unrelated fixes to be split into separate PRs. Remove the 404 row-cap work from this PR. -
[needs fixing]
packages/core/src/database/repositories/redirect.ts:677After every new unique 404 path,
log404callsawait this.enforce404Cap(), which runsCOUNT(*)on_emdash_404_logand can issue aDELETE. This adds queries on a logged-out public hot path, violating AGENTS.md’s rule that the logged-out hot path should only ratchet down. Move cap enforcement off the request path to a deferred job or scheduled pruning task. -
[suggestion]
packages/cloudflare/src/sandbox/bridge-http.ts:246The Cloudflare path only blocks literal private IPs and known internal hostnames. The workerd/Node path uses
resolveAndValidateExternalUrl, which resolves hostnames via DoH and rejects returned private addresses. If Cloudflare Workers’ egress assumption is ever wrong, this becomes a real SSRF gap. Add a regression test or explicit documentation that proves/records the platform egress guarantee, or unify the two paths so they share the same DNS-aware validator.
What does this PR do?
Makes
ctx.http.fetch()binary-safe and behaviorally equivalent across native execution, Cloudflare Worker Loader, and Node/workerd.blob(), andclone()behavior.@emdash-cms/plugin-test, plus public and generated authoring guidance.This is Stack D PR 1 and builds on the runtime test foundation in #3162. The existing
network:requestandnetwork:request:unrestrictedcapabilities and install consent remain unchanged.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
Not applicable: this PR does not change the UI.
Validation:
pnpm buildpnpm typecheckpnpm lintpnpm lint:json— 0 diagnosticspnpm --dir docs build— 85 pages@emdash-cms/plugin-test— 13 passing Worker Loader/runtime-host testsmain...aa9830dd2— passed after all findings were resolved and re-checkedLocal
publintchecks pass.attw --packcurrently exits inside the CLI foremdashand@emdash-cms/cloudflarewithCannot read properties of undefined (reading 'filename')after packing; builds and typechecks pass.