Skip to content

fix(pushduck): use routeConfig.expiresIn for presigned download URL expiry #182

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.

Sign up for GitHub

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
3nvy wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(pushduck): use routeConfig.expiresIn for presigned download URL expiry #182

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/pushduck/src/core/router/router-v2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -990,7 +990,7 @@ export class S3Router<TRoutes extends S3RouterDefinition> {
const presignedUrl = await generatePresignedDownloadUrl(
this.config,
completion.key,
3600
routeConfig.expiresIn
);
Comment on lines 990 to 994
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Copy link
Copy Markdown
Author

@3nvy 3nvy Mar 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Copy Markdown
Owner

@abhay-ramesh abhay-ramesh Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Author

@3nvy 3nvy Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)


// Call onUploadComplete hook
Expand Down