-
Notifications
You must be signed in to change notification settings - Fork 778
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
fix(playground-preview-worker,edge-preview-authenticated-proxy): ensure no body is passed when constructing a GET or HEAD request to the preview worker #7793
Conversation
🦋 Changeset detectedLatest commit: 795620c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
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/12929815285/npm-package-wrangler-7793 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7793/npm-package-wrangler-7793 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12929815285/npm-package-wrangler-7793 dev path/to/script.js Additional artifacts:cloudflare-workers-bindings-extension: wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12929815285/npm-package-cloudflare-workers-bindings-extension-7793 -O ./cloudflare-workers-bindings-extension.0.0.0-v7ffe58091.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v7ffe58091.vsix create-cloudflare: npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12929815285/npm-package-create-cloudflare-7793 --no-auto-update @cloudflare/kv-asset-handler: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12929815285/npm-package-cloudflare-kv-asset-handler-7793 miniflare: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12929815285/npm-package-miniflare-7793 @cloudflare/pages-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12929815285/npm-package-cloudflare-pages-shared-7793 @cloudflare/unenv-preset: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12929815285/npm-package-cloudflare-unenv-preset-7793 @cloudflare/vite-plugin: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12929815285/npm-package-cloudflare-vite-plugin-7793 @cloudflare/vitest-pool-workers: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12929815285/npm-package-cloudflare-vitest-pool-workers-7793 @cloudflare/workers-editor-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12929815285/npm-package-cloudflare-workers-editor-shared-7793 @cloudflare/workers-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12929815285/npm-package-cloudflare-workers-shared-7793 @cloudflare/workflows-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12929815285/npm-package-cloudflare-workflows-shared-7793 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
beea6fa
to
d7dc19c
Compare
@@ -487,6 +489,30 @@ compatibility_date = "2023-01-01" | |||
expect(await resp.text()).toEqual("PUT"); | |||
}); | |||
|
|||
it.each(["GET", "POST", "PUT", "PATCH", "DELETE", "HEAD", "OPTIONS"])( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️ 🙏 🚀
d7dc19c
to
795620c
Compare
method, | ||
headers, | ||
body: method === "GET" || method === "HEAD" ? null : request.body, | ||
redirect: "manual", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked which properties we can pass here and it looks like only these 4 are relevant. But if you think differently, please let me know 👍🏼
async (method) => { | ||
const token = randomBytes(4096).toString("hex"); | ||
const resp = await worker.fetch( | ||
`https://0000.rawhttp.devprod.cloudflare.dev/method`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for testing a remote worker here? Can we just use Miniflare for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is local 👍🏼 . We are running two separate instances through unstable_dev
to mimick proxy <-> remote communication:
workers-sdk/packages/edge-preview-authenticated-proxy/tests/index.test.ts
Lines 309 to 379 in 795620c
worker = await unstable_dev(path.join(__dirname, "../src/index.ts"), { | |
// @ts-expect-error TODO: figure out the right way to get the server to accept host from the request | |
host: "0000.rawhttp.devprod.cloudflare.dev", | |
ip: "127.0.0.1", | |
experimental: { | |
disableExperimentalWarning: true, | |
}, | |
}); | |
tmpDir = await fs.realpath( | |
await fs.mkdtemp(path.join(os.tmpdir(), "preview-tests")) | |
); | |
await fs.writeFile( | |
path.join(tmpDir, "remote.js"), | |
/*javascript*/ ` | |
export default { | |
fetch(request) { | |
const url = new URL(request.url) | |
if(url.pathname === "/exchange") { | |
return Response.json({ | |
token: "TEST_TOKEN", | |
prewarm: "TEST_PREWARM" | |
}) | |
} | |
if(url.pathname === "/redirect") { | |
return Response.redirect("https://example.com", 302) | |
} | |
if(url.pathname === "/method") { | |
return new Response(request.method, { | |
headers: { "Test-Http-Method": request.method }, | |
}) | |
} | |
if(url.pathname === "/status") { | |
return new Response(407) | |
} | |
if(url.pathname === "/header") { | |
return new Response(request.headers.get("X-Custom-Header")) | |
} | |
if(url.pathname === "/cookies") { | |
const headers = new Headers(); | |
headers.append("Set-Cookie", "foo=1"); | |
headers.append("Set-Cookie", "bar=2"); | |
return new Response(undefined, { | |
headers, | |
}); | |
} | |
return Response.json({ | |
url: request.url, | |
headers: [...request.headers.entries()] | |
}, { headers: { "Content-Encoding": "identity" } }) | |
} | |
} | |
`.trim() | |
); | |
await fs.writeFile( | |
path.join(tmpDir, "wrangler.toml"), | |
/*toml*/ ` | |
name = "remote-worker" | |
compatibility_date = "2023-01-01" | |
`.trim() | |
); | |
remote = await unstable_dev(path.join(tmpDir, "remote.js"), { | |
config: path.join(tmpDir, "wrangler.toml"), | |
ip: "127.0.0.1", | |
experimental: { disableExperimentalWarning: true }, | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
…re no body is passed when constructing a GET or HEAD request to the preview worker (#7793)
Fixes #7791, #7630
This PR fixes the following error we see after releasing the change on #7639.
The implementation has been updated in both
playground-preview-worker
andedge-preview-authenticated-proxy
, which are used by the worker playground and the dashboard.Note: This change itself will not affect how playground and dashboard works as the behavior is opt-in. We will have to un-revert #7639 and we should be able to verify this on the preview playground.