Phase 1: Security Hardening & Global RLS Enforcement#26
Merged
aish-am22 merged 3 commits intoJun 7, 2026
Conversation
- Refactored `authenticateToken` middleware to enforce Row Level Security (RLS) on every request by setting `app.current_user_id` and `app.current_user_role` session variables. - Migrated all database queries in `documents`, `folders`, `libraryItems`, `jobs`, and `admin` controllers to use the RLS-enforced `req.dbClient`. - Updated `AgentOrchestrator` to support multi-tenant isolation via session-scoped clients. - Removed redundant `withTransaction` blocks in favor of the unified middleware-driven transaction management. Co-authored-by: aish-am22 <146462239+aish-am22@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR implements per-request Row-Level Security (RLS) via PostgreSQL session variables, migrates all controllers and agents to request-scoped database clients, adds system settings infrastructure, validates agent output with Zod schemas, and integrates real-time job tracking in the frontend via Server-Sent Events. Branding updates and test data cleanup finalize the migration. ChangesRLS Migration and Settings System
Sequence Diagram(s)sequenceDiagram
participant Request as HTTP Request
participant Auth as authenticateToken
participant DB as PostgreSQL
participant Controller as Controller
participant Orch as Orchestrator
Request->>Auth: JWT token
Auth->>Auth: Verify JWT & fetch user
Auth->>DB: pool.connect()
DB-->>Auth: PoolClient
Auth->>Auth: set_config(app.current_user_id)
Auth->>Auth: set_config(app.current_user_role)
Auth->>Request: req.dbClient = PoolClient
Request->>Controller: Route handler
Controller->>DB: client.query(WHERE current_setting(...))
DB-->>Controller: Filtered results
Controller->>Orch: Invoke with req.dbClient
Orch->>DB: RLS-filtered document access
DB-->>Orch: Authorized data
Orch-->>Controller: Result
Controller-->>Request: Response
Request->>Auth: res.finish
Auth->>DB: COMMIT & release client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/agents/legalAgent.ts`:
- Around line 39-42: The UPDATE can silently no-op when current_setting(...) is
unavailable; change the persistence path to require or use a request-scoped DB
client and explicit creator id instead of relying on current_setting.
Concretely, update runAnalysis (and any callers such as jobQueue.runAnalysis) to
accept a dbClient parameter (or throw if not provided), and in legalAgent.ts
replace the client/query call to use that dbClient and a creator_id parameter
(use the userId passed into runAnalysis) in the WHERE clause rather than
current_setting('app.current_user_id', true); ensure callers pass the
request-scoped client through so the UPDATE affects rows as intended.
In `@backend/src/controllers/documents.ts`:
- Around line 39-40: The single-row fetch for the document in the documents
controller is failing for users who have access via shared_with because the SQL
only checks id and relies on RLS that restricts to creator_id/current user;
update the query used in the document fetch handler (the SELECT against the
files table in this controller) to also allow rows where the current user is
listed in the file's shared access (e.g. add an OR condition using
current_setting('app.current_user_id', true)::uuid = ANY(shared_with) or an
EXISTS(...) join against the shared access table), so the SQL returns the row
when the requester is in shared_with and not just the creator or admin;
reference the same identifying symbols "files", "shared_with", and
current_setting('app.current_user_id', true) when making the change.
- Around line 15-18: The SQL in the client.query call in documents.ts
re-introduces a creator_id check that overrides the intended RLS ADMIN behavior;
edit the query string passed to client.query (the one currently assigning to
docs) to remove the "creator_id = current_setting('app.current_user_id', true)
OR" clause so the SQL relies on RLS for owner visibility and only includes the
shared_with::jsonb @> $1::jsonb condition (keep the same parameter array
[JSON.stringify([userEmail])] and ordering), ensuring client.query and the docs
result handling remain unchanged.
In `@backend/src/controllers/folders.ts`:
- Around line 9-11: The query in the folders controller hard-codes "user_id =
current_setting('app.current_user_id', true)" which overrides RLS-based admin
visibility; update the SQL used in the client.query call in the folders
controller (the SELECT in backend/src/controllers/folders.ts) to remove the
owner filter and simply select from folders (e.g., "SELECT * FROM folders ORDER
BY created_at DESC") so RLS/verification/verifyRLS.ts can correctly widen
visibility for admins; locate the client.query invocation in the get/list
folders handler and replace the SQL string accordingly without adding an
explicit user_id predicate.
In `@backend/src/controllers/jobs.ts`:
- Around line 8-11: The current client.query call that selects from jobs using
the hardcoded WHERE clause "WHERE user_id =
current_setting('app.current_user_id', true)" prevents the RLS admin override
from taking effect; update the query in the function calling client.query (the
SELECT in backend/src/controllers/jobs.ts) to remove that WHERE filter so RLS
policies (including the ADMIN override) can control visibility (i.e., use a
plain "SELECT * FROM jobs ORDER BY created_at DESC" or an equivalent that does
not restrict by user_id in the SQL).
In `@backend/src/controllers/libraryItems.ts`:
- Around line 9-11: The query in libraryItems controller explicitly restricts
rows with "user_id = current_setting('app.current_user_id', true)", which
prevents ADMIN sessions from seeing all rows and overrides RLS; remove that
explicit owner filter from the client.query call (leave the SELECT * FROM
library_items ORDER BY created_at DESC) so Postgres RLS and the ADMIN role
policy can enforce visibility, or if you must conditionally restrict for
non-admins, apply the user_id filter only when the current session role is not
ADMIN (check current_setting('app.current_user_role', true) or the equivalent
session flag) before composing the client.query.
In `@backend/src/middleware/auth.ts`:
- Around line 80-107: The middleware currently always runs a COMMIT in the
res.on("finish") handler which can persist failed requests and race with
res.on("close"); change the logic so the middleware only commits when the
response is successful (e.g., res.statusCode < 400) and ensure commit/rollback
runs only once by guarding with a per-request flag (e.g.,
req.transactionHandled) or by using a single cleanup function registered once
for both finish/close; keep using req.dbClient and still catch and ignore errors
from commit/rollback but always release the client and set req.dbClient =
undefined in the same guarded finally block to avoid races (references:
res.on("finish"), res.on("close"), req.dbClient, uploadDocument/addJobToQueue).
- Around line 73-78: In authenticateToken: start the DB transaction before
calling set_config (call client.query("BEGIN") first, then call set_config with
is_local=true) so the per-transaction session vars apply to controller queries;
replace the dual res.on("finish")/res.on("close") logic with a single idempotent
finalize function (referencing req.dbClient and the existing listener setup)
that runs exactly once, checks res.statusCode (or success condition) to decide
COMMIT vs ROLLBACK, wraps COMMIT/ROLLBACK in try/catch, releases the client, and
removes both listeners to avoid race conditions.
In `@verification/verifyRLS.ts`:
- Around line 15-21: The setup INSERTs using client.query (creating users and
folders with userId1/userId2 and ids 'f1'..'f3') run outside the
transaction/rollback and are not deleted in finally, leaving fixtures behind;
fix by moving these INSERTs into the same transaction scope as the verification
(wrap with client.query('BEGIN') before the INSERTs and ensure
client.query('ROLLBACK') in the finally block) or explicitly delete those rows
in the finally (DELETE FROM users WHERE email IN
('user1@test.com','user2@test.com') and DELETE FROM folders WHERE id IN
('f1','f2','f3')) so that users/folders created by the setup (references:
client.query, userId1, userId2, folders, users, finally) are always cleaned up
even on success or error.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6a00592-c714-4359-acf3-f10e1d8627ea
📒 Files selected for processing (10)
backend/src/agents/legalAgent.tsbackend/src/controllers/admin.tsbackend/src/controllers/documents.tsbackend/src/controllers/folders.tsbackend/src/controllers/jobs.tsbackend/src/controllers/libraryItems.tsbackend/src/middleware/auth.tsbackend/src/routes/analyze.tsbackend/src/services/jobQueue.tsverification/verifyRLS.ts
Comment on lines
+39
to
42
| await client.query( | ||
| "UPDATE files SET analysis = $1 WHERE id = $2 AND (creator_id = current_setting('app.current_user_id', true) OR current_setting('app.current_user_role', true) = 'ADMIN')", | ||
| [JSON.stringify(result), documentId] | ||
| ); |
There was a problem hiding this comment.
Background analysis updates can silently no-op without a request-scoped client.
When dbClient is absent, this predicate depends on current_setting(...); in non-request paths it can evaluate false, so UPDATE files affects 0 rows. backend/src/services/jobQueue.ts Line 286 calls runAnalysis(documentId, content, userId) without dbClient, so jobs can report success while analysis is never persisted.
Proposed fix
- await client.query(
- "UPDATE files SET analysis = $1 WHERE id = $2 AND (creator_id = current_setting('app.current_user_id', true) OR current_setting('app.current_user_role', true) = 'ADMIN')",
- [JSON.stringify(result), documentId]
- );
+ const updateRes = dbClient
+ ? await client.query(
+ "UPDATE files SET analysis = $1 WHERE id = $2 AND (creator_id = current_setting('app.current_user_id', true) OR current_setting('app.current_user_role', true) = 'ADMIN')",
+ [JSON.stringify(result), documentId]
+ )
+ : await client.query(
+ "UPDATE files SET analysis = $1 WHERE id = $2 AND creator_id = $3",
+ [JSON.stringify(result), documentId, userId]
+ );
+
+ if (updateRes.rowCount === 0) {
+ throw new Error("Analysis update denied or target document not found.");
+ }📝 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
| await client.query( | |
| "UPDATE files SET analysis = $1 WHERE id = $2 AND (creator_id = current_setting('app.current_user_id', true) OR current_setting('app.current_user_role', true) = 'ADMIN')", | |
| [JSON.stringify(result), documentId] | |
| ); | |
| const updateRes = dbClient | |
| ? await client.query( | |
| "UPDATE files SET analysis = $1 WHERE id = $2 AND (creator_id = current_setting('app.current_user_id', true) OR current_setting('app.current_user_role', true) = 'ADMIN')", | |
| [JSON.stringify(result), documentId] | |
| ) | |
| : await client.query( | |
| "UPDATE files SET analysis = $1 WHERE id = $2 AND creator_id = $3", | |
| [JSON.stringify(result), documentId, userId] | |
| ); | |
| if (updateRes.rowCount === 0) { | |
| throw new Error("Analysis update denied or target document not found."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/agents/legalAgent.ts` around lines 39 - 42, The UPDATE can
silently no-op when current_setting(...) is unavailable; change the persistence
path to require or use a request-scoped DB client and explicit creator id
instead of relying on current_setting. Concretely, update runAnalysis (and any
callers such as jobQueue.runAnalysis) to accept a dbClient parameter (or throw
if not provided), and in legalAgent.ts replace the client/query call to use that
dbClient and a creator_id parameter (use the userId passed into runAnalysis) in
the WHERE clause rather than current_setting('app.current_user_id', true);
ensure callers pass the request-scoped client through so the UPDATE affects rows
as intended.
Comment on lines
+15
to
+18
| 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])] | ||
| ); |
There was a problem hiding this comment.
This list query narrows away the RLS admin override.
The files policy in backend/src/config/initDb.ts already widens visibility for ADMIN. Re-adding creator_id = current_setting('app.current_user_id', true) here means admin sessions only see their own documents (plus anything explicitly shared to their email), not the full tenant-spanning view that the new RLS layer is supposed to provide.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/controllers/documents.ts` around lines 15 - 18, The SQL in the
client.query call in documents.ts re-introduces a creator_id check that
overrides the intended RLS ADMIN behavior; edit the query string passed to
client.query (the one currently assigning to docs) to remove the "creator_id =
current_setting('app.current_user_id', true) OR" clause so the SQL relies on RLS
for owner visibility and only includes the shared_with::jsonb @> $1::jsonb
condition (keep the same parameter array [JSON.stringify([userEmail])] and
ordering), ensuring client.query and the docs result handling remain unchanged.
Comment on lines
+39
to
40
| const { rows } = await client.query("SELECT * FROM files WHERE id = $1", [req.params.id]); | ||
| if (rows.length > 0) { |
There was a problem hiding this comment.
Shared documents will now 404 for non-owners.
The old application-side shared-with authorization check is gone, but the current files RLS policy only allows creator_id = current_setting('app.current_user_id', true) or ADMIN. That means users who were granted access via shared_with can no longer load document details here, and the shared_with branch in getDocuments cannot recover those rows on an RLS-scoped client either.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/controllers/documents.ts` around lines 39 - 40, The single-row
fetch for the document in the documents controller is failing for users who have
access via shared_with because the SQL only checks id and relies on RLS that
restricts to creator_id/current user; update the query used in the document
fetch handler (the SELECT against the files table in this controller) to also
allow rows where the current user is listed in the file's shared access (e.g.
add an OR condition using current_setting('app.current_user_id', true)::uuid =
ANY(shared_with) or an EXISTS(...) join against the shared access table), so the
SQL returns the row when the requester is in shared_with and not just the
creator or admin; reference the same identifying symbols "files", "shared_with",
and current_setting('app.current_user_id', true) when making the change.
Comment on lines
+9
to
+11
| const { rows } = await client.query( | ||
| "SELECT * FROM folders WHERE user_id = current_setting('app.current_user_id', true) ORDER BY created_at DESC" | ||
| ); |
There was a problem hiding this comment.
This owner filter disables the admin override for folders.
verification/verifyRLS.ts validates admin access by doing a plain SELECT on folders and letting the RLS policy widen visibility. Hard-coding user_id = current_setting('app.current_user_id', true) here narrows admins back to their own 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
‼️ 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
| 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" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/controllers/folders.ts` around lines 9 - 11, The query in the
folders controller hard-codes "user_id = current_setting('app.current_user_id',
true)" which overrides RLS-based admin visibility; update the SQL used in the
client.query call in the folders controller (the SELECT in
backend/src/controllers/folders.ts) to remove the owner filter and simply select
from folders (e.g., "SELECT * FROM folders ORDER BY created_at DESC") so
RLS/verification/verifyRLS.ts can correctly widen visibility for admins; locate
the client.query invocation in the get/list folders handler and replace the SQL
string accordingly without adding an explicit user_id predicate.
Comment on lines
+8
to
+11
| // 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" | ||
| ); |
There was a problem hiding this comment.
This query defeats the RLS admin override for jobs.
The new jobs policy already uses app.current_user_id plus the ADMIN override. Keeping WHERE user_id = current_setting('app.current_user_id', true) here means admin requests can only list their own jobs, not the broader view the RLS layer is configured to allow.
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
‼️ 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
| // 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" | |
| ); | |
| // Phase 1: Security Hardening - use RLS client | |
| const { rows } = await client.query( | |
| "SELECT * FROM jobs ORDER BY created_at DESC" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/controllers/jobs.ts` around lines 8 - 11, The current
client.query call that selects from jobs using the hardcoded WHERE clause "WHERE
user_id = current_setting('app.current_user_id', true)" prevents the RLS admin
override from taking effect; update the query in the function calling
client.query (the SELECT in backend/src/controllers/jobs.ts) to remove that
WHERE filter so RLS policies (including the ADMIN override) can control
visibility (i.e., use a plain "SELECT * FROM jobs ORDER BY created_at DESC" or
an equivalent that does not restrict by user_id in the SQL).
Comment on lines
+9
to
+11
| const { rows } = await client.query( | ||
| "SELECT * FROM library_items WHERE user_id = current_setting('app.current_user_id', true) ORDER BY created_at DESC" | ||
| ); |
There was a problem hiding this comment.
This owner filter disables the admin override for library items.
library_items is part of the same RLS policy set that grants ADMIN broader visibility. Adding user_id = current_setting('app.current_user_id', true) here narrows admin sessions back to their own rows.
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
‼️ 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
| 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" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/controllers/libraryItems.ts` around lines 9 - 11, The query in
libraryItems controller explicitly restricts rows with "user_id =
current_setting('app.current_user_id', true)", which prevents ADMIN sessions
from seeing all rows and overrides RLS; remove that explicit owner filter from
the client.query call (leave the SELECT * FROM library_items ORDER BY created_at
DESC) so Postgres RLS and the ADMIN role policy can enforce visibility, or if
you must conditionally restrict for non-admins, apply the user_id filter only
when the current session role is not ADMIN (check
current_setting('app.current_user_role', true) or the equivalent session flag)
before composing the client.query.
Comment on lines
+80
to
+107
| // Ensure client is released back to pool when response finishes | ||
| res.on("finish", async () => { | ||
| if (req.dbClient) { | ||
| try { | ||
| // Commit if not already rolled back/committed by controller | ||
| await req.dbClient.query("COMMIT"); | ||
| } catch (e) { | ||
| // Ignore if transaction already finished | ||
| } finally { | ||
| req.dbClient.release(); | ||
| req.dbClient = undefined; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // Handle cases where the connection might close prematurely | ||
| res.on("close", async () => { | ||
| if (req.dbClient) { | ||
| try { | ||
| await req.dbClient.query("ROLLBACK"); | ||
| } catch (e) { | ||
| // Ignore | ||
| } finally { | ||
| req.dbClient.release(); | ||
| req.dbClient = undefined; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant files
ls -la backend/src/middleware/auth.ts backend/src/controllers/documents.ts
# Show auth middleware section around specified lines
sed -n '1,200p' backend/src/middleware/auth.ts | nl -ba | sed -n '60,140p'
# Search for uploadDocument implementation
rg -n "uploadDocument" backend/src/controllers/documents.ts
# Show around uploadDocument
sed -n '1,260p' backend/src/controllers/documents.ts | nl -ba | sed -n '1,260p'
# Show any addJobToQueue usage and error handling in uploadDocument
rg -n "addJobToQueue|queue|files" backend/src/controllers/documents.tsRepository: aish-am22/CookieCare
Length of output: 291
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la backend/src/middleware/auth.ts backend/src/controllers/documents.ts || true
echo "---- auth.ts lines 60-140 ----"
sed -n '1,220p' backend/src/middleware/auth.ts | nl -ba | sed -n '60,140p'
echo "---- uploadDocument search ----"
rg -n "uploadDocument" backend/src/controllers/documents.ts
echo "---- documents.ts first 260 lines ----"
sed -n '1,260p' backend/src/controllers/documents.ts | nl -ba | sed -n '1,260p'
echo "---- documents.ts relevant patterns ----"
rg -n "addJobToQueue|ROLLBACK|COMMIT|files\b|INSERT INTO|queue" backend/src/controllers/documents.tsRepository: aish-am22/CookieCare
Length of output: 322
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -laRepository: aish-am22/CookieCare
Length of output: 1285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Listing files..."
ls -la backend/src/middleware/auth.ts backend/src/controllers/documents.ts
echo "---- auth.ts excerpt ----"
sed -n '60,140p' backend/src/middleware/auth.ts | nl -ba
echo "---- documents.ts: find uploadDocument ----"
rg -n "uploadDocument" backend/src/controllers/documents.ts
echo "---- documents.ts excerpt around uploadDocument (if found) ----"
# Try to infer line numbers by searching for function signature
line=$(rg -n "uploadDocument" backend/src/controllers/documents.ts | head -n1 | cut -d: -f1)
if [ -n "${line:-}" ]; then
start=$((line-80))
end=$((line+140))
sed -n "${start},${end}p" backend/src/controllers/documents.ts | nl -ba
fi
echo "---- documents.ts search for addJobToQueue, files insert ----"
rg -n "addJobToQueue|INSERT|files\b" backend/src/controllers/documents.tsRepository: aish-am22/CookieCare
Length of output: 334
🏁 Script executed:
echo testRepository: aish-am22/CookieCare
Length of output: 69
🏁 Script executed:
pwd
lsRepository: aish-am22/CookieCare
Length of output: 253
🏁 Script executed:
echo testRepository: aish-am22/CookieCare
Length of output: 69
🏁 Script executed:
ls -laRepository: aish-am22/CookieCare
Length of output: 1285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print with line numbers using awk (since nl is unavailable)
print_range() {
local file="$1"
local start="$2"
local end="$3"
echo "---- $file lines $start-$end ----"
sed -n "${start},${end}p" "$file" | awk '{printf "%4d\t%s\n", NR+'$start'-1, $0}'
}
print_range backend/src/middleware/auth.ts 60 140
# Find uploadDocument and print a surrounding window
upload_line="$(rg -n "function\s+uploadDocument|uploadDocument\s*\(" backend/src/controllers/documents.ts | head -n1 | cut -d: -f1 || true)"
echo "---- uploadDocument candidate line: ${upload_line:-none} ----"
if [ -n "${upload_line:-}" ]; then
start=$((upload_line-120))
end=$((upload_line+220))
# clamp start to >=1
if [ "$start" -lt 1 ]; then start=1; fi
print_range backend/src/controllers/documents.ts "$start" "$end"
fi
# Show any insertion into files and calls to addJobToQueue/queueing
echo "---- documents.ts mentions: files insert / addJobToQueue / queue ----"
rg -n "addJobToQueue|INSERT\s+INTO\s+files|files\b\s*\(|create.*file|uploadDocument" backend/src/controllers/documents.tsRepository: aish-am22/CookieCare
Length of output: 3730
🏁 Script executed:
#!/bin/bash
set -euo pipefail
print_range() {
local file="$1"; local start="$2"; local end="$3"
echo "---- $file lines $start-$end ----"
sed -n "${start},${end}p" "$file" | awk '{printf "%4d\t%s\n", NR+'$start'-1, $0}'
}
# Print uploadDocument section
print_range backend/src/controllers/documents.ts 60 170
# Show any dbClient usage inside documents.ts
echo "---- documents.ts: req.dbClient usages ----"
rg -n "req\.dbClient" backend/src/controllers/documents.ts || true
# Search other controllers for COMMIT/ROLLBACK explicit usage
echo "---- backend/src/controllers: explicit COMMIT/ROLLBACK ----"
rg -n "\b(COMMIT|ROLLBACK)\b" backend/src/controllers || trueRepository: aish-am22/CookieCare
Length of output: 5879
🏁 Script executed:
#!/bin/bash
set -euo pipefail
print_range() {
local file="$1"; local start="$2"; local end="$3"
echo "---- $file lines $start-$end ----"
sed -n "${start},${end}p" "$file" | awk '{printf "%4d\t%s\n", NR+'$start'-1, $0}'
}
print_range backend/src/controllers/documents.ts 70 150
echo "---- documents.ts: req.dbClient usages ----"
rg -n "req\.dbClient" backend/src/controllers/documents.ts || true
echo "---- backend/src/controllers: explicit COMMIT/ROLLBACK ----"
rg -n "\b(COMMIT|ROLLBACK)\b" backend/src/controllers || trueRepository: aish-am22/CookieCare
Length of output: 4438
Unconditional COMMIT on res.finish can persist failed requests (and race with res.close).
backend/src/middleware/auth.tsregistersres.on("finish")to always runCOMMIT(whenreq.dbClientis set), even if the controller already caught an error and returned a 500.backend/src/controllers/documents.tsuploadDocumentinserts intofilesand only then callsaddJobToQueue; ifaddJobToQueuethrows, the catch returns 500 but the middlewarefinishhandler still commits that INSERT.backend/src/middleware/auth.tsalso registersres.on("close")to runROLLBACK;finishandclosecan race on the same client beforereq.dbClientis cleared.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/middleware/auth.ts` around lines 80 - 107, The middleware
currently always runs a COMMIT in the res.on("finish") handler which can persist
failed requests and race with res.on("close"); change the logic so the
middleware only commits when the response is successful (e.g., res.statusCode <
400) and ensure commit/rollback runs only once by guarding with a per-request
flag (e.g., req.transactionHandled) or by using a single cleanup function
registered once for both finish/close; keep using req.dbClient and still catch
and ignore errors from commit/rollback but always release the client and set
req.dbClient = undefined in the same guarded finally block to avoid races
(references: res.on("finish"), res.on("close"), req.dbClient,
uploadDocument/addJobToQueue).
Phase 1 (Security): - Implemented global RLS middleware in `auth.ts` to enforce tenant isolation via `app.current_user_id`. - Refactored all backend controllers to use RLS-enforced `req.dbClient`. Phase 2 (Functionality): - Replaced `handleFileUploadSimulated` in `AskAILawyer.tsx` with real multi-agent indexing pipeline. - Implemented functional Font, Size, and Paragraph controls in `DraftAgreement.tsx` editor toolbar. - Moved hardcoded jurisdictions and sources to a managed `system_settings` table. Phase 3 (Agentic Resilience): - Deepened the Drafting Critic loop to 5 iterations for improved legal accuracy. - Enforced strict structured JSON output in AI agents using `zod` schema validation. - Improved error handling for LLM hallucinations in RAG re-ranking. Phase 4 (Final Polish): - Deleted legacy prototype artifacts (`db.json`, `metadata.json`). - Purged all residual `CookieCare` branding from the source code and configuration. - Standardized real-time task tracking on SSE, removing all legacy `setTimeout` polling fallbacks. Co-authored-by: aish-am22 <146462239+aish-am22@users.noreply.github.com>
- **Phase 1 (Security):** Implemented global RLS middleware and migrated all controllers to use RLS-enforced `req.dbClient`. Added RLS policies for all modules including `agent_execution_logs` and `system_settings`. - **Phase 2 (Functionality):** Replaced all mock/simulated upload handlers with real RAG-connected pipelines. Implemented functional editor toolbar dropdowns. Integrated `system_settings` for dynamic jurisdictions and sources. - **Phase 3 (Resilience):** Deepened Drafting Critic loop to 5 iterations. Enforced strict structured JSON output for AI Agents using Zod schemas for validation. Added performance indexes (including HNSW) to the database schema. - **Phase 4 (Polish):** Deleted legacy artifacts (`db.json`, `metadata.json`). Purged all residual `CookieCare` branding. Standardized all background task tracking on SSE (removed `setTimeout` polling). - **Hardening:** Refactored `authenticateToken` middleware for robust connection pool lifecycle management and transaction handling. Passed final static analysis and build checks. Co-authored-by: aish-am22 <146462239+aish-am22@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/CookieScanner.tsx (1)
100-102:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
finallyblock prematurely resets scanning state for SSE path.When the scan returns HTTP 202 with
job_id, the SSE path is taken but the function continues to thefinallyblock, which immediately callssetScanning(false). This hides the loading indicator while SSE is still waiting for job completion. The SSE callbacks already handlesetScanning(false)on completion/failure.🐛 Proposed fix - restructure to avoid finally for SSE path
if (res.status === 202 && data.job_id) { // SSE for real-time progress - standardized const eventSource = new EventSource(apiUrl(`/api/jobs/sse?token=${authToken}`)); // ... onmessage and onerror handlers ... + return; // Exit - SSE handles completion } else { setResult(data); setScanning(false); } } catch (err: any) { setError(err.message || "An unexpected error occurred during privacy web audit."); - } finally { setScanning(false); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/CookieScanner.tsx` around lines 100 - 102, The finally block calling setScanning(false) is clearing the loading state even when the SSE path (HTTP 202 with job_id) is taken; update the logic in the function that handles the fetch response so that you only call setScanning(false) for non-SSE paths (e.g., after successful immediate scan or on catch failure) and return early when response.status === 202 or job_id is present so the finally block is not reached for SSE; alternatively remove the global finally and place setScanning(false) explicitly in the non-SSE success and error branches, leaving the SSE callbacks (EventSource onmessage/onerror/onclose) responsible for clearing setScanning(false). Ensure you reference setScanning, response.status/job_id, and the EventSource callbacks (onmessage/onerror/onclose) when making the change.backend/src/agents/analysisAgent.ts (1)
94-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
heuristicAuditoutput drifts fromAuditSchemaand bypasses validation.The fallback path returns data directly without Zod validation, creating two problems:
Extra field not in schema:
non_compliance_tag(line 103) is not declared inAuditSchema, so consumers usingz.infer<typeof AuditSchema>won't see it in types but it exists at runtime.Validation bypass: If
heuristicAuditreturns data violating schema constraints (e.g., wrong severity value), it won't be caught, breaking the contract thatrunAuditreturns validated data.Either validate the heuristic output or align
heuristicAuditwith the schema and add it to the type.Proposed fix: validate heuristic output and align with schema
private heuristicAudit(content: string, type: string) { const risks = []; if (content.toLowerCase().includes("liquidated damages")) { risks.push({ id: "h_risk_1", clause: "Liquidated damages clause detected", severity: "high", risk_level: "CRITICAL", reasons: ["Potential uncapped liability"], - non_compliance_tag: "UNCAPPED_LIABILITY", description: "Static penalties can be punitive.", - actionableInsight: "Negotiate for direct proven damages." + actionableInsight: "Negotiate for direct proven damages.", + remediation: "Replace liquidated damages with direct proven damages clause." }); } - return { + const result = { summary: "Heuristic audit completed.", risks, complianceGaps: [] }; + return AuditSchema.parse(result); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/agents/analysisAgent.ts` around lines 94 - 113, The heuristicAudit fallback returns objects that diverge from AuditSchema (extra field non_compliance_tag and unchecked enum values) and thus bypasses validation; update heuristicAudit to produce only fields declared by AuditSchema (rename/remove non_compliance_tag, ensure severity/risk_level use allowed enum values and structure matches AuditSchema) and then run AuditSchema.parse(...) (or safeParse) on the final return value before returning from heuristicAudit (or at the runAudit caller) so the heuristic output is validated and any violations are caught; reference heuristicAudit, AuditSchema, and runAudit when making these changes.
🧹 Nitpick comments (3)
src/components/DraftAgreement.tsx (2)
1754-1768: ⚡ Quick winInconsistent formatting logic and confusing paragraph option.
Inconsistency: The h1 and h2 options call
handleToolbarFormat, but the subtitle option directly callsinsertTextAtCursor("\n### ", "\n"). For consistency, subtitle should also usehandleToolbarFormat.Confusing paragraph option: The "p" (Paragraph) option only inserts newlines (
\n\n) without any actual paragraph markup. This doesn't match user expectations for a "Paragraph" format option.♻️ Suggested improvements for consistency
Add an h3 case to
handleToolbarFormat(around line 520):const handleToolbarFormat = (action: string) => { if (action === "h1") insertTextAtCursor("\n# ", "\n"); else if (action === "h2") insertTextAtCursor("\n## ", "\n"); + else if (action === "h3") insertTextAtCursor("\n### ", "\n"); else if (action === "bold") insertTextAtCursor("**", "**");Then update the paragraph dropdown to use it consistently:
<select onChange={(e) => { const val = e.target.value; if (val === "p") insertTextAtCursor("\n", "\n"); else if (val === "h1") handleToolbarFormat("h1"); else if (val === "h2") handleToolbarFormat("h2"); - else if (val === "subtitle") insertTextAtCursor("\n### ", "\n"); + else if (val === "subtitle") handleToolbarFormat("h3"); }}Consider either removing the "Paragraph" option or making it insert actual paragraph markup consistent with your rendering system.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/DraftAgreement.tsx` around lines 1754 - 1768, The select's formatting is inconsistent: update handleToolbarFormat to support an "h3"/"subtitle" case (add logic inside handleToolbarFormat to insert "\n### " or apply subtitle formatting) and change the select's subtitle option to call handleToolbarFormat("h3" or "subtitle") instead of directly calling insertTextAtCursor; also decide whether the "p" option should be removed or wired to handleToolbarFormat("p") so it applies a consistent paragraph action (e.g., insert "\n\n" or whatever your renderer expects) rather than directly mutating via insertTextAtCursor.
1734-1742: ⚡ Quick winDropdown does not reset after selection, causing confusing UX.
The font dropdown lacks a
valueprop, so after the user selects a font, the dropdown remains on that selection. This creates a confusing experience:
- User selects "Inter" → markup inserted → dropdown still shows "Inter"
- User wants to apply "Mono" elsewhere → selects "Mono" again → nothing appears to happen if already on "Mono"
The same issue affects the size and paragraph dropdowns below.
♻️ Recommended fix to reset dropdown after selection
Make the select controlled and reset to default after each change:
+const [fontDropdownValue, setFontDropdownValue] = useState(""); + <select + value={fontDropdownValue} - onChange={(e) => insertTextAtCursor(`[font:${e.target.value}]`, "[/font]")} + onChange={(e) => { + const val = e.target.value; + if (val) { + insertTextAtCursor(`[font:${val}]`, "[/font]"); + setFontDropdownValue(""); // Reset to placeholder + } + }} className="bg-slate-50 border border-gray-200 text-gray-700 text-xs rounded-md px-1 py-1 focus:outline-none cursor-pointer" > - <option value="sans">Default</option> + <option value="">Font</option> + <option value="sans">Sans (Default)</option> <option value="inter">Inter</option> <option value="mono">Fira Code</option> <option value="jetbrains">JetBrains Mono</option> </select>Apply the same pattern to the size and paragraph dropdowns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/DraftAgreement.tsx` around lines 1734 - 1742, The font select in DraftAgreement.tsx is uncontrolled and stays on the chosen option; make it controlled by adding local state (e.g., fontSelectValue) for the <select> used with insertTextAtCursor, bind that state to the select's value, and in the onChange handler call insertTextAtCursor(...) then reset the state back to the default (e.g., "sans") so the dropdown visually returns to Default; apply the same controlled-state + reset pattern to the size and paragraph <select> elements as well.backend/src/routes/lawyer.ts (1)
10-13: 💤 Low valueConsider stronger typing for the
clientparameter.Using
anyfor the database client loses type safety. Consider typing it asPool | PoolClientfrom thepgpackage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/routes/lawyer.ts` around lines 10 - 13, The getSystemSettings function currently types the database client as any; change its signature to accept a typed PG client (e.g., Pool | PoolClient from the 'pg' package) by importing those types and updating the client parameter type in getSystemSettings to improve type safety and IDE/autocomplete support; ensure any callers pass a compatible Pool or PoolClient and update related usage if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/config/index.ts`:
- Line 10: The config currently sets jwtSecret with a hardcoded fallback
("privsec-ai-enterprise-secret-2026") via process.env.JWT_SECRET || "...", which
is insecure; remove the fallback so jwtSecret is assigned only from
process.env.JWT_SECRET and fail fast if it's missing (throw an error or exit
during app startup) in the module that exports jwtSecret (referencing the
jwtSecret variable and process.env.JWT_SECRET), and update deployment/config
docs to ensure a strong, cryptographically-generated JWT_SECRET is provided in
all environments.
In `@backend/src/config/initDb.ts`:
- Around line 177-183: RLS for table system_settings only creates a SELECT
policy (system_settings_read_policy) so write operations are blocked; add
explicit admin write policies by dropping any existing write policy and creating
a new policy (eg. system_settings_write_policy) that allows INSERT, UPDATE and
DELETE for admins — use CREATE POLICY ... FOR INSERT, UPDATE, DELETE ON
system_settings USING (current_setting('app.current_user_role', true) = 'admin')
WITH CHECK (current_setting('app.current_user_role', true) = 'admin') (or the
project’s existing admin flag setting name), ensuring INSERT/UPDATE use WITH
CHECK and DELETE/UPDATE use USING so admins can modify rows while RLS remains
enabled.
In `@backend/src/routes/lawyer.ts`:
- Around line 75-79: The variable dbSources is fetched but never used; update
the logic in the handler that calls getSystemSettings('web_discovery_sources',
client) so you either (A) remove the unused call to getSystemSettings and the db
client fallback (req.dbClient || pool) if DB sources are not needed, or (B)
merge dbSources into the static/verified list by combining the result of
getSystemSettings with getVerifiedSources(jurisdiction, prompt) (e.g., normalize
types and concatenate/filter duplicates) and then use that combined array
(instead of only getVerifiedSources) when building the prompt/response; adjust
references to dbSources/getVerifiedSources accordingly to ensure the combined
sources are used.
In `@backend/src/routes/settings.ts`:
- Around line 14-16: In the catch block in backend/src/routes/settings.ts (the
catch that currently does res.status(500).json({ error: err.message })), stop
returning err.message to the client; instead log the full err internally (e.g.,
console.error or your logger) and return a generic response like
res.status(500).json({ error: "Internal server error" }) so database/internal
details aren't leaked—keep the existing variable name err and the same status
code, just change the logged vs returned content.
In `@src/components/AskAILawyer.tsx`:
- Around line 88-90: The current initialization in AskAILawyer uses
jData[0].label and jData[4]?.label without ensuring those indices exist; update
the setSelectedJurisdictions call to defensively check jData length and use safe
access (e.g., optional chaining or conditional index checks) so you only read
.label from existing entries in jData before calling setSelectedJurisdictions,
referencing the setSelectedJurisdictions function and the jData array to locate
the code and ensure you never attempt to read properties from undefined entries.
- Around line 211-226: The SSE logic using EventSource (eventSource) needs an
onerror handler and proper cleanup to avoid UI stalls and leaks: add
eventSource.onerror to handle connection errors (e.g., close the eventSource,
setStepperPhase to "idle" and surface the error via setStepperMessage or alert)
and ensure the EventSource is closed when the component unmounts by returning a
cleanup from the surrounding useEffect (or equivalent) that checks and calls
eventSource.close(); update the existing onmessage handling to only operate if
the eventSource is still open to avoid race conditions with the cleanup.
In `@src/components/VulnerabilityScannerView.tsx`:
- Line 104: The finally block always calls setScanning(false) even after
starting the SSE path, which hides the loading UI while the SSE EventSource is
still active; update the scan flow in VulnerabilityScannerView so that when you
create/start the SSE/EventSource (the code that sets up the server-sent events),
you avoid clearing scanning in the finally: either remove the global finally and
call setScanning(false) only in the non-SSE error/rejection flows, or introduce
a local flag (e.g., sseStarted or usingSSE) set to true when EventSource is
created and guard the finally so it only calls setScanning(false) when usingSSE
is false; ensure the SSE onmessage/onclose handlers call setScanning(false) when
the job completes or the connection ends (apply same change to the other similar
block referenced around lines 111–113).
---
Outside diff comments:
In `@backend/src/agents/analysisAgent.ts`:
- Around line 94-113: The heuristicAudit fallback returns objects that diverge
from AuditSchema (extra field non_compliance_tag and unchecked enum values) and
thus bypasses validation; update heuristicAudit to produce only fields declared
by AuditSchema (rename/remove non_compliance_tag, ensure severity/risk_level use
allowed enum values and structure matches AuditSchema) and then run
AuditSchema.parse(...) (or safeParse) on the final return value before returning
from heuristicAudit (or at the runAudit caller) so the heuristic output is
validated and any violations are caught; reference heuristicAudit, AuditSchema,
and runAudit when making these changes.
In `@src/components/CookieScanner.tsx`:
- Around line 100-102: The finally block calling setScanning(false) is clearing
the loading state even when the SSE path (HTTP 202 with job_id) is taken; update
the logic in the function that handles the fetch response so that you only call
setScanning(false) for non-SSE paths (e.g., after successful immediate scan or
on catch failure) and return early when response.status === 202 or job_id is
present so the finally block is not reached for SSE; alternatively remove the
global finally and place setScanning(false) explicitly in the non-SSE success
and error branches, leaving the SSE callbacks (EventSource
onmessage/onerror/onclose) responsible for clearing setScanning(false). Ensure
you reference setScanning, response.status/job_id, and the EventSource callbacks
(onmessage/onerror/onclose) when making the change.
---
Nitpick comments:
In `@backend/src/routes/lawyer.ts`:
- Around line 10-13: The getSystemSettings function currently types the database
client as any; change its signature to accept a typed PG client (e.g., Pool |
PoolClient from the 'pg' package) by importing those types and updating the
client parameter type in getSystemSettings to improve type safety and
IDE/autocomplete support; ensure any callers pass a compatible Pool or
PoolClient and update related usage if necessary.
In `@src/components/DraftAgreement.tsx`:
- Around line 1754-1768: The select's formatting is inconsistent: update
handleToolbarFormat to support an "h3"/"subtitle" case (add logic inside
handleToolbarFormat to insert "\n### " or apply subtitle formatting) and change
the select's subtitle option to call handleToolbarFormat("h3" or "subtitle")
instead of directly calling insertTextAtCursor; also decide whether the "p"
option should be removed or wired to handleToolbarFormat("p") so it applies a
consistent paragraph action (e.g., insert "\n\n" or whatever your renderer
expects) rather than directly mutating via insertTextAtCursor.
- Around line 1734-1742: The font select in DraftAgreement.tsx is uncontrolled
and stays on the chosen option; make it controlled by adding local state (e.g.,
fontSelectValue) for the <select> used with insertTextAtCursor, bind that state
to the select's value, and in the onChange handler call insertTextAtCursor(...)
then reset the state back to the default (e.g., "sans") so the dropdown visually
returns to Default; apply the same controlled-state + reset pattern to the size
and paragraph <select> elements as well.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff59a755-5042-4d06-8008-90ee9a63d26c
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonverification/auth_modal.pngis excluded by!**/*.pngverification/error.pngis excluded by!**/*.png
📒 Files selected for processing (21)
backend/src/RAG/ragService.tsbackend/src/agents/analysisAgent.tsbackend/src/agents/legalAgent.tsbackend/src/agents/negotiationAgent.tsbackend/src/config/index.tsbackend/src/config/initDb.tsbackend/src/middleware/auth.tsbackend/src/routes/index.tsbackend/src/routes/lawyer.tsbackend/src/routes/settings.tsdb.jsonindex.htmlmetadata.jsonpackage.jsonsrc/components/AskAILawyer.tsxsrc/components/CookieScanner.tsxsrc/components/DashboardHome.tsxsrc/components/DraftAgreement.tsxsrc/components/VulnerabilityScannerView.tsxsrc/types.tsverification/verify.py
💤 Files with no reviewable changes (3)
- metadata.json
- verification/verify.py
- db.json
✅ Files skipped from review due to trivial changes (6)
- backend/src/routes/index.ts
- src/components/DashboardHome.tsx
- backend/src/RAG/ragService.ts
- src/types.ts
- package.json
- index.html
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/middleware/auth.ts
- backend/src/agents/legalAgent.ts
| databaseUrl: process.env.DATABASE_URL || "", | ||
| geminiApiKey: process.env.GEMINI_API_KEY || "", | ||
| jwtSecret: process.env.JWT_SECRET || "cookie-care-enterprise-secret-2026", | ||
| jwtSecret: process.env.JWT_SECRET || "privsec-ai-enterprise-secret-2026", |
There was a problem hiding this comment.
Critical: Hardcoded JWT secret undermines authentication security.
The fallback JWT secret is visible in source control and known to anyone with repository access. If process.env.JWT_SECRET is not set in production, this hardcoded value will be used, allowing anyone who has seen the code to forge valid JWTs and bypass authentication entirely. This directly contradicts the "Security Hardening" objective of this PR.
Additionally, changing the fallback secret will invalidate all existing JWTs if the previous fallback was in use (i.e., when JWT_SECRET was not set).
🔒 Recommended fix
Remove the fallback and require the environment variable:
- jwtSecret: process.env.JWT_SECRET || "privsec-ai-enterprise-secret-2026",
+ jwtSecret: process.env.JWT_SECRET || (() => {
+ throw new Error("CRITICAL: JWT_SECRET environment variable must be set. Server cannot start without it.");
+ })(),Then ensure JWT_SECRET is properly set in all deployment environments with a cryptographically strong secret (e.g., generated via openssl rand -base64 32).
📝 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
| jwtSecret: process.env.JWT_SECRET || "privsec-ai-enterprise-secret-2026", | |
| jwtSecret: process.env.JWT_SECRET || (() => { | |
| throw new Error("CRITICAL: JWT_SECRET environment variable must be set. Server cannot start without it."); | |
| })(), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/config/index.ts` at line 10, The config currently sets jwtSecret
with a hardcoded fallback ("privsec-ai-enterprise-secret-2026") via
process.env.JWT_SECRET || "...", which is insecure; remove the fallback so
jwtSecret is assigned only from process.env.JWT_SECRET and fail fast if it's
missing (throw an error or exit during app startup) in the module that exports
jwtSecret (referencing the jwtSecret variable and process.env.JWT_SECRET), and
update deployment/config docs to ensure a strong, cryptographically-generated
JWT_SECRET is provided in all environments.
Comment on lines
+177
to
+183
| // System Settings RLS: Global Read for authenticated users, Admin Write | ||
| await client.query(`ALTER TABLE system_settings ENABLE ROW LEVEL SECURITY;`); | ||
| await client.query(`DROP POLICY IF EXISTS system_settings_read_policy ON system_settings;`); | ||
| await client.query(` | ||
| CREATE POLICY system_settings_read_policy ON system_settings | ||
| FOR SELECT USING (current_setting('app.current_user_id', true) IS NOT NULL); | ||
| `); |
There was a problem hiding this comment.
Missing write policies for system_settings blocks admin modifications.
RLS is enabled but only a SELECT policy is created. PostgreSQL denies all operations without explicit policies when RLS is active. The seed insert works only because it executes before line 178, but any subsequent INSERT/UPDATE/DELETE by admins will fail, contradicting the "Admin Write" comment.
🔐 Proposed fix to add admin write policies
await client.query(`
CREATE POLICY system_settings_read_policy ON system_settings
FOR SELECT USING (current_setting('app.current_user_id', true) IS NOT NULL);
`);
+
+ await client.query(`DROP POLICY IF EXISTS system_settings_admin_write ON system_settings;`);
+ await client.query(`
+ CREATE POLICY system_settings_admin_write ON system_settings
+ FOR ALL USING (current_setting('app.current_user_role', true) = 'ADMIN')
+ WITH CHECK (current_setting('app.current_user_role', true) = 'ADMIN');
+ `);📝 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
| // System Settings RLS: Global Read for authenticated users, Admin Write | |
| await client.query(`ALTER TABLE system_settings ENABLE ROW LEVEL SECURITY;`); | |
| await client.query(`DROP POLICY IF EXISTS system_settings_read_policy ON system_settings;`); | |
| await client.query(` | |
| CREATE POLICY system_settings_read_policy ON system_settings | |
| FOR SELECT USING (current_setting('app.current_user_id', true) IS NOT NULL); | |
| `); | |
| // System Settings RLS: Global Read for authenticated users, Admin Write | |
| await client.query(`ALTER TABLE system_settings ENABLE ROW LEVEL SECURITY;`); | |
| await client.query(`DROP POLICY IF EXISTS system_settings_read_policy ON system_settings;`); | |
| await client.query(` | |
| CREATE POLICY system_settings_read_policy ON system_settings | |
| FOR SELECT USING (current_setting('app.current_user_id', true) IS NOT NULL); | |
| `); | |
| await client.query(`DROP POLICY IF EXISTS system_settings_admin_write ON system_settings;`); | |
| await client.query(` | |
| CREATE POLICY system_settings_admin_write ON system_settings | |
| FOR ALL USING (current_setting('app.current_user_role', true) = 'ADMIN') | |
| WITH CHECK (current_setting('app.current_user_role', true) = 'ADMIN'); | |
| `); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/config/initDb.ts` around lines 177 - 183, RLS for table
system_settings only creates a SELECT policy (system_settings_read_policy) so
write operations are blocked; add explicit admin write policies by dropping any
existing write policy and creating a new policy (eg.
system_settings_write_policy) that allows INSERT, UPDATE and DELETE for admins —
use CREATE POLICY ... FOR INSERT, UPDATE, DELETE ON system_settings USING
(current_setting('app.current_user_role', true) = 'admin') WITH CHECK
(current_setting('app.current_user_role', true) = 'admin') (or the project’s
existing admin flag setting name), ensuring INSERT/UPDATE use WITH CHECK and
DELETE/UPDATE use USING so admins can modify rows while RLS remains enabled.
Comment on lines
+75
to
79
| const client = req.dbClient || pool; | ||
| const dbSources = await getSystemSettings('web_discovery_sources', client); | ||
|
|
||
| // Combine static hardcoded logic with potential DB sources for robustness | ||
| const verifiedSources = getVerifiedSources(jurisdiction, prompt); |
There was a problem hiding this comment.
Unused dbSources variable.
dbSources is fetched from getSystemSettings('web_discovery_sources', client) but never used in the subsequent logic. The comment on line 78 mentions combining with DB sources, but no actual combination occurs. Either integrate dbSources into the response/prompt or remove the unnecessary database call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/lawyer.ts` around lines 75 - 79, The variable dbSources is
fetched but never used; update the logic in the handler that calls
getSystemSettings('web_discovery_sources', client) so you either (A) remove the
unused call to getSystemSettings and the db client fallback (req.dbClient ||
pool) if DB sources are not needed, or (B) merge dbSources into the
static/verified list by combining the result of getSystemSettings with
getVerifiedSources(jurisdiction, prompt) (e.g., normalize types and
concatenate/filter duplicates) and then use that combined array (instead of only
getVerifiedSources) when building the prompt/response; adjust references to
dbSources/getVerifiedSources accordingly to ensure the combined sources are
used.
Comment on lines
+14
to
+16
| } catch (err: any) { | ||
| res.status(500).json({ error: err.message }); | ||
| } |
There was a problem hiding this comment.
Avoid exposing raw database error messages to clients.
Returning err.message directly could leak internal database details (table names, column names, connection information) to clients. Return a generic error message instead.
🛡️ Proposed fix
} catch (err: any) {
- res.status(500).json({ error: err.message });
+ console.error("Settings fetch error:", err);
+ res.status(500).json({ error: "Failed to retrieve setting" });
}📝 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
| } catch (err: any) { | |
| res.status(500).json({ error: err.message }); | |
| } | |
| } catch (err: any) { | |
| console.error("Settings fetch error:", err); | |
| res.status(500).json({ error: "Failed to retrieve setting" }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/settings.ts` around lines 14 - 16, In the catch block in
backend/src/routes/settings.ts (the catch that currently does
res.status(500).json({ error: err.message })), stop returning err.message to the
client; instead log the full err internally (e.g., console.error or your logger)
and return a generic response like res.status(500).json({ error: "Internal
server error" }) so database/internal details aren't leaked—keep the existing
variable name err and the same status code, just change the logged vs returned
content.
Comment on lines
+88
to
+90
| if (jData.length >= 2) { | ||
| setSelectedJurisdictions([jData[0].label, jData[4]?.label].filter(Boolean)); | ||
| } |
There was a problem hiding this comment.
Add bounds checking for array access.
The code assumes jData[0] and jData[4] exist without validation. If the settings return fewer than 5 jurisdictions, accessing jData[4] will return undefined, which is handled by .filter(Boolean), but accessing properties on undefined jurisdictions (like .label) in subsequent renders could cause issues if the data structure changes.
🛡️ Safer initialization
if (jData.length >= 2) {
- setSelectedJurisdictions([jData[0].label, jData[4]?.label].filter(Boolean));
+ const defaults = [jData[0]?.label, jData[4]?.label].filter(Boolean) as string[];
+ setSelectedJurisdictions(defaults);
}📝 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
| if (jData.length >= 2) { | |
| setSelectedJurisdictions([jData[0].label, jData[4]?.label].filter(Boolean)); | |
| } | |
| if (jData.length >= 2) { | |
| const defaults = [jData[0]?.label, jData[4]?.label].filter(Boolean) as string[]; | |
| setSelectedJurisdictions(defaults); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/AskAILawyer.tsx` around lines 88 - 90, The current
initialization in AskAILawyer uses jData[0].label and jData[4]?.label without
ensuring those indices exist; update the setSelectedJurisdictions call to
defensively check jData length and use safe access (e.g., optional chaining or
conditional index checks) so you only read .label from existing entries in jData
before calling setSelectedJurisdictions, referencing the
setSelectedJurisdictions function and the jData array to locate the code and
ensure you never attempt to read properties from undefined entries.
Comment on lines
+211
to
+226
| const eventSource = new EventSource(apiUrl(`/api/jobs/sse?token=${authToken}`)); | ||
| eventSource.onmessage = (event) => { | ||
| const data = JSON.parse(event.data); | ||
| if (data.event === "job_update" && data.job.id === payload.job_id) { | ||
| if (data.job.status === "completed") { | ||
| eventSource.close(); | ||
| fetchKnowledgeBase(); | ||
| setStepperPhase("completed"); | ||
| setStepperMessage(`Success: ${file.name} indexed and ready for advisory.`); | ||
| } else if (data.job.status === "failed") { | ||
| eventSource.close(); | ||
| setStepperPhase("idle"); | ||
| alert("Indexing failed: " + data.job.error); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add error handling for EventSource and cleanup on unmount.
The SSE connection lacks an onerror handler, so if the connection fails, the UI will remain stuck in the "extracting" phase. Additionally, if the component unmounts while SSE is active, the EventSource won't be closed, causing a memory leak.
🛡️ Proposed fix
const eventSource = new EventSource(apiUrl(`/api/jobs/sse?token=${authToken}`));
eventSource.onmessage = (event) => {
const data = JSON.parse(event.data);
if (data.event === "job_update" && data.job.id === payload.job_id) {
if (data.job.status === "completed") {
eventSource.close();
fetchKnowledgeBase();
setStepperPhase("completed");
setStepperMessage(`Success: ${file.name} indexed and ready for advisory.`);
} else if (data.job.status === "failed") {
eventSource.close();
setStepperPhase("idle");
alert("Indexing failed: " + data.job.error);
}
}
};
+ eventSource.onerror = () => {
+ eventSource.close();
+ setStepperPhase("idle");
+ alert("Real-time connection interrupted. Please retry.");
+ };📝 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
| const eventSource = new EventSource(apiUrl(`/api/jobs/sse?token=${authToken}`)); | |
| eventSource.onmessage = (event) => { | |
| const data = JSON.parse(event.data); | |
| if (data.event === "job_update" && data.job.id === payload.job_id) { | |
| if (data.job.status === "completed") { | |
| eventSource.close(); | |
| fetchKnowledgeBase(); | |
| setStepperPhase("completed"); | |
| setStepperMessage(`Success: ${file.name} indexed and ready for advisory.`); | |
| } else if (data.job.status === "failed") { | |
| eventSource.close(); | |
| setStepperPhase("idle"); | |
| alert("Indexing failed: " + data.job.error); | |
| } | |
| } | |
| }; | |
| const eventSource = new EventSource(apiUrl(`/api/jobs/sse?token=${authToken}`)); | |
| eventSource.onmessage = (event) => { | |
| const data = JSON.parse(event.data); | |
| if (data.event === "job_update" && data.job.id === payload.job_id) { | |
| if (data.job.status === "completed") { | |
| eventSource.close(); | |
| fetchKnowledgeBase(); | |
| setStepperPhase("completed"); | |
| setStepperMessage(`Success: ${file.name} indexed and ready for advisory.`); | |
| } else if (data.job.status === "failed") { | |
| eventSource.close(); | |
| setStepperPhase("idle"); | |
| alert("Indexing failed: " + data.job.error); | |
| } | |
| } | |
| }; | |
| eventSource.onerror = () => { | |
| eventSource.close(); | |
| setStepperPhase("idle"); | |
| alert("Real-time connection interrupted. Please retry."); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/AskAILawyer.tsx` around lines 211 - 226, The SSE logic using
EventSource (eventSource) needs an onerror handler and proper cleanup to avoid
UI stalls and leaks: add eventSource.onerror to handle connection errors (e.g.,
close the eventSource, setStepperPhase to "idle" and surface the error via
setStepperMessage or alert) and ensure the EventSource is closed when the
component unmounts by returning a cleanup from the surrounding useEffect (or
equivalent) that checks and calls eventSource.close(); update the existing
onmessage handling to only operate if the eventSource is still open to avoid
race conditions with the cleanup.
| setError("Real-time diagnostic connection interrupted."); | ||
| setScanning(false); | ||
| }; | ||
| return; // Exit early as SSE handles completion |
There was a problem hiding this comment.
finally block incorrectly resets scanning state after SSE setup.
The finally { setScanning(false) } block executes even when the early return on line 104 is reached after setting up SSE. This immediately sets scanning to false, causing the loading skeleton to disappear while SSE is still waiting for job completion.
🐛 Proposed fix
if (res.status === 202 && data.job_id) {
// SSE for real-time progress - standardized
const eventSource = new EventSource(apiUrl(`/api/jobs/sse?token=${authToken}`));
eventSource.onmessage = (event) => {
const payload = JSON.parse(event.data);
if (payload.event === "job_update" && payload.job.id === data.job_id) {
const job = payload.job;
if (job.status === "completed") {
setResult(job.result);
setScanning(false);
eventSource.close();
} else if (job.status === "failed") {
setError(job.error || "Vulnerability scan aborted.");
setScanning(false);
eventSource.close();
}
}
};
eventSource.onerror = (err) => {
console.error("SSE Error:", err);
eventSource.close();
setError("Real-time diagnostic connection interrupted.");
setScanning(false);
};
- return; // Exit early as SSE handles completion
+ return; // Exit function entirely - SSE callbacks handle state
} else {
setResult(data);
+ setScanning(false);
}
} catch (err: any) {
console.error(err);
setError(err.message || "An unexpected network error occurred while performing security audit.");
- } finally {
setScanning(false);
}
};Alternatively, guard the finally block or restructure to not use finally for the SSE path.
Also applies to: 111-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/VulnerabilityScannerView.tsx` at line 104, The finally block
always calls setScanning(false) even after starting the SSE path, which hides
the loading UI while the SSE EventSource is still active; update the scan flow
in VulnerabilityScannerView so that when you create/start the SSE/EventSource
(the code that sets up the server-sent events), you avoid clearing scanning in
the finally: either remove the global finally and call setScanning(false) only
in the non-SSE error/rejection flows, or introduce a local flag (e.g.,
sseStarted or usingSSE) set to true when EventSource is created and guard the
finally so it only calls setScanning(false) when usingSSE is false; ensure the
SSE onmessage/onclose handlers call setScanning(false) when the job completes or
the connection ends (apply same change to the other similar block referenced
around lines 111–113).
This was referenced Jun 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements Phase 1 of the Production-Ready Roadmap: Security Hardening.
The primary goal of this change is to eliminate Row Level Security (RLS) bypasses that occurred when controllers used direct
pool.querycalls without initializing session variables.Key Changes:
auth.tsmiddleware now acquires a dedicatedPoolClientfor every authenticated request, initializes it with the user's ID and role using Postgresset_config, and starts a transaction.req.dbClientinstead of the globalpool. This ensures that the Postgres RLS policies (e.g.,creator_id = current_setting('app.current_user_id')) are strictly enforced for every query.AgentOrchestratorand its routes (e.g.,/api/analyze/interact) now correctly pass through the RLS-scoped client to prevent data leakage during multi-document analysis.withTransactionhelpers were removed as the middleware now manages the transaction lifecycle for most standard requests.These changes provide a bulletproof foundation for multi-tenancy as the platform moves toward enterprise production.
PR created automatically by Jules for task 767483514296842253 started by @aish-am22
Summary by CodeRabbit
Release Notes
New Features
Improvements
Branding
Other