fix: restrict document upload file types to prevent XSS (#7959)

[wakqasahmed]

Fixes #7959

Vulnerability

App\Services\Contact\Document\UploadDocument validated only that the document field was required|file, with no restriction on file type/extension/MIME. Uploaded documents are stored on the public filesystem disk (the default configured disk), which is symlinked into public/storage and served directly by the webserver, outside of any Laravel auth/routing.

Because there was no allowlist, an attacker with any valid Monica account could upload an .html file. The webserver serves it with Content-Type: text/html from a publicly reachable, unauthenticated URL under Monica's own origin. A victim who opens that link (e.g. framed as a "business report") gets attacker JavaScript executed in Monica's origin, with access to the victim's session cookie and XSRF token — enabling contact-data exfiltration and full account takeover.

Both the web upload path (Contacts\DocumentsController) and the API upload path (Api\Contact\ApiDocumentController) call the same UploadDocument service, so both were affected and both are now fixed by this single change.

Fix

Added a mimes: allowlist to the document validation rule in UploadDocument::rules(), restricting uploads to common, non-executable document/image types (pdf, doc, docx, odt, xls, xlsx, ods, ppt, pptx, odp, txt, csv, rtf, jpg, jpeg, png, gif, heic). Laravel's mimes rule validates the file's real, sniffed extension (not the client-supplied name/MIME), so a renamed .html payload is rejected regardless of its filename.

This directly closes the reported vector: HTML/SVG/script-bearing files can no longer be accepted by the upload endpoint at all, so the webserver can never be tricked into serving attacker-controlled markup with a script-executing content type from this feature.

I scoped the fix to the validation allowlist (the issue's own recommendation #1) rather than also restructuring file serving to go through an authenticated controller route (recommendation #2) or adding webserver-level headers on /storage/documents/ (recommendation #3) — both of those are broader architectural/infra changes outside a single, reviewable PR, and the allowlist alone removes the ability to store any content type a browser would execute.

Tests

Added:

• tests/Unit/Services/Contact/Document/UploadDocumentTest.php::it_fails_if_the_file_type_is_not_allowed — asserts a .html upload throws ValidationException.
• tests/Api/Contact/ApiDocumentControllerTest.php::document_store_gets_an_error_if_the_file_type_is_not_allowed — asserts the API returns a validation error for a .html upload.

Existing tests use UploadedFile::fake()->image('...pdf'), which is real generated image content (accepted by the new png/jpg allowlist entries), so legitimate uploads are unaffected.

Disclosure: PHP is not available in the environment these changes were authored in, so these tests could not be executed locally. They were written to match this repo's existing Pest/PHPUnit conventions in the same test files and should be run in CI/a working dev environment before merge.

[CLAassistant]

All committers have signed the CLA.

[gitar-bot]

💡 Quality: Allowlist narrows previously-unrestricted uploads (behavior change)

Previously any file type was accepted; the allowlist now silently rejects formats users may legitimately store (e.g. zip, mp3, mp4, mov, epub, key/numbers/pages). This is the correct security tradeoff, but it is a behavior change for existing users. Consider documenting the accepted types in the upload UI/error surface, or confirm with maintainers that this narrowed set is intended.

_{Was this helpful? React with 👍 / 👎}

[wakqasahmed]

Added zip to the allowlist — that's the right call given docx/xlsx/odt etc. are zip containers and can sniff as generic zip on some libmagic versions. Couldn't add a test that actually exercises this: Laravel's UploadedFile::fake() doesn't do real content-based MIME sniffing, so the existing test suite can't distinguish a real vs. fake docx here — the happy-path pdf test above this one relies on the same fake behavior. Updated the existing rejection test's expected message to include zip in the list.

[gitar-bot]

Code Review 👍 Approved with suggestions 1 resolved / 2 findings

Restricts document upload file types to an allowlist of common document and image formats, closing an XSS vulnerability where attackers could upload and serve HTML files from the public filesystem. The fix addresses both web and API upload paths. Consider documenting the accepted types in the upload UI or confirming with maintainers that this narrowed set is the intended scope, as existing users storing other formats (e.g. zip, mp3, mp4, mov, epub) will now see silent rejections.

💡 Quality: Allowlist narrows previously-unrestricted uploads (behavior change)

📄 app/Services/Contact/Document/UploadDocument.php:23

Previously any file type was accepted; the allowlist now silently rejects formats users may legitimately store (e.g. zip, mp3, mp4, mov, epub, key/numbers/pages). This is the correct security tradeoff, but it is a behavior change for existing users. Consider documenting the accepted types in the upload UI/error surface, or confirm with maintainers that this narrowed set is intended.

✅ 1 resolved

✅ Edge Case: mimes rule may reject legit docx/xlsx/odt (zip-based) uploads

📄 app/Services/Contact/Document/UploadDocument.php:23
Laravel 9's mimes rule validates against UploadedFile::guessExtension(), which sniffs the file's real content (via fileinfo/libmagic) and maps it to an extension — it does not use the client-supplied name. OOXML (docx/xlsx/pptx) and ODF (odt/ods/odp) files are ZIP containers, and on many libmagic versions their content is detected as application/zip → guessed extension zip, which is NOT in the allowlist. On those environments legitimate Office/OpenDocument uploads — the very types this list intends to permit — would be rejected. Since the PR discloses tests couldn't be run, verify real docx/xlsx/odt uploads in a working environment; if they fail, add zip to the allowlist or use mimetypes: with the concrete OOXML/ODF MIME types.

🤖 Prompt for agents

Code Review: Restricts document upload file types to an allowlist of common document and image formats, closing an XSS vulnerability where attackers could upload and serve HTML files from the public filesystem. The fix addresses both web and API upload paths. Consider documenting the accepted types in the upload UI or confirming with maintainers that this narrowed set is the intended scope, as existing users storing other formats (e.g. zip, mp3, mp4, mov, epub) will now see silent rejections.

1. 💡 Quality: Allowlist narrows previously-unrestricted uploads (behavior change)
   Files: app/Services/Contact/Document/UploadDocument.php:23

   Previously any file type was accepted; the allowlist now silently rejects formats users may legitimately store (e.g. zip, mp3, mp4, mov, epub, key/numbers/pages). This is the correct security tradeoff, but it is a behavior change for existing users. Consider documenting the accepted types in the upload UI/error surface, or confirm with maintainers that this narrowed set is intended.

Review coverage

Rules No rules evaluated

Functional validation Not enabled · Set up

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source}

[wakqasahmed]

Good catch on the OOXML/ODF zip-detection edge case — the narrowed allowlist is intentional (this closes a stored-XSS path via arbitrary file upload), but you're right that some environments' fileinfo/libmagic could misdetect docx/xlsx/odt as plain zip. Rather than adding zip back (which would reopen the same upload-anything hole), I'd lean toward using mimetypes: with the explicit OOXML/ODF MIME types if that turns out to be a real problem in practice — happy to follow up once/if a maintainer weighs in, since you mentioned focus is on the next major version right now.

[wakqasahmed]

Hi @gitar-bot — this has been sitting green and mergeable for a bit now — happy to make any changes if something's blocking it, otherwise just flagging in case it slipped by. Thanks!

[gitar-bot]

Hi @gitar-bot — this has been sitting green and mergeable for a bit now — happy to make any changes if something's blocking it, otherwise just flagging in case it slipped by. Thanks!

Gitar can change code and merge on your behalf, so it only acts on requests from people who can push. This needs Write access to this repository.