[object Object]

← back to Grant

Security audit P1/P2 fixes: CSRF guard, sanitize outreach HTML + rel=noopener, COLUMN_MAP PATCH, proposals tenant isolation, AI rate-limit 10→5, x-request-id validation, resolveOrgId ordering, x-real-ip preference, gated SQL logging, rotate-script dollar-quote, /river cache

ee53e6f3b28d09edcb7e72b3ad02714785e2975d · 2026-05-30 11:40:45 -0700 · Steve Abrams

Files touched

Diff

commit ee53e6f3b28d09edcb7e72b3ad02714785e2975d
Author: Steve Abrams <steve@designerwallcoverings.com>
Date:   Sat May 30 11:40:45 2026 -0700

    Security audit P1/P2 fixes: CSRF guard, sanitize outreach HTML + rel=noopener, COLUMN_MAP PATCH, proposals tenant isolation, AI rate-limit 10→5, x-request-id validation, resolveOrgId ordering, x-real-ip preference, gated SQL logging, rotate-script dollar-quote, /river cache
---
 app/api/auth/login/route.ts            |  8 +++++---
 app/api/collaborations/route.ts        | 17 +++++++++++------
 app/api/grants/[id]/proposals/route.ts |  8 +++++++-
 app/api/grants/route.ts                | 26 +++++++++++++++++---------
 app/api/outreach/generate/route.ts     |  8 ++++++--
 app/api/outreach/route.ts              | 10 +++++++++-
 app/river/page.tsx                     |  5 ++++-
 lib/auth.ts                            |  5 ++++-
 lib/db.ts                              |  8 ++++++--
 lib/rate-limit.ts                      |  6 +++++-
 lib/sanitize.ts                        |  9 +++++++++
 middleware.ts                          | 25 +++++++++++++++++++++++--
 scripts/rotate-pg-grant.sh             | 16 +++++++++++++---
 13 files changed, 119 insertions(+), 32 deletions(-)

diff --git a/app/api/auth/login/route.ts b/app/api/auth/login/route.ts
index 7350fde..730df90 100644
--- a/app/api/auth/login/route.ts
+++ b/app/api/auth/login/route.ts
@@ -64,9 +64,11 @@ function loadCredentials(): CredentialRecord {
 
   const salt = crypto.randomBytes(16);
   const hash = deriveHash(password, salt);
-  // Best-effort scrub: remove the plaintext from process.env so it doesn't
-  // sit in /proc/<pid>/environ for the life of the process.
-  delete process.env.AUTH_PASSWORD;
+  // 2026-05-30 (audit P1-1): do NOT delete process.env.AUTH_PASSWORD here.
+  // lib/sister-auth.ts reads it at module-load to derive SISTER_AUTH; module
+  // eval order is non-deterministic, so deleting it could silently zero out
+  // sister-auth (a latent auth-bypass once a sister route exists). The env is
+  // already readable from process start, so the "scrub" bought almost nothing.
 
   return { username, salt, hash };
 }
diff --git a/app/api/collaborations/route.ts b/app/api/collaborations/route.ts
index 7365461..6be0377 100644
--- a/app/api/collaborations/route.ts
+++ b/app/api/collaborations/route.ts
@@ -115,18 +115,23 @@ export async function PATCH(request: NextRequest) {
       return NextResponse.json({ error: 'id is required' }, { status: 400 });
     }
 
