feat: Add an endpoint override to the r2 adapter - #133
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 784604a 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 |
📝 WalkthroughWalkthroughThe R2 SDK adds optional endpoint overrides to HTTP and hybrid binding options. The override applies to signing, fetch requests, and the lazy AWS SDK adapter. Tests cover custom hosts and generated URLs. ChangesR2 endpoint override
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zachdunn
changed the title
accountId only ever feeds the default endpoint hostname, so an explicit endpoint stands in for it — both in the HTTP path (MinIO/LocalStack stand-ins no longer need a dummy accountId) and in hybrid binding mode (the signing fallback now activates on endpoint + credentials + bucket).
|
pushed 784604a on top: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/files-sdk/src/r2/index.ts (1)
71-79: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDefine the accepted endpoint URL shape.
The fetch consumer builds path-style URLs from
endpointUrl.origin. An endpoint such ashttps://proxy.example/s3therefore loses the/s3prefix, so fetch and hybrid requests target the wrong path.Either preserve the endpoint pathname in
packages/files-sdk/src/internal/s3-fetch.ts, or reject non-root paths consistently for both clients. Add regression tests forR2HttpOptions.endpointandR2BindingOptions.endpoint.Also applies to: 140-147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/files-sdk/src/r2/index.ts` around lines 71 - 79, Define and enforce the endpoint URL shape consistently for R2HttpOptions and R2BindingOptions. Preserve endpointUrl.pathname when constructing path-style requests in the fetch client, or reject non-root paths in both clients; ensure the selected behavior is shared consistently. Add regression tests covering endpoint values for both option types, including a non-root pathname.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/files-sdk/src/r2/index.ts`:
- Line 360: Update the responseContentDisposition validation error near its
guard to accept either accountId or endpoint, matching the hybrid-signing
guidance in the updated R2 binding error while preserving the existing
credential requirements.
- Around line 335-348: The R2 configuration currently hardcodes the SigV4
region; add an optional region property to both R2 option types, defaulting to
"auto", and propagate that resolved value through the hybrid s3FetchAdapter path
and the fetch and AWS SDK configuration paths near the referenced option
construction points. Ensure custom endpoints can override the default while
existing callers retain "auto".
---
Outside diff comments:
In `@packages/files-sdk/src/r2/index.ts`:
- Around line 71-79: Define and enforce the endpoint URL shape consistently for
R2HttpOptions and R2BindingOptions. Preserve endpointUrl.pathname when
constructing path-style requests in the fetch client, or reject non-root paths
in both clients; ensure the selected behavior is shared consistently. Add
regression tests covering endpoint values for both option types, including a
non-root pathname.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e18babf-04dc-440a-bb8f-2cdb263c218d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.changeset/r2-endpoint-override.mdpackages/files-sdk/src/r2/index.tspackages/files-sdk/test/r2.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/r2-endpoint-override.md
- packages/files-sdk/test/r2.test.ts
| // An explicit `endpoint` stands in for `accountId`, which only ever feeds | ||
| // the default signing hostname. | ||
| const signerEndpoint = | ||
| opts.endpoint ?? | ||
| (opts.accountId | ||
| ? `https://${opts.accountId}.r2.cloudflarestorage.com` | ||
| : undefined); | ||
| const hybrid = | ||
| // oxlint-disable-next-line sonarjs/expression-complexity -- the inline && chain is what narrows each opt to a non-undefined string inside the branch; extracting the guard loses that narrowing | ||
| opts.accountId && opts.accessKeyId && opts.secretAccessKey && httpBucket | ||
| signerEndpoint && opts.accessKeyId && opts.secretAccessKey && httpBucket | ||
| ? s3FetchAdapter({ | ||
| accessKeyId: opts.accessKeyId, | ||
| bucket: httpBucket, | ||
| endpoint: `https://${opts.accountId}.r2.cloudflarestorage.com`, | ||
| endpoint: signerEndpoint, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'region:\s*"auto"|endpoint|R2HttpOptions|R2BindingOptions' \
packages/files-sdk/src/r2/index.ts \
packages/files-sdk/src/internal/s3-fetch.ts \
packages/files-sdk/test/r2.test.tsRepository: haydenbleasel/files-sdk
Length of output: 24270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- R2 option definitions and construction sites ---'
sed -n '35,155p;585,660p' packages/files-sdk/src/r2/index.ts
printf '%s\n' '--- Shared S3 option definitions and adapters ---'
rg -n -C 5 'interface S3AdapterOptions|region\??:|s3FetchAdapter|new S3Client|region' packages/files-sdk/src packages/files-sdk/test/r2.test.ts
printf '%s\n' '--- R2 package documentation and configuration examples ---'
rg -n -i -C 4 'endpoint|region|LocalStack|MinIO|compatible' packages/files-sdk README.md packages/files-sdk/test 2>/dev/null | head -n 300Repository: haydenbleasel/files-sdk
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Shared adapter region contract ---'
rg -n -C 8 'export interface S3AdapterOptions|interface S3AdapterOptions|region\??:' packages/files-sdk/src/s3 packages/files-sdk/src/internal
printf '%s\n' '--- All R2 region references and tests ---'
rg -n -C 3 'region|endpoint|client: "fetch"|client: "aws-sdk"' packages/files-sdk/src/r2/index.ts packages/files-sdk/test/r2.test.ts
printf '%s\n' '--- Package metadata and dependency versions ---'
rg -n -C 3 '"aws4fetch"|"`@aws-sdk/client-s3`"|localstack|minio' package.json packages/files-sdk/package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null | head -n 160Repository: haydenbleasel/files-sdk
Length of output: 25179
🌐 Web query:
LocalStack S3 SigV4 signing region us-east-1 configured region documentation
💡 Result:
When using LocalStack for S3, the region is typically set to us-east-1, which is the default for AWS SDKs and tools when connecting to LocalStack [1][2][3]. LocalStack supports SigV4 signing, which is standard for S3 requests [4]. Key configuration details for S3 and region handling in LocalStack: 1. Region Configuration: LocalStack supports multi-region emulation [5][6]. While us-east-1 is the commonly used default [6][7], you must ensure your client is explicitly configured to use this region, especially if you have enabled multi-region features [5][8]. 2. AWS SDK Configuration: When using SDKs (such as the AWS SDK for Java), you should set the region to Regions.US_EAST_1.getName and point the endpoint to the LocalStack S3 service (typically s3.localhost.localstack.cloud:4566) [1][2]. 3. AWS CLI Configuration: You can set the region using environment variables or profile configurations [3]: - Environment Variable: export AWS_DEFAULT_REGION=us-east-1 - AWS Config File: Set region=us-east-1 in your [profile localstack] configuration [3]. 4. SigV4 Signing: LocalStack uses SigV4 for request validation, including for pre-signed URLs [9][10][4]. If you encounter signature validation issues, you can use the S3_SKIP_SIGNATURE_VALIDATION flag (set to 1 to skip, 0 to validate) [5][8]. Note that for validation to pass, your AWS_SECRET_ACCESS_KEY must be set to test, or you must be using credentials returned from STS [5][8]. For most development scenarios, configuring your client with the standard us-east-1 region and pointing the endpoint to LocalStack is sufficient for proper SigV4 interaction [1][2][3].
Citations:
- 1: https://docs.localstack.cloud/aws/connecting/aws-sdks/java/
- 2: https://hashnode.localstack.cloud/efficient-localstack-s3-endpoint-configuration
- 3: https://docs.localstack.cloud/aws/connecting/aws-cli/
- 4: S3 ASF Presigned URL localstack/localstack#6908
- 5: https://docs.localstack.cloud/aws/capabilities/config/configuration/
- 6: https://docs.localstack.cloud/aws/configuration/config/regions-coverage/
- 7: https://docs.localstack.cloud/aws/customization/advanced/regions-coverage/
- 8: https://docs.localstack.cloud/aws/customization/configuration-options/
- 9: https://fossies.org/dox/localstack-4.14.0/services_2s3_2presigned__url_8py_source.html
- 10: https://fossies.org/dox/localstack-4.14.0/utils_2auth_8py_source.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from hashlib import sha256
from hmac import new as hmac_new
def signing_key(secret: bytes, date: bytes, region: bytes) -> bytes:
k_date = hmac_new(b"AWS4" + secret, date, sha256).digest()
k_region = hmac_new(k_date, region, sha256).digest()
k_service = hmac_new(k_region, b"s3", sha256).digest()
return hmac_new(k_service, b"aws4_request", sha256).digest()
date = b"20260801"
secret = b"SECRET"
for region in (b"auto", b"us-east-1"):
scope = date + b"/" + region + b"/s3/aws4_request"
key = signing_key(secret, date, region)
print(region.decode(), "scope:", scope.decode(), "key:", key.hex())
assert signing_key(secret, date, b"auto") != signing_key(
secret, date, b"us-east-1"
)
assert b"/auto/s3/aws4_request" != b"/us-east-1/s3/aws4_request"
print("Different regions produce different SigV4 scopes and signing keys.")
PYRepository: haydenbleasel/files-sdk
Length of output: 462
Make the SigV4 region configurable for custom endpoints.
region: "auto" produces a different SigV4 scope from us-east-1, which can reject requests to LocalStack or another region-validating S3-compatible service. Add an optional region to both R2 option types, default it to "auto", and pass it through the hybrid, fetch, and AWS SDK paths at Lines 352, 625, and 654.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/files-sdk/src/r2/index.ts` around lines 335 - 348, The R2
configuration currently hardcodes the SigV4 region; add an optional region
property to both R2 option types, defaulting to "auto", and propagate that
resolved value through the hybrid s3FetchAdapter path and the fetch and AWS SDK
configuration paths near the referenced option construction points. Ensure
custom endpoints can override the default while existing callers retain "auto".
| throw new FilesError( | ||
| "Provider", | ||
| "r2 binding: signing requires either `publicBaseUrl` (for url()) or HTTP credentials (`accountId`, `accessKeyId`, `secretAccessKey`, `bucket`) for presigned URLs. See https://developers.cloudflare.com/r2/api/s3/tokens/." | ||
| "r2 binding: signing requires either `publicBaseUrl` (for url()) or HTTP credentials (`accountId` or `endpoint`, plus `accessKeyId`, `secretAccessKey`, `bucket`) for presigned URLs. See https://developers.cloudflare.com/r2/api/s3/tokens/." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep hybrid-signing error guidance consistent.
The updated error accepts accountId or endpoint, but the responseContentDisposition guard at Line 559 still tells callers to pass accountId only. Endpoint-only configuration is valid, so that branch gives incorrect remediation. Use the same accountId-or-endpoint wording in both messages.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/files-sdk/src/r2/index.ts` at line 360, Update the
responseContentDisposition validation error near its guard to accept either
accountId or endpoint, matching the hybrid-signing guidance in the updated R2
binding error while preserving the existing credential requirements.
2 participants
What
Adds an optional
endpointto the r2 adapter — onR2HttpOptionsand onR2BindingOptions(hybrid signing mode) — falling back to the current defaulthttps://<accountId>.r2.cloudflarestorage.comat all three construction sites (aws-sdk client, fetch client, hybrid signer).Why
The r2 wrapper delegates to the s3 machinery, which already accepts
endpointpublicly — the wrapper just seals that knob shut by hardcoding the hostname. That makes two real cases unreachable:eu,fedramp) live on their own hostnames (https://<accountId>.eu.r2.cloudflarestorage.com); the r2 adapter can't talk to them at all today, which locks out GDPR/FedRAMP-constrained users.Notes
endpointbehaves exactly as before.Summary by CodeRabbit
New Features
accountIdis optional when a custom endpoint is provided.Documentation