Fix/upload storage guardrails#14129
Conversation
`filterFile` gated uploads on the browser-supplied `file.mimetype`/filename, which are attacker-controlled, so a file could claim an allowed type while its bytes were something else. After the size/claimed-type checks, sniff the saved file's magic bytes and require the detected type to also pass the endpoint's allow-list; files with no detectable signature (plain text, code, csv, svg) fall through to the existing claimed-type gate. `filterFile` becomes async and all call sites now await it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`deleteFirebaseFile` guarded deletion with `filepath.includes(req.user.id)`, a substring check against the persisted URL. Parse the storage path and require the file record's structured owner (`file.user`) to match as an exact path segment, so a crafted filepath cannot target another user's object. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46a83ff3e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return; | ||
| } | ||
|
|
||
| if (!fileConfig.checkType(detected.mime, endpointFileConfig.supportedMimeTypes)) { |
There was a problem hiding this comment.
When an endpoint's allow-list is broad (the default config allows both images and PDFs), this check still accepts a PDF uploaded with a claimed image/png MIME because application/pdf is also allowed by the endpoint. Downstream regular uploads still persist type: file.mimetype (processFileUpload stores the browser-supplied value), and image/avatar routes are likewise relying on this guard to reject spoofed content, so the new MIME-spoofing protection is bypassed for default/broad endpoint configs instead of verifying that the detected type matches the claimed/upload context.
Useful? React with 👍 / 👎.
| const deleteFirebaseFile = async (req, file) => { | ||
| await deleteRagFile({ userId: req.user.id, file }); | ||
|
|
||
| const ownerId = file.user?.toString?.(); |
There was a problem hiding this comment.
When a user with EDIT access to a shared agent deletes an embedded Firebase file owned by the agent author, the new owner-based path validation below allows deleting that non-owned storage object, but deleteRagFile has already run with req.user.id instead of the file owner. Since the RAG API token is generated for the supplied user, the owner's embedded document is not removed while the Firebase object and DB metadata can still be deleted, leaving orphaned vectors for shared-agent Firebase files.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| if (!fileConfig.checkType(detected.mime, endpointFileConfig.supportedMimeTypes)) { |
There was a problem hiding this comment.
When users upload otherwise-supported WAV or AVI files, file-type reports the magic bytes with vendor MIME names such as audio/vnd.wave / video/vnd.avi, while the default allow-list in packages/data-provider/src/file-config.ts accepts audio/wav/audio/x-wav and video/avi. Comparing detected.mime directly here makes those legitimate uploads fail with File content does not match its file type; normalize the sniffer result (or include these aliases) before applying the allow-list.
Useful? React with 👍 / 👎.
| const fileName = extractFirebaseFilePath(file.filepath); | ||
| if (!fileName.includes(req.user.id)) { | ||
| throw new Error('Invalid file path'); | ||
| if (!fileName || !fileName.split('/').includes(ownerId)) { |
There was a problem hiding this comment.
This accepts any Firebase object path that contains the owner id as some segment, but Firebase uploads in this service are written as ${basePath}/${userId}/${fileName}. If a persisted URL is ever crafted as images/victim/${ownerId}/secret.png, it passes this check for file.user === ownerId and deletes that arbitrary object; validate the parsed path shape/prefix instead of searching all segments.
Useful? React with 👍 / 👎.
Content sniffing: compare the sniffed type against the claimed `file.mimetype` instead of the endpoint allow-list. On broad endpoint configs the previous check accepted e.g. a PDF claimed as `image/png` because PDF was also allowed; now a mismatch in content category is rejected and executables are rejected outright. Category comparison (audio/video merged for shared containers) tolerates benign sniffer aliases so legitimate WAV/AVI/legacy-Office uploads are not falsely rejected. Firebase delete: run RAG cleanup under the file's owner (`file.user`) rather than the requester, and only after ownership is validated, so a shared-agent editor deleting an owner's file removes the owner's vectors. Validate the owner id at its fixed path position (second segment) instead of searching all segments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afa44ea4e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (contentCategory(detectedMime) !== contentCategory(claimedMime)) { | ||
| throw new Error('File content does not match its file type'); | ||
| } |
There was a problem hiding this comment.
When an endpoint narrows supportedMimeTypes within a top-level category (for example PDF-only), this comparison still accepts an unsupported same-category upload such as bytes detected as application/zip while claiming application/pdf, because both collapse to application. The earlier check in filterFile has already trusted file.mimetype, so the sniffed MIME needs to be checked against endpointFileConfig.supportedMimeTypes (with explicit aliases where needed) rather than only comparing categories; otherwise the new spoofing guard can be bypassed by unsupported same-category content.
Useful? React with 👍 / 👎.
The category-only comparison accepted an unsupported same-category upload (e.g. bytes detected as application/zip while claimed as application/pdf), since both collapse to `application`. Compare a container-aware content signature instead: application/* is kept fine-grained so zip never matches pdf, while ZIP-based (OOXML/ODF/epub, sniffed as application/zip) and OLE/CFB (legacy Office, sniffed as application/x-cfb) container families are each treated as one signature so genuine Office/OpenDocument uploads still pass. The claimed type is already validated against the endpoint allow-list, so requiring the sniffed content to match the claim also enforces the allow-list against the real bytes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d151d98852
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (contentSignature(detectedMime) !== contentSignature(claimedMime)) { | ||
| throw new Error('File content does not match its file type'); | ||
| } |
There was a problem hiding this comment.
For endpoints with a narrower supportedMimeTypes list, this comparison still accepts unsupported bytes whenever they share a contentSignature with the claimed type. For example, the default config rejects image/bmp, but a BMP uploaded as image/png reaches this line with both signatures equal to image, so filterFile passes even though the detected MIME never matched the endpoint allow-list; the same pattern applies to ZIP containers claimed as OOXML. Please validate detectedMime against endpointFileConfig.supportedMimeTypes rather than only comparing it with the client-supplied MIME.
Useful? React with 👍 / 👎.
The signature-only comparison accepted an unsupported type whenever it shared a signature with the (allowed) claimed type — e.g. image/bmp claimed as image/png on an image/png-only endpoint, since both are `image`. Also check the sniffed type itself against endpointFileConfig.supportedMimeTypes (via checkType), so a type the endpoint disallows cannot ride in under an allowed claim. The claimed-signature comparison is kept too, so an allowed-but-different type (a PDF claimed as image/png on an endpoint that also allows PDF) is still rejected. `file-type` reports genuine OOXML/ODF/epub uploads as their specific type, so a plain application/zip detection is a real ZIP and only passes where ZIP is allowed — closing the raw-ZIP-claimed-as-OOXML bypass. Legacy Office (.doc/.xls/.ppt) all report application/x-cfb and cannot be told apart, so the OLE family is expanded to avoid rejecting genuine legacy Office uploads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7567764e07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const allowListCandidates = (detectedMime) => { | ||
| const candidates = new Set([detectedMime]); | ||
| const [type, subtype = ''] = detectedMime.split('/'); | ||
| candidates.add(`${type}/${subtype.replace(/^x-/, '').replace(/^vnd\./, '')}`); |
There was a problem hiding this comment.
When a valid upload uses one of the media types already allowed by LibreChat, file-type can report a different canonical MIME than the configured allow-list: for example a .mov upload can be claimed/accepted as video/mov by the existing config (packages/data-provider/src/file-config.ts:198) but is sniffed as video/quicktime. Since this candidate set only strips leading x-/vnd. and does not add equivalents such as video/mov, detectedIsSupported becomes false and the upload is now rejected as spoofed even though it previously matched the configured supported type.
Useful? React with 👍 / 👎.
`file-type` reports canonical media names that diverge from the configured allow-list tokens (video/quicktime vs video/mov, video/matroska vs video/mkv, Ogg/ASF under application/*), so the exact sniffed-type allow-list check falsely rejected legitimate .mov/.3gp/.ogg/.wmv uploads. Recognize the application/* media containers as media and skip the exact allow-list check for media, constraining it to the same category as the (already allow-listed) claim. The executable rejection and the claim-signature check are unchanged, so a document or executable disguised as media — and media disguised as a document — are still rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
Two small, non-breaking hardening changes to the file upload/storage path. Both replace a "trust the client-supplied label" check with a "verify the actual value" check.
1. Content sniffing on upload (
filterFile)filterFilegated uploads on the browser-suppliedfile.mimetype/filename, which are attacker-controlled — a file could claim an allowed type while its bytes are something else. After the existingsize/claimed-type checks, it now sniffs the saved file's magic bytes (via
file-type, already a dependency) and enforces:image/bmpasimage/png, or a raw ZIP asapplication/pdf).filterFilebecomes async; all call sites nowawaitit.Scope/limitations of magic-byte sniffing (by design):
file-type's canonical names diverge from the configured tokens (video/quicktimevsvideo/mov,video/matroskavsvideo/mkv,Ogg/ASF reported under
application/*) and containers don't reveal audio-vs-video, so an exact check would falsely reject legitimate.mov/.mkv/.ogg/.wmvuploads. Executable/cross-category disguises arestill blocked for media.
2. Firebase delete validates structured owner, not URL substring
deleteFirebaseFileguarded deletion withfilepath.includes(req.user.id)— a substring check against the persisted URL. It now:file.user) at its fixed path position (${basePath}/${ownerId}/${fileName}), rejecting crafted paths likeimages/victim/${ownerId}/secret.png, anddeleteRagFile) under the file owner rather than the requester, and only after ownership is validated — so a shared-agent editor deleting an owner's file removes the owner's vectors.No new dependencies, env vars, interface changes, or version bumps.
filterFile's call signature is unchanged ({ req, image, isAvatar }); it just returns a promise now.Change Type
Testing
Added/updated unit tests for both changes, and verified the sniffer strings the tests use match real
file-typeoutput for real files of each type (file-typeis ESM and can't load under Jest, so the sniffer ismocked with values confirmed against real files).
process.spec.js: cross-category spoofs rejected (PDF-as-image); unsupported-but-allowed-claim types rejected (BMP-as-PNG, ZIP-as-PDF); executables rejected; genuineimages/PDF/Office/legacy-Office/media (including
.mov/.3gp/.ogg/.wmv) accepted; disguises within media rejected (PDF-as-.mov,.mov-as-PDF); non-sniffable content still passes the claimed-type gate.Firebase/crud.spec.js: same-owner delete succeeds; cross-user path rejected; substring-not-segment owner rejected; wrong-position owner path rejected; missing-owner rejected; RAG cleanupruns under the file owner.
Run:
cd api && npx jest server/services/Files/process.spec.js server/services/Files/Firebase/crud.spec.js
Lint clean on all touched files.
Test Configuration:
apiworkspace); no external services required (Firebase SDK boundary is mocked, real path/owner-validation logic is exercised).Checklist