-    const allowedFields = [
-      'status', 'notes', 'last_contacted', 'email', 'phone',
-      'website_url', 'title', 'organization', 'district', 'state',
-    ];
+    // 2026-05-30 (audit P1-7): map allowed field → literal SQL identifier
+    // instead of interpolating the user-supplied key.
+    const COLUMN_MAP: Record<string, string> = {
+      status: '"status"', notes: '"notes"', last_contacted: '"last_contacted"',
+      email: '"email"', phone: '"phone"', website_url: '"website_url"',
+      title: '"title"', organization: '"organization"', district: '"district"',
+      state: '"state"',
+    };
 
     const setClauses: string[] = [];
     const params: unknown[] = [];
     let paramIdx = 1;
 
     for (const key of Object.keys(fields)) {
-      if (allowedFields.includes(key)) {
-        setClauses.push(`"${key}" = $${paramIdx}`);
+      const col = COLUMN_MAP[key];
+      if (col) {
+        setClauses.push(`${col} = $${paramIdx}`);
         params.push(fields[key]);
         paramIdx++;
       }
diff --git a/app/api/grants/[id]/proposals/route.ts b/app/api/grants/[id]/proposals/route.ts
index 0ebc294..98d8948 100644
--- a/app/api/grants/[id]/proposals/route.ts
+++ b/app/api/grants/[id]/proposals/route.ts
@@ -63,8 +63,14 @@ export async function POST(
     const MAX_CONTEXT_CHARS = 2000;
     const context = String(body.context || '').slice(0, MAX_CONTEXT_CHARS);
 
+    // 2026-05-30 (audit P2-3): tenant isolation — fetch the grant scoped to
+    // the caller's org so a user can't generate a proposal against another
+    // org's grant (the insert below uses grant.org_id).
+    const orgId = await resolveOrgId(session);
+    if (!orgId) return NextResponse.json({ error: 'No organization found' }, { status: 400 });
+
     // Get grant details
-    const grantRes = await query('SELECT id, org_id, title, funder, amount_min, amount_max, description, eligibility, deadline, ai_suggestion FROM grants WHERE id = $1', [grantId]);
+    const grantRes = await query('SELECT id, org_id, title, funder, amount_min, amount_max, description, eligibility, deadline, ai_suggestion FROM grants WHERE id = $1 AND org_id = $2', [grantId, orgId]);
     if (grantRes.rows.length === 0) {
       return NextResponse.json({ error: 'Grant not found' }, { status: 404 });
     }
diff --git a/app/api/grants/route.ts b/app/api/grants/route.ts
index ed7f2f0..516e04b 100644
--- a/app/api/grants/route.ts
+++ b/app/api/grants/route.ts
@@ -117,21 +117,29 @@ export async function PATCH(request: NextRequest) {
       return NextResponse.json({ error: 'id is required' }, { status: 400 });
     }
 
-    const allowedFields = [
-      'title', 'funder', 'funder_url', 'amount_min', 'amount_max',
-      'description', 'eligibility', 'deadline', 'focus_areas', 'priority',
-      'application_url', 'cycle', 'notes', 'tags', 'status',
-      'is_bookmarked', 'applied_at', 'amount_requested', 'amount_awarded',
-      'ai_fit_score', 'ai_suggestion',
-    ];
+    // 2026-05-30 (audit P1-7): map allowed field → literal SQL identifier
+    // instead of interpolating the user-supplied key. Removes the "allowlist
+    // typo/removal = SQL injection" failure mode entirely.
+    const COLUMN_MAP: Record<string, string> = {
+      title: '"title"', funder: '"funder"', funder_url: '"funder_url"',
+      amount_min: '"amount_min"', amount_max: '"amount_max"',
+      description: '"description"', eligibility: '"eligibility"',
+      deadline: '"deadline"', focus_areas: '"focus_areas"', priority: '"priority"',
+      application_url: '"application_url"', cycle: '"cycle"', notes: '"notes"',
+      tags: '"tags"', status: '"status"', is_bookmarked: '"is_bookmarked"',
+      applied_at: '"applied_at"', amount_requested: '"amount_requested"',
+      amount_awarded: '"amount_awarded"', ai_fit_score: '"ai_fit_score"',
+      ai_suggestion: '"ai_suggestion"',
+    };
 
     const setClauses: string[] = [];
     const params: unknown[] = [];
     let paramIdx = 1;
 
     for (const key of Object.keys(fields)) {
-      if (allowedFields.includes(key)) {
-        setClauses.push(`"${key}" = $${paramIdx}`);
+      const col = COLUMN_MAP[key];
+      if (col) {
+        setClauses.push(`${col} = $${paramIdx}`);
         params.push(fields[key]);
         paramIdx++;
       }
diff --git a/app/api/outreach/generate/route.ts b/app/api/outreach/generate/route.ts
index 97722d1..3696124 100644
--- a/app/api/outreach/generate/route.ts
+++ b/app/api/outreach/generate/route.ts
@@ -3,6 +3,7 @@ import { query } from '@/lib/db';
 import { verifyAuthWithOrg, resolveOrgId } from '@/lib/auth';
 import { auditLog } from '@/lib/audit';
 import { callGemini } from '@/lib/gemini';
+import { sanitizeProposalBody } from '@/lib/sanitize';
 
 type OutreachShape = { subject?: string; body_html?: string; body_text?: string };
 
@@ -83,6 +84,9 @@ Return ONLY a JSON object (no markdown, no code fences) with:
       );
     }
     const generated = result.data;
+    // 2026-05-30 (audit P1-5): sanitize AI-generated HTML before storage —
+    // Gemini output is untrusted input once it lands in the DB.
+    const cleanHtml = sanitizeProposalBody(generated.body_html || '');
 
     // Save as a template
     const title = `${typeLabels[template_type] || 'Email'} - ${target_name || target_org || 'General'}`;
@@ -96,7 +100,7 @@ Return ONLY a JSON object (no markdown, no code fences) with:
         target_type || 'general',
         title,
         generated.subject || 'Outreach from ' + org.name,
-        generated.body_html || '',
+        cleanHtml,
         generated.body_text || '',
       ],
     );
@@ -110,7 +114,7 @@ Return ONLY a JSON object (no markdown, no code fences) with:
 
     return NextResponse.json({
       subject: generated.subject,
-      body_html: generated.body_html,
+      body_html: cleanHtml,
       body_text: generated.body_text,
       template: saveRes.rows[0],
     });
diff --git a/app/api/outreach/route.ts b/app/api/outreach/route.ts
index ce04262..b5dc631 100644
--- a/app/api/outreach/route.ts
+++ b/app/api/outreach/route.ts
@@ -1,6 +1,7 @@
 import { NextRequest, NextResponse } from 'next/server';
 import { query } from '@/lib/db';
 import { verifyAuthWithOrg, resolveOrgId } from '@/lib/auth';
+import { sanitizeProposalBody, bodyHtmlTooLarge } from '@/lib/sanitize';
 
 /* ─── GET /api/outreach ───────────────────────────────────────────────────── */
 export async function GET(request: NextRequest) {
@@ -51,6 +52,13 @@ export async function POST(request: NextRequest) {
     const orgId = await resolveOrgId(session);
     if (!orgId) return NextResponse.json({ error: 'No organization found' }, { status: 400 });
 
+    // 2026-05-30 (audit P1-5): sanitize body_html at the storage boundary —
+    // outreach templates were stored raw while proposals were sanitized.
+    if (bodyHtmlTooLarge(body.body_html ?? '')) {
+      return NextResponse.json({ error: 'body_html too large or invalid' }, { status: 400 });
+    }
+    const cleanHtml = sanitizeProposalBody(body.body_html || '');
+
     const result = await query(
       `INSERT INTO outreach_templates (org_id, template_type, target_type, title, subject, body_html, body_text, is_ai_generated)
        VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
@@ -61,7 +69,7 @@ export async function POST(request: NextRequest) {
         body.target_type || 'general',
         body.title || 'Untitled Template',
         body.subject || null,
-        body.body_html || '',
+        cleanHtml,
         body.body_text || '',
         body.is_ai_generated !== undefined ? body.is_ai_generated : false,
       ],
diff --git a/app/river/page.tsx b/app/river/page.tsx
index fa8bff4..de21c09 100644
--- a/app/river/page.tsx
+++ b/app/river/page.tsx
@@ -11,7 +11,10 @@
 import RiverCanvas from './RiverCanvas';
 import RiverTable from './RiverTable';
 
-export const dynamic = 'force-dynamic';
+// 2026-05-30 (audit P0-5): removed `dynamic = 'force-dynamic'` — it overrode
+// `revalidate`, forcing a fresh outbound grants.gov fetch on EVERY request
+// (unauthenticated public page = fetch-amplification surface). With only
+// revalidate, the page is cached and re-fetches grants.gov at most every 10min.
 export const revalidate = 600; // 10 min cache
 
 type GrantsGovHit = {
diff --git a/lib/auth.ts b/lib/auth.ts
index 2c06b3e..7b97dd2 100644
--- a/lib/auth.ts
+++ b/lib/auth.ts
@@ -26,6 +26,9 @@ export type { AuthSession };
  */
 export async function resolveOrgId(session: AuthSession): Promise<string | null> {
   if (session.orgId) return session.orgId;
-  const orgRes = await query('SELECT id FROM organizations LIMIT 1');
+  // 2026-05-30 (audit P2-1): deterministic fallback — without ORDER BY,
+  // LIMIT 1 picks an arbitrary org row, which would mis-route legacy v1
+  // sessions in a multi-org deployment. Pin to the oldest org.
+  const orgRes = await query('SELECT id FROM organizations ORDER BY created_at ASC LIMIT 1');
   return orgRes.rows.length > 0 ? orgRes.rows[0].id : null;
 }
diff --git a/lib/db.ts b/lib/db.ts
index 443c7b5..e4b0cf8 100644
--- a/lib/db.ts
+++ b/lib/db.ts
@@ -25,11 +25,15 @@ export async function query<T extends QueryResultRow = QueryResultRow>(
     const result = await pool.query<T>(text, params);
     const duration = Date.now() - start;
     if (duration > 2000) {
-      console.warn(`[db] Slow query (${duration}ms):`, text.slice(0, 120));
+      // 2026-05-30 (audit P2-5): only echo SQL text outside production so query
+      // structure doesn't sit in prod log rotation.
+      const detail = process.env.NODE_ENV === 'production' ? '' : ` ${text.slice(0, 120)}`;
+      console.warn(`[db] Slow query (${duration}ms):${detail}`);
     }
     return result;
   } catch (err) {
-    console.error('[db] Query error:', (err as Error).message, '\n  SQL:', text.slice(0, 200));
+    const detail = process.env.NODE_ENV === 'production' ? '' : `\n  SQL: ${text.slice(0, 200)}`;
+    console.error('[db] Query error:', (err as Error).message, detail);
     throw err;
   }
 }
diff --git a/lib/rate-limit.ts b/lib/rate-limit.ts
index 141fab3..16dd067 100644
--- a/lib/rate-limit.ts
+++ b/lib/rate-limit.ts
@@ -36,9 +36,13 @@ export interface LoginAttemptResult {
 }
 
 export function loginClientIp(request: NextRequest): string {
+  // 2026-05-30 (audit P1-2): prefer x-real-ip (nginx sets it from $remote_addr
+  // and the client cannot spoof it) over x-forwarded-for (client-appendable).
+  // The Grant nginx vhost also pins X-Forwarded-For to $remote_addr, so both
+  // are trustworthy behind the proxy; x-real-ip first is the safer default.
   return (
+    request.headers.get('x-real-ip')?.trim() ||
     request.headers.get('x-forwarded-for')?.split(',')[0]?.trim() ||
-    request.headers.get('x-real-ip') ||
     'unknown'
   );
 }
diff --git a/lib/sanitize.ts b/lib/sanitize.ts
index 3f09287..cf07c7d 100644
--- a/lib/sanitize.ts
+++ b/lib/sanitize.ts
@@ -26,6 +26,15 @@ export const PROPOSAL_HTML_OPTS: IOptions = {
   allowedAttributes: { a: ['href', 'title', 'target', 'rel'] },
   allowedSchemes: ['http', 'https', 'mailto'],
   disallowedTagsMode: 'discard',
+  // 2026-05-30 (audit P1-5): force rel="noopener noreferrer" on every anchor
+  // so a target="_blank" link can never hand the opener a window reference
+  // (tab-napping) if body_html is ever rendered via dangerouslySetInnerHTML.
+  transformTags: {
+    a: (tagName, attribs) => ({
+      tagName,
+      attribs: { ...attribs, rel: 'noopener noreferrer' },
+    }),
+  },
 };
 
 export function sanitizeProposalBody(html: string): string {
diff --git a/middleware.ts b/middleware.ts
index c03a0e1..4fe26ec 100644
--- a/middleware.ts
+++ b/middleware.ts
@@ -25,7 +25,10 @@ export const config = {
 type Bucket = { count: number; resetAt: number };
 const buckets: Map<string, Bucket> = new Map();
 const WINDOW_MS = 60_000;
-const AI_LIMIT_PER_MINUTE = 10;
+// 2026-05-30 (audit P1-3): tightened 10→5 for public exposure. Each Gemini
+// call can cost ~$0.0025; a human won't fire 5 generations of one type per
+// minute, but this caps automated/CSRF-driven cost abuse.
+const AI_LIMIT_PER_MINUTE = 5;
 
 const AI_ROUTES = new Set([
   '/api/grants/discover',
@@ -59,6 +62,22 @@ export function middleware(request: NextRequest) {
     return NextResponse.json({ error: 'Unauthorized' }, { status: 401 });
   }
 
+  // 2026-05-30 (audit P1-4): CSRF guard. The session cookie is SameSite=Lax,
+  // which still permits cross-site fetch() POSTs. For state-mutating methods,
+  // require the Origin header to match our own host (browsers always send
+  // Origin on cross-origin and same-origin non-GET requests).
+  if (['POST', 'PATCH', 'PUT', 'DELETE'].includes(request.method)) {
+    const origin = request.headers.get('origin');
+    if (origin) {
+      let originHost: string;
+      try { originHost = new URL(origin).host; } catch { originHost = ''; }
+      const host = request.headers.get('host') ?? '';
+      if (originHost !== host) {
+        return NextResponse.json({ error: 'Cross-origin request rejected' }, { status: 403 });
+      }
+    }
+  }
+
   // Rate-limit the Gemini-paid AI routes. Key by orgId+route so two users
   // in the same org share a quota; falls back to username for legacy v1
   // sessions where orgId is null.
@@ -83,8 +102,10 @@ export function middleware(request: NextRequest) {
 }
 
 function injectRequestId(res: NextResponse, request: NextRequest): NextResponse {
+  // 2026-05-30 (audit P2-2): only honor a client-supplied request-id if it's a
+  // safe token; otherwise mint one. Stops log-injection via crafted header.
   const incoming = request.headers.get('x-request-id');
-  const reqId = incoming || crypto.randomUUID();
+  const reqId = incoming && /^[A-Za-z0-9_-]{1,64}$/.test(incoming) ? incoming : crypto.randomUUID();
   res.headers.set('x-request-id', reqId);
   return res;
 }
diff --git a/scripts/rotate-pg-grant.sh b/scripts/rotate-pg-grant.sh
index 065d262..eca67f2 100755
--- a/scripts/rotate-pg-grant.sh
+++ b/scripts/rotate-pg-grant.sh
@@ -59,15 +59,25 @@ fi
 # but the SQL never lands in any shell argv.
 echo "Applying PG role change..."
 NEW_PG="$NEW_PG" python3 - <<'PYEOF'
-import os, subprocess, sys
+import os, subprocess, sys, secrets
 pw = os.environ['NEW_PG']
+# audit P2-7: wrap the password in a RANDOM dollar-quote tag instead of a
+# single-quoted f-string literal. A password containing a single quote,
+# backslash, or "$$" could otherwise break out of the literal and inject SQL.
+# The tag is random and asserted absent from the password, so the literal
+# boundary is unforgeable — and the password still never touches argv (stdin).
+_tag = 'pw_' + secrets.token_hex(8)
+if _tag in pw:
+    print('dollar-quote tag collision (1-in-2^64) — re-run', file=sys.stderr)
+    sys.exit(1)
+_lit = f"${_tag}${pw}${_tag}$"
 sql = f"""
 DO $$
 BEGIN
   IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'grant_app_user') THEN
-    CREATE ROLE grant_app_user WITH LOGIN PASSWORD '{pw}';
+    CREATE ROLE grant_app_user WITH LOGIN PASSWORD {_lit};
   ELSE
-    ALTER ROLE grant_app_user WITH LOGIN PASSWORD '{pw}';
+    ALTER ROLE grant_app_user WITH LOGIN PASSWORD {_lit};
   END IF;
 END
 $$;

← 1967d65 Remove debug logging from auth-branch page  ·  back to Grant  ·  Add admin-level account + Live/Admin view toggle d27ef8c →