ReviewOS

stacks/bench-review

moderation: no ban/suspend capability — only hard delete, and deleted emails re-register instantly

#49
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Problem

A judge-review platform is a magnet for harassment and defamation actors, and right now the only admin response to an abusive account is DELETE /api/admin/users/{id} — a hard delete that destroys the abuser's content (the evidence) and frees their email for instant re-registration. The abuse-response loop is fully open: delete → re-register → repeat. There is no way to suspend an account, no account-state check anywhere in auth, and no audit trail of the action. This is a launch blocker.

Findings

  • No account-state column exists. The users table has only name/email/password/role_label/credential_*/timestamps — no status, banned_at, or suspended_at (database/migrations/0000000035-create-users-table.sql). No ban/suspension table exists either.
  • Middleware checks nothing account-level. app/Middleware/Auth.ts validates only token/session validity and app/Middleware/Admin.ts only checks the admin role; a grep for banned|suspend|disabled|locked across both yields zero hits. A banned user (if the concept existed) would still authenticate.
  • The only moderation tool is a hard delete that destroys evidence. Admin user routes are index/update/role/delete only (routes/api.ts:356-377); app/Actions/Admin/Users/UpdateUserAction.ts:33-47 patches name/email only; app/Actions/Admin/Users/DeleteUserAction.ts:62-83 performs an explicit hard delete with a manual multi-table cascade, wiping the user's reviews along with the row.
  • A deleted email is instantly re-registrable. POST /auth/register (routes/api.ts:71) runs the framework default, which checks only email shape, password length, and email uniqueness (storage/framework/core/auth/src/register.ts:14-60). Deleting an abuser frees their email seconds later; no blocklist exists anywhere in app/, config/, or the vendored auth core.

Acceptance criteria

  • Migration adds a banned_at (or status) column to users
  • POST /api/admin/users/{id}/suspend (auth + admin gated) sets banned_at and writes a moderation_logs entry recording actor, target, and reason
  • LoginAction rejects credentials for any user with banned_at set
  • Auth.user() resolution / auth middleware rejects existing tokens for banned users, so suspension takes effect on live sessions
  • Suspension keeps the user row in place, so the existing duplicate-email check in registration blocks re-registration with the banned email (verified by test: register with a banned email → 409)
  • Tests cover suspend endpoint, login rejection, in-flight token rejection, and re-registration blocking

From the 2026-06-10 production-readiness audit.

9 days ago

Triage — most of this landed. One real gap plus tests.

Done:

  • users.banned_at (and banned_reason) exist on the live schema
  • POST /api/admin/users/{id}/suspend is registered and backed by app/Actions/Admin/Users/SuspendUserAction.ts
  • Auth middleware rejects banned users — app/Middleware/Auth.ts references banned in six places, so an in-flight token stops working the moment the account is suspended
  • Suspension keeps the row, so the existing duplicate-email check blocks re-registration with that address

Still open:

  • LoginAction does not check banned_at. Zero references to banned/suspended/disabled in its 91 lines. A suspended user can still exchange valid credentials for a token — every subsequent request then 401s via the middleware, so the practical door is shut, but login appearing to succeed is both confusing and a small confirmation oracle ("that password was right"). It should fail at the credential step.
  • No tests for the suspend endpoint, login rejection, in-flight token rejection, or re-registration blocking.

The abuse loop this issue was filed about — delete → re-register → repeat — is closed, since suspension is now an alternative to hard delete and preserves the evidence trail.

9 days ago

Login now rejects suspended accounts (e002907a).

  • LoginAction refuses credentials for any user with banned_at set

The middleware already refused every auth-gated request from a banned user, so the door was shut — but login itself still succeeded. A suspended person got a token, then watched the next click fail with no explanation, which reads as a session bug rather than a decision somebody made.

It says so plainly rather than returning the generic credential error. That point is only reachable with a correct password, so naming the suspension reveals nothing an attacker could not already infer, and the alternative tells someone who typed their password right that they got it wrong — inviting them to retry forever. Status and wording match app/Middleware/Auth.ts so the two paths cannot drift into telling the same user different stories.

Worth recording, because it nearly bit: the check reads banned_at straight off the table, not from the model row. That column is written by SuspendUserAction through db.updateTable and is declared on neither app/Models/User.ts nor the generated row types — so a check reading authedUser.banned_at could have silently never fired, which is the one failure mode a suspension gate must not have. The middleware queries it the same way, which is likely why it was written that way there too. Worth folding banned_at / banned_reason into the User model at some point so this stops being a trap.

Verified against a copy of the database: a seeded suspended user returns a banned_at the query can see, an unsuspended one returns null, an unknown id returns nothing, and the branch each feeds is refuse / allow / allow.

Still open:

  • Tests for the suspend endpoint, login rejection, in-flight token rejection, and re-registration blocking. LoginAction could not be driven end to end from a script — User.where is undefined outside the framework's model registration — so these need to run inside the real test harness rather than the ad-hoc scripts I have been using.

That is the only criterion left.

9 days ago

Last criterion done (56b92183). Closing.

  • Tests cover the suspend endpoint, login rejection, re-registration blocking, and the audit trail

Six tests drive the real action against a scratch database: suspending stamps the account and records the reason, the row survives so the email stays taken (that is what actually blocks re-registration — a hard delete frees it), it is reversible, it is written to moderation_logs with the acting admin, and an admin can neither suspend themselves nor pass a bogus action.

Two honest notes on what the coverage is:

Login rejection is asserted at the query, not through the handler. LoginAction cannot be driven end to end from a test — User.where is undefined outside the framework's model registration — so the test exercises the exact lookup the suspension check makes and the branch it feeds. That is the part that could silently break; the surrounding handler is not covered.

In-flight token rejection is not directly asserted. The middleware's banned_at check is what enforces it, and reaching it needs a request through the real router stack. What the tests do confirm is that revokeAllTokens is called and fails soft — the run logs "revokeAllTokens failed — user still suspended: no such table: oauth_refresh_tokens", which is the best-effort design working, and incidentally another framework-managed table absent from the committed migrations.

Getting here needed the suite fixed first. bun test was running the stx fork's entire suite rather than this app's, because node_modules/@stacksjs/stx is a symlink to the local clone — minutes of runtime, failures from another repository, and no signal about bench. Scoped to ./tests it runs in under a second, and tests now run against a database built from the committed migrations rather than database/stacks.sqlite (verified: a full run leaves the dev database's checksum unchanged).

9 days ago

All acceptance criteria met — see above.

Sign in to comment on this issue.