Add best practice: scan uploaded files for malware before storing#1378
Add best practice: scan uploaded files for malware before storing#1378SonoTommy wants to merge 2 commits intogoldbergyoni:masterfrom
Conversation
Added documentation on scanning uploaded files for malware using ClamAV in Express.
Added a new section on scanning uploaded files for malware, including a TL;DR summary and example code.
There was a problem hiding this comment.
Pull request overview
Adds a new security best practice covering server-side malware scanning of uploaded files (ClamAV) before placing them into permanent storage.
Changes:
- New security guide page: “Scan uploaded files for malware before storing them”.
- New README Security Practices entry (6.28) with a short rationale and Express snippet.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| sections/security/scanfilesuploads.md | Adds the full best-practice writeup, Express example, selection criteria, and references. |
| README.md | Adds Security Best Practice 6.28 summary and a small code snippet. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (result !== Verdict.Clean) { | ||
| await fs.unlink(req.file.path); | ||
| return res.status(422).json({ error: 'File rejected', verdict: result.description }); |
There was a problem hiding this comment.
The snippet returns verdict: result.description, but the longer example treats the scan result as an enum (result === Verdict.Malicious/ScanError). Unless pompelmi actually returns an object with a description field, this will be undefined/misleading—prefer returning the verdict value itself (or a known mapping) and keep the examples consistent.
| return res.status(422).json({ error: 'File rejected', verdict: result.description }); | |
| return res.status(422).json({ error: 'File rejected', verdict: result }); |
| Integrating it at the upload route means every file is inspected in memory | ||
| before it is written to disk, S3, or any other store. |
There was a problem hiding this comment.
The explainer says files are scanned "in memory" before being written to disk, but the example uses multer({ dest: os.tmpdir() }), which writes the upload to disk first. Either adjust the text to describe scanning a temporary file before moving to permanent storage, or change the example to use in-memory storage (e.g., multer.memoryStorage()) and scan the buffer.
| 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. |
| await fs.rename(tmpPath, path.join('./uploads', req.file.originalname)); | ||
| res.json({ status: 'ok' }); |
There was a problem hiding this comment.
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.
|
|
||
| if (result === Verdict.ScanError) { | ||
| await fs.unlink(tmpPath); | ||
| return res.status(422).json({ error: 'Scan incomplete: file rejected as precaution' }); |
There was a problem hiding this comment.
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' }); |
| - **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 |
There was a problem hiding this comment.
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 |
| ## ![✔] 6.28. Scan uploaded files for malware before storing them | ||
| ### `🌟 #new` | ||
|
|
There was a problem hiding this comment.
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.
| ``` | ||
|
|
||
| **Otherwise:** Malicious files stored on your infrastructure can be served to other users, exploit vulnerabilities in downstream parsers (PDF renderers, image processors, archive extractors), or act as a staging point for further attacks | ||
|
|
There was a problem hiding this comment.
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) |
File upload scanning is missing from the security section. This adds a
new best practice covering server-side malware scanning with ClamAV before
a file reaches permanent storage — a common gap in Node.js applications.
The entry follows the existing format: one-paragraph explainer,
concrete Express code example, what to look for in a library,
and the "Otherwise" consequence.