Skip to content

docs: polish README wording and consistency#63

Closed
weroperking wants to merge 1 commit intomainfrom
codex/update-readme.md-for-clarity
Closed

docs: polish README wording and consistency#63
weroperking wants to merge 1 commit intomainfrom
codex/update-readme.md-for-clarity

Conversation

@weroperking
Copy link
Copy Markdown
Owner

@weroperking weroperking commented Apr 26, 2026

Motivation

  • Improve grammar, punctuation, and phrasing across the project README to make the documentation more professional and readable.
  • Normalize capitalization and naming (e.g., AI-native, BaaS, DeepWiki) for consistent brand and technical wording.
  • Preserve all technical examples and instructions while improving clarity and developer-facing copy.

Description

  • Updated top-level product description and intro phrasing in README.md to be clearer and more consistent.
  • Standardized comparison table and feature descriptions (e.g., Traditional BaaS, Self-hosted anywhere, migrations and type-safety wording).
  • Clarified CLI and deployment descriptions and adjusted RLS and Serverless/endpoint wording for precision.
  • Normalized Ask DeepWiki naming/capitalization and tightened several section sentences including changelog phrasing.

Testing

  • Ran git diff -- README.md | sed -n '1,220p' to review the final diff and confirm intended changes succeeded. (succeeded)
  • Verified workspace status with git status --short to confirm only documentation was modified. (succeeded)
  • Applied and committed the patch via git add README.md && git commit -m "docs: polish README wording and consistency", and the commit completed successfully. (succeeded)

Codex Task

Summary by CodeRabbit

  • Documentation
    • Updated README for improved clarity and consistency, including refinements to marketing copy, feature descriptions, and command instructions.
    • Normalized branding conventions and comparison table labels throughout the guide.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 26, 2026

Walkthrough

README.md underwent cosmetic updates across multiple sections: capitalization and phrasing adjustments in marketing copy, comparison table labels ("Traditional BaaS", "Self-hosted anywhere"), Key Features entries, CLI command descriptions, and branding/casing normalization (Betterbase, DeepWiki).

Changes

Cohort / File(s) Summary
Documentation
README.md
Wording and naming convention updates across marketing copy, comparison tables, Key Features, CLI descriptions, and branding/casing normalization. No functional changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title accurately describes the main change: polishing README wording and consistency across the documentation file.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/update-readme.md-for-clarity

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@README.md`:
- Line 438: The section heading "Using Ask Deepwiki" uses incorrect
capitalization; update the heading text to "Using Ask DeepWiki" to match the
normalized product name used throughout the repository (search for the heading
string "Using Ask Deepwiki" and replace it with "Using Ask DeepWiki" in
README.md).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ef47a544-ebbd-48b7-8c4b-8818764f69f4

📥 Commits

Reviewing files that changed from the base of the PR and between 7b3bae5 and aa4e637.

📒 Files selected for processing (1)
  • README.md

Comment thread README.md
**Deepwiki Badge**: The badge at the top of this README links to [Ask Deepwiki](https://deepwiki.com/weroperking/Betterbase), where you can chat with an AI that understands your entire Betterbase project.
**DeepWiki Badge**: The badge at the top of this README links to [Ask DeepWiki](https://deepwiki.com/weroperking/Betterbase), where you can chat with an AI that understands your entire Betterbase project.

### Using Ask Deepwiki
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 | 🟡 Minor

Fix branding capitalization drift in section heading.

Line 438 uses Deepwiki, which breaks the normalized product naming used elsewhere (DeepWiki). Use consistent casing to avoid documentation drift.

Suggested patch
-### Using Ask Deepwiki
+### Using Ask DeepWiki

As per coding guidelines, "Treat this file as the source of truth for monorepo overview phrasing and naming conventions."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
### Using Ask Deepwiki
### Using Ask DeepWiki
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` at line 438, The section heading "Using Ask Deepwiki" uses
incorrect capitalization; update the heading text to "Using Ask DeepWiki" to
match the normalized product name used throughout the repository (search for the
heading string "Using Ask Deepwiki" and replace it with "Using Ask DeepWiki" in
README.md).

@weroperking
Copy link
Copy Markdown
Owner Author

@coderabbitai Full review , try to find the bugs that are vurnabilty in them , these examples of what we are looking at , but you must extend it way further and catch all bugs in production self-hosted , all possibilities : # Betterbase Backend/Core Hardening Review (v3)

1) Objective (final outcome)

Deliver a backend-first security and reliability hardening baseline for Betterbase, focused on:

  • packages/server runtime and auth boundaries,
  • database/migration safety,
  • Docker/container defaults and edge proxying,
  • operational protections for production readiness.

Out of scope by request: dashboard/frontend quality review.


2) Scope map (what is reviewed)

