fix(storage): correct presigned download URL signing and add visibility config#169
Open
abhay-ramesh wants to merge 4 commits intomainfrom
Open
fix(storage): correct presigned download URL signing and add visibility config#169abhay-ramesh wants to merge 4 commits intomainfrom
abhay-ramesh wants to merge 4 commits intomainfrom
Conversation
…ility config - Fix generatePresignedDownloadUrl to sign against buildS3Url (S3 API endpoint) instead of buildPublicUrl. The host header is part of the SigV4 canonical request — signing against a custom domain or CDN URL produced invalid signatures for all users with customDomain configured. - Add visibility?: 'public' | 'private' to BaseProviderConfig (default 'private'). When set to 'public', generatePresignedDownloadUrl returns the plain public URL (customDomain if set, otherwise S3 URL) with no signing. When 'private' (default), generates a presigned GET URL as before but now correctly signed against the S3 API endpoint. - Add discriminated union to CloudflareR2Config: visibility: 'public' requires customDomain to be set. R2 presigned URLs cannot be used with custom domains (per Cloudflare docs), and R2 has no public API endpoint URL — public access requires a custom domain or r2.dev subdomain. Closes #168 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds a visibility flag for storage providers and changes download URL logic: public visibility returns unsigned public/CDN URLs, private visibility returns signed URLs against the provider API endpoint. A new generateDownloadUrl entrypoint is added and used across router and tests; presigning now signs against the API endpoint. Changes
Sequence Diagram(s)sequenceDiagram
participant Router as Router
participant Storage as StorageClient
participant API as S3/R2 API or CDN
Router->>Storage: generateDownloadUrl(config, key, expiresIn)
alt visibility = "public"
Storage->>API: buildPublicUrl(key, config) (no signing)
API-->>Storage: publicUrl
Storage-->>Router: publicUrl
else visibility = "private"
Storage->>API: buildS3Url(key, config)
Storage->>API: sign URL (query-string SigV4, expiresIn)
API-->>Storage: presignedUrl
Storage-->>Router: presignedUrl
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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/pushduck/src/__tests__/s3-fallback.test.ts (1)
69-73:⚠️ Potential issue | 🟡 MinorAdd regression coverage for the new visibility paths.
This still only exercises the default AWS/no-
customDomaincase. The bug fixed by this PR is the private+customDomainsigning host, and the newvisibility: "public"branch is also untested, so the key behavior change can regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pushduck/src/__tests__/s3-fallback.test.ts` around lines 69 - 73, The test currently only calls generateDownloadUrl(...) for the default AWS/no-customDomain case; add regression tests that cover the private+customDomain signing-host path and the new visibility: "public" branch. Specifically, add one test that passes config.customDomain (and visibility: "private") into generateDownloadUrl and assert the returned URL uses the custom signing host/hostname logic expected, and another test that calls generateDownloadUrl with visibility: "public" and asserts the URL follows the public URL branch (no signing host behavior). Target the generateDownloadUrl function in the s3-fallback.test.ts suite and assert on URL host/path to validate both new paths.
🧹 Nitpick comments (1)
packages/pushduck/src/core/router/router-v2.ts (1)
990-994: Don't keep the download expiry hardcoded here.This still forces completion URLs to
3600seconds and leaves the router-side expiry enhancement from#168unimplemented. If that requirement is in scope for this PR, thread a route-level download expiry throughhandleUploadComplete()instead of baking it in here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pushduck/src/core/router/router-v2.ts` around lines 990 - 994, The code hardcodes the download expiry to 3600 when calling generateDownloadUrl in router-v2 (generateDownloadUrl(this.config, completion.key, 3600)); instead, add a route-level expiry parameter to handleUploadComplete and thread that value down to this call so the expiry isn't fixed. Update handleUploadComplete signature (and any callers) to accept an optional downloadExpirySeconds (use a sensible default if undefined) and pass that value into generateDownloadUrl instead of 3600, referencing completion.key and this.config as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pushduck/src/core/providers/providers.ts`:
- Around line 94-111: createProvider() / createProviderBuilder() is dropping the
new visibility field because createProviderBuilder only copies keys listed in
each ProviderSpec.configKeys (none include "visibility"), so configs built via
the factory lose visibility and generateDownloadUrl() falls back to private. Fix
by ensuring the builder preserves visibility: either add "visibility" to the
ProviderSpec.configKeys for all specs or change createProviderBuilder to also
copy/include any extra top-level fields (specifically "visibility") from the
provided config when constructing the provider config; update references in
createProvider(), createProviderBuilder(), ProviderSpec.configKeys and verify
generateDownloadUrl() receives the preserved visibility value.
- Around line 212-216: The Cloudflare R2 discriminated union CloudflareR2Config
is being flattened by ProviderConfigMap["cloudflareR2"]/Partial<Omit<...>> which
lets callers pass visibility: "public" without customDomain; fix by restoring
the union at the API boundary or adding runtime guard: update createProvider
(the "cloudflareR2" branch) and validateProviderConfig to explicitly check
CloudflareR2Config invariants and throw when visibility === "public" &&
!customDomain (or when visibility !== "public" and customDomain is present if
you want stricter rules); reference CloudflareR2Config,
ProviderConfigMap["cloudflareR2"], createProvider and validateProviderConfig
when applying the change.
---
Outside diff comments:
In `@packages/pushduck/src/__tests__/s3-fallback.test.ts`:
- Around line 69-73: The test currently only calls generateDownloadUrl(...) for
the default AWS/no-customDomain case; add regression tests that cover the
private+customDomain signing-host path and the new visibility: "public" branch.
Specifically, add one test that passes config.customDomain (and visibility:
"private") into generateDownloadUrl and assert the returned URL uses the custom
signing host/hostname logic expected, and another test that calls
generateDownloadUrl with visibility: "public" and asserts the URL follows the
public URL branch (no signing host behavior). Target the generateDownloadUrl
function in the s3-fallback.test.ts suite and assert on URL host/path to
validate both new paths.
---
Nitpick comments:
In `@packages/pushduck/src/core/router/router-v2.ts`:
- Around line 990-994: The code hardcodes the download expiry to 3600 when
calling generateDownloadUrl in router-v2 (generateDownloadUrl(this.config,
completion.key, 3600)); instead, add a route-level expiry parameter to
handleUploadComplete and thread that value down to this call so the expiry isn't
fixed. Update handleUploadComplete signature (and any callers) to accept an
optional downloadExpirySeconds (use a sensible default if undefined) and pass
that value into generateDownloadUrl instead of 3600, referencing completion.key
and this.config as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7954ce3b-bd26-458b-9c4e-0e312cb9b2f1
📒 Files selected for processing (5)
packages/pushduck/src/__tests__/s3-fallback.test.tspackages/pushduck/src/core/providers/providers.tspackages/pushduck/src/core/router/router-v2.tspackages/pushduck/src/core/storage/client.tspackages/pushduck/src/core/storage/storage-api.ts
Code fixes: - Extract generatePresignedDownloadUrl (always presigns against S3 API endpoint) from generateDownloadUrl (visibility-aware). storage-api.ts download.presignedUrl now uses generatePresignedDownloadUrl so it always presigns regardless of visibility — callers explicitly asking for a presigned URL get one even on public buckets. - Fix createProviderBuilder to pass through config keys not in spec.configKeys (e.g. visibility). Previously any BaseProviderConfig field not listed in a provider spec was silently dropped. - Update router-v2.ts internal variable presignedUrl → downloadUrl to reflect that it may be a plain URL for public buckets. Response field name (presignedUrl) unchanged for backwards compatibility. Tests (download-url-visibility.test.ts, 16 tests): - Private bucket: presigned GET signed against S3 API endpoint - Private + customDomain: presigns against S3 API, not CDN - Public + no customDomain: plain S3 URL, no signing - Public + customDomain: CDN URL, no signing - R2 private/public combinations - generatePresignedDownloadUrl always presigns regardless of visibility - R2 TypeScript discriminated union valid/invalid configs Docs: - upload-config.mdx: visibility reference table, R2 constraint note - aws-s3.mdx: private and public bucket config examples - cloudflare-r2.mdx: private and public bucket config examples Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/pushduck/src/core/providers/providers.ts (1)
455-465:⚠️ Potential issue | 🟡 MinorSwitch the merge operator from logical OR to nullish coalescing to preserve explicit falsy config values.
Line 456 still uses
||, which loses caller-provided falsy values likefalse,0, or empty strings. For example,createProvider("aws", { forcePathStyle: false })replaces the explicitfalsewith defaults or"". The passthrough block cannot fix this because the key is already populated in the first loop.Use nullish coalescing (
??) instead, which only falls back fornullandundefined:Proposed fix
// Only use explicit config and defaults (no env vars) for (const [key] of Object.entries(spec.configKeys)) { - result[key] = config[key as keyof T] || spec.defaults[key] || ""; + const value = config[key as keyof T]; + result[key] = value ?? spec.defaults[key] ?? ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pushduck/src/core/providers/providers.ts` around lines 455 - 465, The merge currently uses logical OR which overwrites explicit falsy values; in the provider config build loop (the for ... of Object.entries(spec.configKeys) that assigns result[key] = config[key as keyof T] || spec.defaults[key] || ""), change the fallback behavior to use nullish coalescing so caller-provided false/0/"" are preserved: use the config value if it is not null/undefined, otherwise fall back to spec.defaults[key], and finally to "". Keep the passthrough loop unchanged.packages/pushduck/src/core/router/router-v2.ts (1)
989-996:⚠️ Potential issue | 🟠 MajorStop hardcoding the download URL expiry here.
This keeps every private download URL at 3600s and bypasses the new per-route configurability called out in the PR objectives. Use route-level config here instead of a literal.
Suggested fix
- const downloadUrl = await generateDownloadUrl( - this.config, - completion.key, - 3600 - ); + const downloadUrl = await generateDownloadUrl( + this.config, + completion.key, + routeConfig.expiresIn ?? 3600 + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pushduck/src/core/router/router-v2.ts` around lines 989 - 996, The code currently hardcodes 3600 when calling generateDownloadUrl(this.config, completion.key, 3600); instead of using the per-route setting; change the third arg to read the route-level expiry (for example use a route-level property like this.routeConfig.downloadUrlExpiry or fallback to this.config.downloadUrlExpiry or a DEFAULT_DOWNLOAD_URL_EXPIRY constant) so generateDownloadUrl receives the configured TTL; ensure the lookup uses a sensible fallback order and the default constant is defined if missing.
♻️ Duplicate comments (1)
packages/pushduck/src/core/providers/providers.ts (1)
212-216:⚠️ Potential issue | 🟠 MajorThis R2 invariant still leaks through
createProvider().The union here is correct, but
ProviderConfigMap["cloudflareR2"] = Partial<Omit<CloudflareR2Config, "provider">>still flattens it, socreateProvider("cloudflareR2", { visibility: "public" })can still type-check withoutcustomDomain.validateProviderConfig()also does not reject that runtime state, which means the storage layer can still build an unusable public URL from the R2 API endpoint. Please preserve the union at the factory boundary or add an explicit runtime guard.In TypeScript, does Partial<Omit<({ visibility: "public"; customDomain: string } | { visibility?: "private"; customDomain?: string }), "provider">> preserve the requirement that customDomain is required when visibility is "public"?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pushduck/src/core/providers/providers.ts` around lines 212 - 216, The CloudflareR2Config union is being flattened by ProviderConfigMap and Partial/Omit so createProvider("cloudflareR2", ...) can omit customDomain; fix by adding a runtime guard in createProvider and by narrowing the config before using it: inside createProvider (and in validateProviderConfig) detect when provider === "cloudflareR2", then if config.visibility === "public" assert/throw if config.customDomain is missing or not a non-empty string; alternatively preserve the union at the type boundary by changing ProviderConfigMap["cloudflareR2"] to accept the original CloudflareR2Config union (not Partial<Omit<...>>) so TypeScript enforces customDomain for visibility === "public". Ensure both createProvider and validateProviderConfig reference CloudflareR2Config when checking and validating.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/docs/providers/cloudflare-r2.mdx`:
- Around line 132-156: The example under provider("cloudflareR2") is
inconsistent with the earlier setup: it uses process.env.R2_ACCOUNT_ID! but the
docs instruct readers to set AWS_ENDPOINT_URL; either change the example to use
the documented env var (replace R2_ACCOUNT_ID with AWS_ENDPOINT_URL and keep
accountId/endpoint mapping consistent) or update the setup section to require
R2_ACCOUNT_ID instead; adjust the provider(...) block
(accountId/endpoint/customDomain fields) and any explanatory text so the env var
names (AWS_ENDPOINT_URL or R2_ACCOUNT_ID) match across createUploadConfig(),
provider("cloudflareR2"), and the setup instructions.
In `@packages/pushduck/src/__tests__/download-url-visibility.test.ts`:
- Around line 35-44: The test helper makeR2 currently accepts overrides:
Record<string, unknown>, which weakens the CloudflareR2Config
discriminated-union; update tests to include compile-time negative cases by
adding `@ts-expect-error` usages that attempt to call makeR2({ visibility:
"public" }) (and variants missing customDomain) to assert TypeScript errors,
and/or change makeR2's overrides type to Partial<CloudflareR2Config> to avoid
bypassing the constraint; ensure the new `@ts-expect-error` lines appear alongside
the existing runtime tests so the compiler enforces that visibility: "public"
without customDomain is a type error.
In `@packages/pushduck/src/core/providers/providers.ts`:
- Around line 199-208: The example for CloudflareR2Config uses customDomain
without a scheme which yields a non-absolute URL when buildPublicUrl()
concatenates it; update the example value for customDomain in the
CloudflareR2Config snippet to a full origin (e.g. "https://assets.myapp.com") or
add a short note next to the CloudflareR2Config/customDomain description stating
that customDomain must include the URL scheme (http/https) because
buildPublicUrl() uses it verbatim.
---
Outside diff comments:
In `@packages/pushduck/src/core/providers/providers.ts`:
- Around line 455-465: The merge currently uses logical OR which overwrites
explicit falsy values; in the provider config build loop (the for ... of
Object.entries(spec.configKeys) that assigns result[key] = config[key as keyof
T] || spec.defaults[key] || ""), change the fallback behavior to use nullish
coalescing so caller-provided false/0/"" are preserved: use the config value if
it is not null/undefined, otherwise fall back to spec.defaults[key], and finally
to "". Keep the passthrough loop unchanged.
In `@packages/pushduck/src/core/router/router-v2.ts`:
- Around line 989-996: The code currently hardcodes 3600 when calling
generateDownloadUrl(this.config, completion.key, 3600); instead of using the
per-route setting; change the third arg to read the route-level expiry (for
example use a route-level property like this.routeConfig.downloadUrlExpiry or
fallback to this.config.downloadUrlExpiry or a DEFAULT_DOWNLOAD_URL_EXPIRY
constant) so generateDownloadUrl receives the configured TTL; ensure the lookup
uses a sensible fallback order and the default constant is defined if missing.
---
Duplicate comments:
In `@packages/pushduck/src/core/providers/providers.ts`:
- Around line 212-216: The CloudflareR2Config union is being flattened by
ProviderConfigMap and Partial/Omit so createProvider("cloudflareR2", ...) can
omit customDomain; fix by adding a runtime guard in createProvider and by
narrowing the config before using it: inside createProvider (and in
validateProviderConfig) detect when provider === "cloudflareR2", then if
config.visibility === "public" assert/throw if config.customDomain is missing or
not a non-empty string; alternatively preserve the union at the type boundary by
changing ProviderConfigMap["cloudflareR2"] to accept the original
CloudflareR2Config union (not Partial<Omit<...>>) so TypeScript enforces
customDomain for visibility === "public". Ensure both createProvider and
validateProviderConfig reference CloudflareR2Config when checking and
validating.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9fabee6-13c6-4ce5-bbd7-d9266b77dd9e
📒 Files selected for processing (8)
docs/content/docs/api/configuration/upload-config.mdxdocs/content/docs/providers/aws-s3.mdxdocs/content/docs/providers/cloudflare-r2.mdxpackages/pushduck/src/__tests__/download-url-visibility.test.tspackages/pushduck/src/core/providers/providers.tspackages/pushduck/src/core/router/router-v2.tspackages/pushduck/src/core/storage/client.tspackages/pushduck/src/core/storage/storage-api.ts
✅ Files skipped from review due to trivial changes (1)
- docs/content/docs/api/configuration/upload-config.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/pushduck/src/core/storage/storage-api.ts
Comment on lines
+132
to
156
| export const { s3 } = createUploadConfig() | ||
| .provider("cloudflareR2", { | ||
| accessKeyId: process.env.AWS_ACCESS_KEY_ID!, | ||
| secretAccessKey: process.env.AWS_SECRET_ACCESS_KEY!, | ||
| accountId: process.env.R2_ACCOUNT_ID!, // Found in R2 dashboard | ||
| accountId: process.env.R2_ACCOUNT_ID!, | ||
| bucket: process.env.S3_BUCKET_NAME!, | ||
| // Optional: Custom domain for faster access | ||
| customDomain: process.env.CLOUDFLARE_R2_CUSTOM_DOMAIN, | ||
| }) | ||
| .defaults({ | ||
| maxFileSize: "10MB", | ||
| acl: "public-read", // For public access | ||
| .defaults({ maxFileSize: "10MB" }) | ||
| .build(); | ||
| ``` | ||
|
|
||
| **Public bucket with custom domain:** | ||
|
|
||
| ```typescript | ||
| export const { s3 } = createUploadConfig() | ||
| .provider("cloudflareR2", { | ||
| accessKeyId: process.env.AWS_ACCESS_KEY_ID!, | ||
| secretAccessKey: process.env.AWS_SECRET_ACCESS_KEY!, | ||
| accountId: process.env.R2_ACCOUNT_ID!, | ||
| bucket: process.env.S3_BUCKET_NAME!, | ||
| customDomain: process.env.CLOUDFLARE_R2_CUSTOM_DOMAIN!, // e.g. https://cdn.yourdomain.com // [!code highlight] | ||
| visibility: 'public', // returns CDN URLs after upload instead of presigned URLs // [!code highlight] | ||
| }) | ||
| .defaults({ maxFileSize: "10MB" }) | ||
| .build(); |
There was a problem hiding this comment.
Align this example with the env vars documented above.
This snippet now depends on process.env.R2_ACCOUNT_ID!, but the setup section still tells users to define AWS_ENDPOINT_URL instead. As written, the guide no longer gives readers the env var they need to make this example work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/docs/providers/cloudflare-r2.mdx` around lines 132 - 156, The
example under provider("cloudflareR2") is inconsistent with the earlier setup:
it uses process.env.R2_ACCOUNT_ID! but the docs instruct readers to set
AWS_ENDPOINT_URL; either change the example to use the documented env var
(replace R2_ACCOUNT_ID with AWS_ENDPOINT_URL and keep accountId/endpoint mapping
consistent) or update the setup section to require R2_ACCOUNT_ID instead; adjust
the provider(...) block (accountId/endpoint/customDomain fields) and any
explanatory text so the env var names (AWS_ENDPOINT_URL or R2_ACCOUNT_ID) match
across createUploadConfig(), provider("cloudflareR2"), and the setup
instructions.
…and fix jsdoc URL - Export CloudflareR2BaseConfig so it can be referenced independently - Update ProviderConfigMap["cloudflareR2"] to use Partial<Omit<CloudflareR2BaseConfig>> & union instead of Partial<Omit<CloudflareR2Config>> which flattened the discriminated union and lost the TypeScript constraint requiring customDomain when visibility is 'public' - Fix jsdoc example: customDomain value now includes https:// scheme Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ate any
- Restore createProviderBuilder<T extends ProviderConfig> so the public
function signature is fully typed — callers retain specific config types
instead of widening to ProviderConfig
- Add ProviderOutputMap mapping ProviderType keys to concrete config subtypes
so createProvider<T> now returns ProviderOutputMap[T] instead of loose
ProviderConfig union
- Replace Record<string, any> / customLogic: (any, any) => any in ProviderSpec
with Record<string, unknown> — PROVIDER_SPECS uses satisfies so lambdas
get contextual types without inline annotations
- defaults type widened to { [key: string]: unknown } to allow boolean values
(e.g. forcePathStyle: true in s3Compatible)
- Single justified boundary cast remains: result as unknown as T, after
validateProviderConfig confirms all required fields are present
- Runtime validation: R2 visibility='public' without customDomain now throws
- Fix R2 docs env vars: R2_ACCOUNT_ID replaces AWS_ENDPOINT_URL + AWS_REGION
- Clean test helpers: typed AWSOverrides, split makeR2Private/makeR2Public,
@ts-expect-error on correct line covering both compile and runtime checks
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pushduck/src/__tests__/download-url-visibility.test.ts`:
- Around line 205-209: The test currently uses an in-test `@ts-expect-error` which
isn't checked by the repo's TypeScript build because tests are excluded from the
tsconfig; move the negative type assertion into a real typechecked path by
adding a new type-test file (e.g.,
packages/pushduck/src/__type_tests__/r2-visibility.test-d.ts) that imports
createUploadConfig() and attempts to call .provider("cloudflareR2", { ...,
visibility: "public" }) without customDomain so the compiler will catch the
error; ensure this new file is included by the package tsconfig (or add it to
the "include" array) so tsc --noEmit runs the check during CI.
In `@packages/pushduck/src/core/providers/providers.ts`:
- Around line 462-465: The loop in createProviderBuilder is only reading
configRecord[key] and spec.defaults[key], ignoring env-backed names in
spec.configKeys and skipping keys that exist only in spec.defaults; change the
iteration to cover the union of Object.keys(spec.configKeys) and
Object.keys(spec.defaults) and set result[key] = configRecord[key] ??
process.env[spec.configKeys[key]] ?? spec.defaults[key] ?? "" so env var names
stored in spec.configKeys (and default-only keys in spec.defaults) are
respected; update the code that references spec.configKeys, spec.defaults, and
configRecord accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c6e6431-ed2d-4eba-af4c-0a1bc13ebf74
📒 Files selected for processing (3)
docs/content/docs/providers/cloudflare-r2.mdxpackages/pushduck/src/__tests__/download-url-visibility.test.tspackages/pushduck/src/core/providers/providers.ts
Comment on lines
+205
to
+209
| it("R2 public without customDomain throws at runtime", () => { | ||
| expect(() => | ||
| createUploadConfig() | ||
| // @ts-expect-error: visibility: 'public' without customDomain is a type error | ||
| .provider("cloudflareR2", { accessKeyId: "k", secretAccessKey: "s", accountId: "id", bucket: "b", visibility: "public" }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tsconfig files =="
fd -HI '^tsconfig.*\.json$' . | while read -r file; do
echo "--- $file ---"
jq '{include, exclude, files, references}' "$file" 2>/dev/null || cat "$file"
echo
done
echo "== package.json scripts mentioning typecheck / tsc / vitest =="
fd -HI '^package\.json$' . | while read -r file; do
echo "--- $file ---"
jq '.scripts' "$file" 2>/dev/null || cat "$file"
echo
done
echo "== CI / repo references to TypeScript checks =="
rg -n --hidden -g '!**/node_modules/**' '"typecheck"|\btsc\b|vitest' .github package.json packages
echo "== references to test-file inclusion patterns =="
rg -n --hidden -g '!**/node_modules/**' '__tests__|download-url-visibility\.test\.ts' .Repository: abhay-ramesh/pushduck
Length of output: 50377
Move the negative TS assertion into a real typecheck path.
The @ts-expect-error at line 208 is ineffective: the repo's type-check script runs tsc --noEmit against packages/pushduck/tsconfig.json, which explicitly includes only ["src"] and excludes test files. This means the TypeScript constraint on visibility: "public" without customDomain is never validated—the test passes at runtime regardless of whether the type error regresses. Move this assertion to a dedicated type-test file that is included in the tsconfig, or create a dedicated type-checking step for test files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/pushduck/src/__tests__/download-url-visibility.test.ts` around lines
205 - 209, The test currently uses an in-test `@ts-expect-error` which isn't
checked by the repo's TypeScript build because tests are excluded from the
tsconfig; move the negative type assertion into a real typechecked path by
adding a new type-test file (e.g.,
packages/pushduck/src/__type_tests__/r2-visibility.test-d.ts) that imports
createUploadConfig() and attempts to call .provider("cloudflareR2", { ...,
visibility: "public" }) without customDomain so the compiler will catch the
error; ensure this new file is included by the package tsconfig (or add it to
the "include" array) so tsc --noEmit runs the check during CI.
Comment on lines
+462
to
+465
| // Apply spec-declared keys with config values or fallback defaults | ||
| for (const key of Object.keys(spec.configKeys)) { | ||
| result[key] = configRecord[key] ?? spec.defaults[key] ?? ""; | ||
| } |
There was a problem hiding this comment.
createProviderBuilder() no longer applies env-backed values, and it still skips default-only keys.
This loop only reads configRecord[key] and spec.defaults[key]; it never consults the env var names stored in spec.configKeys[key]. That breaks the documented createProvider("aws", { bucket }) / createProvider("cloudflareR2", { bucket }) flow, and it also drops defaults like s3Compatible.forcePathStyle because that key exists only in defaults, not configKeys.
Possible fix
- // Apply spec-declared keys with config values or fallback defaults
- for (const key of Object.keys(spec.configKeys)) {
- result[key] = configRecord[key] ?? spec.defaults[key] ?? "";
- }
+ // Apply explicit config, then env-backed values, then defaults.
+ const allKeys = new Set([
+ ...Object.keys(spec.configKeys),
+ ...Object.keys(spec.defaults),
+ ]);
+
+ for (const key of allKeys) {
+ const envKey = (spec.configKeys[key] ?? []).find(
+ (name) => process.env[name] !== undefined
+ );
+ const value =
+ configRecord[key] ??
+ (envKey ? process.env[envKey] : undefined) ??
+ spec.defaults[key];
+
+ if (value !== undefined) {
+ result[key] = value;
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/pushduck/src/core/providers/providers.ts` around lines 462 - 465,
The loop in createProviderBuilder is only reading configRecord[key] and
spec.defaults[key], ignoring env-backed names in spec.configKeys and skipping
keys that exist only in spec.defaults; change the iteration to cover the union
of Object.keys(spec.configKeys) and Object.keys(spec.defaults) and set
result[key] = configRecord[key] ?? process.env[spec.configKeys[key]] ??
spec.defaults[key] ?? "" so env var names stored in spec.configKeys (and
default-only keys in spec.defaults) are respected; update the code that
references spec.configKeys, spec.defaults, and configRecord accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #168
Problem
generatePresignedDownloadUrlinclient.tshad two bugs:1. Signing against the wrong URL
The
hostheader is a mandatory signed component in SigV4. When acustomDomainis configured,buildPublicUrlreturned the CDN/custom domain URL (e.g.cdn.example.com/key). The signature was computed against that host, making every presigned download URL invalid for users with a custom domain configured.2. Always presigning even for public buckets
For public buckets, a presigned URL is unnecessary — it bypasses CDN caching and returns an ugly S3 API URL instead of the clean CDN URL.
Changes
visibilityconfig option (new)Added
visibility?: 'public' | 'private'toBaseProviderConfig(defaults to'private'):'private'— generates a presigned GET URL signed against the S3 API endpoint (correct behaviour for private buckets)'public'— returns the plain public URL (custom domain if configured, otherwise S3 URL). No signing. Use when your bucket/objects are already publicly accessible.R2 discriminated union (TypeScript error for invalid config)
Cloudflare R2 presigned URLs cannot be used with custom domains — R2 public access requires a custom domain or
r2.devsubdomain. Sovisibility: 'public'withoutcustomDomainon R2 is unresolvable. This is now a TypeScript error:Rename
generatePresignedDownloadUrl→generateDownloadUrlThe function now returns either a presigned URL or a plain public URL depending on
visibility. The old name was misleading. This function is internal (not exported in the public API) so there is no breaking change.Behaviour by scenario
visibility'private'(default)'private'(default)'public''public''private'(default)'public''public'Test plan
customDomain/keywith no signingvisibility: 'public'+ nocustomDomain: TypeScript error🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes
Documentation
Tests