Skip to content

Commit 79cd25e

Browse files
author
StackMemory Bot (CLI)
committed
fix(sync): address code review findings
- C2: Whitelist valid columns per table in upsertRow to prevent SQL injection from remote data keys - C4: Restrict CORS to known origins instead of wildcard - W1: Add conflict target column to ON CONFLICT clause - W3: Register cloud sync tools in getAvailableTools() - W5: Validate sync table names in MCP handler - W6: Merge duplicate os imports - W8: Use actual pushed_at for pull cursor instead of approximate time
1 parent e43fee1 commit 79cd25e

5 files changed

Lines changed: 137 additions & 20 deletions

File tree

‎packages/provenant-api/src/index.js‎

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ export default {
2929
if (request.method === 'OPTIONS') {
3030
return new Response(null, {
3131
status: 204,
32-
headers: corsHeaders(),
32+
headers: corsHeaders(request),
3333
});
3434
}
3535

@@ -152,7 +152,7 @@ async function handlePull(request, sql, projectId, clientId) {
152152
let rows;
153153
if (tables && tables.length > 0) {
154154
rows = await sql`
155-
SELECT id, table_name, version, tier, data
155+
SELECT id, table_name, version, tier, data, pushed_at
156156
FROM sync_entities
157157
WHERE project_id = ${projectId}
158158
AND pushed_at > ${since}::timestamptz
@@ -163,7 +163,7 @@ async function handlePull(request, sql, projectId, clientId) {
163163
`;
164164
} else {
165165
rows = await sql`
166-
SELECT id, table_name, version, tier, data
166+
SELECT id, table_name, version, tier, data, pushed_at
167167
FROM sync_entities
168168
WHERE project_id = ${projectId}
169169
AND pushed_at > ${since}::timestamptz
@@ -182,11 +182,12 @@ async function handlePull(request, sql, projectId, clientId) {
182182
data: typeof r.data === 'string' ? JSON.parse(r.data) : r.data,
183183
}));
184184

185-
// Server cursor = pushed_at of last returned entity, or `since` if empty
186-
const serverCursor =
187-
entities.length > 0
188-
? new Date().toISOString() // approximate — good enough for alpha
189-
: since;
185+
// Server cursor = pushed_at of last returned row (not approximate)
186+
const lastRow =
187+
rows.length > 0 ? rows[Math.min(rows.length, limit) - 1] : null;
188+
const serverCursor = lastRow?.pushed_at
189+
? new Date(lastRow.pushed_at).toISOString()
190+
: since;
190191

191192
// Update pull cursor
192193
await sql`
@@ -227,19 +228,27 @@ async function handleStatus(sql, projectId, clientId) {
227228

228229
// --- Helpers ---
229230

230-
function json(data, status = 200) {
231+
function json(data, status = 200, request = null) {
231232
return new Response(JSON.stringify(data), {
232233
status,
233234
headers: {
234235
'Content-Type': 'application/json',
235-
...corsHeaders(),
236+
...corsHeaders(request),
236237
},
237238
});
238239
}
239240

240-
function corsHeaders() {
241+
const ALLOWED_ORIGINS = [
242+
'https://app.stackmemory.ai',
243+
'https://stackmemory.ai',
244+
'http://localhost:3456',
245+
];
246+
247+
function corsHeaders(request) {
248+
const origin = request?.headers?.get('Origin');
241249
return {
242-
'Access-Control-Allow-Origin': '*',
250+
'Access-Control-Allow-Origin':
251+
origin && ALLOWED_ORIGINS.includes(origin) ? origin : ALLOWED_ORIGINS[0],
243252
'Access-Control-Allow-Methods': 'GET, POST, OPTIONS',
244253
'Access-Control-Allow-Headers': 'Content-Type, Authorization, X-Client-Id',
245254
};

‎src/cli/commands/sync.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,7 @@
55

66
import { Command } from 'commander';
77
import chalk from 'chalk';
8-
import { homedir } from 'os';
9-
import { hostname } from 'os';
8+
import { homedir, hostname } from 'os';
109
import { join } from 'path';
1110
import { existsSync, readFileSync } from 'fs';
1211
import { createHash } from 'crypto';

‎src/core/storage/cloud-sync.ts‎

Lines changed: 99 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,91 @@ const PK_COLUMN: Record<SyncTable, string> = {
7272
entity_states: 'id',
7373
};
7474

75+
// Valid columns per table — whitelist for SQL injection prevention on pull/upsert
76+
const VALID_COLUMNS: Record<SyncTable, Set<string>> = {
77+
frames: new Set([
78+
'frame_id',
79+
'run_id',
80+
'project_id',
81+
'parent_frame_id',
82+
'depth',
83+
'type',
84+
'name',
85+
'state',
86+
'inputs',
87+
'outputs',
88+
'digest_text',
89+
'digest_json',
90+
'created_at',
91+
'closed_at',
92+
'retention_policy',
93+
'importance_score',
94+
'prov_source',
95+
'prov_derivation',
96+
'prov_confidence',
97+
'prov_superseded_by',
98+
'prov_program_version',
99+
'access_count',
100+
'last_accessed',
101+
]),
102+
events: new Set([
103+
'event_id',
104+
'frame_id',
105+
'run_id',
106+
'seq',
107+
'event_type',
108+
'payload',
109+
'ts',
110+
]),
111+
anchors: new Set([
112+
'anchor_id',
113+
'frame_id',
114+
'project_id',
115+
'type',
116+
'text',
117+
'priority',
118+
'created_at',
119+
'metadata',
120+
'prov_source',
121+
'prov_confidence',
122+
'prov_superseded_by',
123+
]),
124+
trace_events: new Set([
125+
'id',
126+
'timestamp',
127+
'session_id',
128+
'trace_id',
129+
'parent_trace_id',
130+
'tenant_id',
131+
'actor_host',
132+
'actor_agent',
133+
'actor_user',
134+
'operation',
135+
'inputs',
136+
'outputs',
137+
'tokens_in',
138+
'tokens_out',
139+
'cost_usd',
140+
'duration_ms',
141+
'score',
142+
'feedback',
143+
'provenance',
144+
'error',
145+
'tags',
146+
]),
147+
entity_states: new Set([
148+
'id',
149+
'project_id',
150+
'entity_name',
151+
'relation',
152+
'value',
153+
'context',
154+
'source_frame_id',
155+
'valid_from',
156+
'superseded_at',
157+
]),
158+
};
159+
75160
export class CloudSyncEngine {
76161
private db: Database.Database;
77162
private config: CloudSyncConfig;
@@ -458,19 +543,28 @@ export class CloudSyncEngine {
458543
private upsertRow(
459544
table: SyncTable,
460545
data: Record<string, unknown>,
461-
_pk: string
546+
pk: string
462547
): void {
463-
const keys = Object.keys(data);
548+
// Whitelist columns to prevent SQL injection from remote data keys
549+
const valid = VALID_COLUMNS[table];
550+
const safeEntries = Object.entries(data).filter(([k]) => valid.has(k));
551+
if (safeEntries.length === 0) return;
552+
553+
const keys = safeEntries.map(([k]) => k);
554+
const values = safeEntries.map(([, v]) => v);
464555
const placeholders = keys.map(() => '?').join(', ');
465556
const columns = keys.join(', ');
466-
const updates = keys.map((k) => `${k} = excluded.${k}`).join(', ');
557+
const updates = keys
558+
.filter((k) => k !== pk)
559+
.map((k) => `${k} = excluded.${k}`)
560+
.join(', ');
467561

468562
this.db
469563
.prepare(
470564
`INSERT INTO ${table} (${columns}) VALUES (${placeholders})
471-
ON CONFLICT DO UPDATE SET ${updates}`
565+
ON CONFLICT(${pk}) DO UPDATE SET ${updates}`
472566
)
473-
.run(...keys.map((k) => data[k]));
567+
.run(...values);
474568
}
475569

476570
// --- Internal: Sync state management ---

‎src/integrations/mcp/handlers/cloud-sync-handlers.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,14 @@
66
import type { CloudSyncManager } from '../../../core/storage/cloud-sync-manager.js';
77
import type { SyncTable } from '../../../core/storage/cloud-sync-types.js';
88

9+
const VALID_SYNC_TABLES = new Set<string>([
10+
'frames',
11+
'events',
12+
'anchors',
13+
'trace_events',
14+
'entity_states',
15+
]);
16+
917
export interface CloudSyncHandlerDependencies {
1018
syncManager: CloudSyncManager | null;
1119
}
@@ -56,7 +64,9 @@ export class CloudSyncHandlers {
5664
tables?: string[];
5765
}): Promise<{ content: Array<{ type: string; text: string }> }> {
5866
const manager = this.ensureManager();
59-
const tables = args.tables as SyncTable[] | undefined;
67+
const tables = args.tables?.filter((t) => VALID_SYNC_TABLES.has(t)) as
68+
| SyncTable[]
69+
| undefined;
6070
const result = await manager.performPull('manual', tables);
6171

6272
return {

‎src/integrations/mcp/handlers/index.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,11 @@ export class MCPHandlerFactory {
307307
'sm_cross_discover',
308308
'sm_cross_register',
309309
'sm_cross_list',
310+
311+
// Cloud sync tools
312+
'cloud_sync_push',
313+
'cloud_sync_pull',
314+
'cloud_sync_status',
310315
];
311316
}
312317

0 commit comments

Comments
 (0)