-
Notifications
You must be signed in to change notification settings - Fork 0
Phase 1: Security Hardening & Global RLS Enforcement #26
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 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,18 +8,14 @@ import crypto from "crypto"; | |
| import * as diff from "diff"; | ||
|
|
||
| export const getDocuments = async (req: Request, res: Response) => { | ||
| const userId = req.user!.id; | ||
| const userRole = req.user!.role; | ||
| const userEmail = req.user!.email.toLowerCase(); | ||
| const client = req.dbClient || pool; // Fallback to pool if middleware bypassed (should not happen) | ||
|
|
||
| try { | ||
| const docs = await withTransaction(userId, userRole, async (client) => { | ||
| const { rows } = await client.query( | ||
| "SELECT * FROM files WHERE creator_id = $1 OR shared_with::jsonb @> $2::jsonb ORDER BY created_at DESC", | ||
| [userId, JSON.stringify([userEmail])] | ||
| ); | ||
| return rows; | ||
| }); | ||
| const { rows: docs } = await client.query( | ||
| "SELECT * FROM files WHERE creator_id = current_setting('app.current_user_id', true) OR shared_with::jsonb @> $1::jsonb ORDER BY created_at DESC", | ||
| [JSON.stringify([userEmail])] | ||
| ); | ||
|
Comment on lines
+15
to
+18
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. This list query narrows away the RLS admin override. The 🤖 Prompt for AI Agents |
||
|
|
||
| const formattedDocs = docs.map((r: any) => ({ | ||
| ...r, | ||
|
|
@@ -37,19 +33,12 @@ export const getDocuments = async (req: Request, res: Response) => { | |
| }; | ||
|
|
||
| export const getDocumentById = async (req: Request, res: Response) => { | ||
| const userId = req.user!.id; | ||
| const userEmail = req.user!.email.toLowerCase(); | ||
| const client = req.dbClient || pool; | ||
|
|
||
| try { | ||
| const { rows } = await pool.query("SELECT * FROM files WHERE id = $1", [req.params.id]); | ||
| const { rows } = await client.query("SELECT * FROM files WHERE id = $1", [req.params.id]); | ||
| if (rows.length > 0) { | ||
|
Comment on lines
+39
to
40
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. Shared documents will now 404 for non-owners. The old application-side shared-with authorization check is gone, but the current 🤖 Prompt for AI Agents |
||
| const r = rows[0]; | ||
| const isShared = r.shared_with.some((e: string) => e.toLowerCase() === userEmail); | ||
| const isOwner = r.creator_id === userId; | ||
|
|
||
| if (!isOwner && !isShared) { | ||
| return res.status(403).json({ error: "Access denied to this document." }); | ||
| } | ||
|
|
||
| const doc = { | ||
| ...r, | ||
|
|
@@ -75,12 +64,13 @@ export const createDocument = async (req: Request, res: Response) => { | |
| const { title, type, content } = req.body; | ||
| const userId = req.user!.id; | ||
| const email = req.user!.email; | ||
| const client = req.dbClient || pool; | ||
|
|
||
| const id = "doc_" + crypto.randomUUID(); | ||
| const encryptedContent = encryptData(content || ""); | ||
|
|
||
| try { | ||
| await pool.query( | ||
| await client.query( | ||
| `INSERT INTO files (id, title, type, content, creator_id, creator_email, is_encrypted, versions, shared_with, audit_logs) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10)`, | ||
| [id, title, type, encryptedContent, userId, email, true, JSON.stringify([]), JSON.stringify([]), JSON.stringify([])] | ||
|
|
@@ -98,9 +88,10 @@ export const uploadDocument = async (req: Request, res: Response) => { | |
| const { title, folder_id } = req.body; | ||
| const fileId = "doc_" + crypto.randomUUID(); | ||
| const fileTitle = title || file.originalname; | ||
| const client = req.dbClient || pool; | ||
|
|
||
| try { | ||
| await pool.query( | ||
| await client.query( | ||
| `INSERT INTO files (id, title, type, content, creator_id, creator_email, mime_type, folder_id) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8)`, | ||
| [fileId, fileTitle, "upload", "", req.user!.id, req.user!.email, file.mimetype, folder_id || null] | ||
|
|
@@ -124,21 +115,23 @@ export const uploadDocument = async (req: Request, res: Response) => { | |
| export const createRedline = async (req: Request, res: Response) => { | ||
| const { id } = req.params; | ||
| const { originalText, proposedText, comment } = req.body; | ||
| const client = req.dbClient || pool; | ||
| try { | ||
| const { rows } = await pool.query("SELECT redlines FROM files WHERE id = $1", [id]); | ||
| const { rows } = await client.query("SELECT redlines FROM files WHERE id = $1", [id]); | ||
| if (rows.length === 0) return res.status(404).json({ error: "Document not found" }); | ||
| const redlines = rows[0].redlines || []; | ||
| const newRedline = { id: crypto.randomUUID(), originalText, proposedText, comment, proposedByEmail: req.user!.email, proposedAt: new Date().toISOString(), status: "pending" }; | ||
| redlines.push(newRedline); | ||
| await pool.query("UPDATE files SET redlines = $1 WHERE id = $2", [JSON.stringify(redlines), id]); | ||
| await client.query("UPDATE files SET redlines = $1 WHERE id = $2", [JSON.stringify(redlines), id]); | ||
| res.status(201).json(newRedline); | ||
| } catch (err: any) { res.status(500).json({ error: err.message }); } | ||
| }; | ||
|
|
||
| export const acceptRedline = async (req: Request, res: Response) => { | ||
| const { id, redlineId } = req.params; | ||
| const client = req.dbClient || pool; | ||
| try { | ||
| const { rows } = await pool.query("SELECT * FROM files WHERE id = $1", [id]); | ||
| const { rows } = await client.query("SELECT * FROM files WHERE id = $1", [id]); | ||
| if (rows.length === 0) return res.status(404).json({ error: "Document not found" }); | ||
|
|
||
| const doc = rows[0]; | ||
|
|
@@ -164,10 +157,10 @@ export const acceptRedline = async (req: Request, res: Response) => { | |
| } | ||
|
|
||
| redlines[index].status = "accepted"; | ||
| await pool.query("UPDATE files SET content = $1, redlines = $2 WHERE id = $3", [encryptData(fallbackReplaced), JSON.stringify(redlines), id]); | ||
| await client.query("UPDATE files SET content = $1, redlines = $2 WHERE id = $3", [encryptData(fallbackReplaced), JSON.stringify(redlines), id]); | ||
| } else { | ||
| redlines[index].status = "accepted"; | ||
| await pool.query("UPDATE files SET content = $1, redlines = $2 WHERE id = $3", [encryptData(applied), JSON.stringify(redlines), id]); | ||
| await client.query("UPDATE files SET content = $1, redlines = $2 WHERE id = $3", [encryptData(applied), JSON.stringify(redlines), id]); | ||
| } | ||
|
|
||
| res.json({ success: true }); | ||
|
|
@@ -179,14 +172,15 @@ export const acceptRedline = async (req: Request, res: Response) => { | |
|
|
||
| export const rejectRedline = async (req: Request, res: Response) => { | ||
| const { id, redlineId } = req.params; | ||
| const client = req.dbClient || pool; | ||
| try { | ||
| const { rows } = await pool.query("SELECT redlines FROM files WHERE id = $1", [id]); | ||
| const { rows } = await client.query("SELECT redlines FROM files WHERE id = $1", [id]); | ||
| if (rows.length === 0) return res.status(404).json({ error: "Document not found" }); | ||
| const redlines = rows[0].redlines || []; | ||
| const index = redlines.findIndex((r: any) => r.id === redlineId); | ||
| if (index === -1) return res.status(404).json({ error: "Redline not found" }); | ||
| redlines[index].status = "rejected"; | ||
| await pool.query("UPDATE files SET redlines = $1 WHERE id = $2", [JSON.stringify(redlines), id]); | ||
| await client.query("UPDATE files SET redlines = $1 WHERE id = $2", [JSON.stringify(redlines), id]); | ||
| res.json({ success: true }); | ||
| } catch (err: any) { res.status(500).json({ error: err.message }); } | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,14 +4,11 @@ import { withTransaction } from "../utils/dbUtils.js"; | |||||||||||||
| import crypto from "crypto"; | ||||||||||||||
|
|
||||||||||||||
| export const getFolders = async (req: Request, res: Response) => { | ||||||||||||||
| const client = req.dbClient || pool; | ||||||||||||||
| try { | ||||||||||||||
| const rows = await withTransaction(req.user!.id, req.user!.role, async (client) => { | ||||||||||||||
| const { rows } = await client.query( | ||||||||||||||
| "SELECT * FROM folders WHERE user_id = $1 ORDER BY created_at DESC", | ||||||||||||||
| [req.user!.id] | ||||||||||||||
| ); | ||||||||||||||
| return rows; | ||||||||||||||
| }); | ||||||||||||||
| const { rows } = await client.query( | ||||||||||||||
| "SELECT * FROM folders WHERE user_id = current_setting('app.current_user_id', true) ORDER BY created_at DESC" | ||||||||||||||
| ); | ||||||||||||||
|
Comment on lines
+9
to
+11
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. This owner filter disables the admin override for folders.
Suggested fix- const { rows } = await client.query(
- "SELECT * FROM folders WHERE user_id = current_setting('app.current_user_id', true) ORDER BY created_at DESC"
- );
+ const { rows } = await client.query(
+ "SELECT * FROM folders ORDER BY created_at DESC"
+ );📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| res.json(rows); | ||||||||||||||
| } catch (err: any) { | ||||||||||||||
| res.status(500).json({ error: err.message }); | ||||||||||||||
|
|
@@ -20,11 +17,12 @@ export const getFolders = async (req: Request, res: Response) => { | |||||||||||||
|
|
||||||||||||||
| export const createFolder = async (req: Request, res: Response) => { | ||||||||||||||
| const { name } = req.body; | ||||||||||||||
| const client = req.dbClient || pool; | ||||||||||||||
| if (!name) return res.status(400).json({ error: "Folder name is required." }); | ||||||||||||||
|
|
||||||||||||||
| try { | ||||||||||||||
| const id = "fld_" + crypto.randomUUID(); | ||||||||||||||
| const { rows } = await pool.query( | ||||||||||||||
| const { rows } = await client.query( | ||||||||||||||
| "INSERT INTO folders (id, name, user_id) VALUES ($1, $2, $3) RETURNING *", | ||||||||||||||
| [id, name, req.user!.id] | ||||||||||||||
| ); | ||||||||||||||
|
|
@@ -35,10 +33,11 @@ export const createFolder = async (req: Request, res: Response) => { | |||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| export const deleteFolder = async (req: Request, res: Response) => { | ||||||||||||||
| const client = req.dbClient || pool; | ||||||||||||||
| try { | ||||||||||||||
| const result = await pool.query( | ||||||||||||||
| "DELETE FROM folders WHERE id = $1 AND user_id = $2", | ||||||||||||||
| [req.params.id, req.user!.id] | ||||||||||||||
| const result = await client.query( | ||||||||||||||
| "DELETE FROM folders WHERE id = $1", | ||||||||||||||
| [req.params.id] | ||||||||||||||
| ); | ||||||||||||||
| if (result.rowCount === 0) return res.status(404).json({ error: "Folder not found." }); | ||||||||||||||
| res.json({ success: true }); | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,25 +1,28 @@ | ||||||||||||||||||
| import { Request, Response } from "express"; | ||||||||||||||||||
| import { jobRegistry } from "../services/jobQueue.js"; | ||||||||||||||||||
| import { pool } from "../config/database.js"; | ||||||||||||||||||
|
|
||||||||||||||||||
| export const getJobs = async (req: Request, res: Response) => { | ||||||||||||||||||
| const client = req.dbClient || pool; | ||||||||||||||||||
| try { | ||||||||||||||||||
| const userJobs = await jobRegistry.getUserJobs(req.user!.id); | ||||||||||||||||||
| res.json(userJobs); | ||||||||||||||||||
| // Phase 1: Security Hardening - use RLS client | ||||||||||||||||||
| const { rows } = await client.query( | ||||||||||||||||||
| "SELECT * FROM jobs WHERE user_id = current_setting('app.current_user_id', true) ORDER BY created_at DESC" | ||||||||||||||||||
| ); | ||||||||||||||||||
|
Comment on lines
+8
to
+11
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. This query defeats the RLS admin override for jobs. The new Suggested fix- const { rows } = await client.query(
- "SELECT * FROM jobs WHERE user_id = current_setting('app.current_user_id', true) ORDER BY created_at DESC"
- );
+ const { rows } = await client.query(
+ "SELECT * FROM jobs ORDER BY created_at DESC"
+ );📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| res.json(rows); | ||||||||||||||||||
| } catch (err: any) { | ||||||||||||||||||
| res.status(500).json({ error: "Failed to fetch jobs" }); | ||||||||||||||||||
| } | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
| export const getJobById = async (req: Request, res: Response) => { | ||||||||||||||||||
| const client = req.dbClient || pool; | ||||||||||||||||||
| try { | ||||||||||||||||||
| const job = await jobRegistry.getJob(req.params.id); | ||||||||||||||||||
| if (!job) { | ||||||||||||||||||
| const { rows } = await client.query("SELECT * FROM jobs WHERE id = $1", [req.params.id]); | ||||||||||||||||||
| if (rows.length === 0) { | ||||||||||||||||||
| return res.status(404).json({ error: "Background task not found." }); | ||||||||||||||||||
| } | ||||||||||||||||||
| if (job.userId !== req.user!.id) { | ||||||||||||||||||
| return res.status(403).json({ error: "Access denied. Multi-tenant boundary constraint rule." }); | ||||||||||||||||||
| } | ||||||||||||||||||
| res.json(job); | ||||||||||||||||||
| res.json(rows[0]); | ||||||||||||||||||
| } catch (err: any) { | ||||||||||||||||||
| res.status(500).json({ error: "Failed to fetch job details" }); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,16 +4,11 @@ import { withTransaction } from "../utils/dbUtils.js"; | |||||||||||||
| import crypto from "crypto"; | ||||||||||||||
|
|
||||||||||||||
| export const getLibraryItems = async (req: Request, res: Response) => { | ||||||||||||||
| const userId = req.user!.id; | ||||||||||||||
| const userRole = req.user!.role; | ||||||||||||||
| const client = req.dbClient || pool; | ||||||||||||||
| try { | ||||||||||||||
| const rows = await withTransaction(userId, userRole, async (client) => { | ||||||||||||||
| const { rows } = await client.query( | ||||||||||||||
| "SELECT * FROM library_items WHERE user_id = $1 ORDER BY created_at DESC", | ||||||||||||||
| [userId] | ||||||||||||||
| ); | ||||||||||||||
| return rows; | ||||||||||||||
| }); | ||||||||||||||
| const { rows } = await client.query( | ||||||||||||||
| "SELECT * FROM library_items WHERE user_id = current_setting('app.current_user_id', true) ORDER BY created_at DESC" | ||||||||||||||
| ); | ||||||||||||||
|
Comment on lines
+9
to
+11
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. This owner filter disables the admin override for library items.
Suggested fix- const { rows } = await client.query(
- "SELECT * FROM library_items WHERE user_id = current_setting('app.current_user_id', true) ORDER BY created_at DESC"
- );
+ const { rows } = await client.query(
+ "SELECT * FROM library_items ORDER BY created_at DESC"
+ );📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| res.json(rows); | ||||||||||||||
| } catch (err: any) { | ||||||||||||||
| res.status(500).json({ error: err.message }); | ||||||||||||||
|
|
@@ -24,9 +19,10 @@ export const createLibraryItem = async (req: Request, res: Response) => { | |||||||||||||
| const { type, name, description, tags, details } = req.body; | ||||||||||||||
| const userId = req.user!.id; | ||||||||||||||
| const id = "lib_" + crypto.randomUUID(); | ||||||||||||||
| const client = req.dbClient || pool; | ||||||||||||||
|
|
||||||||||||||
| try { | ||||||||||||||
| const { rows } = await pool.query( | ||||||||||||||
| const { rows } = await client.query( | ||||||||||||||
| "INSERT INTO library_items (id, user_id, type, name, description, tags, details) VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING *", | ||||||||||||||
| [id, userId, type, name, description, tags, details] | ||||||||||||||
| ); | ||||||||||||||
|
|
@@ -37,11 +33,11 @@ export const createLibraryItem = async (req: Request, res: Response) => { | |||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| export const deleteLibraryItem = async (req: Request, res: Response) => { | ||||||||||||||
| const userId = req.user!.id; | ||||||||||||||
| const client = req.dbClient || pool; | ||||||||||||||
| try { | ||||||||||||||
| const result = await pool.query( | ||||||||||||||
| "DELETE FROM library_items WHERE id = $1 AND user_id = $2", | ||||||||||||||
| [req.params.id, userId] | ||||||||||||||
| const result = await client.query( | ||||||||||||||
| "DELETE FROM library_items WHERE id = $1", | ||||||||||||||
| [req.params.id] | ||||||||||||||
| ); | ||||||||||||||
| if (result.rowCount === 0) return res.status(404).json({ error: "Item not found." }); | ||||||||||||||
| res.json({ success: true }); | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Background analysis updates can silently no-op without a request-scoped client.
When
dbClientis absent, this predicate depends oncurrent_setting(...); in non-request paths it can evaluate false, soUPDATE filesaffects 0 rows.backend/src/services/jobQueue.tsLine 286 callsrunAnalysis(documentId, content, userId)withoutdbClient, so jobs can report success while analysis is never persisted.Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents