allow avatar remove to delete profile pic in R2 - #127
Conversation
📝 WalkthroughWalkthroughThe photo upload flow no longer signs byte length. Clearing a profile photo now deletes the matching avatar object from R2 before updating the user record. Integration tests verify both storage operations. ChangesPhoto storage behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant photoSessions.ts
participant R2
Client->>photoSessions.ts: request presigned upload
photoSessions.ts->>R2: create PutObjectCommand
photoSessions.ts->>R2: request one-hour signed URL
R2-->>photoSessions.ts: return presigned URL
photoSessions.ts-->>Client: return upload URL
sequenceDiagram
participant Client
participant users.ts
participant R2
participant Database
Client->>users.ts: clear profile photo
users.ts->>R2: delete matching avatar object
users.ts->>Database: update profile photo record
Database-->>users.ts: confirm update
users.ts-->>Client: return updated profile
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
backend/tests/users.integration.test.ts (1)
347-351: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the no-delete branches.
This assertion covers only the positive clear path. Add cases for setting a new key and clearing a legacy or non-avatar key, then assert that
r2.sendis not called. These cases protect the deletion guards inbackend/src/routes/users.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/users.integration.test.ts` around lines 347 - 351, Add regression cases in the user integration tests alongside the existing avatar-clear assertion for setting a new key and clearing a legacy or non-avatar key. Exercise each no-delete branch in the users route and assert that r2.send is not called, while preserving the existing positive deletion-path coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/routes/photoSessions.ts`:
- Around line 247-250: Update photoSessionPresignSchema in the photoSessions
validators to accept the handler’s current request body with contentType and
fileSizeKb only: remove the required fileSizeBytes field and its refinement,
unless another caller requires a separate schema with byte-level validation. Add
an integration test for the presign route that omits fileSizeBytes and confirms
validation succeeds.
- Around line 99-110: Update createPresignedReportUpload and the registration
flow to enforce the actual uploaded object size, not only client-reported
fileSizeKb: apply a ContentLength constraint where supported, otherwise use
HeadObjectCommand before registration to reject and delete objects exceeding 5
MB or mismatching the reserved size. Also clean up uploaded photo objects whose
upload URL is never registered.
In `@backend/src/routes/users.ts`:
- Around line 447-452: Move the avatarToDelete derivation into the
prisma.$transaction callback, using the transaction’s current user row value
when profilePhotoUrl is null. Validate that current value with
AVATAR_KEY_PATTERN, delete the resulting key before clearing the profile photo
row, and leave non-clear updates unchanged.
- Around line 447-452: Update the avatar handling around avatarToDelete to
authorize both operations using the authenticated user’s completed upload
ownership, not just AVATAR_KEY_PATTERN: reject profile updates with a new avatar
key that was not uploaded by the user, and reject clearing when the existing
avatar is not their uploaded object. Preserve the existing valid-key behavior
and only set or delete authorized avatar keys.
- Around line 454-462: Move the avatar deletion out of the prisma.$transaction
callback: remove the r2.send(DeleteObjectCommand) call from the transaction and
enqueue avatarToDelete for post-commit cleanup using a durable outbox/retry
mechanism. Ensure the cleanup runs only after the user update and writeAuditLog
commit, with retry handling for failures, rather than deleting the R2 object
inline.
---
Nitpick comments:
In `@backend/tests/users.integration.test.ts`:
- Around line 347-351: Add regression cases in the user integration tests
alongside the existing avatar-clear assertion for setting a new key and clearing
a legacy or non-avatar key. Exercise each no-delete branch in the users route
and assert that r2.send is not called, while preserving the existing positive
deletion-path coverage.
🪄 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 Plus
Run ID: c592a666-1f7a-4a08-bb59-e48399c24557
📒 Files selected for processing (4)
backend/src/routes/photoSessions.tsbackend/src/routes/users.tsbackend/tests/photoSessions.integration.test.tsbackend/tests/users.integration.test.ts
| fileSizeKb: number | ||
| ) { | ||
| const ext = contentType.split('/')[1]; | ||
|
|
||
| const command = new PutObjectCommand({ | ||
| Bucket: R2_BUCKET, | ||
| Key: key, | ||
| ContentType: contentType, | ||
| ContentLength: fileSizeBytes, | ||
| }); | ||
|
|
||
| const uploadUrl = await getSignedUrl(r2, command, { | ||
| expiresIn: 60 * 60, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'backend/src/routes/photoSessions\.ts|photoSession.*schema|presign' || true
echo "== photoSessions outline =="
ast-grep outline backend/src/routes/photoSessions.ts --view expanded || true
echo "== relevant photoSessions sections =="
cat -n backend/src/routes/photoSessions.ts | sed -n '1,140p'
echo "----"
cat -n backend/src/routes/photoSessions.ts | sed -n '220,300p'
echo "== presign schema references =="
rg -n "photoSessionPresignSchema|fileSizeKb|ContentLength|headObject|HeadObject|PutObjectCommand|getSignedUrl" backend/src -SRepository: 86unj/Foundit
Length of output: 13116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== validators/photoSessions.ts =="
cat -n backend/src/validators/photoSessions.ts
echo "== photoSessions delete/cleanup/register sections =="
cat -n backend/src/routes/photoSessions.ts * sed -n '300,460p'
echo "== uploadMetadata.ts =="
cat -n backend/src/utils/uploadMetadata.tsRepository: 86unj/Foundit
Length of output: 50371
Keep an actual object-size check.
fileSizeKb is client-reported metadata. createPresignedReportUpload() builds the R2 presigned PUT without a ContentLength constraint, and registration only compares upload metadata with the stored row. A caller can declare a value within the 5 MB limit and upload a larger object. If this limit remains required, check the object with HeadObjectCommand before registration. Reject and delete objects that exceed the limit or do not match the reserved size, and clean up photos whose upload URL is never registered.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/photoSessions.ts` around lines 99 - 110, Update
createPresignedReportUpload and the registration flow to enforce the actual
uploaded object size, not only client-reported fileSizeKb: apply a ContentLength
constraint where supported, otherwise use HeadObjectCommand before registration
to reject and delete objects exceeding 5 MB or mismatching the reserved size.
Also clean up uploaded photo objects whose upload URL is never registered.
| const { contentType, fileSizeKb } = req.body as { | ||
| contentType: string; | ||
| fileSizeKb: number; | ||
| fileSizeBytes: number; | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align photoSessionPresignSchema with the new request body.
validate(photoSessionPresignSchema) runs before Line 247. The supplied backend/src/validators/photoSessions.ts schema still requires fileSizeBytes and still validates fileSizeKb against it. A request that sends only contentType and fileSizeKb fails before this handler runs.
Remove fileSizeBytes and its refinement from that schema, or split the schema if another caller still needs byte-level validation. Add an integration case that omits fileSizeBytes.
Proposed validator update
export const photoSessionPresignSchema = z
.object({
fileName: z.string().min(1).max(255).trim(),
contentType: z.enum(['image/jpeg', 'image/png', 'image/webp']),
fileSizeKb: z.coerce
.number()
.int()
.min(1)
.max(5 * 1024),
- fileSizeBytes: z.coerce
- .number()
- .int()
- .min(1)
- .max(5 * 1024 * 1024),
- })
- .refine((data) => data.fileSizeKb === Math.ceil(data.fileSizeBytes / 1024), {
- message: 'fileSizeKb must match fileSizeBytes',
- path: ['fileSizeKb'],
- });
+ });Also applies to: 276-280
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/photoSessions.ts` around lines 247 - 250, Update
photoSessionPresignSchema in the photoSessions validators to accept the
handler’s current request body with contentType and fileSizeKb only: remove the
required fileSizeBytes field and its refinement, unless another caller requires
a separate schema with byte-level validation. Add an integration test for the
presign route that omits fileSizeBytes and confirms validation succeeds.
| const avatarToDelete = | ||
| profilePhotoUrl === null && | ||
| existing.profilePhotoUrl && | ||
| AVATAR_KEY_PATTERN.test(existing.profilePhotoUrl) | ||
| ? existing.profilePhotoUrl | ||
| : null; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate users route and nearby lines"
fd -a 'users\.ts$' . | sed 's#^\./##'
echo
if [ -f backend/src/routes/users.ts ]; then
wc -l backend/src/routes/users.ts
sed -n '400,475p' backend/src/routes/users.ts | cat -n -v | sed 's/^/ /'
fi
echo
echo "Search for profile photo update/clear functions and transaction usage"
rg -n "profilePhotoUrl|AVATAR_KEY_PATTERN|prisma.*\$transaction|avatarToDelete|delete.*profile|clear.*photo" backend/src -SRepository: 86unj/Foundit
Length of output: 5566
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect loadActiveUserProfile definition/usages"
rg -n "loadActiveUserProfile|loadUserProfile|profilePhotoUrl|userPhoto|photo" backend/src -S
echo
echo "Prisma/schema fields around user profile"
fd -a 'schema\.prisma$|\.prisma$' backend -e prisma | sed 's#^\./##'
for f in $(fd -a 'schema\.prisma$' backend); do
echo "--- $f"
rg -n "model User|profilePhotoUrl|createdAt|updatedAt" "$f" -A 8 -B 3 || true
doneRepository: 86unj/Foundit
Length of output: 17538
Derive the deletion key inside the photo-clear transaction.
existing is read before prisma.$transaction, while a concurrent photo update can change profilePhotoUrl. When profilePhotoUrl === null, use the current row’s value for avatarToDelete and delete that key before clearing the row so newer R2 objects are not orphaned.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/users.ts` around lines 447 - 452, Move the avatarToDelete
derivation into the prisma.$transaction callback, using the transaction’s
current user row value when profilePhotoUrl is null. Validate that current value
with AVATAR_KEY_PATTERN, delete the resulting key before clearing the profile
photo row, and leave non-clear updates unchanged.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'profilePhotoUrl|AVATAR_KEY_PATTERN|photoSession|ownerId|userId' \
backend/src backend/testsRepository: 86unj/Foundit
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files backend/src/backend/src | sed 's#backend/src/##' | rg 'users|uploads|photoSessions|validator.users|schemas|prisma' || true
echo "== users route outline =="
ast-grep outline backend/src/routes/users.ts --view compact || true
echo "== around profile photo update =="
sed -n '410,485p' backend/src/routes/users.ts
echo "== profile photo setters around users =="
rg -n 'profilePhotoUrl|AVATAR_KEY_PATTERN|presigned-url|UploadSession|photoSession|photoSession.*avatar|avatar|profilePhoto' backend/src/routes/users.ts backend/src/routes/uploads.ts backend/src/routes/photoSessions.ts backend/src/validators/users.ts backend/src/validators/uploads.ts backend/src/validators/photoSessions.ts 2>/dev/null | head -n 220Repository: 86unj/Foundit
Length of output: 7152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== route metadata around profile photo =="
sed -n '380,430p' backend/src/routes/users.ts
echo "== avatar upload route =="
sed -n '20,180p' backend/src/routes/uploads.ts
echo "== load active user profile =="
sed -n '1,80p' backend/src/routes/users.ts
echo "== schema validators =="
cat -n backend/src/validators/uploads.ts 2>/dev/null || true
cat -n backend/src/validators/users.ts 2>/dev/null || true
echo "== prisma schema references =="
rg -n 'model User|profilePhotoUrl|photoSessionImage|PhotoSessionImage|photoUploadSession|PhotoUploadSession|purpose|avatar' backend/prisma backend/schema.prisma 2>/dev/null || trueRepository: 86unj/Foundit
Length of output: 13110
🌐 Web query:
Cloudflare Workers R2 CORS signed URLs preflight options method DELETEObjectCommand
💡 Result:
To successfully use CORS with a DELETE operation via a presigned URL in Cloudflare R2, you must configure a CORS policy on your bucket that explicitly permits the DELETE method [1][2]. When a browser performs a cross-origin request using a presigned URL, it automatically sends an OPTIONS preflight request [1][3]. Cloudflare R2 handles this preflight request by validating it against the bucket's CORS policy [1]. Key requirements for CORS and preflight handling: 1. Bucket CORS Policy: You must configure your bucket with an AllowedMethods entry that includes DELETE [1][3]. 2. Allowed Headers: Ensure AllowedHeaders in your CORS policy includes any custom headers you might be sending with the request [1][3]. 3. Preflight Handling: You do not need to manually create a DELETEObjectCommand for the preflight request; the browser handles the OPTIONS request automatically [1][3]. R2 correctly identifies OPTIONS requests and, provided your CORS policy is configured, will respond with the appropriate CORS headers (e.g., Access-Control-Allow-Origin, Access-Control-Allow-Methods) rather than rejecting the request with a signature error [1][4]. If you encounter a 403 Forbidden error during the preflight phase, ensure that: - The bucket's AllowedMethods explicitly contains DELETE [1][3]. - The Origin of your request matches one of the entries in your bucket's AllowedOrigins list [1][5]. - If you are using custom headers in your request, they are included in the AllowedHeaders configuration [1][5]. Note that while DeleteObjectCommand is used in the AWS SDK to generate a presigned URL for a delete operation, the browser interaction via the presigned URL relies entirely on the underlying CORS policy set on the R2 bucket, not on the command code itself during the browser's preflight phase [2][6].
Citations:
- 1: https://developers.cloudflare.com/r2/buckets/cors/
- 2: https://developers.cloudflare.com/r2/api/s3/presigned-urls/
- 3: https://developers.cloudflare.com/r2/buckets/cors/index.md
- 4: [R2] Document CORS configurations cloudflare/cloudflare-docs#6003
- 5: https://developers.cloudflare.com/api/resources/r2/subresources/buckets/subresources/cors/
- 6: https://developers.cloudflare.com/r2/objects/delete-objects/index.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== r2 cors/config and bucket env =="
rg -n -C 4 'R2_BUCKET|R2_CORS|AllowedMethods|DELETE|CORS|cors|R2_CLIENT|AWS|S3_ENDPOINT|endpoint|public_url|R2_PUBLIC_BASE_URL' backend .github backend/prisma backend/src/backend/prisma 2>/dev/null || true
echo "== user update profile photo implementation remainder =="
sed -n '435,520p' backend/src/routes/users.ts
echo "== test coverage/coverage-related references =="
rg -n 'user_profile_photo_updated|presigned-url|avatars/|profilePhotoUrl|photoPhoto|user/profile/photo' backend/tests backend/docs backend/src 2>/dev/null | head -n 200
echo "== deterministic auth/route checks for endpoint =="
python3 - <<'PY'
from pathlib import Path
p=Path('backend/src/routes/users.ts')
s=p.read_text()
start=s.find("router.patch\\n '/me/photo'")
end=s.find(');', start)
text=s[start:end+2]
print(text)
print("contains requireRole:", "requireRole" in text)
print("contains avatar ownership/ownership:", ("ownerId" in text or "createdBy" in text or "photoSession" in text or "r2.getObject" in text or "findUnique" in text))
PYRepository: 86unj/Foundit
Length of output: 50371
Authorize the avatar key before setting or deleting it.
AVATAR_KEY_PATTERN only checks the key format. This route can store an arbitrary avatars/... key and can delete the existing object when clearing the profile. Reject a new key that is not an upload completed for the authenticated user, and reject clearing when the current avatar was not uploaded for them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/users.ts` around lines 447 - 452, Update the avatar
handling around avatarToDelete to authorize both operations using the
authenticated user’s completed upload ownership, not just AVATAR_KEY_PATTERN:
reject profile updates with a new avatar key that was not uploaded by the user,
and reject clearing when the existing avatar is not their uploaded object.
Preserve the existing valid-key behavior and only set or delete authorized
avatar keys.
| const updated = await prisma.$transaction(async (tx) => { | ||
| if (avatarToDelete) { | ||
| await r2.send( | ||
| new DeleteObjectCommand({ | ||
| Bucket: R2_BUCKET, | ||
| Key: avatarToDelete, | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'outbox|queue|retry|DeleteObjectCommand|r2\.send|prisma\.\$transaction' \
backend/src backend/testsRepository: 86unj/Foundit
Length of output: 50369
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== users.ts relevant section =="
sed -n '420,495p' backend/src/routes/users.ts
echo
echo "== r2 configuration =="
fd -a 'r2\.(ts|js)$|r2.config' backend/src backend/tests 2>/dev/null | sed 's#^\./##' | while read -r f; do
echo "--- $f"
sed -n '1,140p' "$f"
done
echo
echo "== outbox/queue/retry definitions =="
rg -n -i -C 4 \
'(outbox|transactional|queue|worker|cron|rabbit|kafka|bull|sq|cronjob|retryable|retry|retrying)' \
backend/src/db backend/src/lib backend/src/jobs backend/prisma 2>/dev/null || true
echo
echo "== package scripts/deps relevant =="
if [ -f package.json ]; then sed -n '1,220p' package.json; fi
if [ -f backend/package.json ]; then sed -n '1,220p' backend/package.json; fiRepository: 86unj/Foundit
Length of output: 15266
Do not delete the R2 object inside the Prisma transaction.
r2.send() executes before tx.user.update() and writeAuditLog() commit. If the transaction later rolls back, the user still has profilePhotoUrl set but the R2 object is already deleted. Add the deletion to post-commit outbox/retry processing instead of calling DeleteObjectCommand inside prisma.$transaction. There is no existing outbox, queue, or retry infrastructure for this cleanup path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/users.ts` around lines 454 - 462, Move the avatar deletion
out of the prisma.$transaction callback: remove the r2.send(DeleteObjectCommand)
call from the transaction and enqueue avatarToDelete for post-commit cleanup
using a durable outbox/retry mechanism. Ensure the cleanup runs only after the
user update and writeAuditLog commit, with retry handling for failures, rather
than deleting the R2 object inline.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests