Spot the Bug #01: The Access Check That Promotes Every Visitor

Spot the Bug is my weekly series: one short piece of code, one security bug. Find it before you scroll. This one is four lines long, and it still gets through code review.

The code

Only moderators should be able to delete posts. A visitor calls this function. What do the two log lines print?

// Only moderators can delete posts
function canDeletePost(user) {
  if (user.role = "moderator") {
    return true;
  }
  return false;
}

const visitor = { role: "visitor" };

console.log(canDeletePost(visitor));
console.log(visitor.role);

1. false, then visitor
2. true, then visitor
3. true, then moderator
4. TypeError, the code crashes

Pick your answer before you open the box below.

Why it happens

Line 3 is meant to ask a question: is this user a moderator? It doesn’t ask anything.

In JavaScript, a single = assigns. It doesn’t compare. So line 3 does two things. It sets user.role to "moderator", and then the whole expression evaluates to the string "moderator".

user.role = "moderator"
// 1. sets role to "moderator"
// 2. the expression returns "moderator"
// 3. a non-empty string is truthy, so the if passes

A non-empty string is truthy, so the if passes for anyone who calls it. That’s your first log line. And because the function changed the object it was handed, the second line prints moderator.

Here’s what makes it nasty: nothing complains. JavaScript took assignment-inside-a-condition from C, and loops like while ((line = reader.next())) depend on it, so the language won’t stop you. No error, so option D is out.

Why this one is worse than it looks

A check that always returns true is already an access control failure. Anyone can delete anything.

The side effect is the bigger problem. In a real app, the user object often lives for the whole request, or gets written back to the session. Every check that runs after this one now sees a moderator. One typo in one function quietly changes permissions everywhere downstream.

It also passes the lazy test. Call it with a real moderator, you get true, and everyone moves on. You only see the bug when you test the user who should be refused.

If you need the formal labels for a ticket or a report: the coding mistake is CWE-481, Assigning instead of Comparing. The impact is broken access control, which sits at A01 in the OWASP Top 10.

The fix

Use strict comparison. And return the result directly, so there’s no branch left to get wrong.

function canDeletePost(user) {
  return user.role === "moderator";
}

Then make sure it can’t ship again. Turn on these two ESLint rules and let them fail the build. This is the flat config format, the default since ESLint 9.

// eslint.config.mjs
export default [
  {
    rules: {
      "no-cond-assign": ["error", "always"],
      "eqeqeq": ["error", "always"],
    },
  },
];

no-cond-assign blocks assignment inside an if, a while or a ternary test. The "always" option matters: on the default setting, someone can wrap the assignment in an extra pair of brackets and it gets through. eqeqeq forces === over ==, so type coercion can’t sneak into the same check. Neither one depends on a reviewer being awake at 6pm on a Thursday.

If you use ESLint’s recommended config (js.configs.recommended), no-cond-assign is already in it, but on the default setting. eqeqeq isn’t in it at all, so add both lines yourself. And check that CI actually runs the linter, and that a lint error fails the pipeline instead of printing a warning nobody reads.

A method for spotting this class of bug

Spotting this one bug is a quiz. Spotting the next one inside a 900-line pull request takes a method. Here’s an order that works for any permission check, cheapest step first.

1. Write the rule down before you read the code

One sentence, in plain words: “Moderators can delete posts. Nobody else can.” Then read the function and point at the exact line that enforces that sentence. If you cannot point at it, or the line you point at does something other than compare, you have found your problem.

Most of us read code to understand what it does. With an access check, read it to prove what it refuses.

2. A check asks. It never changes anything.

A permission check should be read-only. Treat any write inside one as a finding, even if it looks harmless. That means = inside a condition, ++ or --, push, splice, Object.assign, a quiet default like user.role = user.role || "visitor", and anything that saves to the session or the database.

In a review, this is quick. Open every function with can, is, has or allow in its name and look only for writes. It catches more than this one typo.

