Skip to content

Fix share link download counter incrementing before file existence check#146

Open
G4brym wants to merge 1 commit intomainfrom
fix/share-link-download-counter-ordering
Open

Fix share link download counter incrementing before file existence check#146
G4brym wants to merge 1 commit intomainfrom
fix/share-link-download-counter-ordering

Conversation

@G4brym
Copy link
Owner

@G4brym G4brym commented Mar 7, 2026

Summary

  • Fixes a bug in packages/worker/src/modules/buckets/getShareLink.ts where the download counter was incremented before verifying the shared file still exists in the bucket
  • If a shared file was deleted from the bucket after the share link was created, accessing the link would still increment currentDownloads before returning a 404 error
  • This could silently exhaust the maxDownloads limit without any actual file downloads occurring
  • The fix moves file retrieval before the counter increment, so the count only increases on successful downloads

What changed

Reordered operations in GetShareLink.handle():

  1. Before: increment counter → get file → return 404 if missing (counter already bumped)
  2. After: get file → return 404 if missing → increment counter (counter only bumped on success)

Test plan

  • All existing tests pass (78 passed, 7 skipped — same as before)
  • Linter passes
  • Verify share link with deleted file returns 404 without incrementing counter
  • Verify normal share link downloads still increment counter correctly

🤖 Generated with Claude Code

The download counter was incremented before verifying the shared file
still exists in the bucket. If the file was deleted, accessing the share
link would increment the counter and eventually exhaust the download
limit without any actual downloads. Move file retrieval before counter
increment so the count only increases on successful downloads.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@cloudflare-workers-and-pages
Copy link

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
r2-explorer-docs e1d2e48 Commit Preview URL

Branch Preview URL
Mar 07 2026, 06:44 PM

@G4brym
Copy link
Owner Author

G4brym commented Mar 8, 2026

Automated Code Review — APPROVED ✅

Review Scores: 5/5 reviewers approved

Summary

This PR fixes a real bug where the share link download counter was incremented before verifying the shared file still exists in the bucket. The fix correctly reorders operations so the counter only increments after confirming the file exists, preventing silent exhaustion of maxDownloads when files are deleted after share link creation.

Review Perspectives

  1. Correctness: ✅ Reordering is logically sound. File retrieval now gates counter increment. Pre-existing race condition on concurrent counter updates is not introduced or worsened by this change.
  2. Security: ✅ No new attack surface. Actually improves resilience against download-counter exhaustion as a DoS vector.
  3. Performance: ✅ Same number of R2 operations on happy path; one fewer write on the error path (file deleted). Net neutral to positive.
  4. Code Quality: ✅ Minimal, focused diff with clear comments explaining the reordering rationale. Follows existing conventions.
  5. Testing: ✅ All 78 existing tests pass. A dedicated test for the "deleted file doesn't increment counter" scenario would be a nice future addition but is non-blocking given the trivially correct nature of the change.

Minor notes (non-blocking)

  • Consider adding a test case that verifies a share link to a deleted file returns 404 without incrementing currentDownloads
  • Pre-existing race condition on concurrent counter reads could cause undercounting under high concurrency, but that's outside the scope of this fix

🤖 Automated review by prodboard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant