Repository navigation
[vite-plugin] Fix v1 signed R2 S3 requests - #16580
petebacondarwin wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 6f1770b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
@pmiguel (original author) @dario-piotrowicz (verified human ant-core approver): please check this v1 backport of #16426 and approve it if it looks good. V1 exposes the local S3 endpoint via |
739e363 to
9676432
Compare
|
The backport approach looks correct to me at first glance. The pending release that includes the fix (#16556) now targets Wrangler 4.156.0, so you should update the pinned version. Follow up validation to be done after the new packages get published, as noted. |
9676432 to
31ecdfc
Compare
commit: |
|
@dexvdev (original author) @petebacondarwin (verified human ant-core approver): please check this v1 backport coverage of #16327 and approve it if it looks good. This existing PR's published Miniflare 5.20261023.3-beta pin supplies the cross-process binding crash-restart fix to V1's development registry. No separate source copy, upstream test, or mapping-only changeset is needed. It also contains the necessary V1 signed-S3 request adapter fix and coupled Wrangler 4.156.0 upgrade; build, types, retained tests and actual dev/preview signed requests pass. |
|
Codeowners approval required for this PR:
|
|
@workers-devprod (recorded original author, ANT Bot) @dario-piotrowicz (verified human ant-core approver): please check this v1 backport of the compatible runtime release from #16556 and approve it if it looks good. This existing PR now pins the actually published Miniflare 5.20261023.3-beta and Wrangler 4.156.0 with the matching helper graph and regenerated lockfile. It supplies #16426 and #16327 without another release-only PR. V1 additionally needed to preserve signed S3 headers in its own adapter; build/types, retained suites and signed PUT/GET through dev and preview pass. |
There was a problem hiding this comment.
Superseded by the review of
936469bd.
This review, of 22ad8138
💬 Approved with comments — The source change is small and correct, and the tests cover it. The one remaining note asks for a fuller changeset and does not block merging.
Findings: 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🚀 Release: 💬 1 info
The changeset leaves out the second runtime fix that comes with the Miniflare upgrade
Recommendation: 💬 Optional Nits
- [INFO] The changeset could mention the bundled Miniflare fix for cross-process service, Durable Object and tail bindings after a crash restart, which is part of this upgrade but missing from the release note. (
.changeset/v1-r2-s3-signed-encoding.md:7)
View full review
The Miniflare 5.20261023.3-beta upgrade also fixes cross-process service, Durable Object and tail bindings losing their routing after a runtime crash restart. The changeset only describes the signed S3 header fix. Mentioning the restart fix too would give plugin users the full effect of the upgrade.
✅ All other checks passed
🔒 Security: ✅ LGTM
No security issues found.
📖 AGENTS.md: ✅ LGTM
The change follows the repository's maintenance-branch rules. Private packages keep workspace:* references, public SDK packages are pinned to exact published versions, and there is a plugin changeset.
📋 Code Quality: ✅ LGTM
No issues found. The bypass applies only to the exact /cdn-cgi/local/r2/s3 route and paths below it, and the regression tests drive the real request handler.
⚡ Performance: ✅ LGTM
No performance issues found.
📝 Documentation: ✅ LGTM
No documentation issues found.
Reviewed commit: 22ad8138
22ad813 to
936469b
Compare
There was a problem hiding this comment.
Superseded by the review of
07e313c1.
This review, of 936469bd
💬 Approved with comments — The header-preservation fix is correctly limited to the S3 route and has tests that drive the real request handler. The only open item is an optional note about how complete the changeset is.
Findings: 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🚀 Release: 💬 1 info
One optional release-note note remains open.
Recommendation: 💬 Optional Nits
- [INFO] The changeset could mention the bundled Miniflare fix that keeps cross-process service, Durable Object and tail bindings routed after a runtime crash restart. This fix is part of the upgrade but is missing from the release note. (
.changeset/v1-r2-s3-signed-encoding.md:7)
View full review
Risk: Low
.changeset/v1-r2-s3-signed-encoding.md:7 describes the signed S3 header fix. The Miniflare 5.20261023.3-beta upgrade also keeps cross-process service, Durable Object and tail bindings routed after a runtime crash restart, and the changeset does not mention that. A short mention would give plugin users the full user-visible effect of the upgrade. The pull request description says no separate changeset is needed, so this is optional.
✅ All other checks passed
🔒 Security: ✅ LGTM
The route match uses the parsed URL path, so lookalike paths do not match. The change gives no new access to the endpoint. The dependency changes are version pins only.
📖 AGENTS.md: ✅ LGTM
The change follows the maintenance-branch rules:
- Private packages keep their
workspace:*references. - Public SDK packages are pinned to exact published versions.
- The change includes a plugin changeset.
📋 Code Quality: ✅ LGTM
The bypass applies only to the exact /cdn-cgi/local/r2/s3 route and paths below it. The regression tests cover the boundary cases, including the lookalike /cdn-cgi/local/r2/s3-other and the absent-header case. The Playwright catalog bump in pnpm-workspace.yaml matches the lockfile.
⚡ Performance: ✅ LGTM
No performance issues found.
📝 Documentation: ✅ LGTM
No documentation issues found. The note about how complete the changeset is appears in the Release section.
Reviewed commit: 936469bd
936469b to
07e313c
Compare
There was a problem hiding this comment.
Superseded by the review of
8e0d24a2.
This review, of 07e313c1
✅ Approved — No new issues: the header preservation is limited to the exact /cdn-cgi/local/r2/s3 route and the paths under it, and real HTTP tests cover it. The dependency pins follow the maintenance-branch rules.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low
The version pins and the patch changeset match the user-facing fix.
🔒 Security: ✅ LGTM
No security issues found.
📖 AGENTS.md: ✅ LGTM
The change follows the maintenance-branch rules:
- Private packages keep their
workspace:*references. - Public SDK packages are pinned to exact versions.
- The change includes a plugin changeset.
📋 Code Quality: ✅ LGTM
- Route match:
packages/vite-plugin-cloudflare/src/utils.tsskips addingidentityonly on the exact/cdn-cgi/local/r2/s3route and on paths under it. It matches on the parsed URL pathname, so a lookalike such as/cdn-cgi/local/r2/s3-otherstill gets the normal handling. - Tests:
utils.spec.tssends real HTTP requests to the handler. The cases are the exact route, a nested path, a multi-value header, a missing header, the lookalike path and a normal Worker path. - Dependency pins: the pins are exact, and private packages keep their
workspace:*references.
⚡ Performance: ✅ LGTM
No performance issues found.
📝 Documentation: ✅ LGTM
No new documentation issues. The earlier optional note about how complete the changeset is remains open; whether to act on it is up to the author.
Reviewed commit: 07e313c1
07e313c to
8e0d24a
Compare
There was a problem hiding this comment.
Superseded by the review of
6f1770bd.
This review, of 8e0d24a2
✅ Approved — No new issues: the header preservation is limited to the exact /cdn-cgi/local/r2/s3 route and the paths under it, and real HTTP tests cover it. The dependency pins follow the maintenance-branch rules.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low
The version pins and the patch changeset match the user-facing fix.
🔒 Security: ✅ LGTM
No security issues found.
📖 AGENTS.md: ✅ LGTM
The change follows the maintenance-branch rules:
- Private packages keep their
workspace:*references. - Public SDK packages are pinned to exact versions.
- The change includes a plugin changeset.
📋 Code Quality: ✅ LGTM
- Route match:
packages/vite-plugin-cloudflare/src/utils.tsskips addingidentityonly on the exact/cdn-cgi/local/r2/s3route and on paths under it. It matches on the parsed URL pathname, so a lookalike such as/cdn-cgi/local/r2/s3-otherstill gets the normal handling. - Tests:
utils.spec.tssends real HTTP requests to the handler. The cases are the exact route, a nested path, a multi-value header, a missing header, the lookalike path and a normal Worker path. - Dependency pins: the pins are exact, and private packages keep their
workspace:*references.
⚡ Performance: ✅ LGTM
No performance issues found.
📝 Documentation: ✅ LGTM
No new documentation issues. The earlier optional note about how complete the changeset is remains open; whether to act on it is up to the author.
Reviewed commit: 8e0d24a2 — same changes as 07e313c1, so this is that commit's review (no new review was run; line numbers are as of 07e313c1)
8e0d24a to
6f1770b
Compare
There was a problem hiding this comment.
✅ Approved — No findings. The adapter keeps the original Accept-Encoding header only on the canonical /cdn-cgi/local/r2/s3 route, real HTTP tests cover it, and the changeset correctly describes the fix.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
📋 Code Quality: ✅ LGTM
No issues found. utils.ts still keeps the original Accept-Encoding header only on the exact /cdn-cgi/local/r2/s3 route and the paths under it, so other Worker requests keep their normal encoding handling. The tests in utils.spec.ts send real HTTP requests for six cases:
- the exact route
- a nested path
- a multi-value header
- a missing header
- a lookalike path
- a normal Worker path
📝 Documentation: ✅ LGTM
The patch changeset for @cloudflare/vite-plugin correctly describes the fix: the original S3 header is kept, so clients that sign Accept-Encoding: identity no longer get SignatureDoesNotMatch in Vite dev or preview.
Reviewed commit: 6f1770bd
Backports the remaining Vite plugin v1 fix from #16426. Clients signing
Accept-Encoding: identityon the experimental local R2 S3 endpoint receive403 SignatureDoesNotMatchbecause the Vite adapter prepends anotheridentity. Preserve the original header (including absence) on the canonical S3 route; other Worker requests retain their encoding handling.The runtime fix and dependency upgrades are already merged through #16639 and released in v1.67.0. This PR now contains only the adapter, six HTTP request-handler regression cases and one plugin patch changeset: one commit, three files. It preserves Miniflare 5.20261101.0-beta, Wrangler 4.156.1 and their coupled helper pins. Original #16327 and release #16556 have dependency coverage through the newer merged #16639; no separate PR or dependency changelog entry is needed. #16601 remains stacked above this PR.
Validation with Node 22.22.1 / pnpm 10.33.0: genuine frozen install;
pnpm check(58 tasks, including build/types/lint/format);pnpm test:ci(8 tasks, including 237 plugin tests); existing S3 integration passes through actual Vite dev and build/preview. Prior signed identity-header dev/preview validation and regression evidence remain applicable to the unchanged adapter. No credentialed remote tests were run.Note
This is a contribution from an AI agent: Codex, under the authorized v1 backport automation.