-
-
Notifications
You must be signed in to change notification settings - Fork 11
fix(storage): correct presigned download URL signing and add visibility config #169
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
Open
abhay-ramesh
wants to merge
4
commits into
main
Choose a base branch
from
fix/presigned-download-url-visibility
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
2c5e2db
fix(storage): correct presigned download URL generation and add visib…
abhay-ramesh 0d013fb
fix(storage): correct download URL logic, add tests and docs
abhay-ramesh a847797
fix(providers): preserve R2 discriminated union in ProviderConfigMap …
abhay-ramesh 2c95440
refactor(providers): restore generic T extends ProviderConfig, elimin…
abhay-ramesh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
214 changes: 214 additions & 0 deletions
214
packages/pushduck/src/__tests__/download-url-visibility.test.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,214 @@ | ||
| /** | ||
| * Tests for download URL generation with visibility config | ||
| * | ||
| * Covers: | ||
| * - Private bucket (default): presigned GET signed against S3 API endpoint | ||
| * - Private bucket + custom domain: presigned GET still against S3 API, not CDN | ||
| * - Public bucket + no custom domain: plain S3 URL, no signing | ||
| * - Public bucket + custom domain: custom domain URL, no signing | ||
| * - R2 private: presigned GET against R2 API endpoint | ||
| * - R2 public + custom domain: custom domain URL, no signing | ||
| * - storage.download.presignedUrl(): always presigns regardless of visibility | ||
| */ | ||
|
|
||
| import { describe, expect, it } from "vitest"; | ||
| import { createUploadConfig } from "../core/config/upload-config"; | ||
| import { | ||
| generateDownloadUrl, | ||
| generatePresignedDownloadUrl, | ||
| } from "../core/storage/client"; | ||
|
|
||
| // ─── Helpers ──────────────────────────────────────────────────────────────── | ||
|
|
||
| function makeAWS(overrides: Record<string, unknown> = {}) { | ||
| return createUploadConfig() | ||
| .provider("aws", { | ||
| accessKeyId: "test-key", | ||
| secretAccessKey: "test-secret", | ||
| region: "us-east-1", | ||
| bucket: "test-bucket", | ||
| ...overrides, | ||
| }) | ||
| .build().config; | ||
| } | ||
|
|
||
| function makeR2(overrides: Record<string, unknown> = {}) { | ||
| return createUploadConfig() | ||
| .provider("cloudflareR2", { | ||
| accessKeyId: "test-key", | ||
| secretAccessKey: "test-secret", | ||
| accountId: "abc123", | ||
| bucket: "test-bucket", | ||
| ...overrides, | ||
| }) | ||
| .build().config; | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| const KEY = "uploads/photo.jpg"; | ||
| const S3_URL = `https://test-bucket.s3.us-east-1.amazonaws.com/${KEY}`; | ||
| const R2_API_URL = `https://abc123.r2.cloudflarestorage.com/test-bucket/${KEY}`; | ||
| // customDomain must include the protocol — buildPublicUrl uses the value as-is | ||
| const CUSTOM_DOMAIN = "https://cdn.example.com"; | ||
| const CDN_URL = `${CUSTOM_DOMAIN}/${KEY}`; | ||
|
|
||
| // ─── generateDownloadUrl — private (default) ──────────────────────────────── | ||
|
|
||
| describe("generateDownloadUrl — private bucket (default)", () => { | ||
| it("returns a presigned URL signed against the S3 API endpoint", async () => { | ||
| const config = makeAWS(); | ||
| const url = await generateDownloadUrl(config, KEY); | ||
| expect(url).toContain(S3_URL); | ||
| expect(url).toContain("X-Amz-Signature"); | ||
| expect(url).toContain("X-Amz-Expires=3600"); | ||
| }); | ||
|
|
||
| it("respects custom expiresIn", async () => { | ||
| const config = makeAWS(); | ||
| const url = await generateDownloadUrl(config, KEY, 300); | ||
| expect(url).toContain("X-Amz-Expires=300"); | ||
| }); | ||
|
|
||
| it("signs against S3 API endpoint even when customDomain is set", async () => { | ||
| const config = makeAWS({ customDomain: CUSTOM_DOMAIN }); | ||
| const url = await generateDownloadUrl(config, KEY); | ||
| // Must use the S3 API endpoint for signing, NOT the custom domain | ||
| expect(url).toContain("s3.us-east-1.amazonaws.com"); | ||
| expect(url).not.toContain("cdn.example.com"); | ||
| expect(url).toContain("X-Amz-Signature"); | ||
| }); | ||
|
|
||
| it("explicit visibility: 'private' behaves the same as default", async () => { | ||
| const config = makeAWS({ visibility: "private" }); | ||
| const url = await generateDownloadUrl(config, KEY); | ||
| expect(url).toContain(S3_URL); | ||
| expect(url).toContain("X-Amz-Signature"); | ||
| }); | ||
| }); | ||
|
|
||
| // ─── generateDownloadUrl — public bucket ──────────────────────────────────── | ||
|
|
||
| describe("generateDownloadUrl — public bucket", () => { | ||
| it("returns plain S3 URL with no signing when no custom domain", async () => { | ||
| const config = makeAWS({ visibility: "public" }); | ||
| const url = await generateDownloadUrl(config, KEY); | ||
| expect(url).toBe(S3_URL); | ||
| expect(url).not.toContain("X-Amz-Signature"); | ||
| expect(url).not.toContain("X-Amz-Expires"); | ||
| }); | ||
|
|
||
| it("returns custom domain URL with no signing when customDomain is set", async () => { | ||
| const config = makeAWS({ | ||
| visibility: "public", | ||
| customDomain: CUSTOM_DOMAIN, | ||
| }); | ||
| const url = await generateDownloadUrl(config, KEY); | ||
| expect(url).toBe(CDN_URL); | ||
| expect(url).not.toContain("X-Amz-Signature"); | ||
| expect(url).not.toContain("amazonaws.com"); | ||
| }); | ||
|
|
||
| it("strips trailing slash from custom domain", async () => { | ||
| const config = makeAWS({ | ||
| visibility: "public", | ||
| customDomain: CUSTOM_DOMAIN + "/", | ||
| }); | ||
| const url = await generateDownloadUrl(config, KEY); | ||
| expect(url).toBe(CDN_URL); | ||
| }); | ||
| }); | ||
|
|
||
| // ─── generateDownloadUrl — R2 ─────────────────────────────────────────────── | ||
|
|
||
| describe("generateDownloadUrl — Cloudflare R2", () => { | ||
| it("private (default): presigned GET against R2 API endpoint", async () => { | ||
| const config = makeR2(); | ||
| const url = await generateDownloadUrl(config, KEY); | ||
| expect(url).toContain(R2_API_URL); | ||
| expect(url).toContain("X-Amz-Signature"); | ||
| }); | ||
|
|
||
| it("private + custom domain: still signs against R2 API, not custom domain", async () => { | ||
| const config = makeR2({ customDomain: CUSTOM_DOMAIN }); | ||
| const url = await generateDownloadUrl(config, KEY); | ||
| // R2 presigned URLs must use the API endpoint, not custom domain | ||
| expect(url).toContain("r2.cloudflarestorage.com"); | ||
| expect(url).not.toContain("cdn.example.com"); | ||
| expect(url).toContain("X-Amz-Signature"); | ||
| }); | ||
|
|
||
| it("public + custom domain: returns custom domain URL with no signing", async () => { | ||
| const config = makeR2({ visibility: "public", customDomain: CUSTOM_DOMAIN }); | ||
| const url = await generateDownloadUrl(config, KEY); | ||
| expect(url).toBe(CDN_URL); | ||
| expect(url).not.toContain("X-Amz-Signature"); | ||
| expect(url).not.toContain("r2.cloudflarestorage.com"); | ||
| }); | ||
| }); | ||
|
|
||
| // ─── generatePresignedDownloadUrl — always presigns ───────────────────────── | ||
|
|
||
| describe("generatePresignedDownloadUrl — always presigns (ignores visibility)", () => { | ||
| it("presigns for private bucket", async () => { | ||
| const config = makeAWS({ visibility: "private" }); | ||
| const url = await generatePresignedDownloadUrl(config, KEY); | ||
| expect(url).toContain("X-Amz-Signature"); | ||
| expect(url).toContain(S3_URL); | ||
| }); | ||
|
|
||
| it("presigns even when visibility is 'public'", async () => { | ||
| const config = makeAWS({ | ||
| visibility: "public", | ||
| customDomain: CUSTOM_DOMAIN, | ||
| }); | ||
| const url = await generatePresignedDownloadUrl(config, KEY); | ||
| // Ignores visibility — always presigns against S3 API endpoint | ||
| expect(url).toContain("X-Amz-Signature"); | ||
| expect(url).toContain("s3.us-east-1.amazonaws.com"); | ||
| expect(url).not.toContain("cdn.example.com"); | ||
| }); | ||
|
|
||
| it("respects custom expiresIn", async () => { | ||
| const config = makeAWS(); | ||
| const url = await generatePresignedDownloadUrl(config, KEY, 900); | ||
| expect(url).toContain("X-Amz-Expires=900"); | ||
| }); | ||
|
|
||
| it("defaults to 3600s expiry", async () => { | ||
| const config = makeAWS(); | ||
| const url = await generatePresignedDownloadUrl(config, KEY); | ||
| expect(url).toContain("X-Amz-Expires=3600"); | ||
| }); | ||
| }); | ||
|
|
||
| // ─── R2 TypeScript discriminated union (documented, not runtime) ───────────── | ||
|
|
||
| describe("R2 visibility: 'public' TypeScript constraint", () => { | ||
| it("R2 private without customDomain is valid", () => { | ||
| expect(() => | ||
| createUploadConfig() | ||
| .provider("cloudflareR2", { | ||
| accessKeyId: "k", | ||
| secretAccessKey: "s", | ||
| accountId: "id", | ||
| bucket: "b", | ||
| // no customDomain, no visibility — valid private config | ||
| }) | ||
| .build() | ||
| ).not.toThrow(); | ||
| }); | ||
|
|
||
| it("R2 public with customDomain is valid", () => { | ||
| expect(() => | ||
| createUploadConfig() | ||
| .provider("cloudflareR2", { | ||
| accessKeyId: "k", | ||
| secretAccessKey: "s", | ||
| accountId: "id", | ||
| bucket: "b", | ||
| customDomain: CUSTOM_DOMAIN, | ||
| visibility: "public", | ||
| }) | ||
| .build() | ||
| ).not.toThrow(); | ||
| }); | ||
| }); | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
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 defineAWS_ENDPOINT_URLinstead. As written, the guide no longer gives readers the env var they need to make this example work.🤖 Prompt for AI Agents