fix(pushduck): use routeConfig.expiresIn for presigned download URL expiry#182
fix(pushduck): use routeConfig.expiresIn for presigned download URL expiry#1823nvy wants to merge 1 commit intoabhay-ramesh:mainfrom
Conversation
|
@3nvy is attempting to deploy a commit to the Abhay's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe pull request replaces a hard-coded 3600-second expiration value with a configurable Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
3nvy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/router/router-v2.ts`:
- Around line 990-994: The code is reusing routeConfig.expiresIn
(S3Route.expiresIn / S3RouteConfig.expiresIn) for download links when calling
generatePresignedDownloadUrl with completion.key, which unintentionally shortens
download TTLs; introduce a separate download expiry (e.g.,
S3Route.downloadExpiresIn or S3RouteConfig.downloadExpiresIn), update the route
config/type declarations and docs to describe it, and change the call site to
pass routeConfig.downloadExpiresIn (falling back to a sensible default constant
if unset) instead of routeConfig.expiresIn so upload and download TTLs are
independent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a22a511b-3efd-4326-866c-6aa0eb256eb0
📒 Files selected for processing (1)
packages/pushduck/src/core/router/router-v2.ts
| const presignedUrl = await generatePresignedDownloadUrl( | ||
| this.config, | ||
| completion.key, | ||
| 3600 | ||
| routeConfig.expiresIn | ||
| ); |
There was a problem hiding this comment.
Don't reuse the upload TTL as the download TTL implicitly.
routeConfig.expiresIn is currently documented in this file as the presigned upload URL expiry (S3Route.expiresIn and S3RouteConfig.expiresIn), but this change also applies it to the completion-time download URL. That makes short upload windows also shorten returned download links, which is a visible contract change for existing routes. Please either introduce a separate download-expiry setting or update the route API/docs explicitly so this isn't an undocumented behavior change.
🤖 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 is reusing routeConfig.expiresIn (S3Route.expiresIn /
S3RouteConfig.expiresIn) for download links when calling
generatePresignedDownloadUrl with completion.key, which unintentionally shortens
download TTLs; introduce a separate download expiry (e.g.,
S3Route.downloadExpiresIn or S3RouteConfig.downloadExpiresIn), update the route
config/type declarations and docs to describe it, and change the call site to
pass routeConfig.downloadExpiresIn (falling back to a sensible default constant
if unset) instead of routeConfig.expiresIn so upload and download TTLs are
independent.
There was a problem hiding this comment.
Uhm, I guess this makes sense if we want different times for both upload and download, tho not sure this matters for the purpose of the upload flow, as expirations can be set separately afterwards on subsquent requests. I'll leave the choice to you :)
There was a problem hiding this comment.
@3nvy, thanks for the context! Here's my take:
The concern is subtle but real. The completion-time download URL isn't just used immediately — onUploadComplete hooks commonly persist it (e.g., save it to a DB), where it must survive until users actually access the file. If a route sets expiresIn(300) for a tight upload window, the returned download URL would silently expire in 5 minutes, which could be surprising.
Option A – Separate downloadExpiresIn (recommended)
This keeps upload and download TTLs independent and explicit:
// In S3RouteConfig
interface S3RouteConfig<TMetadata = any> {
expiresIn?: number; // upload URL TTL
downloadExpiresIn?: number; // download URL TTL (falls back to expiresIn, then 3600)
// ...
}// In S3Route
downloadExpiresIn(seconds: number): this {
if (seconds <= 0 || seconds > 604800) {
throw new Error(
`downloadExpiresIn must be between 1 and 604800 seconds (7 days), got ${seconds}`
);
}
this.config.downloadExpiresIn = seconds;
return this;
}// In handleUploadComplete
const presignedUrl = await generatePresignedDownloadUrl(
this.config,
completion.key,
routeConfig.downloadExpiresIn ?? routeConfig.expiresIn // fallback chain
);Option B – Keep this PR, update docs only
If you'd prefer to ship the simplest fix now and treat expiresIn as controlling both, at minimum the stale comment on CompletionResponse and the JSDoc on S3Route.expiresIn() should be updated:
/** Presigned upload URL expiration time in seconds (default: 3600 = 1 hour) */
expiresIn?: number;→
/** Presigned URL expiration time in seconds for both upload and download URLs (default: 3600 = 1 hour) */
expiresIn?: number;And in CompletionResponse:
presignedUrl?: string; // Temporary download URL (expires per routeConfig.expiresIn)Option A is cleaner long-term; Option B is an acceptable minimal fix if scope needs to stay small. Your call!
There was a problem hiding this comment.
Uhm, I guess this makes sense if we want different times for both upload and download, tho not sure this matters for the purpose of the upload flow, as expirations can be set separately afterwards on subsquent requests. I'll leave the choice to you :)
Yep @3nvy even i was thinking of a better way to get this right, working on beta branch for slightly different api structure
was thinking of doing it like this
.expiriesIn(3600) // Sets Both Upload and Download TTL
// and can also be done like
.expiriesIn({ upload: 3600, download: 600 })would like your opinion!
might be good if people want to specify per route TTL, if needed they can override anytime (which is already implemented)
upload TTL per route was definitely required, thanks for that @3nvy
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Oh yeah that works perfectly 🙌 , is that's something your already doing for the beta api? if so Im happy to close this PR and wait for your changes :)
2 participants
📝 Pull Request
📋 Description
Uses
expiresInvalue from routeConfig forgeneratePresignedDownloadUrlwhen generating presigned urls from the uploadcompletephase🎯 What does this PR do?
Uses
expiresInvalue from routeConfig instead of the hardcoded3600🔗 Related Issues
🧪 Type of Change
✅ Testing
🧪 How Has This Been Tested?
📊 Test Coverage
🔍 Manual Testing Steps
📸 Screenshots / Videos (if applicable)
Before:
After:
🔧 Configuration Changes
📚 Documentation
🛡️ Security
📦 Bundle Impact
🚀 Deployment
🔄 Migration Required?
✅ Pre-Release Checklist
📝 Changeset Information
📦 Package(s) Affected:
🔖 Version Bump Type:
📋 Changeset Summary:
🤝 Reviewer Guidelines
🔍 What to Review:
⚡ Priority Review Areas:
📎 Additional Notes
📧 Author: @
🏷️ Labels:
🎯 Milestone:
🙏 Thank You!
Thanks for contributing to pushduck! Your changes help make file uploads easier for everyone. 🚀
Summary by CodeRabbit