Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: add stripHeader service to Miniflare instance on ProxyController #7914

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

chore: add stripHeader service to Miniflare instance on ProxyController #7914

wants to merge 3 commits into from

Conversation

andyjessop
Copy link
Contributor

@andyjessop andyjessop commented Jan 27, 2025 •
edited
Loading

Fixes #7835

Workerd expects to be able to set the cf-connecting-ip header itself and will throw an error if it is already set. As we added the header in this PR, we need to manually remove it.

This PR utilises the outboundService field in Miniflare that allows the client to hook into outbound requests. Here, we just remove the header and then call fetch as normal.

  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: internal change only

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jan 27, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: 60d1e61

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
wrangler Patch
@cloudflare/vitest-pool-workers Patch

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 GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 27, 2025 •
edited
Loading

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243072590/npm-package-wrangler-7914

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7914/npm-package-wrangler-7914

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243072590/npm-package-wrangler-7914 dev path/to/script.js
Additional artifacts:

cloudflare-workers-bindings-extension:

wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243072590/npm-package-cloudflare-workers-bindings-extension-7914 -O ./cloudflare-workers-bindings-extension.0.0.0-v970ebf071.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v970ebf071.vsix

create-cloudflare:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243072590/npm-package-create-cloudflare-7914 --no-auto-update

@cloudflare/kv-asset-handler:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243072590/npm-package-cloudflare-kv-asset-handler-7914

miniflare:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243072590/npm-package-miniflare-7914

@cloudflare/pages-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243072590/npm-package-cloudflare-pages-shared-7914

@cloudflare/unenv-preset:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243072590/npm-package-cloudflare-unenv-preset-7914

@cloudflare/vite-plugin:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243072590/npm-package-cloudflare-vite-plugin-7914

@cloudflare/vitest-pool-workers:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243072590/npm-package-cloudflare-vitest-pool-workers-7914

@cloudflare/workers-editor-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243072590/npm-package-cloudflare-workers-editor-shared-7914

@cloudflare/workers-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243072590/npm-package-cloudflare-workers-shared-7914

@cloudflare/workflows-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13243072590/npm-package-cloudflare-workflows-shared-7914

Note that these links will no longer work once the GitHub Actions artifact expires.

[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20250129.0
workerd 1.20250204.0 1.20250204.0
workerd --version 1.20250204.0 2025-02-04

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@andyjessop andyjessop force-pushed the aj/strip-cf-connecting-ip-header branch from 0adafea to 63500ea Compare January 27, 2025 10:34
@andyjessop andyjessop marked this pull request as ready for review January 27, 2025 10:34
@andyjessop andyjessop requested a review from a team as a code owner January 27, 2025 10:34
@andyjessop andyjessop added the e2e Run wrangler e2e tests on a PR label Jan 27, 2025
@andyjessop andyjessop force-pushed the aj/strip-cf-connecting-ip-header branch from 63500ea to d4c3b39 Compare January 27, 2025 10:42
penalosa
penalosa requested changes Jan 27, 2025
View reviewed changes
packages/wrangler/src/__tests__/api/startDevWorker/utils.test.ts Outdated
});
});

global.fetch = mockFetch as typeof global.fetch;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use MSW if possible

packages/wrangler/src/api/startDevWorker/ProxyController.ts Outdated
@@ -162,6 +162,8 @@ export class ProxyController extends Controller<ProxyControllerEventMap> {
}),
handleRuntimeStdio,
liveReload: false,

outboundService: stripHeader("cf-connecting-ip"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is the right place for this. This will strip cf-connecting-ip on outgoing fetches from the Proxy worker, which we don't want to do. I think we should be adding this for local user workers? (LocalRuntimeController)

@penalosa penalosa force-pushed the aj/strip-cf-connecting-ip-header branch from 28b301a to 60d1e61 Compare February 10, 2025 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@penalosa penalosa penalosa requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
e2e Run wrangler e2e tests on a PR
Projects
workers-sdk
Status: In Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🐛 BUG: fetch() call to public Zero Trust URL from Worker results in error 1000
2 participants
@andyjessop @penalosa