feat(storage): replace proxy upload with presigned URL architecture#129
Merged
yash-pouranik merged 7 commits intogeturbackend:mainfrom Apr 23, 2026
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 31 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a two-step presigned-upload workflow: new Changes
Sequence DiagramsequenceDiagram
participant Client as SDK Client
participant API as API Controller
participant SM as Storage Manager
participant Cloud as Cloud Storage
participant DB as Database
Client->>API: POST /api/storage/upload-request (filename, contentType, size)
API->>DB: check project quota headroom
API->>SM: getPresignedUploadUrl(project, filePath, contentType, size)
SM->>Cloud: request presigned PUT URL
Cloud-->>SM: signedUrl (and token if needed)
SM-->>API: signedUrl, token, filePath
API-->>Client: { signedUrl, token, filePath }
Client->>Cloud: PUT signedUrl (raw bytes, Content-Type)
Cloud-->>Client: 200 OK
Client->>API: POST /api/storage/upload-confirm (filePath, size, token)
API->>SM: verifyUploadedFile(project, filePath)
SM->>Cloud: Head/List to confirm object and size
Cloud-->>SM: actualSize
SM-->>API: actualSize
API->>DB: atomic update (increment storageUsed if actualSize matches and under limit via $expr)
DB-->>API: updated project record
API-->>Client: { publicUrl?, provider, (warning if public URL unavailable) }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/public-api/src/__tests__/storage.controller.test.js (1)
11-34: 🛠️ Refactor suggestion | 🟠 MajorMissing test coverage for the new
requestUpload/confirmUploadendpoints.This test file adds the
getBucketmock but doesn't add anydescribe('requestUpload', ...)/describe('confirmUpload', ...)suites. Given the delicate quota logic (peek-then-charge, ownership check,verifyUploadedFile, rollback semantics), shipping without tests for the primary new flow leaves regressions easy to miss.At minimum, consider tests for:
requestUpload: 400 on missing fields, 413 on oversize, 403 on quota headroom exhaustion, 200 returns{ signedUrl, filePath }, S3/R2 path wheretokenisundefined.confirmUpload: 400 on missing fields, 403 on foreign-project/..paths, 500 whenverifyUploadedFilethrows (ensures quota is not charged), 403 when the$exprguard matches 0, 200 happy path returns public URL.Want me to draft these test cases?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/__tests__/storage.controller.test.js` around lines 11 - 34, The test file mocks getBucket but lacks suites for the new requestUpload and confirmUpload flows; add describe blocks for requestUpload and confirmUpload that exercise the quota/ownership/verification and rollback semantics. For requestUpload, add tests asserting 400 on missing fields, 413 on oversize, 403 when quota headroom is insufficient, and 200 returns { signedUrl, filePath } (also assert S3/R2 branch where token is undefined) by calling the requestUpload handler and using the __mockStorageFrom.upload/getPublicUrl mocks; for confirmUpload, add tests asserting 400 on missing fields, 403 on foreign-project or path traversal, 500 when verifyUploadedFile throws (and assert quota was not charged/rolled back), 403 when the MongoDB $expr guard returns 0, and 200 happy path returning public URL while verifying getBucket, verifyUploadedFile, Project.updateOne and storage.from().remove/list behaviors are stubbed appropriately. Ensure tests mock/inspect verifyUploadedFile, Project.updateOne, getStorage().__mockStorageFrom, and getBucket to simulate each condition and assert side-effects (quota changes, calls to remove on rollback, and returned responses).apps/public-api/src/controllers/storage.controller.js (1)
1-307:⚠️ Potential issue | 🟠 MajorCoding-guideline violation: response shape and error handling.
The new
requestUpload/confirmUpload(and the existing handlers alongside them) return{ error: ... }/ ad-hoc success shapes and rawres.status(...).json(...)on failure instead of the standard{ success, data, message }envelope andAppError-based error flow. Per the repo's controller guideline, endpoints insrc/controllers/**must return{ success, data, message }and must useAppError— never raw throws, never ad-hoc error bodies.If aligning the older handlers is out of scope for this PR, at least wrap the two new endpoints so the public surface starts converging on the standard shape.
As per coding guidelines: "All API endpoints return:
{ success: bool, data: {}, message: "" }. Use AppError class for errors — never raw throw, never expose MongoDB errors to client."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/controllers/storage.controller.js` around lines 1 - 307, The requestUpload and confirmUpload handlers currently send ad-hoc responses and throw raw errors; update both handlers (requestUpload, confirmUpload) to follow the standard envelope { success, data, message } for all success responses (e.g., return res.status(200).json({ success: true, data: { signedUrl, token, filePath }, message: "..." }) and for confirmUpload similarly include url/path/provider in data) and stop throwing raw errors from verifyUploadedFile/getPresignedUploadUrl; instead convert failure cases into AppError instances and pass to next (e.g., next(new AppError("Internal storage limit exceeded.", 403))) rather than res.status(...).json or throw; ensure any quota check failures, validation errors, and caught exceptions use AppError with appropriate status codes and messages and do not leak internal error details.
🧹 Nitpick comments (3)
packages/common/src/index.js (1)
164-166: Fix indentation on the new export.
verifyUploadedFileis missing indentation to match the surrounding exports.nit
- getPresignedUploadUrl, -verifyUploadedFile + getPresignedUploadUrl, + verifyUploadedFile,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/index.js` around lines 164 - 166, The export list has inconsistent indentation: align the new export verifyUploadedFile with the surrounding exports (e.g., match indentation of getPresignedUploadUrl) in the export block so the exported symbols (getPresignedUploadUrl, verifyUploadedFile) are vertically aligned and formatted consistently.sdks/urbackend-sdk/src/modules/storage.ts (1)
115-119: DirectPUTto cloud has no timeout / retry / abort signal.
fetchhas no default timeout. On flaky networks or stalled connections this will hang indefinitely, blocking the caller and never reachingupload-confirm. Consider wiring anAbortController(configurable via anoptionsparam) and documenting retry semantics — presignedPUTis idempotent for the same URL window, so bounded retries are safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdks/urbackend-sdk/src/modules/storage.ts` around lines 115 - 119, The direct fetch PUT to the presigned URL (using signedUrl, fileData and producing putResponse) lacks an AbortController, timeout and retry behavior; modify the upload function to accept an optional options param (e.g., { signal?, timeoutMs?, maxRetries? }), create/derive an AbortController when no external signal is provided, pass its signal into fetch, implement a bounded retry loop for idempotent PUTs honoring backoff and maxRetries, and ensure the controller is aborted after timeoutMs; also update function docstring to describe the new timeout/retry/options semantics so callers can provide their own AbortSignal or rely on the configured timeout.packages/common/src/utils/storage.manager.js (1)
190-198: RecreatingS3Clienton every presign/verify call.Both helpers build a fresh
S3Clientper request rather than leveraging the per-project cache instorageRegistry/createS3Adapter. At high RPS this adds TLS + SDK middleware setup overhead. Consider extendingcreateS3Adapter(orgetStorage) to also expose the rawS3Client(or the presign/head helpers) so the pooling already implemented ingetStorageapplies here too.Also applies to: 240-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/utils/storage.manager.js` around lines 190 - 198, The presign/verify helpers recreate a new S3Client for each call (symbols: S3Client, presign, verify) which bypasses the existing per-project cache in storageRegistry/createS3Adapter/getStorage and causes TLS/SDK setup overhead; update createS3Adapter (or getStorage) to also return the underlying S3Client or presign/head helper functions, then change the presign and verify helper implementations to obtain and reuse that cached client/helper from storageRegistry/getStorage instead of instantiating a new S3Client locally (apply the same change to the other occurrence doing the same work).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/public-api/src/__tests__/storage.controller.test.js`:
- Line 40: The jest mock factory for '@urbackend/common' is missing stubs for
getPresignedUploadUrl and verifyUploadedFile (and ensure getBucket is exported),
so update the mock definition that currently exports getStorage, Project,
isProjectStorageExternal, getBucket, __mockStorageFrom (alias mockStorageFrom)
to also include no-op/mock implementations for getPresignedUploadUrl and
verifyUploadedFile; implement simple stub functions that return predictable
values or resolved promises consistent with existing test patterns so tests
calling getPresignedUploadUrl(...) and verifyUploadedFile(...) do not receive
undefined and fail.
In `@apps/public-api/src/controllers/storage.controller.js`:
- Around line 291-300: The current response returns url: publicUrlData.publicUrl
which can be null for S3/R2 adapters; update the handler that calls
getStorage/getBucket to detect when publicUrlData.publicUrl is null and either
(A) include the adapter error from createS3Adapter.getPublicUrl (e.g.,
publicUrlData.error) in the JSON response as an explicit warning/error field
alongside message and keep 200, or (B, preferred) enforce at
project-config/validation time that a publicUrlHost is provided for S3/R2 so
createS3Adapter.getPublicUrl never returns null; implement one of these fixes by
updating the response logic around publicUrlData.publicUrl and by adding
validation where project config is created/loaded (reference getStorage,
getBucket, publicUrlData.publicUrl, and createS3Adapter.getPublicUrl).
- Around line 275-289: The confirmUpload path verifies the uploaded file but
never updates the monthly usage metric, so add a call to
updateMonthlyUsageCounter(project._id, "storage:uploadedBytes", size) after the
successful Project.updateOne quota increment (use the verified size from
verifyUploadedFile) so billing/usage remains consistent with uploadFile; place
the call alongside the quota update block and only for non-external uploads.
- Around line 229-244: The incoming size must be validated and normalized to a
positive finite numeric value before any checks or DB writes: in the upload
initiation handler (the block using filename, contentType, size and comparing
against MAX_FILE_SIZE and project.storageUsed) coerce/parse the incoming size to
a Number, ensure it is a finite integer > 0 and <= MAX_FILE_SIZE, and return 400
on invalid input; do the same normalization and validation in confirmUpload
before using size in the Mongo $inc (e.g., the code path that performs $inc: {
storageUsed: size }), to prevent string concatenation, negative values, NaN, or
non-number writes to storageUsed.
- Around line 271-276: The ownership check should normalize and validate path
segments rather than banning any occurrence of '..'; update the check around
filePath (before calling verifyUploadedFile) to first normalize the path using
POSIX semantics (e.g., path.posix.normalize), ensure the normalized path begins
with `${project._id}/`, then split on '/' and reject if any segment equals '..'
or '.' or if decoded segments (decodeURIComponent) equal those values; mirror
the per-segment validation used in deleteFile so file names like "my..file.txt"
are allowed while true traversal is blocked, and keep calling
verifyUploadedFile(project, filePath, size) after the validated normalized path
is used.
In `@packages/common/src/utils/storage.manager.js`:
- Around line 213-217: The Supabase storage call using
defaultSupabase.storage.from("dev-files").list(folder, { search: fileName })
uses substring matching and can return other objects; after the list call in the
function that derives folder and fileName from filePath, filter the returned
data array for the exact item.name === fileName (or use Supabase's object
metadata/head endpoint if available) and use that exact match for the
size/metadata checks (also update the same pattern around the other list usage
at the later block referenced). Ensure you still handle the case where no exact
match is found (treat as missing and surface the error).
- Around line 208-252: verifyUploadedFile currently falls back to the
client-supplied expectedSize and returns it, which lets clients cheat; change
verifyUploadedFile to always resolve to the actual object size or throw if the
size cannot be determined (remove any "|| expectedSize" fallbacks and throw when
metadata.size or S3 HeadObject ContentLength is missing), and update the
controller confirmUpload to use the returned actualSize for the $inc and $expr
quota check (instead of req.body.size), rejecting with 400 if the declared size
differs from actualSize beyond a small tolerance; reference verifyUploadedFile,
confirmUpload, and the Supabase list/S3 HeadObject code paths when making the
changes.
- Around line 162-206: Replace the hardcoded bucket names in
getPresignedUploadUrl (internal "dev-files" and external supabase "files") with
the canonical bucket resolved by getBucket(project) (the same helper used by
uploadFile/confirmUpload/deleteFile), and ensure verifyUploadedFile also uses
getBucket(project) where it currently references buckets on lines around 216 and
232; import or use the existing getBucket symbol from `@urbackend/common` and pass
that bucket value into the supabase.storage.from(...) and S3
PutObjectCommand/Bucket resolution so all operations target the same bucket.
- Around line 199-205: The presigned PUT URL currently doesn't bind the allowed
upload size; update the PutObjectCommand creation in the function that calls
PutObjectCommand so it sets ContentLength to the incoming size parameter (the
controller's size/MAX_FILE_SIZE) and call getSignedUrl(s3Client, command, ...)
with options including signableHeaders: new Set(["content-length"]) and a
shorter expiresIn (e.g., 300–900 seconds) so the signed URL enforces the size
limit and has reduced lifetime; look for references to PutObjectCommand,
getSignedUrl, s3Client and the returned signedUrl to apply this change.
In `@sdks/urbackend-sdk/src/modules/storage.ts`:
- Around line 89-105: The runtime check guarding the File branch causes File
instances in non-window environments (Node 20+, Deno, Bun, Workers) to miss
setting resolvedName; update the type check to use typeof File !== "undefined"
&& file instanceof File instead of typeof window !== "undefined" && file
instanceof File, and in the Blob branch set resolvedName = filename || (file as
any).name || resolvedName so anonymous Blobs fall back to the provided filename
or existing default; keep the Buffer branch as-is but ensure resolvedName still
uses filename when passed.
---
Outside diff comments:
In `@apps/public-api/src/__tests__/storage.controller.test.js`:
- Around line 11-34: The test file mocks getBucket but lacks suites for the new
requestUpload and confirmUpload flows; add describe blocks for requestUpload and
confirmUpload that exercise the quota/ownership/verification and rollback
semantics. For requestUpload, add tests asserting 400 on missing fields, 413 on
oversize, 403 when quota headroom is insufficient, and 200 returns { signedUrl,
filePath } (also assert S3/R2 branch where token is undefined) by calling the
requestUpload handler and using the __mockStorageFrom.upload/getPublicUrl mocks;
for confirmUpload, add tests asserting 400 on missing fields, 403 on
foreign-project or path traversal, 500 when verifyUploadedFile throws (and
assert quota was not charged/rolled back), 403 when the MongoDB $expr guard
returns 0, and 200 happy path returning public URL while verifying getBucket,
verifyUploadedFile, Project.updateOne and storage.from().remove/list behaviors
are stubbed appropriately. Ensure tests mock/inspect verifyUploadedFile,
Project.updateOne, getStorage().__mockStorageFrom, and getBucket to simulate
each condition and assert side-effects (quota changes, calls to remove on
rollback, and returned responses).
In `@apps/public-api/src/controllers/storage.controller.js`:
- Around line 1-307: The requestUpload and confirmUpload handlers currently send
ad-hoc responses and throw raw errors; update both handlers (requestUpload,
confirmUpload) to follow the standard envelope { success, data, message } for
all success responses (e.g., return res.status(200).json({ success: true, data:
{ signedUrl, token, filePath }, message: "..." }) and for confirmUpload
similarly include url/path/provider in data) and stop throwing raw errors from
verifyUploadedFile/getPresignedUploadUrl; instead convert failure cases into
AppError instances and pass to next (e.g., next(new AppError("Internal storage
limit exceeded.", 403))) rather than res.status(...).json or throw; ensure any
quota check failures, validation errors, and caught exceptions use AppError with
appropriate status codes and messages and do not leak internal error details.
---
Nitpick comments:
In `@packages/common/src/index.js`:
- Around line 164-166: The export list has inconsistent indentation: align the
new export verifyUploadedFile with the surrounding exports (e.g., match
indentation of getPresignedUploadUrl) in the export block so the exported
symbols (getPresignedUploadUrl, verifyUploadedFile) are vertically aligned and
formatted consistently.
In `@packages/common/src/utils/storage.manager.js`:
- Around line 190-198: The presign/verify helpers recreate a new S3Client for
each call (symbols: S3Client, presign, verify) which bypasses the existing
per-project cache in storageRegistry/createS3Adapter/getStorage and causes
TLS/SDK setup overhead; update createS3Adapter (or getStorage) to also return
the underlying S3Client or presign/head helper functions, then change the
presign and verify helper implementations to obtain and reuse that cached
client/helper from storageRegistry/getStorage instead of instantiating a new
S3Client locally (apply the same change to the other occurrence doing the same
work).
In `@sdks/urbackend-sdk/src/modules/storage.ts`:
- Around line 115-119: The direct fetch PUT to the presigned URL (using
signedUrl, fileData and producing putResponse) lacks an AbortController, timeout
and retry behavior; modify the upload function to accept an optional options
param (e.g., { signal?, timeoutMs?, maxRetries? }), create/derive an
AbortController when no external signal is provided, pass its signal into fetch,
implement a bounded retry loop for idempotent PUTs honoring backoff and
maxRetries, and ensure the controller is aborted after timeoutMs; also update
function docstring to describe the new timeout/retry/options semantics so
callers can provide their own AbortSignal or rely on the configured timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18a01d8c-eb79-4d68-a67a-70eb84517c7b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
apps/public-api/src/__tests__/storage.controller.test.jsapps/public-api/src/controllers/storage.controller.jsapps/public-api/src/routes/storage.jspackages/common/package.jsonpackages/common/src/index.jspackages/common/src/utils/storage.manager.jssdks/urbackend-sdk/src/modules/storage.ts
Comment on lines
+208
to
+252
| async function verifyUploadedFile(project, filePath, expectedSize) { | ||
| const isExternal = !!project.resources?.storage?.isExternal; | ||
|
|
||
| if (!isExternal) { | ||
| // internal supabase | ||
| const folder = filePath.split("/")[0]; | ||
| const fileName = filePath.split("/").slice(1).join("/"); | ||
| const { data, error } = await defaultSupabase.storage | ||
| .from("dev-files") | ||
| .list(folder, { search: fileName }); | ||
| if (error) throw error; | ||
| if (!data || data.length === 0) throw new Error("File not found after upload"); | ||
| return data[0].metadata?.size || expectedSize; | ||
| } | ||
|
|
||
| const decrypted = decrypt(project.resources.storage.config); | ||
| const config = JSON.parse(decrypted); | ||
| const provider = config.storageProvider || "supabase"; | ||
|
|
||
| if (provider === "supabase") { | ||
| const supabase = await getStorage(project); | ||
| const folder = filePath.split("/")[0]; | ||
| const fileName = filePath.split("/").slice(1).join("/"); | ||
| const { data, error } = await supabase.storage | ||
| .from("files") | ||
| .list(folder, { search: fileName }); | ||
| if (error) throw error; | ||
| if (!data || data.length === 0) throw new Error("File not found after upload"); | ||
| return data[0].metadata?.size || expectedSize; | ||
| } | ||
|
|
||
| // S3 / R2 — just ask "does this object exist and what's its size?" | ||
| const s3Client = new S3Client({ | ||
| region: config.region || "auto", | ||
| endpoint: config.endpoint, | ||
| forcePathStyle: true, | ||
| credentials: { | ||
| accessKeyId: config.accessKeyId, | ||
| secretAccessKey: config.secretAccessKey, | ||
| }, | ||
| }); | ||
| const command = new HeadObjectCommand({ Bucket: config.bucket, Key: filePath }); | ||
| const head = await s3Client.send(command); | ||
| return head.ContentLength || expectedSize; | ||
| } |
Contributor
There was a problem hiding this comment.
verifyUploadedFile's return value is ignored — client-supplied size is used to charge quota.
The controller's confirmUpload calls await verifyUploadedFile(project, filePath, size) but discards the return value, then does $inc: { storageUsed: size } using the client-provided size (req.body.size). A client can therefore:
- Upload a large file and claim
size: 1to bypass thestorageLimitcheck. - Upload a small file but claim a larger
sizeto burn another project member's quota (if any).
Also, the fallback return data[0].metadata?.size || expectedSize; silently trusts the client whenever Supabase doesn't return a size — the "verification" has no teeth on that path.
Recommend: have verifyUploadedFile return the actual object size (throw if unknown, never fall back to expectedSize) and have confirmUpload use that value for the $inc and $expr guard. Additionally, consider rejecting with a 400 if the returned size diverges from the declared size beyond a small tolerance.
🛠 Sketch of fix
- if (!data || data.length === 0) throw new Error("File not found after upload");
- return data[0].metadata?.size || expectedSize;
+ if (!data || data.length === 0) throw new Error("File not found after upload");
+ const actual = Number(data[0].metadata?.size);
+ if (!Number.isFinite(actual)) throw new Error("Could not determine uploaded size");
+ return actual;And in the controller:
- await verifyUploadedFile(project, filePath, size);
+ const actualSize = await verifyUploadedFile(project, filePath, size);
...
- { $inc: { storageUsed: size } }
+ { $inc: { storageUsed: actualSize } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/common/src/utils/storage.manager.js` around lines 208 - 252,
verifyUploadedFile currently falls back to the client-supplied expectedSize and
returns it, which lets clients cheat; change verifyUploadedFile to always
resolve to the actual object size or throw if the size cannot be determined
(remove any "|| expectedSize" fallbacks and throw when metadata.size or S3
HeadObject ContentLength is missing), and update the controller confirmUpload to
use the returned actualSize for the $inc and $expr quota check (instead of
req.body.size), rejecting with 400 if the declared size differs from actualSize
beyond a small tolerance; reference verifyUploadedFile, confirmUpload, and the
Supabase list/S3 HeadObject code paths when making the changes.
Nitin-kumar-yadav1307
force-pushed
the
feat/presigned-url-upload
branch
from
8f4b03d to
6ca48ba
Compare
Contributor
Author
|
@yash-pouranik let me know , if it require some changes |
Collaborator
|
@Nitin-kumar-yadav1307 Have u tested locally ?? |
Contributor
Author
|
Yeah ! , but before resolving conflicts and issues , sorry for that @yash-pouranik |
yash-pouranik
requested changes
Apr 23, 2026
Collaborator
yash-pouranik
left a comment
yash-pouranik
left a comment
There was a problem hiding this comment.
2 comments in Storage.controller.js and 1 in storage.manager.js by coderabbit are still not resolved?? @Nitin-kumar-yadav1307
4 tasks
Contributor
Author
|
@yash-pouranik ok, i am resolving |
Contributor
Author
all the tests are passed let see what will happen ! @yash-pouranik waiting for dear coderabbit
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
sdks/urbackend-sdk/src/modules/storage.ts (1)
89-105:⚠️ Potential issue | 🟡 MinorAdd
typeof Blob !== "undefined"guard and preservefile.namein the Blob branch.The unguarded
instanceof Blobcheck on line 95 will throwReferenceErrorin Node.js 16 and earlier (where Blob is not a global). Additionally, the Blob branch should preserveresolvedNamefromfile.nameas a fallback, since in constrained runtimes where the File global is undefined, a File instance may fall through to this branch and lose its name.Proposed fix
- } else if (file instanceof Blob) { - contentType = file.type || contentType; - fileSize = file.size; - fileData = file; + } else if (typeof Blob !== "undefined" && file instanceof Blob) { + resolvedName = filename || (file as { name?: string }).name || resolvedName; + contentType = file.type || contentType; + fileSize = file.size; + fileData = file;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdks/urbackend-sdk/src/modules/storage.ts` around lines 89 - 105, The Blob instanceof check is unguarded and can throw in runtimes without a global Blob, and the Blob branch currently doesn't preserve a filename; update the condition to check typeof Blob !== "undefined" && file instanceof Blob, and in that Blob-handling branch set resolvedName = resolvedName || (file as any).name (or use filename || file.name fallback) so the original file name is retained when a File falls through to the Blob branch; adjust the Blob branch to also set contentType, fileSize, and fileData (as currently done) while preserving resolvedName.
🧹 Nitpick comments (7)
sdks/urbackend-sdk/tests/storage.test.ts (2)
8-8: Stale test name.The test no longer sends FormData; it now posts JSON for the upload-request step and uses
PUTwith a raw body for the signed URL upload. Consider renaming to reflect the new 3-step presigned flow (e.g.,'upload performs 3-step presigned flow and returns { url, path }') to avoid confusing future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdks/urbackend-sdk/tests/storage.test.ts` at line 8, Rename the test description to accurately reflect the new 3-step presigned flow: change the test title string in the test block that currently reads 'upload sends FormData and returns { url, path }' to something like 'upload performs 3-step presigned flow and returns { url, path }' so it matches the updated behavior in the upload test in storage.test.ts (the test function with that title).
39-46: PUT assertion doesn't verify the body was forwarded.The second-call assertion checks method and headers but not that
fileData(the Buffer) is actually passed as thebodyto the signed URL. A regression that dropped the body or sentundefinedwould still pass this test. Consider addingbody: expect.anything()(or a more specific matcher) to theobjectContainingto lock down that contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdks/urbackend-sdk/tests/storage.test.ts` around lines 39 - 46, The test's second fetchMock assertion (in storage.test.ts) only checks method and headers and should also assert the request body was forwarded; update the expect.objectContaining for fetchMock's 2nd call (the PUT to 'https://signed.example/upload') to include a body matcher such as body: expect.anything() or body: fileData (or expect.equal(fileData)) so the Buffer passed as the upload body is verified.apps/public-api/src/__tests__/storage.controller.test.js (1)
356-436: Good coverage of the new presigned flow.Tests cover: non-numeric size validation, happy-path signed URL generation (including that
..in filenames isn't incorrectly rejected as traversal),$expr-guarded quota charging on confirm, declared-vs-verified size mismatch, and the external/missing-public-URL warning path. This matches the controller logic well.One optional gap to consider adding later: a test for
confirmUploadrejecting afilePaththat normalizes into a different project (e.g.,other_project_id/file.txtorproject_id_1/../other/file.txt) to lock downnormalizeProjectPath's ownership check. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/__tests__/storage.controller.test.js` around lines 356 - 436, Add a test to ensure confirmUpload rejects filePath values that normalize to a different project to protect ownership checks: create a case in the requestUpload/confirmUpload suite that calls storageController.confirmUpload with req.body.filePath set to values like "other_project_id/file.txt" and "project_id_1/../other/file.txt", mock normalizeProjectPath (or rely on real normalize function) and assert a 400/forbidden response and error message; reference the confirmUpload handler and normalizeProjectPath behavior so the test verifies the ownership guard preventing cross-project normalization escapes.sdks/urbackend-sdk/src/modules/storage.ts (2)
125-131: Failed/abandoned confirm step leaves orphan objects in storage.If the
PUTsucceeds butupload-confirmfails (network drop, client process killed, server 5xx), the file exists on Supabase/S3/R2 with no server record and no quota charge — a deliberate design choice per the PR description, but it does produce orphans over time.Worth tracking as a follow-up: either (a) a server-side sweeper that lists storage under
${projectId}/and deletes objects not referenced by any DB record older than N minutes, or (b) a client-side retry with backoff around the confirm call before surfacing the error. Not required for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdks/urbackend-sdk/src/modules/storage.ts` around lines 125 - 131, The confirm step ("POST" "/api/storage/upload-confirm") can fail after a successful PUT leaving orphaned objects; add a client-side retry-with-backoff around the this.client.request<UploadResponse>(...) call that posts to "/api/storage/upload-confirm" to reduce orphan creation (e.g., exponential backoff with a small max attempts and jitter), and surface the error only after retries exhaust; alternatively note adding a server-side sweeper to garbage-collect unreferenced objects as a follow-up. Ensure you modify the method that performs the confirm call (the code calling this.client.request for "upload-confirm") so retries are scoped to that request.
115-123: Direct PUT has no timeout/abort and throws a plainError.Two small concerns on the cloud
PUTleg:
fetch(signedUrl, ...)has noAbortSignal/timeout. For large files on flaky networks this can hang indefinitely on the client with no way to cancel. Consider threading an optionalAbortSignalthroughupload()(or at least wrapping with a configurable timeout) so callers can bound the wait.- The other SDK failure paths surface as
StorageError(see the'StorageError thrown on failure'test), but this path throws a genericError. Throwing aStorageError-typed error here would keepcatchblocks in consumer code uniform.Neither is a release blocker, but both will be noticeable once real users hit large-file edge cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdks/urbackend-sdk/src/modules/storage.ts` around lines 115 - 123, The direct PUT call using fetch(signedUrl, ...) in upload() lacks cancellation/timeout and throws a plain Error; modify upload() to accept an optional AbortSignal (or implement a configurable timeout using AbortController) and pass that signal to fetch so callers can cancel long uploads, and replace the plain throw new Error(...) for the failed putResponse path with a StorageError (using the existing StorageError class) that includes HTTP status and statusText to match other SDK error paths and keep catch handling uniform.apps/public-api/src/controllers/storage.controller.js (2)
297-400: Response envelope doesn't match the project-wide API contract.The new
requestUploadandconfirmUploadhandlers return flat shapes like{ signedUrl, token, filePath }and{ error: "..." }, but per the repo's controller guideline all API endpoints should return{ success: bool, data: {}, message: "" }and useAppErrorfor errors rather than rawres.status(...).json({ error }).The rest of this file already drifts from the guideline (so this PR isn't introducing the drift), but since these are brand-new endpoints consumed by the SDK, it's cheap to align them now before any external clients depend on the shape — otherwise the contract becomes load-bearing and harder to migrate later. At minimum, consider wrapping the success payloads in
{ success: true, data: { ... }, message: "..." }and routing errors throughAppError.As per coding guidelines:
All API endpoints return: { success: bool, data: {}, message: "" }. Use AppError class for errors — never raw throw, never expose MongoDB errors to client.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/controllers/storage.controller.js` around lines 297 - 400, Both requestUpload and confirmUpload return flat JSON and raw error shapes; update them to follow the project API contract by returning the envelope { success: bool, data: {...}, message: "" } on success and throw/use AppError for client errors (instead of res.status(...).json({ error })) so error middleware handles status and payload. Specifically, in requestUpload replace the success return to res.status(200).json({ success: true, data: { signedUrl, token, filePath }, message: "Upload URL generated" }) and replace each early client error branch (400/403/413) to throw new AppError("...", statusCode) (or construct AppError with the message and code) instead of returning raw JSON; do the same in confirmUpload: wrap the success response (message/path/provider/url) into data and message, and convert all res.status(...).json({ error, details }) error responses to AppError throws so the centralized error handler emits the proper envelope. Ensure catch blocks do not directly send raw JSON but rethrow or convert to an AppError with a sanitized message.
313-317: TOCTOU betweenrequestUploadquota peek andconfirmUploadquota charge.The peek here is purely advisory — between issuing the signed URL and the later
confirmUpload, nothing prevents N parallelrequestUploadcalls from all seeingstorageUsed + size <= limitand all receiving signed URLs. The first few confirms will succeed, but once the atomic$exprguard inconfirmUploadtrips, the remaining clients will get 403s with files already uploaded to the bucket (→ same orphan problem flagged below).Two options:
- Reserve on peek: do an atomic
$incunder the same$exprguard here, keyed by a pending-upload token, and release/commit it inconfirmUpload. Most correct, most code.- Accept it: document that under high concurrency the client may see a 403 on confirm and should delete locally; pair with the compensating-delete mitigation suggested on lines 370-371.
Not a blocker for v1, but worth a design note in the PR so it isn't silently inherited.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/controllers/storage.controller.js` around lines 313 - 317, The current TOCTOU occurs in requestUpload where the advisory quota peek (calculating quotaLimit from effectiveLimit/SAFETY_MAX_BYTES and checking project.storageUsed + numericSize) can race with confirmUpload's atomic $expr guard, allowing many signed URLs to be issued but later rejected, leaving orphaned bucket objects; fix by either implementing a reserve-on-peek mechanism: atomically $inc a pending_uploads counter or store a pending-upload token on the project record under the same $expr guard when issuing signed URLs and then atomically commit (move pending to used) or release that token in confirmUpload, or explicitly document and handle the race by returning a clear error to clients and ensuring confirmUpload triggers a compensating-delete path; refer to requestUpload, confirmUpload, project.storageUsed, numericSize, SAFETY_MAX_BYTES, effectiveLimit, and the $expr guard when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/public-api/src/controllers/storage.controller.js`:
- Around line 370-371: In confirmUpload, ensure any return path after the cloud
PUT (e.g., the 500 verify failure in the verify step, the 400 size-mismatch, and
the 403 when result.matchedCount === 0) performs a best-effort compensating
delete of the uploaded object by calling
supabase.storage.from(bucket).remove([normalizedPath]) and swallowing any errors
before returning; add this delete call just prior to each non-2xx return (or
factor into a small helper invoked on error) so orphaned objects are removed
even if confirmUpload rejects.
- Around line 350-355: The verifyUploadedFile call inside the confirmUpload
handler currently lets a "File not found after upload" exception bubble up and
produce a 500; change confirmUpload to explicitly catch errors from
verifyUploadedFile (or inspect the thrown error/type/message) and, when the
error indicates the file is not yet present, return a 4xx (e.g. 409 Conflict or
404 Not Found) with a clear machine-readable error code (e.g. "UPLOAD_NOT_FOUND"
or "UPLOAD_NOT_READY") and a human message, otherwise rethrow or let other
errors produce the existing 500 path; update the response where actualSize is
used so that only real verification failures continue to produce 500 and
retryable "not found" cases produce the new 4xx response.
- Around line 362-372: confirmUpload currently uses Project.updateOne with an
$expr that always compares $storageUsed + actualSize <= $storageLimit, which
fails for storageLimit === -1 (unlimited). Change the update filter used in
confirmUpload so it allows unlimited plans by adding an OR condition: either
$storageLimit equals -1 OR the existing $expr comparison holds; keep the same
$inc update of storageUsed. Update the Project.updateOne call (the filter around
storageLimit/$expr and the $inc of storageUsed) to mirror the branching used in
uploadFile (where effectiveLimit === -1 is handled), and add a unit/integration
test that confirms confirmUpload succeeds for projects with storageLimit: -1.
---
Duplicate comments:
In `@sdks/urbackend-sdk/src/modules/storage.ts`:
- Around line 89-105: The Blob instanceof check is unguarded and can throw in
runtimes without a global Blob, and the Blob branch currently doesn't preserve a
filename; update the condition to check typeof Blob !== "undefined" && file
instanceof Blob, and in that Blob-handling branch set resolvedName =
resolvedName || (file as any).name (or use filename || file.name fallback) so
the original file name is retained when a File falls through to the Blob branch;
adjust the Blob branch to also set contentType, fileSize, and fileData (as
currently done) while preserving resolvedName.
---
Nitpick comments:
In `@apps/public-api/src/__tests__/storage.controller.test.js`:
- Around line 356-436: Add a test to ensure confirmUpload rejects filePath
values that normalize to a different project to protect ownership checks: create
a case in the requestUpload/confirmUpload suite that calls
storageController.confirmUpload with req.body.filePath set to values like
"other_project_id/file.txt" and "project_id_1/../other/file.txt", mock
normalizeProjectPath (or rely on real normalize function) and assert a
400/forbidden response and error message; reference the confirmUpload handler
and normalizeProjectPath behavior so the test verifies the ownership guard
preventing cross-project normalization escapes.
In `@apps/public-api/src/controllers/storage.controller.js`:
- Around line 297-400: Both requestUpload and confirmUpload return flat JSON and
raw error shapes; update them to follow the project API contract by returning
the envelope { success: bool, data: {...}, message: "" } on success and
throw/use AppError for client errors (instead of res.status(...).json({ error
})) so error middleware handles status and payload. Specifically, in
requestUpload replace the success return to res.status(200).json({ success:
true, data: { signedUrl, token, filePath }, message: "Upload URL generated" })
and replace each early client error branch (400/403/413) to throw new
AppError("...", statusCode) (or construct AppError with the message and code)
instead of returning raw JSON; do the same in confirmUpload: wrap the success
response (message/path/provider/url) into data and message, and convert all
res.status(...).json({ error, details }) error responses to AppError throws so
the centralized error handler emits the proper envelope. Ensure catch blocks do
not directly send raw JSON but rethrow or convert to an AppError with a
sanitized message.
- Around line 313-317: The current TOCTOU occurs in requestUpload where the
advisory quota peek (calculating quotaLimit from effectiveLimit/SAFETY_MAX_BYTES
and checking project.storageUsed + numericSize) can race with confirmUpload's
atomic $expr guard, allowing many signed URLs to be issued but later rejected,
leaving orphaned bucket objects; fix by either implementing a reserve-on-peek
mechanism: atomically $inc a pending_uploads counter or store a pending-upload
token on the project record under the same $expr guard when issuing signed URLs
and then atomically commit (move pending to used) or release that token in
confirmUpload, or explicitly document and handle the race by returning a clear
error to clients and ensuring confirmUpload triggers a compensating-delete path;
refer to requestUpload, confirmUpload, project.storageUsed, numericSize,
SAFETY_MAX_BYTES, effectiveLimit, and the $expr guard when making the change.
In `@sdks/urbackend-sdk/src/modules/storage.ts`:
- Around line 125-131: The confirm step ("POST" "/api/storage/upload-confirm")
can fail after a successful PUT leaving orphaned objects; add a client-side
retry-with-backoff around the this.client.request<UploadResponse>(...) call that
posts to "/api/storage/upload-confirm" to reduce orphan creation (e.g.,
exponential backoff with a small max attempts and jitter), and surface the error
only after retries exhaust; alternatively note adding a server-side sweeper to
garbage-collect unreferenced objects as a follow-up. Ensure you modify the
method that performs the confirm call (the code calling this.client.request for
"upload-confirm") so retries are scoped to that request.
- Around line 115-123: The direct PUT call using fetch(signedUrl, ...) in
upload() lacks cancellation/timeout and throws a plain Error; modify upload() to
accept an optional AbortSignal (or implement a configurable timeout using
AbortController) and pass that signal to fetch so callers can cancel long
uploads, and replace the plain throw new Error(...) for the failed putResponse
path with a StorageError (using the existing StorageError class) that includes
HTTP status and statusText to match other SDK error paths and keep catch
handling uniform.
In `@sdks/urbackend-sdk/tests/storage.test.ts`:
- Line 8: Rename the test description to accurately reflect the new 3-step
presigned flow: change the test title string in the test block that currently
reads 'upload sends FormData and returns { url, path }' to something like
'upload performs 3-step presigned flow and returns { url, path }' so it matches
the updated behavior in the upload test in storage.test.ts (the test function
with that title).
- Around line 39-46: The test's second fetchMock assertion (in storage.test.ts)
only checks method and headers and should also assert the request body was
forwarded; update the expect.objectContaining for fetchMock's 2nd call (the PUT
to 'https://signed.example/upload') to include a body matcher such as body:
expect.anything() or body: fileData (or expect.equal(fileData)) so the Buffer
passed as the upload body is verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c91e0ae-e613-4122-92a6-4f6bdd4d5464
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
apps/public-api/src/__tests__/storage.controller.test.jsapps/public-api/src/controllers/storage.controller.jsapps/public-api/src/routes/storage.jspackages/common/package.jsonpackages/common/src/index.jspackages/common/src/utils/storage.manager.jssdks/urbackend-sdk/src/modules/storage.tssdks/urbackend-sdk/tests/storage.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/common/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/common/src/index.js
- apps/public-api/src/routes/storage.js
- packages/common/src/utils/storage.manager.js
Collaborator
|
LASTLY @coderabbitai Can we Merge this?? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2146 🏁 Script executed: Length of output: 7117
🔀 Merge ReadinessThe PR is technically mergeable — GitHub shows
However, there are 7 open review comments from this review that should be addressed (or explicitly acknowledged as won't-fix) before merging. These cover topics like path traversal edge cases, size-tolerance constants, error handling, and CORS configuration requirements. You can view them all in the Files changed tab. 📂 Dashboard or Public API?This is public API only. Specifically:
In short: app developers calling the SDK or public API directly benefit from this change (no more server-proxied uploads). The web dashboard itself is unaffected. |
Collaborator
|
@coderabbitai create a follow up issue of this to add the same flow in dashboard side as well. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 52
It covers:
|
yash-pouranik
approved these changes
Apr 23, 2026
Collaborator
yash-pouranik
left a comment
yash-pouranik
left a comment
There was a problem hiding this comment.
Thank you @Nitin-kumar-yadav1307
merging this.
Collaborator
|
@coderabbitai Does this PR also updates Docs? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1502 🏁 Script executed: Length of output: 4888
The following docs exist and are relevant, but were not touched:
The two new public endpoints added by this PR are completely undocumented:
The SDK change is transparent to SDK users (same Before merging, both ✏️ Learnings added
🧠 Learnings used |
Collaborator
|
I have already merge this |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 52
It covers updating both doc files with:
🧠 Learnings used |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes # (122)
summary of problem
the current
POST /api/storage/uploadendpoint using multer to hold the entire file on node.js memory (req.file.buffer). then send it to Supabase/S3. this means every upload require heavy task . due to that our node.js server require more food to eat so it started eating RAM and bandwith for no good reason .Under heavy traffic this wouldcause crashes.
What did I change?
The server no longer touches file bytes at all. Instead:
Files changed
storage.manager.js— two new helpers: one to generate signed URLs(works for internal Supabase, external Supabase, and S3/R2), one to
verify the file exists after upload
storage.controller.js— two new endpoints:requestUploadandconfirmUploadstorage.routes.js— wired the new routes, no multer on themsdk/storage.ts— updatedupload()to do the 3-step flow internally,public API is unchanged so existing devs won't notice anything different
common/package.json— added@aws-sdk/s3-request-presignerA few things to note
/uploadroute so nothing breaks for existing usersdon't eat into anyone's storage limit
PUT from the browser — this is a bucket setting, not a code change
i might be forgetting something or done wrong , just let me know!
long Live urBackend!!
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests