Skip to content

[critical] No default-deny authorization β€” unauthenticated callers can delete accounts and dump the user tableΒ #491

Description

@codesage7

🚨 ALL CI CHECKS MUST PASS

Your PR will not be reviewed or merged until every CI job is green. No exceptions.

Run these locally before you push:

npm run format:check    # Formatting
npm run lint            # Lint (--max-warnings=0)
npm run typecheck       # TypeScript
npm test                # Tests
npm run build           # Build
npm run ci:app-boot     # App boot (needs Postgres + Redis)

A red build is the single most common reason work stalls on this repo. If CI fails and you are stuck, say so in the PR β€” do not push a failing build and go quiet.

Also required: put Closes #<this issue number> in your PR description. Without it, GrantFox cannot link your PR to this issue.


What needs to be done

The application has no global authentication guard. Authorization is opt-in per controller or per route, and several routes were simply never opted in β€” including destructive ones. Move the application to default-deny: every route authenticated unless explicitly marked public.

Why it matters

The only globally registered guard is rate limiting. From src/app.module.ts:

{
  provide: APP_GUARD,
  useClass: ThrottlerGuard,
},

AdminGuard and RolesGuard are provided but not registered globally, and there is no global JwtAuthGuard. Nothing in main.ts calls useGlobalGuards. So a route with no @UseGuards decorator is fully public.

src/users/users.controller.ts has six routes and exactly one guard:

Route Guard Exposure
@Post() line 33 none account creation
@Get() line 45 none returns every user in the system
@Get(':id') line 52 none any user record by ID
@Get(':id/stats') line 60 none any user's statistics
@Patch('profile') line 67 JwtAuthGuard βœ…
@Delete(':id') line 85 none soft-deletes any account by ID

An unauthenticated request to DELETE /users/1 deletes that account. An unauthenticated GET /users dumps the entire user table β€” names, emails, whatever the entity exposes. Neither requires a token, a session, or any relationship to the target.

This is not a single missing decorator, it is a missing default. escrow.controller.ts:22 and orders.controller.ts:43 do apply JwtAuthGuard at the controller level, which shows the pattern is understood β€” it just has to be remembered every time, and it was not. Other controllers with zero @UseGuards: products/product-images.controller.ts, categories/categories.controller.ts, notifications/notifications.controller.ts (covered separately).

Under the current design, every new controller ships public unless someone remembers. The fix is to invert it.

Technical context

  • src/app.module.ts β€” the APP_GUARD provider block; this is where a global JwtAuthGuard belongs
  • src/users/users.controller.ts β€” the six routes above
  • src/auth/ β€” the existing JwtAuthGuard
  • src/guards/ β€” AdminGuard, RolesGuard, and their (currently skipped) tests

You will need a @Public() decorator plus a Reflector check in the global guard so genuinely public routes β€” login, register, health, the Stellar webhook, public product/category reads β€” keep working. The webhook authenticates by signature rather than JWT and must be marked public, not guarded.

Ownership is a separate axis from authentication: being logged in must not be enough to read or delete another user's record. GET /users/:id and DELETE /users/:id need an ownership-or-admin check, not just a token.

Acceptance criteria

  • A global authentication guard is registered via APP_GUARD
  • A @Public() decorator exists and is applied to every route that must remain unauthenticated, with each one justified in the PR description
  • GET /users requires admin
  • GET /users/:id, GET /users/:id/stats and DELETE /users/:id enforce owner-or-admin, not merely authentication
  • A test asserts an unauthenticated DELETE /users/:id is rejected
  • A test asserts an authenticated non-owner, non-admin DELETE /users/:id is rejected
  • An inventory of every route and its final auth status is posted as a comment on this issue
  • npm run ci:app-boot still passes β€” a global guard must not break boot
  • All CI jobs pass

Out of scope

  • The notifications module β€” tracked separately
  • Redesigning roles/permissions beyond what owner-or-admin needs
  • Rate limiting changes

Getting started

npm ci
grep -rn "@UseGuards" src --include="*.controller.ts"
grep -rn "APP_GUARD" src

Do the inventory first and post it before writing code β€” this touches every endpoint in the application, and agreeing the public list up front avoids a painful review.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    BackendauthbugSomething isn't workingp0Backlog label: p0

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions