Skip to content

CWE-863: Incorrect Authorization - JavaScript

Overview

In Express, NestJS, and similar Node.js frameworks, Incorrect Authorization commonly appears as middleware that trusts a role or user ID sent by the client (req.body.role, a decoded-but-unverified JWT claim, or a hidden form field) instead of resolving it server-side, or as a denylist check that refuses a known-bad list and lets every other role through, failing open when a new role is introduced. Another frequent variant is a guard applied to one route (GET /orders/:id) but forgotten on a sibling route (PATCH /orders/:id), or a NestJS guard registered on the wrong controller after a refactor. Fix by re-validating role and ownership from a trusted server-side source on every route, using an allowlist for role comparisons.

Common Vulnerable Patterns

Denylist Role Check

// VULNERABLE - denylist fails open on any role value not explicitly excluded
const BLOCKED_ROLES = ['guest', 'viewer'];

app.delete('/orders/:id', requireAuth, async (req, res) => {
  if (BLOCKED_ROLES.includes(req.user.role)) {
    return res.status(403).json({ error: 'Forbidden' });
  }

  // Every other role reaches this line: 'support' (added after this check was
  // written), 'Guest' (case mismatch against the blocked value), and an
  // unexpected null role from a partially migrated account.
  await Order.findByIdAndDelete(req.params.id);
  res.status(204).end();
});

// Attack: authenticate with role: 'support' (a value the list never anticipated)
// Result: 'support' is not in BLOCKED_ROLES, so the delete proceeds

Why this is vulnerable: A denylist decides who is refused and lets everything else through, so it is wrong by default for every value it has not been told about - and roles are added by people who are not reading this function. A role introduced next quarter is permitted the moment it exists.

JavaScript's comparison rules widen that further. includes() compares with ===, so 'Guest' does not match the blocked 'guest', and a user record where the field is absent yields undefined, which matches nothing in the list and is allowed through. An allowlist inverts all three: an unknown, mis-cased or missing role matches nothing and is refused.

Trusting Client-Supplied Role or Ownership Data

// VULNERABLE - role and ownership resolved from the request body, not the server
app.put('/orders/:id', requireAuth, async (req, res) => {
  const { role, ownerId, status } = req.body;

  if (role === 'admin' || ownerId === req.user.id) {
    // Both role and ownerId are attacker-controlled request fields, not
    // values resolved from the authenticated session or database.
    await Order.findByIdAndUpdate(req.params.id, { status });
    return res.status(204).end();
  }

  res.status(403).json({ error: 'Forbidden' });
});

// Attack: send { "role": "admin", "status": "shipped" } in the request body
// Result: role === 'admin' is satisfied by the attacker's own claim, with
// no server-side verification that they actually hold that role

Why this is vulnerable: Both operands of the check come out of req.body, so the caller is being asked whether they are authorized and their answer is being believed. requireAuth established a trustworthy identity on req.user and the handler then ignores it.

The tell is worth generalising: an authorization comparison in which no side of the expression comes from the session or the database is not a check. ownerId === req.user.id reads as if it compares two identities, and it compares a claim the attacker typed against one - which is satisfied just as easily by sending the victim's id. The owner must be read from the stored record.

Guard Missing on a Duplicate Route

// VULNERABLE - the guard is applied to the single-resource route but the
// bulk route added later never inherited it
@Controller('orders')
export class OrdersController {
  @UseGuards(OrderOwnerGuard)
  @Delete(':id')
  deleteOrder(@Param('id') id: string) {
    return this.ordersService.delete(id);
  }

  @Post('bulk-delete')
  // No @UseGuards here - added in a later change, guard never copied over
  bulkDelete(@Body('ids') ids: string[]) {
    return this.ordersService.deleteMany(ids);
  }
}

Why this is vulnerable: @UseGuards is opt-in per handler, so a route without the decorator is unprotected - Nest raises no error and logs no warning, and the two methods differ by one line that is easy to read past. Nothing in the framework knows that both handlers reach Order documents.

The bulk route is where this lands most often, and for a structural reason: it is added later, by someone working from the ticket rather than from the controller, and it does not take an :id parameter - so the guard, which was written around params.id, would not have worked on it even if it had been copied over. Closing one of these means finding every handler that touches the same collection, not every path matching the reported URL. Applying the guard at the class level, as the secure pattern below does, inverts the default so a new route inherits the check instead of needing it added.

Secure Patterns

Allowlist Role Check with Server-Resolved Ownership

// SECURE - role allowlist + ownership check resolved from verified session data
const allowedRoles = ['admin', 'editor'];

async function authorizeOrderAccess(req, res, next) {
  // req.user is populated by session/JWT verification middleware upstream,
  // never read from req.body or req.query.
  const { id: userId, role } = req.user;

  if (!allowedRoles.includes(role)) {
    return res.status(403).json({ error: 'Forbidden' });
  }

  if (role === 'admin') {
    return next();
  }

  const order = await Order.findById(req.params.id);
  if (!order || order.ownerId !== userId) {
    return res.status(403).json({ error: 'Forbidden' });
  }

  req.order = order;
  next();
}

// The same middleware is applied to every route for this resource, so a
// fix made once cannot be silently skipped by a route added later.
app.put('/orders/:id', requireAuth, authorizeOrderAccess, async (req, res) => {
  const { status } = req.body;
  req.order.status = status;
  await req.order.save();
  res.status(204).end();
});

app.delete('/orders/:id', requireAuth, authorizeOrderAccess, async (req, res) => {
  await req.order.deleteOne();
  res.status(204).end();
});

