fix: track media uploader to prevent cross-agent file theft - #480
Open
0x5t4l1n wants to merge 7 commits into
Open
fix: track media uploader to prevent cross-agent file theft#4800x5t4l1n wants to merge 7 commits into
0x5t4l1n wants to merge 7 commits into
Conversation
Uploaded attachments had no ownership column, allowing any authenticated agent to reference another agent's unattached media ID in their own conversation and claim the file before it was sent. - Add uploaded_by column to media table (migration v2.7.0) - Store uploader's user ID on every upload in handleMediaUpload - Add GetManyByUploader to filter media by owner in getUnassociatedMedia - Update Insert/UploadAndInsert signatures to accept uploaderID - System/email-ingested media passes uploaderID=0 (stored as NULL) and remains accessible for backward compatibility
Contributor
📝 WalkthroughWalkthroughThe change adds nullable uploader ownership to media records, persists authenticated uploader IDs, registers migration ChangesMedia ownership
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 1
🧹 Nitpick comments (2)
internal/media/media.go (1)
162-179: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd manager tests for scoped retrieval.
Cover owned media, foreign-owned media, missing IDs, query failures, and NULL uploader media. This function is the enforcement point for attachment ownership.
🤖 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 `@internal/media/media.go` around lines 162 - 179, Add manager-level tests for GetManyByUploader covering owned records, foreign-owned records, missing IDs, query failures, and media with NULL uploaded_by. Verify owned and NULL-uploader records are returned, foreign or missing records are skipped, and non-ErrNoRows query errors are propagated.internal/media/queries.sql (1)
26-30: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd ownership query regression tests.
Test that an uploader can retrieve its own media. Test that another uploader cannot retrieve it. Test that legacy
uploaded_by IS NULLmedia remains retrievable.🤖 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 `@internal/media/queries.sql` around lines 26 - 30, Add regression tests for the getMediaUploadedBy query covering retrieval by the owning uploader, rejection of a different uploader, and continued retrieval of legacy media where uploaded_by is NULL. Use the existing media query test fixtures and assert both returned records and the expected no-result behavior.
🤖 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 `@internal/migrations/v2.7.0.go`:
- Around line 9-25: The fresh-install path currently omits the schema change
introduced by V2_7_0. Update the installation flow in cmd/install.go to execute
V2_7_0 after applying schema.sql, preserving the migration’s existing column and
index setup so new databases include media.uploaded_by.
---
Nitpick comments:
In `@internal/media/media.go`:
- Around line 162-179: Add manager-level tests for GetManyByUploader covering
owned records, foreign-owned records, missing IDs, query failures, and media
with NULL uploaded_by. Verify owned and NULL-uploader records are returned,
foreign or missing records are skipped, and non-ErrNoRows query errors are
propagated.
In `@internal/media/queries.sql`:
- Around line 26-30: Add regression tests for the getMediaUploadedBy query
covering retrieval by the owning uploader, rejection of a different uploader,
and continued retrieval of legacy media where uploaded_by is NULL. Use the
existing media query test fixtures and assert both returned records and the
expected no-result behavior.
🪄 Autofix
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: 50fa1499-ec1e-4414-83c7-c4d612c8b98a
📒 Files selected for processing (11)
cmd/conversation.gocmd/media.gocmd/messages.gocmd/upgrade.gocmd/users.gointernal/conversation/conversation.gointernal/conversation/message.gointernal/media/media.gointernal/media/models/models.gointernal/media/queries.sqlinternal/migrations/v2.7.0.go
Comment on lines
+9
to
+25
| func V2_7_0(db *sqlx.DB, fs stuffbin.FileSystem, ko *koanf.Koanf) error { | ||
| // Add uploaded_by column to media table to track which user uploaded each file. | ||
| // This is used to prevent cross-agent media theft via sequential ID enumeration (IDOR). | ||
| if _, err := db.Exec(` | ||
| ALTER TABLE media ADD COLUMN IF NOT EXISTS uploaded_by INT NULL REFERENCES users(id) ON DELETE SET NULL; | ||
| `); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if _, err := db.Exec(` | ||
| CREATE INDEX IF NOT EXISTS index_media_on_uploaded_by ON media(uploaded_by); | ||
| `); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
schema_file="$(fd -a '^schema\.sql$' . | head -n 1)"
test -n "$schema_file"
rg -n -C 18 'CREATE TABLE media|uploaded_by' "$schema_file"
rg -n -C 5 'schema\.sql|upgrade\(|getPendingMigrations|migList' \
--glob '*.go' .Repository: abhinavxd/libredesk
Length of output: 6792
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## cmd/install.go outline\n'
ast-grep outline cmd/install.go --view expanded || true
printf '\n## cmd/install.go\n'
cat -n cmd/install.go
printf '\n## cmd/main.go install handling area\n'
sed -n '130,190p' cmd/main.go | cat -n
printf '\n## schema.sql relevant media section\n'
sed -n '360,415p' schema.sql | cat -n
printf '\n## migration files for upload-related schema\n'
rg -n -C 5 'uploaded_by|ALTER TABLE media|CREATE INDEX.*media|V2_7_0|v2\.7\.0' --glob '*.go' internal cmdRepository: abhinavxd/libredesk
Length of output: 12201
Run the v2.7.0 migration from first-time installs.
cmd/install.go only reads and executes /schema.sql, and that schema still lacks media.uploaded_by. Add V2_7_0 to the install path in cmd/install.go, or add the column to schema.sql, so fresh databases match the current media model.
🤖 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 `@internal/migrations/v2.7.0.go` around lines 9 - 25, The fresh-install path
currently omits the schema change introduced by V2_7_0. Update the installation
flow in cmd/install.go to execute V2_7_0 after applying schema.sql, preserving
the migration’s existing column and index setup so new databases include
media.uploaded_by.
Merged
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
1 participant
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.
What
Uploaded attachments had no ownership column on the
mediatable. Any authenticated agent could reference another agent's unattached media ID in their own conversation request, claiming the file before the original uploader hit send. SequentialSERIALprimary keys made IDs trivially enumerable.Root cause
getUnassociatedMedia()incmd/media.gochecked onlymodel_id == 0(file not yet attached), with no check for who uploaded the file.Fix
uploaded_by INT NULL REFERENCES users(id)column to themediatable (migrationv2.7.0)handleMediaUploadGetManyByUploader()to the media manager — returns only records owned by the requesting user, or records withuploaded_by IS NULL(system/email-ingested media, for backward compatibility)getUnassociatedMedia()to callGetManyByUploader()instead ofGetMany()uploaderIDthroughInsert()andUploadAndInsert()signatures; system-ingested attachments pass0(stored asNULL)Verification
Tested locally against a running instance:
Before fix — attacker (user B) can steal agent A's unattached file using sequential ID:
After fix — same request silently drops the attachment:
Agent A can still send their own files normally (no regression).
Files changed
internal/migrations/v2.7.0.gouploaded_bycolumn + indexcmd/upgrade.gov2.7.0internal/media/models/models.goUploadedBy null.Intfieldinternal/media/queries.sqluploaded_byto INSERT; newget-media-uploaded-byqueryinternal/media/media.goGetManyByUploader(); updateInsert()/UploadAndInsert()signaturesinternal/conversation/conversation.gomediaStoreinterfaceinternal/conversation/message.gouploaderID=0for system-ingested attachmentscmd/media.goauser.IDtoInsert();getUnassociatedMediauses ownership checkcmd/messages.goauser.IDtogetUnassociatedMediacmd/conversation.goauser.IDtogetUnassociatedMediacmd/users.gouser.IDtoUploadAndInsertfor avatar uploadsSummary by CodeRabbit
Security
Data Management
Maintenance