Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions docker/nginx/nginx.conf
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ http {
proxy_pass http://betterbase_server;
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-For $remote_addr;
proxy_set_header X-Forwarded-Proto $scheme;
proxy_read_timeout 60s;
}
Expand All @@ -62,13 +62,13 @@ http {
proxy_pass http://betterbase_server;
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-For $remote_addr;
proxy_set_header X-Forwarded-Proto $scheme;
}

location /health {
proxy_pass http://betterbase_server;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-For $remote_addr;
proxy_set_header X-Forwarded-Proto $scheme;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

Expand All @@ -78,7 +78,7 @@ http {
proxy_pass http://minio;
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-For $remote_addr;
proxy_set_header X-Forwarded-Proto $scheme;
client_max_body_size 100m;
}
Expand All @@ -88,7 +88,7 @@ http {
proxy_pass http://betterbase_dashboard;
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-For $remote_addr;
proxy_set_header X-Forwarded-Proto $scheme;
# SPA fallback
proxy_intercept_errors on;
Expand All @@ -98,7 +98,7 @@ http {
location @dashboard_fallback {
proxy_pass http://betterbase_dashboard;
proxy_set_header Host $host;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-For $remote_addr;
proxy_set_header X-Forwarded-Proto $scheme;
}

Expand All @@ -110,7 +110,7 @@ http {
proxy_set_header Connection "upgrade";
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-For $remote_addr;
proxy_set_header X-Forwarded-Proto $scheme;
proxy_read_timeout 3600s;
}
Expand Down
9 changes: 9 additions & 0 deletions packages/server/migrations/017_revoked_admin_tokens.sql
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);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
2 changes: 1 addition & 1 deletion packages/server/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ app.use("*", async (c, next) => {

const projectId = c.req.header("X-Project-ID") ?? null;
const userAgent = c.req.header("User-Agent")?.slice(0, 255) ?? null;
const ip = c.req.header("X-Forwarded-For")?.split(",")[0] ?? null;
const ip = c.req.header("X-Real-IP") ?? null;

// Fire-and-forget log insert (don't await, don't fail requests on log error)
getPool()
Expand Down
4 changes: 1 addition & 3 deletions packages/server/src/lib/audit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,5 @@ export async function writeAuditLog(entry: AuditEntry): Promise<void> {

// Helper: extract IP from Hono context
export function getClientIp(headers: Headers): string {
return (
headers.get("x-forwarded-for")?.split(",")[0]?.trim() ?? headers.get("x-real-ip") ?? "unknown"
);
return headers.get("x-real-ip") ?? headers.get("x-forwarded-for")?.split(",").pop()?.trim() ?? "unknown";
}
29 changes: 25 additions & 4 deletions packages/server/src/lib/auth.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
import bcrypt from "bcryptjs";
import { randomUUID } from "crypto";
import { SignJWT, jwtVerify } from "jose";
import type { Pool } from "pg";
import { getPool } from "./db";
import { validateEnv } from "./env";

const getSecret = () => {
const env = validateEnv();
return new TextEncoder().encode(env.BETTERBASE_JWT_SECRET);
};

const TOKEN_EXPIRY = "30d";
const TOKEN_EXPIRY = "8h";
const BCRYPT_ROUNDS = 12;

// --- Password ---
Expand All @@ -24,18 +26,37 @@ export async function verifyPassword(password: string, hash: string): Promise<bo
// --- JWT for admin sessions ---

export async function signAdminToken(adminUserId: string): Promise<string> {
const env = validateEnv();
return new SignJWT({ sub: adminUserId, type: "admin" })
.setProtectedHeader({ alg: "HS256" })
.setIssuedAt()
.setExpirationTime(TOKEN_EXPIRY)
.setIssuer(env.BETTERBASE_JWT_ISSUER)
.setAudience(env.BETTERBASE_JWT_AUDIENCE)
.setJti(randomUUID())
.sign(getSecret());
}

export async function verifyAdminToken(token: string): Promise<{ sub: string } | null> {
export async function verifyAdminToken(
token: string,
): Promise<{ sub: string; jti: string } | null> {
try {
const { payload } = await jwtVerify(token, getSecret());
const env = validateEnv();
const { payload } = await jwtVerify(token, getSecret(), {
issuer: env.BETTERBASE_JWT_ISSUER,
audience: env.BETTERBASE_JWT_AUDIENCE,
});
if (payload.type !== "admin") return null;
return { sub: payload.sub as string };
if (!payload.sub || !payload.jti) return null;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

const pool = getPool();
const { rows } = await pool.query(
"SELECT 1 FROM betterbase_meta.revoked_admin_tokens WHERE jti = $1 LIMIT 1",
[payload.jti],
);
if (rows.length > 0) return null;

return { sub: payload.sub as string, jti: payload.jti as string };
} catch {
return null;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Expand Down
11 changes: 11 additions & 0 deletions packages/server/src/lib/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ const EnvSchema = z.object({
INNGEST_SIGNING_KEY: z.string().optional(),
INNGEST_EVENT_KEY: z.string().optional(),
PORT: z.string().default("3000"),
BETTERBASE_JWT_ISSUER: z.string().default("betterbase"),
BETTERBASE_JWT_AUDIENCE: z.string().default("betterbase-admin"),
});

export type Env = z.infer<typeof EnvSchema>;
Expand Down Expand Up @@ -53,6 +55,15 @@ export function validateEnv(): Env {
result.data.INNGEST_EVENT_KEY = "betterbase-dev-event-key";
}

if (result.data.STORAGE_ENDPOINT) {
if (!result.data.STORAGE_ACCESS_KEY || !result.data.STORAGE_SECRET_KEY) {
console.error(
"[env] STORAGE_ACCESS_KEY and STORAGE_SECRET_KEY are required when STORAGE_ENDPOINT is set",
);
process.exit(1);
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

validatedEnv = result.data;
return validatedEnv;
}
34 changes: 32 additions & 2 deletions packages/server/src/routes/admin/auth.ts
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,
Expand Down Expand Up @@ -62,8 +63,37 @@ 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();
await pool.query(
`INSERT INTO betterbase_meta.revoked_admin_tokens (jti, admin_user_id)
VALUES ($1, $2)
ON CONFLICT (jti) DO NOTHING`,
[payload.jti, payload.sub],
);

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Logout handler can throw on DB errors and awaits the audit log.

Two issues against the coding guidelines:

  1. Audit log is awaited (line 89). As per coding guidelines: "Audit log writes: fire-and-forget. Never await."

  2. Unhandled exceptions at lines 77 and 85: a Postgres failure on either the revocation INSERT or the admin SELECT propagates out of the handler, hits the global onError, and returns 500. The client perceives logout failure even though the token may already have been revoked (or worse — believes they're logged out when revocation actually failed). As per coding guidelines: "Route handlers must not throw — errors should be caught and return c.json({error}). The global onError handler catches the rest but shouldn't be the primary mechanism."

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
Verify each finding against the current code and only fix it if needed.

In `@packages/server/src/routes/admin/auth.ts` around lines 67 - 99, The logout
handler (authRoutes.post("/logout")) currently performs DB queries (revocation
INSERT and admin SELECT) without error handling and awaits writeAuditLog; wrap
the DB calls that use getPool().query (the INSERT into
betterbase_meta.revoked_admin_tokens and the SELECT from
betterbase_meta.admin_users) in a try/catch so the route does not throw: decide
fail-closed for revocation (if the INSERT fails return c.json({ error:
"revocation_failed" }) and do not proceed) or fail-open if you prefer (log the
error and continue to return success); ensure the SELECT is similarly guarded
and returns a handled error instead of throwing; change the writeAuditLog call
to fire-and-forget (call writeAuditLog(...) without await and attach a
.catch(...) to log failures) so audit writes are never awaited.


// GET /admin/auth/setup-status — check if admin exists (no body validation)
authRoutes.get("/setup-status", async (c) => {
Expand Down
12 changes: 10 additions & 2 deletions packages/server/src/routes/admin/project-scoped/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { getClientIp, writeAuditLog } from "../../../lib/audit";
import { getPool } from "../../../lib/db";

export const projectUserRoutes = new Hono();
const CSV_DANGEROUS_PREFIX = /^[=+\-@\t\r]/;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

function schemaName(project: { slug: string }) {
return `project_${project.slug}`;
Expand Down Expand Up @@ -229,18 +230,25 @@ projectUserRoutes.post("/export", async (c) => {
);

const header = "id,name,email,email_verified,created_at,banned\n";
const escapeCsv = (value: unknown) => {
const raw = String(value ?? "");
const prefixed = CSV_DANGEROUS_PREFIX.test(raw) ? `'${raw}` : raw;
return `"${prefixed.replace(/"/g, '""')}"`;
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Duplicate CSV-escape logic — consolidate with lib/inngest.ts.

escapeCSVValue in packages/server/src/lib/inngest.ts (lines 4–20) already implements this, with a more complete dangerous-char set and conditional quoting. Two divergent implementations of an injection-mitigation primitive is a foot-gun: a future fix to one will not propagate. Extract into a shared helper (e.g. lib/csv.ts) and import from both call sites.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/server/src/routes/admin/project-scoped/users.ts` around lines 233 -
237, There are two implementations of CSV-escaping (the local escapeCsv in the
users route and escapeCSVValue in lib/inngest.ts); extract a single shared
helper (e.g., export function escapeCSVValue in a new lib/csv.ts) that uses the
more complete dangerous-char regex and conditional quoting logic from
lib/inngest.ts, update the users route to import and use that exported
escapeCSVValue (remove the local escapeCsv), and update lib/inngest.ts to import
the same helper so both call sites use the identical implementation.

const csv =
header +
rows
.map(
(r) => `${r.id},"${r.name}","${r.email}",${r.email_verified},${r.created_at},${r.banned}`,
(r) =>
`${escapeCsv(r.id)},${escapeCsv(r.name)},${escapeCsv(r.email)},${r.email_verified},${escapeCsv(r.created_at)},${r.banned}`,
)
.join("\n");

return new Response(csv, {
headers: {
"Content-Type": "text/csv",
"Content-Type": "text/csv; charset=utf-8",
"Content-Disposition": `attachment; filename="users-${project.slug}-${Date.now()}.csv"`,
"Content-Security-Policy": "default-src 'none'",
},
});
});
38 changes: 32 additions & 6 deletions packages/server/src/routes/betterbase/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@ import { PutObjectCommand, S3Client } from "@aws-sdk/client-s3";
import { getSignedUrl } from "@aws-sdk/s3-request-presigner";

export const betterbaseRouter = new Hono();
const SAFE_PROJECT_SLUG = /^[a-z][a-z0-9_]{0,62}$/;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
const ALLOWED_UPLOAD_CONTENT_TYPES = new Set([
"image/jpeg",
"image/png",
"image/webp",
"image/gif",
"application/pdf",
"text/plain",
]);

// All function calls: POST /betterbase/:kind/*
betterbaseRouter.post("/:kind/*", async (c) => {
Expand Down Expand Up @@ -48,11 +57,15 @@ betterbaseRouter.post("/:kind/*", async (c) => {
// Auth context
const token = extractBearerToken(c.req.header("Authorization"));
const adminPayload = token ? await verifyAdminToken(token) : null;
if (!adminPayload) return c.json({ error: "Unauthorized" }, 401);
const authCtx = { userId: adminPayload?.sub ?? null, token };
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

// Build DB context
const pool = getPool();
const projectSlug = c.req.header("X-Project-Slug") ?? "default";
if (!SAFE_PROJECT_SLUG.test(projectSlug)) {
return c.json({ error: "Invalid project slug" }, 400);
}
const dbSchema = `project_${projectSlug}`;

try {
Expand Down Expand Up @@ -97,8 +110,8 @@ function buildStorageCtx(pool: any, projectSlug: string): StorageCtx {
pool,
projectSlug,
endpoint: env.STORAGE_ENDPOINT ?? "http://minio:9000",
accessKey: env.STORAGE_ACCESS_KEY ?? "minioadmin",
secretKey: env.STORAGE_SECRET_KEY ?? "minioadmin",
accessKey: env.STORAGE_ACCESS_KEY ?? "",
secretKey: env.STORAGE_SECRET_KEY ?? "",
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
bucket: env.STORAGE_BUCKET ?? "betterbase",
publicBase: env.STORAGE_PUBLIC_BASE,
});
Expand Down Expand Up @@ -177,8 +190,21 @@ function buildActionCtx(pool: any, dbSchema: string, auth: any, projectSlug: str

// Direct browser upload endpoint: POST /betterbase/storage/generate-upload-url
betterbaseRouter.post("/storage/generate-upload-url", async (c) => {
const token = extractBearerToken(c.req.header("Authorization"));
const adminPayload = token ? await verifyAdminToken(token) : null;
if (!adminPayload) return c.json({ error: "Unauthorized" }, 401);

const { contentType, filename } = await c.req.json();
const safeContentType =
typeof contentType === "string" && ALLOWED_UPLOAD_CONTENT_TYPES.has(contentType)
? contentType
: null;
if (!safeContentType) return c.json({ error: "Unsupported content type" }, 400);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

const projectSlug = c.req.header("X-Project-Slug") ?? "default";
if (!SAFE_PROJECT_SLUG.test(projectSlug)) {
return c.json({ error: "Invalid project slug" }, 400);
}
const storageId = `st_${nanoid(20)}`;
const ext = filename?.split(".").pop() ?? "";
const s3Key = `project_${projectSlug}/${storageId}${ext ? "." + ext : ""}`;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Expand All @@ -188,8 +214,8 @@ betterbaseRouter.post("/storage/generate-upload-url", async (c) => {
endpoint: env.STORAGE_ENDPOINT ?? "http://minio:9000",
region: "us-east-1",
credentials: {
accessKeyId: env.STORAGE_ACCESS_KEY ?? "minioadmin",
secretAccessKey: env.STORAGE_SECRET_KEY ?? "minioadmin",
accessKeyId: env.STORAGE_ACCESS_KEY ?? "",
secretAccessKey: env.STORAGE_SECRET_KEY ?? "",
},
forcePathStyle: true,
});
Expand All @@ -199,7 +225,7 @@ betterbaseRouter.post("/storage/generate-upload-url", async (c) => {
new PutObjectCommand({
Bucket: env.STORAGE_BUCKET ?? "betterbase",
Key: s3Key,
ContentType: contentType ?? "application/octet-stream",
ContentType: safeContentType,
}),
{ expiresIn: 300 },
);
Expand All @@ -214,7 +240,7 @@ betterbaseRouter.post("/storage/generate-upload-url", async (c) => {
storageId,
s3Key,
env.STORAGE_BUCKET ?? "betterbase",
contentType ?? "application/octet-stream",
safeContentType,
],
);

Expand Down
7 changes: 6 additions & 1 deletion packages/server/src/routes/betterbase/ws.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
subscriptionTracker,
} from "@betterbase/core";
import { nanoid } from "nanoid";
import { verifyAdminToken } from "../../lib/auth";

const HEARTBEAT_INTERVAL_MS = 15_000; // ping every 15s
const HEARTBEAT_TIMEOUT_MS = 30_000; // disconnect after 30s without pong
Expand Down Expand Up @@ -140,8 +141,12 @@ export function getBunServeConfig() {
fetch(req: Request, server: any) {
const url = new URL(req.url);
if (url.pathname === "/betterbase/ws") {
const token = url.searchParams.get("token");
const payload = token ? await verifyAdminToken(token) : null;
if (!payload) return new Response("Unauthorized", { status: 401 });

const projectSlug = url.searchParams.get("project") ?? "default";
const upgraded = server.upgrade(req, { data: { projectSlug } });
const upgraded = server.upgrade(req, { data: { projectSlug, userId: payload.sub } });
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
if (upgraded) return undefined;
return new Response("WebSocket upgrade failed", { status: 400 });
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Expand Down
14 changes: 14 additions & 0 deletions packages/server/src/routes/device/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,23 @@ import { validateEnv } from "../../lib/env";
export const deviceRouter = new Hono();

const CODE_EXPIRY_MINUTES = 10;
const DEVICE_CODE_RATE_LIMIT_WINDOW_MS = 60_000;
const DEVICE_CODE_RATE_LIMIT_MAX = 5;
const deviceCodeRateLimits = new Map<string, number[]>();

// POST /device/code — CLI calls this to initiate login
deviceRouter.post("/code", async (c) => {
const ip = c.req.header("X-Real-IP") ?? "unknown";
const now = Date.now();
const recent = (deviceCodeRateLimits.get(ip) ?? []).filter(
(ts) => now - ts < DEVICE_CODE_RATE_LIMIT_WINDOW_MS,
);
if (recent.length >= DEVICE_CODE_RATE_LIMIT_MAX) {
return c.json({ error: "Rate limit exceeded. Try again in a minute." }, 429);
}
recent.push(now);
deviceCodeRateLimits.set(ip, recent);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

const pool = getPool();

const deviceCode = nanoid(32);
Expand Down
Loading