feat: S3 presigned URLs for private media delivery#2887
feat: S3 presigned URLs for private media delivery#2887amikofalvy wants to merge 9 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 04bb4ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — When S3 is the blob storage backend, conversation media URLs are now resolved as presigned S3 Key changes
Summary | 18 files | 5 commits | base: S3 presigned URL generationAdded
s3-provider.ts · types.ts · s3-provider.test.ts Async blob URI resolution with graceful fallback
Both conversation route handlers (
resolve-blob-uris.ts · resolve-blob-uris.test.ts · run/conversations.ts · manage/conversations.ts S3 blob storage deployment guideNew documentation page at
|
|
TL;DR — When S3 is the blob storage backend, conversation media URLs are now resolved as presigned S3 Key changes
Summary | 21 files | 9 commits | base: S3 presigned URL generation
Added
Async blob URI resolution with graceful fallback
Configurable presigned URL expiry
The env var is added to both
S3 blob storage deployment guideNew documentation page at
Design spec and evidenceThe
|
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a well-implemented feature that adds S3 presigned URL support for private media delivery. The implementation follows the spec correctly, has good error handling with graceful fallback, and includes comprehensive test coverage.
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (4) 💭
Inline Comments:
- 💭 Consider:
s3-blob-storage.mdx:69S3-compatible services guidance incomplete
💭 1) resolve-blob-uris.test.ts:90 Fallback test logging verification
Issue: The fallback test verifies the proxy URL is returned when presigned URL generation fails, but does not verify the warning log is emitted.
Why: The logger.warn call is a debugging aid that could silently break without detection.
Fix: Optionally add a spy on the logger to verify logger.warn is called with the expected key and error context.
💭 2) resolve-blob-uris.test.ts:240 List-level presigned URL test
Issue: The resolveMessagesListBlobUris test only exercises the proxy URL fallback path.
Why: While the function delegates to resolveMessageBlobUris (indirectly covered), explicit list-level presigned URL coverage would catch any Promise.all handling bugs.
Fix: Consider adding a test that configures mockGetPresignedUrl and verifies presigned URLs work for multiple messages in a list.
💭 3) s3-blob-storage.mdx:20-26 AWS CLI prerequisite note
Issue: Step 1's aws s3 mb command assumes AWS CLI is installed.
Why: Users without AWS CLI will hit an error. While the target audience likely has it, write-docs standards recommend stating prerequisites.
Fix: Consider adding a brief note: "Requires AWS CLI configured with credentials that have s3:CreateBucket permission."
💡 APPROVE WITH SUGGESTIONS
Summary: Solid implementation. The code correctly makes resolveMessageBlobUris() async, implements presigned URL generation in the S3 provider, and gracefully falls back to proxy URLs on failure. Error handling is appropriate with warning logs for debugging. Test coverage is comprehensive covering happy path, fallback, mixed content, and error scenarios. The spec is thorough and the security improvements (domain isolation for cookie/XSS protection) are well-reasoned. The Consider items above are optional polish — this PR is ready to merge.
Discarded (0)
No findings discarded.
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-appsec |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
3 | 0 | 2 | 0 | 0 | 0 | 1 |
pr-review-docs |
4 | 0 | 2 | 0 | 1 | 0 | 1 |
| Total | 7 | 0 | 4 | 0 | 1 | 0 | 2 |
Note: 2 INFO-level findings from docs/tests reviewers classified as Consider; 2 very low-confidence items discarded (dotenv syntax tag, Steps component consistency — both are style preferences matching existing patterns in the codebase).
| ```bash | ||
| BLOB_STORAGE_S3_ENDPOINT=https://your-custom-endpoint | ||
| BLOB_STORAGE_S3_FORCE_PATH_STYLE=true | ||
| ``` |
There was a problem hiding this comment.
💭 Consider: S3-compatible services guidance incomplete
Issue: The section lists Cloudflare R2, DigitalOcean Spaces, and Backblaze B2 but doesn't clarify which services require BLOB_STORAGE_S3_FORCE_PATH_STYLE=true vs which do not.
Why: Users may incorrectly set (or omit) this flag, causing presigned URL generation to fail with confusing errors. R2 supports virtual-hosted style and doesn't need path-style; MinIO and B2 typically do.
Fix: Consider adding clarification like:
- Cloudflare R2: Set
BLOB_STORAGE_S3_ENDPOINTonly (R2 uses virtual-hosted style) - MinIO, Backblaze B2: Set both
BLOB_STORAGE_S3_ENDPOINTandBLOB_STORAGE_S3_FORCE_PATH_STYLE=true
There was a problem hiding this comment.
Medium urgency — clean implementation with good fallback semantics, but one security gap worth addressing before merge and one pre-existing concern to track.
Summary: The core change — making resolveMessageBlobUris async and adding presigned URL generation via @aws-sdk/s3-request-presigner — is well-structured. The fallback from presigned URL → proxy URL on error is a solid resilience pattern. All call sites are correctly awaited, tests cover the happy path, fallback, and mixed-content scenarios, and the lockfile changes are minimal and correct.
Pre-existing concern (not blocking): Streaming/SSE response paths (chat.ts, chatDataStream.ts, executions.ts) do not resolve blob:// URIs. If streamed responses ever include blob:// references in message content parts, they would reach clients unresolved. This is pre-existing behavior and not introduced by this PR, but worth tracking as a follow-up given that presigned URLs make the non-streaming path more capable.
Claude Opus | 𝕏
| return content; | ||
| } | ||
|
|
||
| const provider = getBlobStorageProvider(); |
There was a problem hiding this comment.
getBlobStorageProvider() is called on every resolveMessageBlobUris invocation — once per message in resolveMessagesListBlobUris. Since it's a singleton with a null-check, this is cheap, but it would be cleaner to call it once in resolveMessagesListBlobUris and pass the provider into resolveMessageBlobUris. This avoids N singleton lookups per conversation retrieval and makes the dependency explicit.
Not blocking — the current pattern works correctly.
There was a problem hiding this comment.
we should implement this
agents-api/src/domains/run/services/blob-storage/s3-provider.ts
Outdated
Show resolved
Hide resolved
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('resolves blob file parts to media proxy URLs when presigned URLs are not available', async () => { |
There was a problem hiding this comment.
The first test ("resolves blob file parts to media proxy URLs when presigned URLs are not available") relies on the top-level mock returning { getPresignedUrl: undefined }, while all other tests re-mock getBlobStorageProvider within the test body. This works because beforeEach calls vi.clearAllMocks() but does not reset the module-level mock's return value — so the top-level undefined presigned URL mock is still in effect.
This is fragile: if someone adds a test above that changes the mock return value and forgets to reset it, this test breaks silently. Consider explicitly setting getBlobStorageProvider in this test too, matching the pattern of all other tests in the suite.
Ito Test Report ✅17 test cases ran. 17 passed. All 17 test cases passed, confirming end-to-end conversation media handling works as intended across APIs, UI, edge cases, and documentation: blob:// file parts are correctly rewritten to presigned S3 URLs in S3 mode (including Vercel run format), non-blob/external content is preserved, malformed blob keys are safely filtered without breaking responses, and behavior remains deterministic under concurrent and rapid user interactions. The most important risk checks also passed, showing resilient and secure fallbacks and access controls—proxy URLs are used when S3 is disabled or signing fails (including selective per-part failure), mobile/deep-link navigation continues to render resolved attachments, docs are discoverable with correct setup guidance, traversal/null-byte/backslash mediaKey attacks are rejected with 400, legacy API keys are limited to allowed endpoints, tampered signatures are denied, and stale URL replay is blocked while fresh URLs continue to work. ✅ Passed (17)Commit: Tell us how we did: Give Ito Feedback |
|
Error agent completed without reporting progress
|
|
Acknowledged the author's feedback on the defense-in-depth suggestion for
|
|
Addressed review feedback — made presigned URL expiry configurable via Changes (
|
| - **Domain isolation** — media is served from `*.s3.amazonaws.com`, separate from your API domain | ||
| - **Time-limited access** — presigned URLs expire after 1 hour | ||
|
|
||
| When S3 is not configured, the framework falls back to the local filesystem with a server-side media proxy. |
There was a problem hiding this comment.
I think these docs are not entirely correct. There is an option to set the Vercel Blob storage as a provider as well which can be used on production, but the files will be returned via the media proxy rather than s3 presigned urls.
So order is use s3 if it is available, then use vercel blob if it is available then use local storage. Both local storage and vercel blob serve through the media proxy
Review comments addressed (a1255e2)@amikofalvy's comments:
Pullfrog's comments:
Claude bot's comment:
CI failure: |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review Scope
This is a re-review scoped to 2 commits since the last automated review (33883b1):
6b871fc32— Make presigned URL expiry configurable viaBLOB_STORAGE_PRESIGNED_URL_EXPIRY_SECONDSa1255e2fa— Address PR review comments
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (0) 💭
None.
Delta Changes Assessment
All delta changes correctly address prior review feedback:
| Change | Status | Notes |
|---|---|---|
env.ts — Zod schema for BLOB_STORAGE_PRESIGNED_URL_EXPIRY_SECONDS |
✅ | Proper validation (int, min 60, max 604800, default 7200) with description |
s3-provider.ts — Read expiry from env |
✅ | Removed hardcoded constant, uses env.BLOB_STORAGE_PRESIGNED_URL_EXPIRY_SECONDS |
s3-provider.test.ts — Updated test expectations |
✅ | Default expects 7200, custom expiry test uses 900, all mocks include env var |
resolve-blob-uris.test.ts — Explicit mock setup |
✅ | Addresses fragile test setup noted in prior review |
s3-blob-storage.mdx — Docs updates |
✅ | Added Vercel Blob fallback mention, presigned URL expiry config section, S3-compatible services table |
.env.example files |
✅ | Consistent with env.ts schema |
✅ APPROVE
Summary: The delta changes cleanly address all prior review feedback. The configurable presigned URL expiry is properly validated via Zod with sensible bounds (1 minute to 7 days), tests are updated to cover the new behavior, and documentation now accurately describes the storage backend priority including Vercel Blob fallback. No new issues introduced. Ready to merge! 🎉
Discarded (0)
No findings discarded.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (delta) |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta-scoped re-review — all changes address prior feedback correctly.
- Add optional getPresignedUrl() to BlobStorageProvider interface - Implement in S3BlobStorageProvider using @aws-sdk/s3-request-presigner - Make resolveMessageBlobUris() async with presigned URL first, manage proxy fallback for non-S3 backends (Option D hybrid) - Update both call sites (run + manage conversation routes) with await - Add presigned URL tests to s3-provider and resolve-blob-uris test suites - Include full spec with evidence files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers S3 setup, IAM permissions, env vars, S3-compatible services, storage backend priority, and presigned URL delivery flow. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Wrap getPresignedUrl() in try-catch so failures fall through to manage proxy URL instead of crashing the entire conversation response - Add test for presigned URL failure → proxy fallback path - Add mixed-content test with presigned URLs active - Fix doc icon quoting convention Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… docs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…NED_URL_EXPIRY_SECONDS` - Add `BLOB_STORAGE_PRESIGNED_URL_EXPIRY_SECONDS` to env.ts Zod schema (default 7200s / 2 hours, range 60–604800) - Replace hardcoded `DEFAULT_PRESIGNED_EXPIRY_SECONDS` constant in s3-provider.ts with env var lookup - Update tests to use env var in mocks and verify new default - Add env var to .env.example files and deployment docs
- Clarify Vercel Blob is also a valid production backend (serves via proxy) - Add per-service S3-compatible path-style guidance (R2 vs B2 vs Spaces) - Make first resolve-blob-uris test explicitly set its mock (test isolation) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
a1255e2 to
c8b56c4
Compare
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Review Scope
Third automated review following c8b56c482 — all prior feedback has been addressed. Dispatched 5 domain-specific reviewers:
pr-review-standards— code quality, async patterns, type safetypr-review-errors— error handling, fallback patternspr-review-appsec— presigned URL security, S3 accesspr-review-tests— test coverage qualitypr-review-docs— documentation accuracy
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (0) 💭
None — all Consider items from prior reviews have been addressed or acknowledged.
🕐 Pending Recommendations (2)
- 💭
resolve-blob-uris.test.ts:240List-level presigned URL test coverage (from prior claude review) - 💭
s3-blob-storage.mdx:20-26AWS CLI prerequisite note (from prior claude review)
✅ APPROVE
Summary: Clean implementation of S3 presigned URL support for private media delivery. The code correctly makes resolveMessageBlobUris() async, implements presigned URL generation with proper error handling and graceful fallback to proxy URLs, and includes comprehensive test coverage. All prior review feedback has been addressed — the configurable presigned URL expiry is properly validated via Zod (60s to 7 days, default 2 hours), documentation accurately describes the storage backend priority (S3 → Vercel Blob → local), and the S3-compatible services table clarifies path-style requirements. Ready to ship! 🚀
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
resolve-blob-uris.test.ts:97 |
Missing test for malformed key + presigned URL failure combo | Edge case where two error paths combine; individual paths are tested. Low risk. |
resolve-blob-uris.test.ts:247 |
List-level presigned URL test | Duplicate of prior claude review Consider item |
s3-provider.test.ts:112 |
GetObjectCommand parameters not verified | Low criticality; expiry is verified which is the key behavioral contract |
s3-provider.test.ts:161 |
Error cause not preserved | Nice-to-have; error message contains sufficient context |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-appsec |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
4 | 0 | 0 | 0 | 0 | 1 | 4 |
pr-review-docs |
1 | 0 | 0 | 0 | 0 | 1 | 0 |
| Total | 5 | 0 | 0 | 0 | 0 | 2 | 4 |
Note: Test reviewer findings were assessed as low-priority edge cases or duplicates of prior feedback. Docs reviewer returned INFO-only confirmation that documentation is accurate.
Ito Test Report ✅10 test cases ran. 10 passed. All 10 test cases passed with zero failures, confirming expected behavior across run/manage conversation flows, S3/proxy fallback behavior, docs navigation, stress scenarios, and media-endpoint security protections. Key findings were that blob-backed parts are resolved asynchronously into stable non-blob URLs with consistent ordering across repeated fetches, fallback correctly switches to authenticated manage proxy URLs when S3 is disabled or presign fails (with successful media retrieval), traversal/malformed/null-byte and cross-tenant/project replay attempts are denied (400/404), safe URL schemes were consistently enforced, and the earlier 503s were attributed to local SigNoz connectivity noise rather than a product defect. ✅ Passed (10)Commit: Tell us how we did: Give Ito Feedback |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
CI failure update: All 20 failing tests are in All 16 blob storage tests pass. Our changes are green. |
|
No additional docs changes required. The S3 blob storage documentation at |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Review Scope
Fourth automated review — delta-scoped to 1 commit since c8b56c482:
a4b843c0c—chore: reset lockfile from main to minimize resolution drift
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (0) 💭
None.
Delta Changes Assessment
| File | Change | Assessment |
|---|---|---|
pnpm-lock.yaml |
Reset from main to minimize resolution drift | ✅ Lockfile-only change. Per CLAUDE.md guidance, this is the correct approach: "Start from the base branch's lockfile — check out the pnpm-lock.yaml from the target base (usually main)" and re-install. This preserves all existing resolutions from main and only adds/updates what the branch's package.json changes require. |
No application code, tests, or documentation changed in this delta.
🕐 Pending Recommendations (2)
Carried forward from prior reviews (optional polish, not blocking):
- 💭
resolve-blob-uris.test.ts:240— List-level presigned URL test coverage - 💭
s3-blob-storage.mdx:20-26— AWS CLI prerequisite note
✅ APPROVE
Summary: This delta contains only a lockfile reset following the monorepo's recommended strategy — no functional changes. The core implementation (S3 presigned URL generation, async blob URI resolution, graceful fallback to proxy URLs, configurable expiry) was thoroughly reviewed and approved in prior passes. All prior review feedback has been addressed. The CI failure in @inkeep/agents-email is a pre-existing React version mismatch unrelated to this PR. Ready to ship! 🚀
Discarded (0)
No findings discarded.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 2 | 0 |
Note: No sub-reviewers dispatched — delta contains only lockfile changes with no reviewable code.
|
CI failure root cause confirmed: The The mismatch ( This should be fixed separately on main — not in this PR. |
…drift Pin @aws-sdk/s3-request-presigner to 3.995.0 (same as resolved client-s3) to minimize pnpm-lock.yaml changes and prevent react version mismatch that caused agents-email test failures in CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Review Scope
Fifth automated review — delta-scoped to 1 commit since the last review (a4b843c0c):
04bb4ed13—fix: pin s3-request-presigner to match client-s3 to prevent lockfile drift
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (0) 💭
None.
Delta Changes Assessment
| File | Change | Assessment |
|---|---|---|
package.json |
Pinned @aws-sdk/s3-request-presigner to 3.995.0 (removed caret) |
✅ Correct approach. Pinning prevents pnpm from resolving different patch versions between the two AWS SDK packages, which would cause lockfile drift on every install. |
s3-provider.ts |
Added (getSignedUrl as any) type cast with explanatory comment |
✅ Pragmatic solution. The comment clearly documents the reason: monorepo type resolution can produce incompatible internal types even when packages are runtime-compatible. The as any cast is acceptable here since getSignedUrl is a well-known AWS SDK export with stable API. |
Type Cast Justification
The (getSignedUrl as any) cast in s3-provider.ts:127 is a known pattern when working with AWS SDK packages in monorepos. The root cause:
@aws-sdk/client-s3and@aws-sdk/s3-request-presignershare internal type packages (e.g.,@smithy/types)- In a monorepo with complex resolution, these may resolve to different versions
- TypeScript sees incompatible internal types even though the runtime API is identical
- The cast is narrow (only on the
getSignedUrlcall) and documented with a comment
Alternatives considered:
- Use
overridesinpackage.jsonto force version alignment — more complex, affects entire monorepo - Exact version match on both packages — still may not align transitive types
- Type assertion — chosen approach, minimal footprint, self-documenting
🕐 Pending Recommendations (2)
Carried forward from prior reviews (optional polish, not blocking):
- 💭
resolve-blob-uris.test.ts:240— List-level presigned URL test coverage - 💭
s3-blob-storage.mdx:20-26— AWS CLI prerequisite note
✅ APPROVE
Summary: This delta is a clean lockfile hygiene fix that pins the AWS SDK presigner dependency to prevent version drift. The type cast is a pragmatic workaround for a known monorepo type resolution issue, well-documented with an explanatory comment. All prior review feedback has been addressed across the full PR. The CI failure in @inkeep/agents-email is a pre-existing React version mismatch unrelated to this PR. Ship it! 🚀
Discarded (0)
No findings discarded.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 2 | 0 |
Note: No sub-reviewers dispatched — delta contains only dependency version pinning and a type cast with no functional changes.
Ito Test Report ✅13 test cases ran. 13 passed. All 13 test cases passed in a unified local non-production verification run, with no confirmed production-code defects: conversation attachment URLs were correctly resolved in both S3 mode (valid presigned X-Amz links) and non-S3 mode (manage proxy fallback), including async and concurrent reads, and no unresolved blob:// URLs remained. Key robustness and security checks also passed—presign-failure degraded gracefully to proxy URLs, tampered or expired presigned URLs were denied, malformed/traversal media keys returned HTTP 400 without disclosure, and trace-detail copy flows (full/summarized, rapid interaction stress, and mobile deep-link navigation loops) remained functional, though several scenarios used temporary local auth/project-access bypasses to enable deterministic execution. ✅ Passed (13)Commit: Tell us how we did: Give Ito Feedback |









































Summary
BLOB_STORAGE_S3_BUCKETis configured,resolveMessageBlobUris()generates presignedGetObjectURLs (1hr expiry) via@aws-sdk/s3-request-presigner/managemedia proxy URLs — no behavior changeChanges
blob-storage/types.tsgetPresignedUrl?()toBlobStorageProviderinterfaceblob-storage/s3-provider.tsgetPresignedUrlusing@aws-sdk/s3-request-presignerblob-storage/resolve-blob-uris.tsrun/routes/conversations.tsawaittoresolveMessagesListBlobUriscallmanage/routes/conversations.tsawaittoresolveMessagesListBlobUriscallagents-api/package.json@aws-sdk/s3-request-presigner, align@aws-sdk/client-s3versionresolve-blob-uris.test.tss3-provider.test.tss3-blob-storage.mdxTest plan
pnpm typecheckpassespnpm lintpassesSpec
See
specs/2026-03-23-vercel-private-blob-presigned-urls/SPEC.md— Option D (Hybrid) selected. Supersedesspecs/2026-03-19-run-media-signed-proxy/SPEC.md.🤖 Generated with Claude Code