Why this works: allowedRoles.includes(role) denies any role value that was not explicitly approved, closing the fail-open gap a !== denylist comparison leaves behind. req.user is populated upstream by session or JWT verification middleware, so role and id cannot be set by the client through the request body. Because authorizeOrderAccess is a standalone middleware applied to both PUT and DELETE, adding it to a new route is a one-line change rather than a re-implementation, which removes the main cause of routes silently missing the check.

NestJS Guard Applied Consistently Across Routes

// SECURE - a single reusable guard, applied at the controller level so
// every route (including ones added later) inherits it by default
@Injectable()
export class OrderOwnerGuard implements CanActivate {
  constructor(private readonly ordersService: OrdersService) {}

  async canActivate(context: ExecutionContext): Promise<boolean> {
    const request = context.switchToHttp().getRequest();
    const { id: userId, role } = request.user; // populated by AuthGuard upstream

    if (!['admin', 'editor'].includes(role)) {
      return false;
    }
    if (role === 'admin') {
      return true;
    }

    // Collect every order this request will touch. A single-resource route
    // names one in the path; a bulk route names many in the body. They are
    // different shapes, so resolving them into one variable and passing it
    // to findById() authorizes neither.
    const orderIds = request.params.id
      ? [request.params.id]
      : request.body?.ids;

    // A route that names no order at all is denied rather than allowed - so
    // adding a route this guard does not understand fails closed, which is
    // the whole reason for putting the guard at the class level.
    if (!Array.isArray(orderIds) || orderIds.length === 0) {
      return false;
    }

    const orders = await this.ordersService.findManyByIds(orderIds);

    // Every ID must resolve to an order this caller owns. Comparing the
    // counts first means an ID that does not exist is denied the same way as
    // one owned by someone else, so the endpoint cannot be used to probe
    // which IDs are real.
    return (
      orders.length === orderIds.length &&
      orders.every((order) => order.ownerId === userId)
    );
  }
}

@Controller('orders')
@UseGuards(AuthGuard, OrderOwnerGuard)  // applied once, at the class level
export class OrdersController {
  @Delete(':id')
  deleteOrder(@Param('id') id: string) {
    return this.ordersService.delete(id);
  }

  @Post('bulk-delete')
  bulkDelete(@Body('ids') ids: string[]) {
    return this.ordersService.deleteMany(ids);
  }
}

Why this works: applying @UseGuards at the controller class level, rather than per-method, means every route added to OrdersController - including bulkDelete - inherits the ownership check by default instead of requiring a developer to remember to decorate each new handler individually. The guard's role check is an explicit allowlist (['admin', 'editor'].includes(role)), and request.user is populated by the upstream AuthGuard from a verified token, not from request.body.

A controller-level guard sees routes of more than one shape, and has to say so. DELETE /orders/:id names one order in the path and POST /orders/bulk-delete names many in the body, so the guard normalises both into a list rather than reading whichever happens to be present. Passing request.params.id ?? request.body?.ids to a findById() hands an array to a single-ID lookup: depending on the driver that throws, returns null, or silently matches nothing, and in every case the batch reaches deleteMany() with no order having been checked. The Array.isArray guard is what makes a third route shape - one naming no order at all - a denial instead of an accident.

The alternative is to move the constraint into the query, which some teams will prefer: have OrdersService.deleteMany() take the caller's ID and issue deleteMany({ _id: { $in: ids }, ownerId: userId }). That cannot be skipped by a route the guard does not recognise, but it deletes the subset the caller owns and reports success, where the guard rejects the whole request. Decide which the API should promise and apply it consistently - a bulk endpoint that silently processes part of a batch is its own class of surprise.

Framework-Specific Guidance

Express Middleware Ordering

// SECURE - authentication runs before authorization, and both run before
// the route handler; order is explicit in the middleware chain
app.use('/orders', requireAuth); // populates req.user from a verified session/JWT

app.put('/orders/:id', authorizeOrderAccess, updateOrderHandler);
app.delete('/orders/:id', authorizeOrderAccess, deleteOrderHandler);

Why this works: requireAuth is mounted with app.use() ahead of the route-specific handlers, so every request under /orders has verified identity available before authorizeOrderAccess runs. Listing authorizeOrderAccess explicitly on each route, rather than relying on route ordering alone, makes a route that is missing it easy to spot in review.

Testing

  • Role boundary: call the route with a role value not in the allowlist (a typo, a new role, an undefined role) and confirm 403, not a successful response.
  • Cross-owner access: authenticate as one user and request another user's resource ID through every route that touches it - GET, PUT, PATCH, DELETE, and any bulk route - confirming each is denied independently.
  • Client-supplied claim rejection: send role or ownerId fields in the request body set to privileged values and confirm they have no effect on the authorization decision.
  • Route coverage: grep the router/controller for every method on the resource and confirm each one references the shared middleware or guard, not an inline copy.
  • Use supertest to call routes directly (bypassing the frontend), since a client-side-only check would otherwise appear to work in manual UI testing.

Common Pitfalls

  • Decoding a JWT without verifying its signature: jwt.decode() returns the claims of any well-formed token without checking the signature; use jwt.verify() with the expected algorithm and secret/key so a forged token is rejected before its claims are trusted.
  • Applying the guard per-route instead of per-controller: In NestJS, decorating individual handlers with @UseGuards() means a new handler added to the controller is unprotected until someone remembers to add the decorator - apply the guard at the class level where every route inherits it by default.
  • Checking role in one middleware and ownership in another, independently: Two middleware that can be reordered or partially applied make it easy to mount only one on a new route; a single combined check, or an explicit chain the tests exercise, is easier to confirm complete.
  • Relying on a client-side route guard (React Router, Vue Router) as the authorization boundary: A frontend route guard controls what the UI renders, not what the API accepts - the same check must be re-implemented and enforced on the server for every request.

Additional Resources