fix: address PR review feedback — path safety, SQL injection, gate scoping

- Resolve relative filePath against input.cwd before statSync; early-return on ENOENT
- Replace LIKE '%path%' with exact json_each equality to prevent false matches
- Sanitize and parameterize LIMIT to prevent NaN SQL errors
- Fix day-sorting to use earliest epoch in group, not first (specificity-sorted) item
- Use exact path equality in deduplicateObservations instead of substring includes
- Scope FileReadGate by session+cwd to prevent worktree collisions
- Refresh lastAccess TTL on active sessions; throttle prune to every 50 calls
- Type params as (string | number)[] instead of any[]
- Remove unused permissionDecision fields from HookResult

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Alex Newman
2026-04-06 17:29:59 -07:00
parent a60f79c44d
commit 31910fb265
6 changed files with 175 additions and 142 deletions
File diff suppressed because one or more lines are too long
+13 -8
View File
@@ -89,7 +89,8 @@ function deduplicateObservations(
const filesRead = parseJsonArray(obs.files_read); const filesRead = parseJsonArray(obs.files_read);
const filesModified = parseJsonArray(obs.files_modified); const filesModified = parseJsonArray(obs.files_modified);
const totalFiles = filesRead.length + filesModified.length; const totalFiles = filesRead.length + filesModified.length;
const inModified = filesModified.some(f => f.includes(targetPath) || targetPath.includes(f)); const normalizedTarget = targetPath.replace(/\\/g, '/');
const inModified = filesModified.some(f => f.replace(/\\/g, '/') === normalizedTarget);
let specificityScore = 0; let specificityScore = 0;
if (inModified) specificityScore += 2; if (inModified) specificityScore += 2;
@@ -117,10 +118,10 @@ function formatFileTimeline(observations: ObservationRow[], filePath: string): s
byDay.get(day)!.push(obs); byDay.get(day)!.push(obs);
} }
// Sort days chronologically // Sort days chronologically (use earliest observation in each group, not first — which is specificity-sorted)
const sortedDays = Array.from(byDay.entries()).sort((a, b) => { const sortedDays = Array.from(byDay.entries()).sort((a, b) => {
const aEpoch = a[1][0].created_at_epoch; const aEpoch = Math.min(...a[1].map(o => o.created_at_epoch));
const bEpoch = b[1][0].created_at_epoch; const bEpoch = Math.min(...b[1].map(o => o.created_at_epoch));
return aEpoch - bEpoch; return aEpoch - bEpoch;
}); });
@@ -154,12 +155,16 @@ export const fileContextHandler: EventHandler = {
// Skip gate for files below the token-economics threshold — timeline (~370 tokens) // Skip gate for files below the token-economics threshold — timeline (~370 tokens)
// costs more than reading small files directly. // costs more than reading small files directly.
try { try {
const stat = statSync(filePath); const statPath = path.isAbsolute(filePath)
? filePath
: path.resolve(input.cwd || process.cwd(), filePath);
const stat = statSync(statPath);
if (stat.size < FILE_READ_GATE_MIN_BYTES) { if (stat.size < FILE_READ_GATE_MIN_BYTES) {
return { continue: true, suppressOutput: true }; return { continue: true, suppressOutput: true };
} }
} catch { } catch (err: any) {
// File not found, symlink error, permission denied — fall through and let gate proceed if (err.code === 'ENOENT') return { continue: true, suppressOutput: true };
// Other errors (symlink, permission denied) — fall through and let gate proceed
} }
// Check if project is excluded from tracking // Check if project is excluded from tracking
@@ -211,7 +216,7 @@ export const fileContextHandler: EventHandler = {
const gateResponse = await workerHttpRequest('/api/file-context/gate', { const gateResponse = await workerHttpRequest('/api/file-context/gate', {
method: 'POST', method: 'POST',
headers: { 'Content-Type': 'application/json' }, headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ sessionId: input.sessionId, filePath: relativePath }), body: JSON.stringify({ sessionId: input.sessionId, filePath: relativePath, cwd }),
}); });
if (gateResponse.ok) { if (gateResponse.ok) {
-2
View File
@@ -20,8 +20,6 @@ export interface HookResult {
hookSpecificOutput?: { hookSpecificOutput?: {
hookEventName: string; hookEventName: string;
additionalContext: string; additionalContext: string;
permissionDecision?: string;
permissionDecisionReason?: string;
}; };
systemMessage?: string; systemMessage?: string;
exitCode?: number; exitCode?: number;
+10 -6
View File
@@ -121,9 +121,11 @@ export function getObservationsByFilePath(
filePath: string, filePath: string,
options?: { projects?: string[]; limit?: number } options?: { projects?: string[]; limit?: number }
): ObservationRecord[] { ): ObservationRecord[] {
const likePattern = `%${filePath}%`; const rawLimit = options?.limit;
const limit = options?.limit ?? 15; const limit = Number.isInteger(rawLimit) && rawLimit! > 0
const params: any[] = [likePattern, likePattern]; ? Math.min(rawLimit!, 100)
: 15;
const params: (string | number)[] = [filePath, filePath];
let projectClause = ''; let projectClause = '';
if (options?.projects?.length) { if (options?.projects?.length) {
@@ -132,16 +134,18 @@ export function getObservationsByFilePath(
params.push(...options.projects); params.push(...options.projects);
} }
params.push(limit);
const stmt = db.prepare(` const stmt = db.prepare(`
SELECT * SELECT *
FROM observations FROM observations
WHERE ( WHERE (
EXISTS (SELECT 1 FROM json_each(files_read) WHERE value LIKE ?) EXISTS (SELECT 1 FROM json_each(files_read) WHERE value = ?)
OR EXISTS (SELECT 1 FROM json_each(files_modified) WHERE value LIKE ?) OR EXISTS (SELECT 1 FROM json_each(files_modified) WHERE value = ?)
) )
${projectClause} ${projectClause}
ORDER BY created_at_epoch DESC ORDER BY created_at_epoch DESC
LIMIT ${limit} LIMIT ?
`); `);
return stmt.all(...params) as ObservationRecord[]; return stmt.all(...params) as ObservationRecord[];
+35 -10
View File
@@ -3,45 +3,65 @@
* *
* In-memory session-scoped gate tracking which files have had their timeline * In-memory session-scoped gate tracking which files have had their timeline
* injected. Prevents duplicate timeline injections within the same session. * injected. Prevents duplicate timeline injections within the same session.
* Keys include cwd to prevent worktree collisions.
*/ */
interface SessionEntry { interface SessionEntry {
files: Set<string>; files: Set<string>;
createdAt: number; lastAccess: number;
} }
const TTL_MS = 4 * 60 * 60 * 1000; // 4 hours const TTL_MS = 4 * 60 * 60 * 1000; // 4 hours
const sessionGates = new Map<string, SessionEntry>(); const sessionGates = new Map<string, SessionEntry>();
let pruneCallCount = 0;
const PRUNE_EVERY_N_CALLS = 50;
/** /**
* Lazily prune session entries older than the TTL. * Lazily prune session entries older than the TTL.
* Throttled to run every N calls to avoid iterating all sessions on every check.
*/ */
function pruneExpiredSessions(): void { function pruneExpiredSessions(): void {
pruneCallCount++;
if (pruneCallCount < PRUNE_EVERY_N_CALLS) return;
pruneCallCount = 0;
const now = Date.now(); const now = Date.now();
for (const [sessionId, entry] of sessionGates) { for (const [key, entry] of sessionGates) {
if (now - entry.createdAt > TTL_MS) { if (now - entry.lastAccess > TTL_MS) {
sessionGates.delete(sessionId); sessionGates.delete(key);
} }
} }
} }
/** /**
* Check if this is the first read of a file in a session, and mark it if so. * Build a composite key scoped to session + cwd to prevent worktree collisions.
*/
function makeKey(sessionId: string, cwd?: string): string {
return cwd ? `${sessionId}::${cwd}` : sessionId;
}
/**
* Check if this is the first read of a file in a session+cwd scope, and mark it if so.
* Returns true if this is the first attempt (file was not previously seen). * Returns true if this is the first attempt (file was not previously seen).
* Returns false if the file was already seen in this session. * Returns false if the file was already seen in this session.
*/ */
export function checkAndMark(sessionId: string, filePath: string): boolean { export function checkAndMark(sessionId: string, filePath: string, cwd?: string): boolean {
pruneExpiredSessions(); pruneExpiredSessions();
const key = makeKey(sessionId, cwd);
const normalizedPath = filePath.replace(/\\/g, '/'); const normalizedPath = filePath.replace(/\\/g, '/');
let entry = sessionGates.get(sessionId); let entry = sessionGates.get(key);
if (!entry) { if (!entry) {
entry = { files: new Set(), createdAt: Date.now() }; entry = { files: new Set(), lastAccess: Date.now() };
sessionGates.set(sessionId, entry); sessionGates.set(key, entry);
} }
// Refresh TTL on every access so active sessions don't get re-blocked
entry.lastAccess = Date.now();
if (entry.files.has(normalizedPath)) { if (entry.files.has(normalizedPath)) {
return false; return false;
} }
@@ -52,7 +72,12 @@ export function checkAndMark(sessionId: string, filePath: string): boolean {
/** /**
* Clear all tracked files for a session (e.g., on session end). * Clear all tracked files for a session (e.g., on session end).
* Clears all cwd scopes for the given session.
*/ */
export function clearSession(sessionId: string): void { export function clearSession(sessionId: string): void {
sessionGates.delete(sessionId); for (const key of sessionGates.keys()) {
if (key === sessionId || key.startsWith(`${sessionId}::`)) {
sessionGates.delete(key);
}
}
} }
@@ -127,7 +127,8 @@ export class DataRoutes extends BaseRouteHandler {
const projectsParam = req.query.projects as string | undefined; const projectsParam = req.query.projects as string | undefined;
const projects = projectsParam ? projectsParam.split(',').filter(Boolean) : undefined; const projects = projectsParam ? projectsParam.split(',').filter(Boolean) : undefined;
const limit = req.query.limit ? parseInt(req.query.limit as string, 10) : undefined; const parsedLimit = req.query.limit ? parseInt(req.query.limit as string, 10) : undefined;
const limit = Number.isFinite(parsedLimit) && parsedLimit! > 0 ? parsedLimit : undefined;
const db = this.dbManager.getSessionStore().db; const db = this.dbManager.getSessionStore().db;
const observations = getObservationsByFilePath(db, filePath, { projects, limit }); const observations = getObservationsByFilePath(db, filePath, { projects, limit });
@@ -508,12 +509,12 @@ export class DataRoutes extends BaseRouteHandler {
* Returns: { firstAttempt: boolean } * Returns: { firstAttempt: boolean }
*/ */
private handleFileContextGate = this.wrapHandler((req: Request, res: Response): void => { private handleFileContextGate = this.wrapHandler((req: Request, res: Response): void => {
const { sessionId, filePath } = req.body; const { sessionId, filePath, cwd } = req.body;
if (!sessionId || !filePath) { if (!sessionId || !filePath) {
this.badRequest(res, 'sessionId and filePath are required'); this.badRequest(res, 'sessionId and filePath are required');
return; return;
} }
const firstAttempt = checkAndMark(sessionId, filePath); const firstAttempt = checkAndMark(sessionId, filePath, cwd);
res.json({ firstAttempt }); res.json({ firstAttempt });
}); });
} }