Included

  • API/auth/device/admin flows (packages/server/src/**)
  • Migration and schema lifecycle (packages/server/src/lib/migrate.ts, packages/server/migrations/**)
  • Container/runtime and local infra defaults (Dockerfile, docker-compose.yml, docker/nginx/nginx.conf)

Excluded

  • Dashboard UX, styling, and frontend correctness (apps/dashboard/**)

3) Plan (how this review is executed)

  1. Synchronize branch state before any edits (fetch/pull when tracking exists).
  2. Static backend attack-surface review:
    • auth/session/device/token issuance,
    • admin routing and privilege checks,
    • secret handling and env defaults,
    • SQL use patterns and dynamic identifier safety.
  3. Docker and reverse-proxy hardening pass:
    • insecure defaults,
    • exposed ports/services,
    • transport/auth boundaries.
  4. Reliability pass:
    • migration atomicity,
    • logging correctness,
    • operational behavior under failure.
  5. Produce v3 documentation artifact with severity, impact, evidence, and fixes.

4) Acceptance criteria vs non-acceptance criteria

Accepted criteria

A review is accepted only if it:

  • Prioritizes critical/high backend risks first,
  • Lists concrete findings with file-level evidence,
  • Proposes remediation that is actionable and ordered,
  • Separates scope (core/backend) from excluded dashboard concerns,
  • Defines objective pass/fail criteria for closure.

Not accepted

A review is not accepted if it:

  • Focuses mostly on frontend/dashboard problems,
  • Gives generic best-practice lists without code evidence,
  • Omits severity and business impact,
  • Doesn’t define what “done” looks like.

5) Problem classes targeted in this review

  • Authentication hardening (brute-force, token abuse, device flow safety)
  • Authorization hardening (scope enforcement, privilege boundaries)
  • Input/output security (HTML injection, unsafe interpolation)
  • Secrets/config hardening (weak defaults, exposure risks)
  • Operational resilience (migration safety, observability correctness)
  • Container/proxy hardening (network exposure, transport/auth controls)

6) Findings (backend/core only)

F-01 (HIGH): No brute-force/rate limiting on admin login and device verification

Evidence

  • /admin/auth/login verifies credentials directly with no throttling/lockout/captcha/backoff. (packages/server/src/routes/admin/auth.ts)
  • /device/verify form POST verifies admin credentials with no anti-automation controls. (packages/server/src/routes/device/index.ts)

Impact

  • Enables online password guessing and credential stuffing against administrative entry points.

Recommended fix

  1. Add IP + account-based rate limits (e.g., sliding window).
  2. Add progressive delays/temporary lockouts after repeated failures.
  3. Emit structured security events for failed auth attempts.

F-02 (HIGH): API key scopes are stored but not enforced

Evidence

  • API keys include scopes at creation and persistence. (packages/server/src/routes/admin/api-keys.ts, packages/server/migrations/008_api_keys.sql)
  • Auth middleware validates key hash and expiration only; it does not evaluate scopes for route/action authorization. (packages/server/src/lib/admin-middleware.ts)

Impact

  • A supposedly limited key can act as full-admin bearer credentials, breaking least-privilege expectations.

Recommended fix

  1. Introduce route-level scope requirements (e.g., requireScopes(["projects:read"])).
  2. Validate scopes in middleware before allowing handler execution.
  3. Add tests proving denied access when scope is insufficient.

F-03 (MEDIUM-HIGH): Reflected HTML injection risk in device verification page

Evidence

  • userCode query param is interpolated directly into HTML input value without escaping. (packages/server/src/routes/device/index.ts)

Impact

  • Crafted links can inject markup/script contexts depending on browser parsing behavior, risking phishing/session abuse in admin verification flow.

Recommended fix

  1. Escape HTML entities for all interpolated values.
  2. Prefer server-side templates with automatic escaping.
  3. Add CSP headers and disable inline script where possible.

F-04 (MEDIUM): Migration execution is not wrapped in per-file transactions

Evidence

  • Migration runner executes raw SQL then records applied filename, without explicit transaction guards around each migration file. (packages/server/src/lib/migrate.ts)

Impact

  • Partial migration failures can leave inconsistent schema states that are hard to recover in production rollouts.

Recommended fix

  1. Wrap each migration in BEGIN/COMMIT with rollback on failure.
  2. Abort startup if any migration fails.
  3. Add idempotency checks for unsafe DDL operations.

F-05 (MEDIUM): Insecure local Docker defaults can leak into non-local usage

Evidence

  • Static/dev credentials and wide host port exposure in compose defaults (5432, 9000, 9001, 8288) plus default passwords. (docker-compose.yml)

Impact

  • If reused outside isolated local environments, these defaults materially increase compromise likelihood.

Recommended fix

  1. Gate local-only config via explicit profile names and warnings.
  2. Require strong secrets through .env for non-dev profiles.
  3. Limit host port publishing to required services only.

7) Closure criteria for this hardening cycle

To mark this cycle complete, all must be true:

  1. Login/device endpoints enforce rate limits and lockout policy.
  2. API key scopes are enforced in middleware/route guards.
  3. Device verification HTML interpolation is safely escaped.
  4. Migration runner supports transactional per-file execution.
  5. Container docs/config clearly separate local unsafe defaults from production-secure defaults.

8) Implementation priority order

  1. P0: F-01, F-02
  2. P1: F-03
  3. P2: F-04, F-05

9) Summary

This v3 review identifies backend/core hardening gaps primarily in auth abuse resistance, authorization scope enforcement, HTML output safety, migration resilience, and Docker default safety posture. Addressing P0/P1 items first materially reduces compromise risk while improving operational confidence.


10) AI reviewer operating manual (for the next reviewing agent)

This section is intentionally prescriptive so another AI/code reviewer can continue this work with high consistency and low ambiguity.

10.1 Review intent

You are not only finding bugs. You are validating whether Betterbase can safely operate as:

  • an internet-facing backend API,
  • a multi-project control plane,
  • a migration-driven data platform,
  • and a containerized service.

10.2 Required review mindset

For every endpoint or subsystem, ask:

  1. Can an attacker reach this?
  2. What identity context is assumed?
  3. What authorization boundary is enforced?
  4. What happens on repeated abuse/failure?
  5. How does this fail in production under partial outage?

If any answer is unclear, treat it as a review finding candidate.

10.3 Evidence standard (non-optional)

Every finding should include all of:

  • exact file path(s),
  • function/route names,
  • exploit or failure path in 2–5 steps,
  • business impact,
  • concrete mitigation order (quick patch + durable fix).

Avoid generic “best-practice only” findings with no file-level proof.


11) Backend review blueprint by domain

Use this sequence to avoid missing critical areas.

11.1 Identity and authentication

  • Admin login (/admin/auth/login)
  • Setup flow (/admin/auth/setup)
  • Device flow (/device/code, /device/verify, /device/token)
  • Token issuance/validation (signAdminToken, verifyAdminToken)
  • API key lifecycle and bearer handling

What to catch

  • brute force/stuffing paths,
  • token replay windows,
  • missing audience/issuer constraints,
  • no rotation/revocation strategy,
  • secret leakage in logs/errors.

11.2 Authorization and tenancy boundaries

  • requireAdmin middleware behavior for JWT vs API keys
  • project-scoped routes and project attachment middleware
  • actions that should require scoped permissions but do not

What to catch

  • privilege escalation across routes,
  • “scope exists in DB but not enforced” patterns,
  • project hopping / IDOR possibilities,
  • routes inheriting global admin access unintentionally.

11.3 Data and SQL safety

  • dynamic schema/table identifier usage
  • raw SQL interpolation (especially schema-qualified queries)
  • migration sequencing and rollback resilience

What to catch

  • unsafe identifier injection opportunities,
  • inconsistent transaction handling,
  • ordering collisions in migration naming,
  • partial-apply states causing startup drift.

11.4 Output and interface safety

  • HTML-rendering endpoints (device verification page)
  • plain-text/HTML response interpolation
  • error message leakage

What to catch

  • reflected/stored injection risks,
  • security header gaps (CSP/XFO/etc.),
  • differential error behavior that aids attackers.

11.5 Container, proxy, and runtime posture

  • docker-compose.yml defaults
  • nginx routing and trust headers
  • image pinning and exposed ports

What to catch

  • weak default credentials reused outside dev,
  • unintended public service exposure,
  • over-trusting forwarded headers,
  • missing TLS/security hardening assumptions in docs.

12) Severity rubric for this project

Use this rubric consistently for triage:

  • Critical: direct compromise of admin control plane, tenant data exposure, or remote code/data integrity impact with low attacker cost.
  • High: realistic unauthorized access path, major authz bypass, or high-probability account compromise vector.
  • Medium: meaningful security/reliability weakness requiring additional conditions.
  • Low: hygiene/documentation/observability issues with limited direct exploitability.

When uncertain between two severities, choose the higher one and justify assumptions.


13) Acceptance gates for a “secure-enough” next milestone

The next implementation PR(s) should not be accepted unless all gates pass:

  1. Auth abuse gate
    • login + device verification endpoints have measurable rate limiting and lockout/backoff.
  2. Authorization gate
    • API key scopes are enforceable and tested on representative privileged routes.
  3. Injection gate
    • user-controlled values rendered in HTML are escaped or template-engine protected.
  4. Migration safety gate
    • each migration file executes atomically with rollback on failure.
  5. Runtime hygiene gate
    • local-only insecure defaults are explicitly isolated from production paths.

14) Reviewer handoff format (for future AI agents)

When handing off to another reviewer/engineer, provide:

  1. Top-5 unresolved risks (ordered by exploitability × impact),
  2. Proof snippets (route/file/line references),
  3. Proposed patch plan split by:
    • immediate hotfixes (hours),
    • short-term hardening (days),
    • structural improvements (weeks),
  4. Regression checks that must run after each fix,
  5. Known blind spots (areas not fully reviewed).

This prevents review drift and keeps findings actionable.


15) Suggested follow-up implementation roadmap (non-doc work)

  1. Implement centralized rate-limit middleware for auth-sensitive endpoints.
  2. Introduce explicit scope policy map and enforce in requireAdmin/route guards.
  3. Add safe HTML escaping helper for any server-rendered templates.
  4. Refactor migration runner to transactional execution with deterministic failure logging.
  5. Add a production-hardening compose profile and deployment checklist.

These implementation tracks should be handled in dedicated code PRs with tests.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 26, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Reading the backend source files now to produce a full evidence-based review.


🏁 Script executed:

#!/bin/bash
echo "=== TREE: packages/server/src ==="
find packages/server/src -type f | sort

echo ""
echo "=== TREE: packages/server/migrations ==="
find packages/server/migrations -type f | sort 2>/dev/null || echo "NOT FOUND"

echo ""
echo "=== admin/auth.ts ==="
cat packages/server/src/routes/admin/auth.ts 2>/dev/null || find . -path "*/routes/admin/auth*" -exec cat {} \;

echo ""
echo "=== device/index.ts ==="
cat packages/server/src/routes/device/index.ts 2>/dev/null || find . -path "*/routes/device*" -exec cat {} \;

echo ""
echo "=== admin/api-keys.ts ==="
cat packages/server/src/routes/admin/api-keys.ts 2>/dev/null || find . -path "*/routes/admin/api-keys*" -exec cat {} \;

echo ""
echo "=== admin-middleware.ts ==="
cat packages/server/src/lib/admin-middleware.ts 2>/dev/null || find . -path "*/lib/admin-middleware*" -exec cat {} \;

echo ""
echo "=== migrate.ts ==="
cat packages/server/src/lib/migrate.ts 2>/dev/null || find . -path "*/lib/migrate*" -exec cat {} \;

echo ""
echo "=== docker-compose.yml ==="
cat docker-compose.yml 2>/dev/null || find . -name "docker-compose.yml" -exec cat {} \;

echo ""
echo "=== Dockerfile ==="
cat Dockerfile 2>/dev/null || find . -name "Dockerfile" ! -path "*/node_modules/*" -exec echo "--- {} ---" \; -exec cat {} \;

echo ""
echo "=== nginx.conf ==="
cat docker/nginx/nginx.conf 2>/dev/null || find . -name "nginx.conf" -exec cat {} \;

Length of output: 26591


Got the key files. Reading the remaining backend files now.


🏁 Script executed:

#!/bin/bash
echo "=== lib/auth.ts ==="
cat packages/server/src/lib/auth.ts

echo ""
echo "=== lib/env.ts ==="
cat packages/server/src/lib/env.ts

echo ""
echo "=== lib/db.ts ==="
cat packages/server/src/lib/db.ts

echo ""
echo "=== src/index.ts (main entrypoint) ==="
cat packages/server/src/index.ts

echo ""
echo "=== routes/admin/index.ts ==="
cat packages/server/src/routes/admin/index.ts

echo ""
echo "=== routes/admin/project-scoped/index.ts ==="
cat packages/server/src/routes/admin/project-scoped/index.ts

echo ""
echo "=== routes/admin/project-scoped/database.ts ==="
cat packages/server/src/routes/admin/project-scoped/database.ts

echo ""
echo "=== routes/admin/project-scoped/env.ts ==="
cat packages/server/src/routes/admin/project-scoped/env.ts

echo ""
echo "=== routes/admin/storage.ts ==="
cat packages/server/src/routes/admin/storage.ts

echo ""
echo "=== routes/admin/cli-sessions.ts ==="
cat packages/server/src/routes/admin/cli-sessions.ts

echo ""
echo "=== routes/admin/webhooks.ts ==="
cat packages/server/src/routes/admin/webhooks.ts

echo ""
echo "=== routes/admin/project-scoped/webhooks.ts ==="
cat packages/server/src/routes/admin/project-scoped/webhooks.ts

echo ""
echo "=== routes/betterbase/index.ts ==="
cat packages/server/src/routes/betterbase/index.ts

echo ""
echo "=== routes/betterbase/ws.ts ==="
cat packages/server/src/routes/betterbase/ws.ts

echo ""
echo "=== lib/webhook-dispatcher.ts ==="
cat packages/server/src/lib/webhook-dispatcher.ts

echo ""
echo "=== lib/audit.ts ==="
cat packages/server/src/lib/audit.ts

echo ""
echo "=== routes/admin/roles.ts ==="
cat packages/server/src/routes/admin/roles.ts

echo ""
echo "=== routes/admin/project-scoped/users.ts ==="
cat packages/server/src/routes/admin/project-scoped/users.ts

echo ""
echo "=== migrations/001_initial_schema.sql (first 100 lines) ==="
head -100 packages/server/migrations/001_initial_schema.sql

echo ""
echo "=== migrations/008_api_keys.sql ==="
cat packages/server/migrations/008_api_keys.sql

echo ""
echo "=== migrations/006_rbac.sql ==="
cat packages/server/migrations/006_rbac.sql

echo ""
echo "=== Duplicate migration filenames check ==="
ls packages/server/migrations/ | sed 's/_.*//' | sort | uniq -d

echo ""
echo "=== routes/admin/instance.ts ==="
cat packages/server/src/routes/admin/instance.ts

echo ""
echo "=== routes/admin/smtp.ts ==="
cat packages/server/src/routes/admin/smtp.ts

echo ""
echo "=== routes/admin/project-scoped/functions.ts ==="
cat packages/server/src/routes/admin/project-scoped/functions.ts

Length of output: 68249


@weroperking Here is the complete backend/core hardening review. The five findings in your template are confirmed. Below are twelve additional findings (F-06–F-17) discovered from reading the actual source, ordered by severity.


Betterbase Backend/Core Hardening Review — Extended

Scope: packages/server/src/**, migrations, Dockerfile, docker-compose.yml, nginx.conf
Excluded: apps/dashboard/**


Confirmed from template (F-01 – F-05)

All five original findings are verified against the source code. Evidence details are expanded below where the code adds context.


New Findings


F-06 (CRITICAL): /betterbase/* function router executes without authentication

File: packages/server/src/routes/betterbase/index.ts

Evidence:

// Auth context
const token = extractBearerToken(c.req.header("Authorization"));
const adminPayload = token ? await verifyAdminToken(token) : null;
const authCtx = { userId: adminPayload?.sub ?? null, token };
// ← execution always continues; no 401 gate

There is no if (!adminPayload) return c.json({ error: "Unauthorized" }, 401). Any unauthenticated caller can POST to /betterbase/queries/*, /betterbase/mutations/*, or /betterbase/actions/* and execute any registered function. The authCtx.userId will be null but the handler runs regardless.

Impact: Full unauthenticated access to all user-defined backend functions (queries, mutations, actions) — data reads, writes, and side effects — with no identity or authorization enforcement at the platform layer.

Exploit path:

  1. Attacker discovers the /betterbase/ prefix (public endpoint, no auth).
  2. POSTs { "args": {} } to /betterbase/queries/users/list.
  3. Handler executes as userId: null; unless the handler itself rejects nulls, data is returned.
  4. Same for mutations — any write function executes without a session.

Fix:

const adminPayload = token ? await verifyAdminToken(token) : null;
if (!adminPayload) return c.json({ error: "Unauthorized" }, 401);

Or, if anonymous access is intentional for some functions, introduce a @public decorator flag on the handler registration and explicitly allow-list those paths.


F-07 (CRITICAL): /betterbase/storage/generate-upload-url is unauthenticated

File: packages/server/src/routes/betterbase/index.ts

Evidence:

betterbaseRouter.post("/storage/generate-upload-url", async (c) => {
    const { contentType, filename } = await c.req.json();
    // ← no auth check
    const storageId = `st_${nanoid(20)}`;
    // ... generates signed S3 PUT URL
    const uploadUrl = await getSignedUrl(s3, new PutObjectCommand({
        Bucket: env.STORAGE_BUCKET ?? "betterbase",
        Key: s3Key,
        ContentType: contentType ?? "application/octet-stream",  // ← attacker-controlled
    }), { expiresIn: 300 });

No authentication. contentType is caller-controlled and passed directly to the S3 presigned URL, meaning an attacker can request a URL for text/html or application/javascript and upload arbitrary web-exploitable content.

Impact: Unauthenticated arbitrary file upload to production storage. If the storage bucket or MinIO is publicly accessible, this leads to stored XSS or malware hosting. The DB row is inserted unconditionally, polluting the storage tracking table.

Fix:

  1. Add auth guard (same pattern as F-06).
  2. Allowlist contentType to safe media types: ["image/jpeg","image/png","image/webp","application/pdf",...].
  3. Never trust client-supplied content types for security decisions.

F-08 (CRITICAL): WebSocket /betterbase/ws has zero authentication

File: packages/server/src/routes/betterbase/ws.ts

Evidence:

fetch(req: Request, server: any) {
    const url = new URL(req.url);
    if (url.pathname === "/betterbase/ws") {
        const projectSlug = url.searchParams.get("project") ?? "default";
        const upgraded = server.upgrade(req, { data: { projectSlug } });
        // ← no token check before upgrade
        if (upgraded) return undefined;

The WebSocket upgrade accepts any connection without verifying any token or credential. Once connected, a client can subscribe to arbitrary function paths via the subscribe message type.

Impact: Any anonymous client (browser, curl, attacker) can open a persistent WebSocket and subscribe to real-time data updates from any registered function, receiving live data pushes without any identity check.

Fix:

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 upgraded = server.upgrade(req, { data: { projectSlug, userId: payload.sub } });

F-09 (HIGH): RBAC role system defined but never enforced at runtime

Files: packages/server/migrations/006_rbac.sql, packages/server/src/lib/admin-middleware.ts, all route handlers

Evidence:

  • 006_rbac.sql defines roles, permissions, role_permissions, admin_roles tables and seeds four built-in roles (owner, admin, developer, viewer) with distinct permission sets.
  • requireAdmin in admin-middleware.ts verifies authentication (JWT or API key) only — it never queries admin_roles or role_permissions.
  • No route handler in packages/server/src/routes/admin/** checks the authenticated admin's role or permissions.

Impact: A "viewer" role admin — intended to have read-only access — can invoke DELETE endpoints (/admin/projects/:id, /admin/users/:id, etc.) and they will succeed. The RBAC schema is entirely decorative at runtime.

Fix: Add a requirePermission(domain, action) middleware that queries the authenticated admin's effective permissions and gates routes accordingly. Example:

async function requirePermission(domain: string, action: string) {
  return async (c: Context, next: Next) => {
    const admin = c.get("adminUser") as { id: string };
    const { rows } = await pool.query(
      `SELECT 1 FROM betterbase_meta.admin_roles ar
       JOIN betterbase_meta.role_permissions rp ON rp.role_id = ar.role_id
       JOIN betterbase_meta.permissions p ON p.id = rp.permission_id
       WHERE ar.admin_user_id = $1 AND p.domain = $2 AND p.action = $3`,
      [admin.id, domain, action]
    );
    if (rows.length === 0) return c.json({ error: "Forbidden" }, 403);
    await next();
  };
}

F-10 (HIGH): JWT tokens have no issuer/audience, 30-day lifetime, and no revocation path

File: packages/server/src/lib/auth.ts

Evidence:

const TOKEN_EXPIRY = "30d";

export async function signAdminToken(adminUserId: string): Promise<string> {
    return new SignJWT({ sub: adminUserId, type: "admin" })
        .setProtectedHeader({ alg: "HS256" })
        .setIssuedAt()
        .setExpirationTime(TOKEN_EXPIRY)
        // ← no .setIssuer() / .setAudience() / .setJti()
        .sign(getSecret());
}

export async function verifyAdminToken(token: string): Promise<{ sub: string } | null> {
    try {
        const { payload } = await jwtVerify(token, getSecret());
        if (payload.type !== "admin") return null;
        // ← no issuer/audience check

Also, the logout handler:

// POST /admin/auth/logout  (client-side token discard — stateless)
authRoutes.post("/logout", (c) => c.json({ success: true }));

The token is not invalidated. A stolen token is valid for up to 30 days post-logout.

Impact:

  • 30-day admin JWT window means compromised tokens are long-lived attack tools.
  • Missing iss/aud allows tokens issued for one deployment to be replayed against another.
  • No revocation — a compromised or leaked admin token cannot be invalidated without rotating the entire JWT secret (which kills all other sessions).

Fix:

  1. Add iss/aud claims and validate them in verifyAdminToken.
  2. Reduce TOKEN_EXPIRY to "1h" or "8h" with a refresh token pattern.
  3. Introduce a token blocklist table (keyed by jti) populated on logout, checked in verifyAdminToken.
  4. Record logout in the audit log.

F-11 (HIGH): POST /device/code is unauthenticated — table flood and enumeration risk

File: packages/server/src/routes/device/index.ts

Evidence:

deviceRouter.post("/code", async (c) => {
    // ← no auth, no rate limiting
    const deviceCode = nanoid(32);
    const userCode = nanoid(8).toUpperCase();
    await pool.query(
        `INSERT INTO betterbase_meta.device_codes (user_code, device_code, expires_at) VALUES ($1, $2, $3)`,
        ...
    );

Any unauthenticated caller can create device codes without limit.

Impact:

  • DoS via table flood: Millions of rows inserted into device_codes, degrading DB performance and exhausting storage.
  • User code space exhaustion: nanoid(8).toUpperCase() produces an 8-character alphanumeric space. Under load, collisions are possible, causing INSERT failures for legitimate CLI logins.

Fix:

  1. Rate-limit /device/code by IP (e.g., 5 requests per minute per IP).
  2. Add a periodic cleanup job for expired device codes.
  3. Cap total active device codes per IP.

F-12 (HIGH): Dynamic SQL schema interpolation with unvalidated project slug

Files: packages/server/src/routes/admin/project-scoped/env.ts, database.ts, users.ts, packages/server/src/routes/betterbase/index.ts

Evidence:

// env.ts, database.ts, users.ts, betterbase/index.ts
function schemaName(project: { slug: string }) {
    return `project_${project.slug}`;
}
// Used directly in SQL:
await pool.query(`SELECT key FROM ${s}.env_vars ORDER BY key`);
await pool.query(`SELECT u.* FROM ${s}."user" u ...`);
await pool.query(`INSERT INTO "project_${projectSlug}"._iac_storage ...`);

If a project slug is created with special characters (e.g., legit"; DROP SCHEMA betterbase_meta CASCADE; --), it is interpolated verbatim into SQL strings. The project-scoped/index.ts middleware only validates that the project exists in the DB — it does not sanitize the slug before use in further queries.

Impact: SQL injection via project slug on all project-scoped routes. An admin who can create projects (including lower-privilege roles once RBAC is enforced) could execute arbitrary SQL.

Fix:

  1. Validate slug at creation time: enforce ^[a-z][a-z0-9_]{0,62}$.
  2. Use quote_ident() for all dynamic schema names:
const s = `project_${project.slug}`;
// Replace: `FROM ${s}."user"` 
// With parameterized quote_ident:
await pool.query(`SELECT * FROM ${pool.escapeIdentifier(s)}."user" WHERE id = $1`, [id]);

Or use pg's quoteIdent from pg-format.


F-13 (MEDIUM-HIGH): GET /admin/cli-sessions leaks all pending device codes across all admins

File: packages/server/src/routes/admin/cli-sessions.ts

Evidence:

const { rows: pending } = await pool.query(
    `SELECT user_code, created_at, expires_at
     FROM betterbase_meta.device_codes
     WHERE verified = FALSE AND expires_at > NOW()
     ORDER BY created_at DESC`,
    // ← no filter by admin_user_id
);

The query returns every unverified pending device code instance-wide. Any authenticated admin can see codes initiated by other admins.

Impact: An admin with any role can harvest another admin's pending user_code and attempt to verify it from their own browser session, hijacking the device auth flow.

Fix:

WHERE verified = FALSE AND expires_at > NOW() AND admin_user_id = $1

Note: device_codes.admin_user_id is null until verification, so the better approach is to correlate by the requesting admin's IP or store the initiating admin's session ID at POST /device/code time.


F-14 (MEDIUM): X-Forwarded-For IP address is attacker-controllable in audit and request logs

Files: packages/server/src/index.ts, packages/server/src/lib/audit.ts, docker/nginx/nginx.conf

Evidence — nginx:

proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;

$proxy_add_x_forwarded_for appends to any existing header the client already sent. An attacker sets X-Forwarded-For: 127.0.0.1 in their request; nginx appends its real IP, producing 127.0.0.1, nginx_ip.

Evidence — server:

// index.ts
const ip = c.req.header("X-Forwarded-For")?.split(",")[0] ?? null;
// audit.ts
headers.get("x-forwarded-for")?.split(",")[0]?.trim()

Both read [0] — the attacker-controlled leftmost value. Result: attacker can record any IP (including 127.0.0.1) in all audit log entries, completely defeating forensic traceability.

Fix — nginx:

# Strip any client-provided XFF before appending the real source
proxy_set_header X-Forwarded-For $remote_addr;

Fix — server: Trust only the rightmost XFF entry, or better, use X-Real-IP (set by nginx from $remote_addr), which cannot be forged by the client:

const ip = c.req.header("X-Real-IP") ?? null;

F-15 (MEDIUM): Storage credentials fall back to hardcoded minioadmin defaults silently in production

Files: packages/server/src/routes/admin/storage.ts, packages/server/src/routes/betterbase/index.ts

Evidence:

// storage.ts
credentials: {
    accessKeyId: process.env.STORAGE_ACCESS_KEY ?? "minioadmin",
    secretAccessKey: process.env.STORAGE_SECRET_KEY ?? "minioadmin",
},
// betterbase/index.ts buildStorageCtx
accessKey: env.STORAGE_ACCESS_KEY ?? "minioadmin",
secretKey: env.STORAGE_SECRET_KEY ?? "minioadmin",

STORAGE_ACCESS_KEY and STORAGE_SECRET_KEY are optional() in env.ts — if omitted, the server silently connects to MinIO with the well-known default credentials. A production deployment that forgets to set these env vars is silently exposed.

Fix:

  1. Make STORAGE_ACCESS_KEY and STORAGE_SECRET_KEY required in EnvSchema when STORAGE_ENDPOINT is set.
  2. Remove the "minioadmin" fallbacks; fail at startup instead:
STORAGE_ACCESS_KEY: z.string().min(1).optional(),
// In validateEnv():
if (result.data.STORAGE_ENDPOINT && !result.data.STORAGE_ACCESS_KEY) {
    console.error("[env] STORAGE_ACCESS_KEY is required when STORAGE_ENDPOINT is set");
    process.exit(1);
}

F-16 (MEDIUM): Duplicate migration filenames at prefixes 011 and 012

Confirmed by script output: 011 and 012 both appear as duplicate prefixes.

Evidence:

packages/server/migrations/011_enhance_request_logs.sql
packages/server/migrations/011_iac_storage.sql
packages/server/migrations/012_iac_scheduler.sql
packages/server/migrations/012_webhook_delivery_logs.sql

Migration runner (migrate.ts):

const files = (await readdir(MIGRATIONS_DIR)).filter((f) => f.endsWith(".sql")).sort();

.sort() on these filenames produces an alphabetical ordering. Both 011_enhance_request_logs.sql and 011_iac_storage.sql will be attempted. If either creates overlapping objects, one will fail. The migration runner has no transaction wrapping (F-04), so a partial failure leaves the schema in a broken state. The second file's DDL may silently not apply if a prior error exits the loop.

Fix:

  1. Immediately rename to unique sequential numbers: 011, 012, 013, 014 with no duplicates.
  2. Add a startup check that aborts with a clear error if two migrations share the same numeric prefix.

F-17 (MEDIUM): CSV user export is vulnerable to formula injection

File: packages/server/src/routes/admin/project-scoped/users.ts

Evidence:

rows.map((r) =>
    `${r.id},"${r.name}","${r.email}",${r.email_verified},${r.created_at},${r.banned}`
)

r.name and r.email are user-controlled values interpolated directly into CSV without escaping. A user who registers with name =HYPERLINK("http://evil.com","click") will embed an active formula into the exported spreadsheet, executing when opened in Excel or Google Sheets.

Fix:

  1. Prefix any cell value starting with =, +, -, or @ with a ' (single quote), or wrap all values in double-quotes with internal double-quotes escaped:
const escape = (v: string) =>
  /^[=+\-@\t\r]/.test(v) ? `'${v.replace(/"/g, '""')}` : v.replace(/"/g, '""');
  1. Set Content-Type: text/csv; charset=utf-8 and add Content-Security-Policy: default-src 'none' for the response.

Additional Lower-Severity Findings

ID Severity Location Issue
F-18 Medium smtp.ts POST /admin/smtp/test SMTP error messages containing hostnames/IPs returned raw to client: return c.json({ error: `SMTP error: ${err.message}` })
F-19 Medium smtp.ts PUT /admin/smtp SMTP password stored in plaintext in betterbase_meta.smtp_config
F-20 Medium webhooks.ts POST /admin/webhooks Webhook signing secret stored in plaintext in betterbase_meta.webhooks
F-21 Medium nginx.conf, docker-compose.yml MinIO console port 9001 and Inngest port 8288 published on host directly, bypassing nginx IP restrictions for direct host-network connections
F-22 Medium nginx.conf No security response headers: Content-Security-Policy, X-Frame-Options, X-Content-Type-Options, Strict-Transport-Security not set anywhere
F-23 Low auth.ts POST /admin/auth/logout Logout not written to audit log (admin.logout action exists in AuditAction type but is never called)
F-24 Low env.ts Hardcoded INNGEST_EVENT_KEY = "betterbase-dev-event-key" silently injected when key is absent in non-production environments

Revised Implementation Priority

Priority Findings Time estimate
P0 — Deploy blockers F-06 (betterbase auth gate), F-07 (storage upload auth), F-08 (WS auth), F-09 (RBAC enforcement) Hours
P1 — High risk F-01 (rate limits), F-02 (API key scopes), F-10 (JWT hardening + revocation), F-11 (device/code rate limit), F-12 (slug SQL injection) Days
P2 — Medium risk F-03 (HTML injection), F-13 (device code leak), F-14 (XFF spoofing), F-15 (storage credential defaults), F-16 (duplicate migrations — fix immediately, data integrity), F-17 (CSV injection) Days
P3 — Hygiene F-04 (migration transactions), F-05 (Docker defaults), F-18–F-24 Weeks

Closure Gates (Extended)

In addition to the original five gates, production readiness requires:

  1. /betterbase/* function router enforces authentication before executing any handler.
  2. /betterbase/ws WebSocket upgrade validates a token before accepting the connection.
  3. /betterbase/storage/generate-upload-url requires authentication and validates contentType against an allowlist.
  4. RBAC role/permission assignments are consulted by at least one requirePermission() guard on all mutating admin routes.
  5. JWT tokens use iss/aud, a jti-based blocklist for revocation, and a ≤8h expiry.
  6. Duplicate migration prefix collisions (011, 012) are resolved before the next deployment.
  7. X-Forwarded-For is stripped at the nginx boundary; server reads only X-Real-IP.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant