[miniflare] Authenticate remote bindings with Cloudflare Access service tokens - #14198
[miniflare] Authenticate remote bindings with Cloudflare Access service tokens#14198krys-cf wants to merge 4 commits into
Conversation
…ce tokens When the workers.dev domain is behind Cloudflare Access, remote bindings (AI, Vectorize, Images, Artifacts, etc.) failed with 401/403 because the proxy client never sent Access credentials. Attach CF-Access-Client-Id / CF-Access-Client-Secret (from CLOUDFLARE_ACCESS_CLIENT_ID / CLOUDFLARE_ACCESS_CLIENT_SECRET env vars) to both: - the HTTP makeFetch path (wrapped-fetcher bindings: AI, Vectorize, Images) - the capnweb WebSocket path (RPC bindings: Artifacts) via a fetch()-based Upgrade so headers ride the handshake (new WebSocket(url) cannot set headers) Consistent with getAccessHeaders() which already reads these env vars for the realish-preview HTTP path. No behavior change when the env vars are unset.
🦋 Changeset detectedLatest commit: 8ca8bdc The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Return undefined for 'then' while the capnweb WebSocket is connecting so the proxy is not treated as a thenable (an await would otherwise dispatch a bogus remote 'then' RPC). - Attach a no-op .catch() to stubPromise so a failed WS upgrade isn't an unhandled rejection when only the .fetch() path is used; the error still surfaces on RPC access via the awaited stubPromise.
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
@cloudflare/wrangler-bundler
commit: |
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
Thanks for tackling this @krys-cf. It looks like it will solve the final missing piece of this puzzle.
I will have a quick think about how we can add tests for this.
One thing I would like to change before we land is how the credentials enter Miniflare.
The PR reads CLOUDFLARE_ACCESS_CLIENT_ID / CLOUDFLARE_ACCESS_CLIENT_SECRET from process.env inside packages/miniflare/src/plugins/shared/constants.ts. I noticed that mirrors the existing process.env.CF_TRACE_ID read on the line above, so there's clearly precedent — I don't think we should lean into that precedent.
- @cloudflare/workers-auth's getAccessHeaders(domain) is the canonical helper that the realish-preview hop already goes through (packages/wrangler/src/api/startDevWorker/RemoteRuntimeController.ts:325). On top of the two env vars it also covers:
- Interactive cookie auth via cloudflared access login → Cookie: CF_Authorization=…
- "Only one of two vars set" warning
- Non-interactive UserError with the Service Tokens docs link
Reading the env directly skips all three.
I'm mostly curious about the cookie case — both hops terminate on workers.dev and are gated by the same Access policy, so a user who's authenticated via cloudflared for the realish-preview will still see 403s on the binding hop, right? Or am I missing something?
- More generally, having Miniflare reach into process.env for wrangler-flavoured variable names feels like it's pulling the abstraction backwards a little — Vite plugin / vitest-pool-workers / getPlatformProxy / programmatic users all touch this code path, and they may not have those vars set even when they're configured for remote bindings via the wrangler API.
Since both hops want the same headers, startRemoteProxySession() could compute them once via getAccessHeaders(remoteProxyConnectionString.hostname) and expose them on the returned session as an opaque header bag, which Miniflare then forwards on the binding traffic:
// wrangler: start-remote-proxy-session.ts
export type RemoteProxySession = Pick<Worker, "ready" | "dispose"> & {
updateBindings: (bindings: StartDevWorkerInput["bindings"]) => Promise<void>;
remoteProxyConnectionString: RemoteProxyConnectionString;
remoteProxyHeaders?: Record<string, string>;
};// miniflare: CoreSharedOptionsSchema
remoteProxyHeaders: z.record(z.string()).optional(),Then remoteProxyClientWorker() takes the headers as a parameter (forwarded as a single JSON text binding so the worker side stays generic for either the Service Token pair or a Cookie), each plugin's getServices() passes sharedOptions.remoteProxyHeaders in, and makeFetch / makeRemoteProxyStub / connectWebSocketWithHeaders just spread the bag onto outgoing requests instead of hard-coding CF-Access-Client-Id / CF-Access-Client-Secret.
| // While the WebSocket is still connecting, this branch handles | ||
| // every property access. Return `undefined` for `then` so the | ||
| // proxy is not mistaken for a thenable — otherwise `await`-ing | ||
| // the stub (or any implicit thenable check) would invoke the | ||
| // wrapper for `then` and dispatch a bogus remote `then` RPC. |
There was a problem hiding this comment.
It took me a while to get my head around this comment.
So what we are actually saying here is:
- While we wait for the stub promise to resolve, we are returning a function that when called will wait for the stub and then delegate to the necessary stub property/method.
- In normal processing, capnweb will always return
undefinedfor thethenproperty on the top level stub. So we must not return the above function if the requested property isthen.
|
Regarding testing, the remote-bindings paths are actually testable locally without a real Access-protected origin. There's already most of what's needed:
So a real workers.dev Access origin is really only needed for a true e2e — the plumbing itself can be unit-tested. A suggested set of layers:
One thing layer 4 would surface: if the headers come from And if the headers end up traveling with |
|
@krys-cf - would you like me to help you make these changes and add the tests? |
|
@petebacondarwin would appreciate the help! Anything to get this over the line. 👍 |
…ce then-guard with a capnweb transport - Resolve the proxy host's Access headers wrangler-side via the canonical getAccessHeaders() helper (service token + cloudflared cookie + warnings) instead of reading process.env inside Miniflare. - Carry the opaque header bag on the RemoteProxyConnectionString so it travels per-connection (multiworker-correct) and forward it as a single JSON binding. - Drive the capnweb WebSocket upgrade through a custom RpcTransport so the stub is created synchronously, removing the thenable guard and orphaned-rejection workaround. - Add tests: Miniflare HTTP (service-token + cookie + none) and WebSocket handshake; wrangler getAccessHeaders wiring, per-host resolution, and the Miniflare options plumbing.
|
Thanks @petebacondarwin — took a pass at all of this and just pushed. Summary of what changed and two things I'd like your read on before we call it done. Credentials now come from Decision 1 — where the header bag lives. Your sketch put it on Decision 2 — the capnweb (not a blocker): the underlying gap is that Tests (all using the harnesses you pointed at — thank you, that saved a ton of time): Miniflare HTTP path (service-token + cookie + none-configured), Miniflare WebSocket handshake, wrangler Let me know on the two decisions above and I'll square away the multiworker test. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
getAccessHeaders() can throw (non-interactive UserError without a service token, or a cloudflared failure). On that path — and on the pre-existing worker-error path — the started proxy worker was never disposed, leaking the process for callers that catch and continue (e.g. getPlatformProxy). Dispose before re-throwing. Adds a regression test.
| remoteProxyHeaders = await getAccessHeaders( | ||
| remoteProxyConnectionString.hostname | ||
| ); |
There was a problem hiding this comment.
🔴 getAccessHeaders called with localhost hostname instead of workers.dev hostname, making the feature a no-op
worker.url resolves to the local ProxyWorker URL (e.g. http://127.0.0.1:PORT/), not the remote workers.dev URL. So remoteProxyConnectionString.hostname at line 160 evaluates to 127.0.0.1 or localhost. Calling getAccessHeaders("127.0.0.1") always returns {} because localhost is never behind Cloudflare Access, meaning hasRemoteProxyHeaders is always false and no auth headers are ever attached to the connection string.
The test at packages/wrangler/src/__tests__/api/remoteBindings/start-remote-proxy-session.test.ts passes only because it mocks startWorker to return url: Promise.resolve(new URL("https://proxy-a.example.workers.dev/")) — a workers.dev URL — but the real startWorker → DevEnv → ProxyController creates a local Miniflare instance whose .ready (and thus worker.url) resolves to a local address (ProxyController.ts:228 passes the local Miniflare URL to emitReadyEvent). The entire Access-header feature added by this PR is therefore inert in production.
Prompt for agents
The getAccessHeaders call on line 159-161 uses remoteProxyConnectionString.hostname, but remoteProxyConnectionString comes from worker.url which is the local ProxyWorker URL (e.g. http://127.0.0.1:PORT/). The hostname is always 127.0.0.1 or localhost, never the workers.dev domain, so getAccessHeaders always returns empty headers.
The correct hostname is the remote workers.dev host where the proxy server worker is deployed. This hostname is known to the RemoteRuntimeController as token.host (see RemoteRuntimeController.ts line 325-335 where it already resolves Access headers for the preview hop). You need to either:
1. Extract the workers.dev hostname from the DevEnv/RemoteRuntimeController after the remote worker is deployed and use that for the getAccessHeaders call, OR
2. Expose the remote host from the Worker object returned by startWorker so startRemoteProxySession can access it.
Note that the RemoteRuntimeController already handles Access headers for its own preview-token hop (line 325). Consider whether the binding-proxy traffic also needs separate Access headers, or if the existing ProxyController forwarding already handles it (since binding client workers talk to the local proxy, which then forwards to the remote server with the preview token + access headers from proxyData).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
@krys-cf - this is the conclusion I was coming to as well. I am putting together a test to prove this out.
| abort(reason: unknown): void { | ||
| if (this.#socket) { | ||
| const message = reason instanceof Error ? reason.message : `${reason}`; | ||
| try { | ||
| this.#socket.close(3000, message); | ||
| } catch { | ||
| // best-effort | ||
| } | ||
| } | ||
| if (this.#error === undefined) { | ||
| this.#error = reason; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 FetchWebSocketTransport.abort() does not reject a pending receive() promise, causing potential hang
When abort(reason) is called while a receive() promise is pending (i.e. #receiveRejecter is set), the method sets this.#error but does not call #receiveRejecter(reason). The subsequent WebSocket close event triggers #receivedError, but that method's guard if (this.#error === undefined) prevents it from rejecting the pending promise since abort() already set #error. The pending receive() promise thus never resolves or rejects.
Sequence causing the hang
- capnweb calls
receive()→ creates a Promise with#receiveResolver/#receiveRejecter - capnweb calls
abort(error)→ closes socket, sets#error = error, but does NOT call#receiveRejecter - The close event fires →
#receivedErrorcalled →#error !== undefined→ no-op - The Promise from step 1 never settles → hang
| abort(reason: unknown): void { | |
| if (this.#socket) { | |
| const message = reason instanceof Error ? reason.message : `${reason}`; | |
| try { | |
| this.#socket.close(3000, message); | |
| } catch { | |
| // best-effort | |
| } | |
| } | |
| if (this.#error === undefined) { | |
| this.#error = reason; | |
| } | |
| } | |
| abort(reason: unknown): void { | |
| if (this.#socket) { | |
| const message = reason instanceof Error ? reason.message : `${reason}`; | |
| try { | |
| this.#socket.close(3000, message); | |
| } catch { | |
| // best-effort | |
| } | |
| } | |
| if (this.#error === undefined) { | |
| this.#error = reason; | |
| if (this.#receiveRejecter) { | |
| this.#receiveRejecter(reason); | |
| this.#receiveResolver = undefined; | |
| this.#receiveRejecter = undefined; | |
| } | |
| } | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| let proxyUrl = new URL("https://proxy-a.example.workers.dev/"); | ||
| // Captures the most recently created fake worker so tests can assert on its | ||
| // `dispose` (e.g. the cleanup-on-throw path). | ||
| let lastFakeWorker: ReturnType<typeof makeFakeWorker> | undefined; | ||
| function makeFakeWorker() { | ||
| const worker = { | ||
| ready: Promise.resolve(), | ||
| url: Promise.resolve(proxyUrl), | ||
| dispose: vi.fn(async () => {}), | ||
| patchConfig: vi.fn(async () => {}), | ||
| raw: { | ||
| addListener: vi.fn(), | ||
| proxy: { | ||
| localServerReady: { promise: Promise.resolve() }, | ||
| runtimeMessageMutex: { drained: vi.fn(async () => {}) }, | ||
| }, | ||
| }, | ||
| }; | ||
| lastFakeWorker = worker; | ||
| return worker; |
There was a problem hiding this comment.
🚩 Test mocks don't reflect production worker.url behavior
The unit test at packages/wrangler/src/__tests__/api/remoteBindings/start-remote-proxy-session.test.ts:9 sets proxyUrl = new URL("https://proxy-a.example.workers.dev/") and wires it as the fake worker's .url. In production, worker.url resolves from DevEnv.proxy.ready.promise.then((ev) => ev.url) (DevEnv.ts:214), which is the local Miniflare ProxyWorker's listen address (e.g. http://127.0.0.1:PORT/). The mock therefore doesn't exercise the real hostname resolution path, masking BUG-0001. Consider using a localhost URL in the mock and separately testing the hostname lookup.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Authenticate the binding-proxy hop the same way the realish-preview hop is | ||
| // authenticated: resolve Access headers for the proxy server's host via the | ||
| // canonical `getAccessHeaders()` helper (service token, the | ||
| // "only one var set" warning, the non-interactive UserError, and interactive | ||
| // `cloudflared` cookie auth all come for free) and carry them on the | ||
| // connection string so Miniflare attaches them to every request it makes to | ||
| // the proxy server. Both hops terminate on the same workers.dev host behind | ||
| // the same Access application, so the lookup is typically already cached. | ||
| // | ||
| // Imported lazily: a static import pulls the `@cloudflare/workers-auth` / | ||
| // `@cloudflare/workers-utils` chain into this module's eval, which perturbs | ||
| // module init order for importers that only need the proxy session (e.g. | ||
| // tests that mock this function), so keep it off the module load path. | ||
| // | ||
| // `getAccessHeaders()` can throw (a non-interactive `UserError` when the host | ||
| // is behind Access with no service token, or a `cloudflared` failure), so | ||
| // dispose the already-started worker before propagating to avoid leaking the | ||
| // proxy process for callers that catch and continue (e.g. getPlatformProxy). | ||
| let remoteProxyHeaders: Record<string, string>; | ||
| try { | ||
| const { getAccessHeaders } = await import("../../user/access"); | ||
| remoteProxyHeaders = await getAccessHeaders( | ||
| remoteProxyConnectionString.hostname | ||
| ); | ||
| } catch (error) { | ||
| await worker.dispose().catch(() => {}); | ||
| throw error; | ||
| } | ||
| const hasRemoteProxyHeaders = Object.keys(remoteProxyHeaders).length > 0; | ||
| if (hasRemoteProxyHeaders) { | ||
| remoteProxyConnectionString.remoteProxyHeaders = remoteProxyHeaders; | ||
| } |
There was a problem hiding this comment.
🚩 Existing RemoteRuntimeController already handles Access headers for the preview hop
At RemoteRuntimeController.ts:325, the existing code already calls getAccessHeaders(token.host) with the correct workers.dev hostname and includes the result in proxyData.headers (line 335). The ProxyWorker then attaches these headers when forwarding requests to the remote server. This means the hop from the local ProxyController to the remote workers.dev worker is already authenticated. The new code in this PR attempts to add Access headers to the hop from binding client workers to the local ProxyController — but that hop is localhost-to-localhost and doesn't traverse Access. It's worth investigating whether there's actually a scenario where Access headers are needed on the client→local-proxy hop, or whether the existing mechanism fully covers all cases.
Was this helpful? React with 👍 or 👎 to provide feedback.
…forward-header instrumentation Adds a credentialed E2E repro for PR #14198 (companion to the baseline tests in the rest of this branch). One `wrangler dev` worker exercises both remote-binding proxy code paths and reports each independently in a single JSON response, so a single request tells us which path works/fails and why: - `ai` → HTTP `makeFetch` (wrapped fetcher — the AI `invalid_token` case in #14198) - `serviceFetch` → HTTP `makeFetch` (service binding `.fetch`) - `rpc` → WebSocket `makeRemoteProxyStub` (capnweb / RPC — the Artifacts case in #14198) The repro lives at `investigation/remote-access-repro/`, outside the pnpm-workspace globs (so CI, lint, type-check, fixture validation, and turbo all skip it). It is **manual, account-dependent**: it requires Workers AI, a deployed target worker, Access on the account's workers.dev subdomain with a Service Auth policy, and Service Token creds. The README documents prereqs, the run protocol, scenario matrix (A: main no creds, B: main + creds [the decisive run], C/D: PR #14198 +/- creds), and the interpretation table. Also adds a THROWAWAY debug log in `packages/wrangler/templates/startDevWorker/ProxyWorker.ts` — grep for `mf-access-debug` — that logs, for every request the local ProxyWorker forwards to the workers.dev edge (HTTP and WebSocket upgrades alike), whether the merged headers carry the Access service-token / cookie / preview token. This is the smoking gun for the central question: do the Access headers reach the edge on the WS-upgrade hop? **Must be reverted before any non-investigation merge** — it is a `console.log` in a template, gated only by being in this debug branch.
|
Closing — superseded by released behavior. After end-to-end verification, remote bindings (Workers AI, Vectorize, Artifacts) work behind Cloudflare Access on the current released toolchain ( Verified two ways:
Released wrangler already authenticates the proxy via its own |
3 participants
What
When
wrangler dev(or the Vite plugin) uses remote bindings against a Worker whose*.workers.devdomain is protected by Cloudflare Access, requests from the local remote-bindings proxy client to the remote proxy server are rejected with a401/403. This breaks every remote binding behind Access — Workers AI, AI Gateway, Vectorize, Images, Artifacts, etc.This builds on #14008 / #14011 (which fixed Access service-token auth for the realish-preview HTTP path and added a block warning) by also authenticating the binding proxy traffic itself. There are two distinct code paths and both needed fixing:
makeFetchCF-Access-Client-Id/CF-Access-Client-Secretheaders to the request to the proxy servermakeRemoteProxyStubfetch()upgrade (Upgrade: websocket) so the Access headers ride the handshake —new WebSocket(url)cannot set request headers in the Workers runtimeCredentials are read from
CLOUDFLARE_ACCESS_CLIENT_ID/CLOUDFLARE_ACCESS_CLIENT_SECRET— the same Service Token env vars thatgetAccessHeaders()already uses for the realish-preview HTTP path (packages/wrangler/src/user/access.ts), so this is consistent with existing wrangler conventions. They're forwarded to the proxy client worker as text bindings (accessClientId/accessClientSecret).When the env vars are unset, behaviour is unchanged.
Why the WebSocket change
capnweb'snewWebSocketRpcSessionaccepts either a URL string (which it upgrades withnew WebSocket(url)— no header support) or a pre-connectedWebSocket. When Access credentials are present we do the upgrade ourselves viafetch(httpUrl, { headers: { Upgrade: "websocket", ...accessHeaders } })and hand the resultingresponse.webSocketto capnweb. RPC methods are async over the network, so awaiting the authenticated upgrade is transparent to callers (e.g.await env.ARTIFACTS.<method>()).Testing
Verified end-to-end against a real internal app (D1/Vectorize/AI/Artifacts, all
remote: true) on aworkers.devaccount protected by an Access policy:InferenceUpstreamError: invalid_token, now succeedsinvalid_token; patch only →invalid_tokenpnpm --filter miniflare buildpasses with 0 type errorsCLOUDFLARE_ACCESS_CLIENT_ID/CLOUDFLARE_ACCESS_CLIENT_SECRET; this extends the same mechanism to all remote bindings.