-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Add best practice: scan uploaded files for malware before storing #1378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -1391,6 +1391,32 @@ This style ensures that there is no ambiguity with global npm packages and makes | |||||||
|
|
||||||||
| <p align="right"><a href="#table-of-contents">⬆ Return to top</a></p> | ||||||||
|
|
||||||||
| ## ![✔] 6.28. Scan uploaded files for malware before storing them | ||||||||
| ### `🌟 #new` | ||||||||
|
|
||||||||
| <a href="https://owasp.org/www-community/vulnerabilities/Unrestricted_File_Upload" target="_blank"><img src="https://img.shields.io/badge/%E2%9C%94%20OWASP%20–%20Unrestricted%20File%20Upload-green.svg" alt=""/></a> | ||||||||
|
|
||||||||
| **TL;DR:** Validate uploaded files server-side against a malware engine before writing them to disk or object storage. Filename and MIME type come from the client and are trivially spoofed — only inspecting the actual bytes is reliable. ClamAV is the standard open-source engine for this; wrap it at the route level so a clean verdict is required before any storage operation proceeds: | ||||||||
|
|
||||||||
| ```javascript | ||||||||
| const { scan, Verdict } = require('pompelmi'); // ClamAV wrapper for Node.js | ||||||||
|
|
||||||||
| app.post('/upload', upload.single('file'), async (req, res) => { | ||||||||
| const result = await scan(req.file.path); | ||||||||
|
|
||||||||
| if (result !== Verdict.Clean) { | ||||||||
| await fs.unlink(req.file.path); | ||||||||
| return res.status(422).json({ error: 'File rejected', verdict: result.description }); | ||||||||
|
||||||||
| return res.status(422).json({ error: 'File rejected', verdict: result.description }); | |
| return res.status(422).json({ error: 'File rejected', verdict: result }); |
Copilot
AI
Apr 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The README entry introduces a dedicated details file (sections/security/scanfilesuploads.md) but doesn't include a "Read More" link like most other security practices with dedicated section files. Add a link to keep navigation consistent and avoid orphaning the new document.
| [Read More: Scan files before upload or storage](sections/security/scanfilesuploads.md) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,73 @@ | ||||||||||||||||||||||||
| # Scan uploaded files for malware before storing them | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ### One Paragraph Explainer | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| File upload endpoints are a common attack vector. A file that appears harmless | ||||||||||||||||||||||||
| at the HTTP layer can carry a known malware signature, an embedded script, or | ||||||||||||||||||||||||
| a crafted archive designed to exploit downstream consumers. Validating the | ||||||||||||||||||||||||
| filename or MIME type alone is not enough — those values come from the client | ||||||||||||||||||||||||
| and are trivially spoofed. The only reliable check happens on the file bytes, | ||||||||||||||||||||||||
| server-side, before the file reaches permanent storage. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ClamAV is the standard open-source antivirus engine for server-side scanning. | ||||||||||||||||||||||||
| Integrating it at the upload route means every file is inspected in memory | ||||||||||||||||||||||||
| before it is written to disk, S3, or any other store. | ||||||||||||||||||||||||
|
Comment on lines
+13
to
+14
|
||||||||||||||||||||||||
| Integrating it at the upload route means every file is inspected in memory | |
| before it is written to disk, S3, or any other store. | |
| Integrating it at the upload route means every file is inspected server-side | |
| after upload to a temporary location and before it is moved to permanent | |
| storage such as your uploads directory, S3, or any other store. |
Copilot
AI
Apr 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verdict.ScanError is a server-side scanning failure, but the example returns HTTP 422 (client error). Consider using 503 (or 500) to reflect a temporary/internal failure while still failing closed (rejecting the file).
| return res.status(422).json({ error: 'Scan incomplete: file rejected as precaution' }); | |
| return res.status(503).json({ error: 'Scan incomplete: file rejected as precaution' }); |
Copilot
AI
Apr 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using req.file.originalname directly to build the destination path can allow path traversal (e.g., filenames containing ../) and makes overwrites/collisions likely. Prefer generating a server-side filename (UUID/content hash) and/or sanitizing with path.basename, and keep the original name only as metadata.
Copilot
AI
Apr 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library-selection bullets conflict: "No daemon required" suggests using clamscan only, but the next bullet recommends reaching ClamAV via TCP socket (which typically implies clamd). Reword to clarify that both CLI and daemon modes are acceptable options depending on throughput/ops needs.
| - **No daemon required** — simpler ops; `clamscan` CLI is enough for most | |
| upload volumes | |
| - **Zero runtime dependencies** — nothing to audit beyond ClamAV itself | |
| - **Works in Docker** — ClamAV can run in a sidecar container and be reached | |
| via TCP socket | |
| - **Supports simple CLI mode** — for lower upload volumes, invoking | |
| `clamscan` directly keeps ops simple and avoids requiring a daemon | |
| - **Zero runtime dependencies** — nothing to audit beyond ClamAV itself | |
| - **Also supports daemon/container deployments** — for higher throughput or | |
| Docker sidecar setups, ClamAV can run as `clamd` and be reached via TCP | |
| socket |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new best practice isn't linked from the Table of Contents (and the Security Practices count remains unchanged). Add a TOC entry for 6.28 and update the security section count so readers can discover it.