Implement no credential R2 binding mount support#691
Conversation
🦋 Changeset detectedLatest commit: 0bdabec 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 |
commit: |
🐳 Docker Images Published
Usage: FROM cloudflare/sandbox:0.0.0-pr-691-0bdabecVersion: 📦 Standalone BinaryFor arbitrary Dockerfiles: COPY --from=cloudflare/sandbox:0.0.0-pr-691-0bdabec /container-server/sandbox /sandbox
ENTRYPOINT ["/sandbox"]Download via GitHub CLI: gh run download 26043428471 -n sandbox-binaryExtract from Docker: docker run --rm cloudflare/sandbox:0.0.0-pr-691-0bdabec cat /container-server/sandbox > sandbox && chmod +x sandbox |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
/bonk review this PR |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Implement no credential R2 binding mount support
This is a well-structured feature implementation with clear separation between the egress handler, mount lifecycle, bridge integration, and validation. The architecture is sound — intercepting s3fs egress traffic at the Worker boundary and translating to native R2 binding calls is elegant and keeps credentials entirely out of the container.
Strengths
- Clean architecture: The egress handler is self-contained with no module-level mutable state; bucket params are serialized through
setOutboundByHostso they survive isolate boundaries. - Thorough error handling: Mount failure cleanup (password file, outbound handler deregistration) is handled correctly in catch blocks. The mountpoint verification post-s3fs is a good safety net.
- Good test coverage: 28 handler tests, 11 mount lifecycle tests, 27 validation tests, plus E2E coverage. The mock R2Bucket factory is well-implemented.
- Bridge refactoring: Replacing the hand-rolled bridge mount types with a union alias over the SDK types eliminates type drift and centralizes validation — nice improvement.
Issues
1. PR description says nosignrequest but the code uses dummy credentials
The PR description states: "s3fs is launched pointing at http://r2.internal/<bucket> with nosignrequest so it never attempts to sign requests." However, nosignrequest is never passed as an s3fs option anywhere in the code. Instead, the implementation writes dummy credentials (x:x) to a passwd file. This actually works (the credentials are never validated by the egress handler), but the description is inaccurate and should be updated to match the implementation.
2. CodeQL alerts — polynomial regex on parseCompleteMultipartUploadBody
CodeQL flagged two polynomial regex patterns. The <Part> regex at line 244 operates on user-provided XML from the request body. While this runs in the Worker (not exposed to the public internet), the request body originates from s3fs inside the container which is generally trusted. However, the /<Part>.*<\/Part>/ pattern could be slow on pathological input. The current implementation using indexOf + slice is actually safe — the CodeQL alert appears to be about the inner regexes (/<PartNumber>(\d+)<\/PartNumber>/ and /<ETag>("?[^<]+"?)<\/ETag>/). These run on bounded segments extracted by the indexOf loop, so the risk is low, but it may be worth adding a brief comment explaining why this is safe to suppress the alert.
3. handleGetObject buffers the entire object into memory via arrayBuffer()
At line 416, handleGetObject calls await obj.arrayBuffer() which buffers the entire R2 object into memory before returning it. For large files, this could cause memory pressure in the Worker. Streaming the R2 response body directly would be more memory-efficient:
// Instead of:
const body = await obj.arrayBuffer();
return new Response(body, { ... });
// Consider:
return new Response(obj.body, { ... });The same concern applies to the range request path (line 431) and copy source path (line 491). This is a performance concern, not a correctness issue — it works, but Workers have a 128MB memory limit.
4. R2BindingMountBucketOptions.endpoint?: never doesn't prevent runtime assignment
In packages/shared/src/types.ts:1308, endpoint?: never provides compile-time safety for TypeScript consumers but doesn't prevent bridge/HTTP API callers from passing { endpoint: "" }. The mount dispatch logic in sandbox.ts:1199 checks !remoteOptions.endpoint which is falsy for empty string — this correctly falls through to R2 egress, but an empty string endpoint might be confusing. Consider explicitly checking endpoint === undefined or documenting that empty strings are treated as "no endpoint".
5. getR2EgressParams rebuilds the full params map on every call
getR2EgressParams() iterates all active mounts to collect R2 egress entries. It's called during mount, unmount, error cleanup, and destroy. With many mounts this is fine (mounts are typically few), but consider caching or storing the R2 params separately if this becomes a hot path.
6. handlePutObject copy-source prefix application is asymmetric
At line 477-479, the source key gets the mount prefix prepended only when copySource.bucket === bucketName. But if the copy source is a different bucket that also has an R2 egress mount with its own prefix, that prefix is not applied. This is because mountPrefix is per-bucket-per-request and the handler only has the current bucket's prefix. This is acceptable for cross-bucket copies where the source should use its raw key, but worth documenting the semantics.
7. Missing nosignrequest or equivalent — s3fs may still attempt signing
The R2 egress handler never validates signatures (which is correct), but s3fs may still attempt to sign requests using the dummy credentials. The x:x dummy credentials make this work because the signature just gets ignored by the handler. This is a pragmatic workaround, but worth a code comment explaining why dummy credentials are needed syntactically.
Nits
r2-egress-handler.ts:67-71:trimTrailingSlashesis a nice utility but it's only used once (line 622). Consider inlining or leaving as-is — not a blocker.sandbox.ts:1338:const prefix = options.prefix || undefined—options.prefixis alreadystring | undefined, so|| undefinedconverts empty string to undefined. If that's intentional behavior, a brief comment would help clarify.r2-egress-mount.test.ts:341: The test usesas anyfor the options argument to bypass TypeScript'sendpoint?: nevercheck. This is fine for testing but worth noting that the type guard works correctly at the SDK API level.- The
s3fsOptionsdescription inopenapi.ts:127says "Only applies to R2 binding mounts (when endpoint is omitted)" buts3fsOptionsis also accepted for remote mounts. The description should reflect both.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review posted on PR #691. Here's a summary of the findings: Overall: Solid feature implementation with clean architecture. The egress interception approach is well-designed, keeping credentials entirely out of the container while leveraging the existing s3fs-FUSE infrastructure. Key issues raised:
4 inline comments were posted on the specific lines of code. |
scuffi
force-pushed
the
patch/add-no-credential-r2-mounts
branch
from
b7e82b6 to
1325150
Compare
aron-cf
left a comment
aron-cf
left a comment
There was a problem hiding this comment.
I'm halfway, but wanted to give some feedback while I review the egress handler. This is looking really good. I like the API and the structure.
I've got two major thoughts:
- We need to figure out how to implement the egress handler without touching the public egress methods. Currently it's very easy for a user to wipe out the handler and break the implementation. Ideally we need to isolate our handler.
- I think we need a clearer distinction between bucket and binding. Having both represented as strings isn't ideal. If you pass the binding directly into the mount/unmount functions it would remove the ambiguity. Are there benefits to also supporting the string I'm missing?
aron-cf
left a comment
aron-cf
left a comment
There was a problem hiding this comment.
Okay, left some comments on the egress handler too.
scuffi
force-pushed
the
patch/add-no-credential-r2-mounts
branch
from
a619113 to
dcd5114
Compare
scuffi
force-pushed
the
patch/add-no-credential-r2-mounts
branch
from
dcd5114 to
6940567
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ R2BindingMountBucketOptions not re-exported from public SDK package (packages/sandbox/src/index.ts:45-52)
R2BindingMountBucketOptions is exported from @repo/shared (packages/shared/src/index.ts:198) but is NOT re-exported from @cloudflare/sandbox in packages/sandbox/src/index.ts:22-62. Both other variants of MountBucketOptions — LocalMountBucketOptions (line 45) and RemoteMountBucketOptions (line 52) — are exported. This breaks the established pattern and prevents SDK consumers from referencing the new R2 binding mount type by name when narrowing MountBucketOptions.
View 6 additional findings in Devin Review.
aron-cf
left a comment
aron-cf
left a comment
There was a problem hiding this comment.
This is getting really close. Well done fixing that egress issue. It looks like some tests are failing now though.
| const R2_DEFAULT_S3FS_OPTIONS: readonly string[] = [ | ||
| 'stat_cache_expire=60', | ||
| 'enable_noobj_cache', | ||
| 'multipart_size=5' | ||
| ]; |
There was a problem hiding this comment.
In the container side I refactored this to handle options as an object. It makes it much easier to work with.
{
stat_cache_expire: "60",
enable_noobj_cache: true,
multipart_size: "5",
}
Then just serialize the object when needed (true become single strings only, false is dropped).
There was a problem hiding this comment.
really like this shape! (re: object)
| } else if (mountInfo.mountType === 'r2-egress') { | ||
| if (mountInfo.mounted) { | ||
| try { | ||
| this.logger.debug( | ||
| `Unmounting R2 egress bucket ${mountInfo.bucket} from ${mountPath}` | ||
| ); | ||
| const result = await this.execInternal( | ||
| `fusermount -u ${shellEscape(mountPath)}` | ||
| ); | ||
| if (result.exitCode !== 0) { | ||
| throw new Error( | ||
| `fusermount -u failed (exit ${result.exitCode}): ${result.stderr || 'unknown error'}` | ||
| ); | ||
| } | ||
| mountInfo.mounted = false; | ||
| } catch (error) { | ||
| mountFailures++; | ||
| const errorMsg = | ||
| error instanceof Error ? error.message : String(error); | ||
| this.logger.warn( | ||
| `Failed to unmount R2 egress bucket ${mountInfo.bucket} from ${mountPath}: ${errorMsg}` | ||
| ); | ||
| } | ||
| } | ||
| await this.deletePasswordFile(mountInfo.passwordFilePath); |
There was a problem hiding this comment.
This still duplicates most of the following else block.
| const fetcher = ctx.exports.ContainerProxy({ | ||
| props: { | ||
| enableInternet: this.enableInternet, | ||
| containerId: this.ctx.id.toString(), | ||
| className: R2_EGRESS_PROXY_TARGET_CLASS_NAME, | ||
| outboundByHostOverrides: { | ||
| 'r2.internal': { | ||
| method: 'r2EgressMount', | ||
| params | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| if (!isFetcher(fetcher)) { | ||
| throw new InvalidMountConfigError( | ||
| 'R2 binding mounts require ContainerProxy to return a valid Fetcher' | ||
| ); | ||
| } |
| } | ||
|
|
||
| private async configureR2EgressOutbound( | ||
| params: R2EgressParams = this.getR2EgressParams() |
There was a problem hiding this comment.
This is a bit weird, I think I'd prefer it to always just be passed params for clarity at the call site.
| const remainingR2Params = this.getR2EgressParams(); | ||
| await this.configureR2EgressOutbound(remainingR2Params); |
There was a problem hiding this comment.
This is equivalent to just calling configureR2EgressOutbound() no?
| }); | ||
| } | ||
| const upload = r2.resumeMultipartUpload(key, uploadId); | ||
| const body = await readRequestBody(request); |
There was a problem hiding this comment.
This can't do this. Can we not make this work with streams? As we're acting as a proxy, I'm assuming the request headers must contain the data we need about content length and etag etc?
| For compatibility, `bucket` is also accepted as the binding name when `binding` | ||
| is omitted and `options.endpoint` is not provided. |
There was a problem hiding this comment.
We don't need compatibility, this is a new feature.
scuffi
force-pushed
the
patch/add-no-credential-r2-mounts
branch
from
5dce680 to
6f11de0
Compare
scuffi
force-pushed
the
patch/add-no-credential-r2-mounts
branch
from
a5a6b0c to
72d86e1
Compare
scuffi
force-pushed
the
patch/add-no-credential-r2-mounts
branch
from
72d86e1 to
728f9ec
Compare
scuffi
force-pushed
the
patch/add-no-credential-r2-mounts
branch
from
f86c510 to
5b580f9
Compare
scuffi
force-pushed
the
patch/add-no-credential-r2-mounts
branch
from
348bae5 to
0bdabec
Compare
4 participants
Summary
This PR implements a new mount path for R2 bindings that never passes credentials into the container. Instead of supplying an S3-compatible endpoint and access keys, users pass a
bindingNamereferencing an R2 binding on their Worker. The DO intercepts s3fs HTTP traffic at the Worker boundary, translates S3 API calls into native R2 binding calls, and proxies the result back — keeping credentials entirely in the Worker runtime.How it works
mountBucket()detects noendpointon the options, it takes the R2 egress path./tmp/.passwd-s3fs-<uuid>inside the container (dummy credentials — s3fs requires them syntactically, but they are never validated by R2).http://r2.internal/<bucket>(not HTTPS, meaninginterceptHttpsis not required for this feature).r2.internalis intercepted by the new R2 egress handler, which:readOnlyby rejecting mutating operations with 403.Speed comparison R2 binding vs S3 native
The R2 path does introduce a slight overhead for most operations (mostly negligible ~few ms), except for writes. We have a pretty noticeable overhead for write operations through R2 bindings (~1.5x-2x slower). I'm yet to find an approach to speed this up, so this will likely be come back to in future.
New usage
SDK (production — R2 binding, no credentials):
SDK (existing — remote S3/R2 endpoint with explicit credentials, unchanged):
Bridge HTTP API:
Bridge types
The old bridge-specific mount option types were replaced with a direct union alias over the SDK's own
MountBucketOptions. This removes the impedance mismatch between the bridge API surface and the SDK:MountBucketRequestOptionsis now a union ofRemoteMountBucketOptions | R2BindingMountBucketOptions | LocalMountBucketOptions— the same union exported from the SDK package.routes.ts— all type narrowing happens once beforesandbox.mountBucket()is called.endpoint, non-object or arrayoptions, non-stringcredentials.accessKeyId/secretAccessKey, non-booleanreadOnly, non-stringprefix.Files changed
packages/sandbox/src/storage-mount/r2-egress-handler.tspackages/sandbox/src/sandbox.tsmountBucketR2Egress()path; password file lifecycle; outbound handler registration and cleanup; mountpoint check enforcement; unmount/destroy cleanuppackages/sandbox/src/bridge/types.tspackages/sandbox/src/bridge/routes.tsvalidateMountOptions()andtoSDKMountOptions()helpers; tightened primitive type validationpackages/sandbox/src/bridge/openapi.tss3fsOptionsdescription to cover both remote and R2 mounts; fixedprefixdescriptionpackages/sandbox/src/storage-mount/validation.tsisR2BindingMounttype guardpackages/sandbox/src/storage-mount/types.tsR2EgressMountInfodiscriminant to theMountInfounionpackages/sandbox/src/storage-mount/index.tspackages/shared/src/types.tsR2BindingMountBucketOptions; updatedMountBucketOptionsunionbridge/worker/README.mds3fsOptions,prefix, andreadOnlyin the mount API referencepackages/sandbox/tests/r2-egress-handler.test.tspackages/sandbox/tests/r2-egress-mount.test.tspackages/sandbox/tests/storage-mount/validation.test.tsbridge/worker/src/__tests__/mount.test.tstests/e2e/bucket-mounting.test.ts