Ask
204
@crashloop_cara ·

the session knows who they are, but every handler re-checks what they can do and i've already missed two

about 30 routes, three roles (owner, admin, member), and the permission check is a copy-pasted block at the top of each handler. last week i shipped two endpoints without the block and a member deleted another team's project before we caught it. where does everyone actually put authorization once the login part is solved — middleware, the handler, or the query?

10 answers Share
Report

Answering anonymously — a moderator will review it first.

  • @serger_scared · 4mo ago

    Push it into the data access layer so that forgetting it returns nothing rather than everything.

    Concretely: no handler ever calls db.project.findMany(). It calls projects.forUser(session), and that function always applies the tenant and role filter. Now a missing check is a route that returns an empty list, which is a bug report, instead of a route that returns everyone's data, which is an incident.

    Middleware is still useful for coarse gates — is there a session, is this an admin area — but it can't express "this project, this user" without doing the query anyway, which is why the check belongs next to the query.

    221
    Share
    Reply

    Answering anonymously — a moderator will review it first.

    Report
  • @pivot_pete · 4mo ago · 3 replies

    Adding the cheap safety net that would have caught your two endpoints: a test that enumerates the router.

    Mine walks every registered route, calls it with a member-level session against another org's resource, and asserts 401 or 403 or an empty result. Any new route is opt-in to the test by existing. It found three unguarded endpoints the day I wrote it, and it has found one every couple of months since, usually within an hour of the mistake.

    It took an afternoon. It is the highest-return test in the codebase by a wide margin.

    167
    Share
    Reply

    Answering anonymously — a moderator will review it first.

    Report
    • @crashloop_cara · 4mo ago

      writing this test today. the fact that new routes get covered automatically because they exist is exactly what i need, since the failure mode was me forgetting, not me not knowing.

      44
      Share
      Reply

      Answering anonymously — a moderator will review it first.

      Report
    • @gramgrader_gus · 4mo ago

      Make it fail loudly on a 200 with data rather than only checking the status code. We had an endpoint returning 200 with an empty envelope and a body full of another org's rows.

      26
      Share
      Reply

      Answering anonymously — a moderator will review it first.

      Report
  • @pinned_versions · 4mo ago · 3 replies

    Going to push back on the "middleware isn't enough" consensus, partly.

    If you namespace routes by resource — everything under /api/org/:orgId/* — middleware can resolve membership for :orgId once and attach it, and then a missing check in a handler is a much smaller blast radius because the wrong org can't even reach the handler. That got us from thirty ad-hoc checks to one, and the per-object checks that remain are genuinely about the object, not the tenant.

    So: middleware for tenancy, query scoping for objects. Doing only one of the two is where people get hurt.

    88
    Share
    Reply

    Answering anonymously — a moderator will review it first.

    Report
    • @crashloop_cara · 4mo ago

      our routes are flat right now, /api/projects/:id, so the org only appears after the lookup. restructuring the paths is a bigger change but i can see it removes the whole class of mistake.

      41
      Share
      Reply

      Answering anonymously — a moderator will review it first.

      Report
    • @adjunct_ora · 4mo ago

      Worth doing, and you don't need to break the URLs — resolve the org inside the lookup and fail closed there. The structural version is nicer but the fail-closed part is what actually saves you.

      33
      Share
      Reply

      Answering anonymously — a moderator will review it first.

      Report
  • @migration_marek · 4mo ago

    We went the postgres row-level security route for exactly this, setting the tenant on the connection per request. It genuinely does mean a forgotten check can't leak, because the database refuses.

    Honest costs: debugging is worse, because a policy silently returning zero rows looks identical to a missing record. Connection pooling needs care so a pooled connection never carries someone else's setting. Migrations get more awkward. I'd do it again for a product handling other people's customer data, and I would not bother for an internal tool.

    71
    Share
    Reply

    Answering anonymously — a moderator will review it first.

    Report
  • @arraysformula · 4mo ago

    The thing that bit us was roles being cached in the session object at login. Someone demoted from admin to member kept admin for as long as their session lived, which was a week.

    Whoever holds the check, make sure it reads current state rather than what was true at login. Ours now reads the membership row on each request; it's one indexed lookup and worth every millisecond.

    56
    Share
    Reply

    Answering anonymously — a moderator will review it first.

    Report
  • @four_percent_fi · 4mo ago

    Authentication answers who. Authorization answers whether, for this exact object. Put the second one where the object is fetched and you stop being able to forget it.

    34
    Share
    Reply

    Answering anonymously — a moderator will review it first.

    Report