Perform a security review of a code change or feature

Input

review_subject: File upload feature — users can upload profile photos and document attachments. Files are stored in S3 and served via a presigned URL.
language_framework: Node.js / Express 4.18
language: javascript
code: |
  const multer = require('multer');
  const AWS = require('aws-sdk');
  const path = require('path');

  const upload = multer({ dest: '/tmp/uploads/' });
  const s3 = new AWS.S3();

  app.post('/api/upload', authenticateJWT, upload.single('file'), async (req, res) => {
    const file = req.file;
    const userId = req.user.id;
    const filename = req.body.filename || file.originalname;

    const key = `uploads/${userId}/${filename}`;

    await s3.upload({
      Bucket: process.env.S3_BUCKET,
      Key: key,
      Body: fs.createReadStream(file.path),
      ContentType: file.mimetype,
    }).promise();

    const url = await s3.getSignedUrlPromise('getObject', {
      Bucket: process.env.S3_BUCKET,
      Key: key,
      Expires: 3600,
    });

    res.json({ url, key });

    fs.unlink(file.path, () => {});
  });

  app.get('/api/files/:key', authenticateJWT, async (req, res) => {
    const url = await s3.getSignedUrlPromise('getObject', {
      Bucket: process.env.S3_BUCKET,
      Key: req.params.key,
      Expires: 60,
    });
    res.redirect(url);
  });
environment: Public-facing web API, accessible by authenticated users of a B2B SaaS platform
access_model: Authenticated users (JWT). Multi-tenant — users should only access their own files.
data_description: User-uploaded files including profile photos and business documents. S3 bucket may contain files from all tenants.
auth_model: JWT tokens with user_id and tenant_id claims. No RBAC — all authenticated users have same permissions.

Output

## Security Review Summary

**Request Changes — High Risk**

This upload feature has a critical path traversal vulnerability that allows any authenticated user to overwrite any other user's files (and potentially access them via the `/api/files/:key` endpoint), and has no file type validation allowing malicious file uploads. These must be fixed before shipping.

## Critical Vulnerabilities

**1. Path Traversal via user-controlled filename**
- **Location:** `/api/upload`, line: `const filename = req.body.filename || file.originalname;`
- **Attack scenario:** An attacker sends a POST request with `filename=../../admin/config.json`. The S3 key becomes `uploads/user123/../../admin/config.json` which S3 normalizes to `admin/config.json` — overwriting another tenant's file or a sensitive object in the bucket. With `filename=../otherUserId/document.pdf`, they can overwrite any other user's files.
- **Impact:** Any authenticated user can overwrite any other user's S3 objects (including other tenants). With the `/api/files/:key` endpoint below, they can also read any object in the bucket.
- **Fix:**
  ```javascript
  // Never use user-provided filenames directly in S3 keys
  const safeFilename = `${Date.now()}-${crypto.randomUUID()}${path.extname(file.originalname)}`;
  const key = `uploads/${req.user.tenant_id}/${req.user.id}/${safeFilename}`;
  ```
- **Verification:** Attempt to upload with `filename=../../other-tenant/secret.pdf` and confirm the S3 key does not contain `..`.

**2. Broken Object-Level Authorization on /api/files/:key**
- **Location:** `GET /api/files/:key`
- **Attack scenario:** User A uploads a file. User B calls `/api/files/uploads/userA-id/secret.pdf` — they receive a presigned URL for User A's file, bypassing any intended access control.
- **Impact:** Any authenticated user can access any file in the S3 bucket by guessing or enumerating keys. In a multi-tenant SaaS, this is cross-tenant data leakage.
- **Fix:**
  ```javascript
  app.get('/api/files/:fileId', authenticateJWT, async (req, res) => {
    // Look up file record in database by fileId
    const file = await db.files.findOne({
      id: req.params.fileId,
      user_id: req.user.id,       // enforce ownership
      tenant_id: req.user.tenant_id
    });
    if (!file) return res.status(404).json({ error: 'File not found' });

    const url = await s3.getSignedUrlPromise('getObject', {
      Bucket: process.env.S3_BUCKET,
      Key: file.s3_key,           // key from DB, never from user input
      Expires: 60,
    });
    res.redirect(url);
  });
  ```