3. Run the refused user through it first

We test the happy path by instinct. Reverse it. Take the user who should be denied, walk them through the code line by line, and write down what every expression evaluates to. Here, the visitor reaches line 3, the expression comes out as “moderator”, and the door opens. Then look at the user object after the call. If it changed, the check has side effects.

4. Make the type system refuse the write

One honest caveat: TypeScript on its own won’t catch this. A property typed role: string compiles happily with = in the if. Make the user read-only and limit the role to known values, and the compiler stops it:

type Role = "visitor" | "member" | "moderator";

interface User {
  readonly role: Role;
}

function canDeletePost(user: User): boolean {
  if (user.role = "moderator") { // error TS2540: Cannot assign to 'role'
    return true;                 // because it is a read-only property.
  }
  return false;
}

The union type earns its place twice. Misspell "moderator" in a comparison and TypeScript tells you the comparison can never be true.

5. Write the test that should fail

Test every role that must be refused, and check that the function left the object alone. This uses the test runner built into Node, so there’s nothing to install.

const test = require("node:test");
const assert = require("node:assert");
const { canDeletePost } = require("./canDeletePost");

for (const role of ["visitor", "member", "admin", "", undefined]) {
  test(`role ${role} cannot delete`, () => {
    const user = { role };
    assert.strictEqual(canDeletePost(user), false);
    assert.deepStrictEqual(user, { role }); // the check changed nothing
  });
}

test("moderator can delete", () => {
  assert.strictEqual(canDeletePost({ role: "moderator" }), true);
});

Run it against the buggy function and five of the six tests fail. Run it against the fix and all six pass. The moderator test, the lazy one, passes both times.

Look at the "admin" case. If admins should be able to delete too, this test makes someone decide that on purpose and write it into the rule. Otherwise it happens by accident.

6. Deny by default, in one place

The structural fix is to stop scattering role checks around the codebase. Put every permission decision in one policy module, on the server, and make the default answer no.

const PERMISSIONS = {
  "post:delete": ["moderator"],
  "post:create": ["member", "moderator"],
};

function can(user, action) {
  if (!Object.hasOwn(PERMISSIONS, action)) return false; // unknown action: deny
  return PERMISSIONS[action].includes(user.role);
}

Now there’s one function to review, lint and test hard, instead of an if (user.role ...) in every route handler. includes compares; it can’t assign. An action nobody configured gets refused, not waved through. The Object.hasOwn check matters here: a plain PERMISSIONS[action] lookup also finds built-in names like toString or __proto__, and then the code crashes instead of saying no.

Two fixes that look right and are not

Freezing the object. Object.freeze(visitor) sounds like it should help. If canDeletePost is non-strict code, the assignment fails silently, the role stays visitor, and the if still sees a truthy string. It prints true, then visitor. You’ve just moved from answer C to answer B. The write only throws if the function itself runs in strict mode (ES modules and classes are strict automatically), and that gets you answer D. A strict caller doesn’t help; what counts is the mode of the function doing the write. A crash beats a silent pass. It’s still not the fix.

Yoda conditions. Write "moderator" == user.role and a missing = becomes "moderator" = user.role, which is a SyntaxError. It works. You’ll still see it in C code, and WordPress’s own PHP coding standards require it. But it makes every condition harder to read, and it only protects the people who remember to do it. A lint rule doesn’t need anyone to remember.

What to do this week

• Open your lint config. If no-cond-assign on "always" and eqeqeq aren’t there, add them, and make sure CI fails on errors.
• Pick one permission check in your own codebase and walk the refused user through it, line by line.
• Add a test for that refused user, and assert the check didn’t change anything.
• Next time a pull request touches a can… or is… function, look for writes before you look at anything else.

The next bug lands on Instagram at @cyberphobiatv. Comment your answer before you read the breakdown, or comment FIX and I’ll send it to your DMs.

We start scared. We do not stay that way.