-
Notifications
You must be signed in to change notification settings - Fork 2
Harden backend auth paths, token lifecycle, and spoofable request metadata #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| CREATE TABLE IF NOT EXISTS betterbase_meta.revoked_admin_tokens ( | ||
| jti TEXT PRIMARY KEY, | ||
| admin_user_id TEXT REFERENCES betterbase_meta.admin_users(id) ON DELETE SET NULL, | ||
| revoked_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| expires_at TIMESTAMPTZ | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_revoked_admin_tokens_expires_at | ||
| ON betterbase_meta.revoked_admin_tokens (expires_at); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import { zValidator } from "@hono/zod-validator"; | ||
| import { Hono } from "hono"; | ||
| import { z } from "zod"; | ||
| import { getClientIp, writeAuditLog } from "../../lib/audit"; | ||
| import { | ||
| extractBearerToken, | ||
| signAdminToken, | ||
|
|
@@ -62,8 +63,40 @@ authRoutes.get("/me", async (c) => { | |
| return c.json({ admin: rows[0] }); | ||
| }); | ||
|
|
||
| // POST /admin/auth/logout (client-side token discard — stateless) | ||
| authRoutes.post("/logout", (c) => c.json({ success: true })); | ||
| // POST /admin/auth/logout | ||
| authRoutes.post("/logout", async (c) => { | ||
| const token = extractBearerToken(c.req.header("Authorization")); | ||
| if (!token) return c.json({ success: true }); | ||
|
|
||
| const payload = await verifyAdminToken(token); | ||
| if (!payload) return c.json({ success: true }); | ||
|
|
||
| const pool = getPool(); | ||
| // Only revoke if jti is present | ||
| if (payload.jti && payload.exp) { | ||
| await pool.query( | ||
| `INSERT INTO betterbase_meta.revoked_admin_tokens (jti, admin_user_id, expires_at) | ||
| VALUES ($1, $2, to_timestamp($3)) | ||
| ON CONFLICT (jti) DO NOTHING`, | ||
| [payload.jti, payload.sub, payload.exp], | ||
| ); | ||
| } | ||
|
|
||
| const { rows } = await pool.query("SELECT id, email FROM betterbase_meta.admin_users WHERE id = $1", [ | ||
| payload.sub, | ||
| ]); | ||
| if (rows.length > 0) { | ||
| await writeAuditLog({ | ||
| actorId: rows[0].id, | ||
| actorEmail: rows[0].email, | ||
| action: "admin.logout", | ||
| ipAddress: getClientIp(c.req.raw.headers), | ||
| userAgent: c.req.header("User-Agent") ?? undefined, | ||
| }); | ||
| } | ||
|
|
||
| return c.json({ success: true }); | ||
| }); | ||
|
Comment on lines
+67
to
+99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logout handler can throw on DB errors and awaits the audit log. Two issues against the coding guidelines:
Wrap the DB calls in try/catch and decide deliberately: fail-closed (return 500 with explicit error if revocation fails — admin must know their token is still valid) or fail-open (return success but log the revocation failure for ops). Proposed shape- const pool = getPool();
- // Only revoke if jti is present
- if (payload.jti && payload.exp) {
- await pool.query(
- `INSERT INTO betterbase_meta.revoked_admin_tokens (jti, admin_user_id, expires_at)
- VALUES ($1, $2, to_timestamp($3))
- ON CONFLICT (jti) DO NOTHING`,
- [payload.jti, payload.sub, payload.exp],
- );
- }
-
- const { rows } = await pool.query("SELECT id, email FROM betterbase_meta.admin_users WHERE id = $1", [
- payload.sub,
- ]);
- if (rows.length > 0) {
- await writeAuditLog({
+ const pool = getPool();
+ if (payload.jti && payload.exp) {
+ try {
+ await pool.query(
+ `INSERT INTO betterbase_meta.revoked_admin_tokens (jti, admin_user_id, expires_at)
+ VALUES ($1, $2, to_timestamp($3))
+ ON CONFLICT (jti) DO NOTHING`,
+ [payload.jti, payload.sub, payload.exp],
+ );
+ } catch (err) {
+ console.error("[auth] failed to revoke token", err);
+ return c.json({ error: "Logout failed; please retry." }, 500);
+ }
+ }
+
+ let rows: any[] = [];
+ try {
+ ({ rows } = await pool.query("SELECT id, email FROM betterbase_meta.admin_users WHERE id = $1", [payload.sub]));
+ } catch (err) {
+ console.error("[auth] failed to load admin for audit", err);
+ }
+ if (rows.length > 0) {
+ writeAuditLog({
actorId: rows[0].id,
actorEmail: rows[0].email,
action: "admin.logout",
ipAddress: getClientIp(c.req.raw.headers),
userAgent: c.req.header("User-Agent") ?? undefined,
- });
+ }).catch(() => {});
}As per coding guidelines: "Audit log writes: fire-and-forget. Never await." and "Route handlers must not throw — errors should be caught and return c.json({error})." 🤖 Prompt for AI Agents |
||
|
|
||
| // GET /admin/auth/setup-status — check if admin exists (no body validation) | ||
| authRoutes.get("/setup-status", async (c) => { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.