## High Vulnerabilities

**1. No file type validation — malicious file upload**
- **Issue:** `file.mimetype` is client-provided and not validated. An attacker can upload a `.html` file with a `text/plain` MIME type, or an executable disguised as an image.
- **Impact:** If any part of the system serves these files with original content types, stored XSS is possible. HTML files served from S3 execute in the context of the S3 domain.
- **Fix:**
  ```javascript
  const ALLOWED_MIME_TYPES = new Set(['image/jpeg', 'image/png', 'image/webp', 'application/pdf']);
  const ALLOWED_EXTENSIONS = new Set(['.jpg', '.jpeg', '.png', '.webp', '.pdf']);

  // Check both MIME type AND extension (use file-type library for magic bytes check)
  const ext = path.extname(file.originalname).toLowerCase();
  if (!ALLOWED_MIME_TYPES.has(file.mimetype) || !ALLOWED_EXTENSIONS.has(ext)) {
    return res.status(400).json({ error: 'File type not allowed' });
  }
  ```
  For images, also validate magic bytes using the `file-type` npm package.

**2. No file size limit configured**
- **Issue:** `multer({ dest: '/tmp/uploads/' })` has no `limits` configured. Users can upload arbitrarily large files.
- **Impact:** Disk exhaustion on the server's `/tmp` partition, potential DoS.
- **Fix:** `multer({ dest: '/tmp/uploads/', limits: { fileSize: 10 * 1024 * 1024 } })` (10MB example)

## Medium / Low Findings

- **`key` returned in API response:** The S3 key is returned to the client. If keys contain predictable patterns, they can be used for enumeration. Use opaque file IDs instead.
- **ContentType set from client MIME type:** Even after validation, set `ContentType` to the validated type, not the client-provided one.
- **Temp file cleanup:** `fs.unlink` is called without error handling — temp files may persist if upload fails. Use a try/finally pattern.

## OWASP Top 10 Assessment

- A01 Broken Access Control: **Finding** — BOLA on /api/files/:key; path traversal on upload
- A02 Cryptographic Failures: **Checked ✓** — presigned URLs are appropriate
- A03 Injection: **Finding** — path traversal via filename (form of injection)
- A04 Insecure Design: **Finding** — no file ownership model designed; access control is an afterthought
- A05 Security Misconfiguration: **Checked ✓** — S3 bucket config not shown but presigned URLs suggest it's not public
- A06 Vulnerable and Outdated Components: **N/A** — not assessed (check `npm audit`)
- A07 Identification and Authentication Failures: **Checked ✓** — authenticateJWT present on both endpoints
- A08 Software and Data Integrity Failures: **N/A**
- A09 Security Logging and Monitoring Failures: **Finding** — no audit log of file uploads or access
- A10 SSRF: **N/A** — not applicable here

## Data Handling Assessment

- **Logging:** No PII logging observed — good. Ensure the `key` (which includes user_id) is not logged at INFO level in high-volume environments.
- **Encryption:** S3 server-side encryption should be enabled at the bucket level — confirm this is configured.
- **Data minimization:** File metadata (filename, type, size, uploader) should be stored in the database for audit purposes. Currently no audit trail exists.

## Hardening Recommendations

- **Rate limit upload endpoint:** Without limits, authenticated users can flood uploads. Use express-rate-limit: 10 uploads/minute per user.
- **Add Content-Disposition header:** When serving files, set `Content-Disposition: attachment` for non-image types to prevent browser execution.
- **Scan for malware:** For document uploads in a B2B context, consider scanning with ClamAV or a cloud service before making files accessible.
- **Audit log:** Log every upload and access with user_id, tenant_id, filename, size, and timestamp (no file contents).

## Security Test Cases

```
[ ] Upload a file with filename=../../other-user-id/malicious.pdf — key should not contain '..'
[ ] Upload a .html file disguised as image/jpeg — should be rejected
[ ] Access /api/files/:key with another user's key — should return 404
[ ] Upload a 100MB file — should be rejected with 413
[ ] Attempt upload without JWT — should return 401
[ ] Upload with empty filename — should generate a safe default name
```