Skip to content

Commit 255d446

Browse files
jayzalowitzclaude
jayzalowitz
and
claude
committed
fix(#187 AC#2): address Copilot review — 3 IDOR + OOM + races
12 substantive findings from Copilot's review of PR #247, all fixed: Security (3 IDOR vulns): - POST /downloads/start: requireOwnership middleware (body.userId must match req.authenticatedUserId) - GET /downloads/:id and pause/resume/cancel: loadOwnedDownload() helper fetches row, rejects 403 if row.user_id ≠ authenticated user Correctness: - SHA-256 verify: stream via createReadStream so multi-GB GGUFs don't OOM the API process (was readFile() into memory) - Schema: UNIQUE partial index on (user_id, model_id) for non-terminal statuses; startDownload() catches 23505 race - inFlight guard in startDownload() prevents two concurrent runners writing same .partial when caller rapidly retries - Persist bytes_downloaded=0 immediately when server ignores Range - Persist total_bytes when Content-Length differs from registry approxBytes (added updateTotalBytes repo method) Frontend: - Stable data-role selectors (embedded-llm-progress-text / -status) replace the brittle "labelLine.firstElementChild" walk that was clobbering action buttons after first poll - ensureListener() no longer closes over container/userId from first mount; re-derives both at click time via getElementById(CARD_TARGET_ID) + getCurrentUserId() Tests: - Switched from injecting req.user.id to req.authenticatedUserId to match production sessionAuth contract - 7 new cross-user 403 tests covering every mutating endpoint + dev-bypass-allowed path. Total: 25 tests (was 14), all passing. Build clean. 486 API tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent ff73461 commit 255d446

8 files changed

Lines changed: 319 additions & 49 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 57 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,14 +53,67 @@ minutes later the twin works fully offline.
5353
existing `.confidence-bar` styles), pause/resume/cancel buttons,
5454
error surface. Polls `/downloads/:id` every 1s while a download is
5555
active; stops polling on terminal status.
56-
- 14 unit tests for the route layer in
56+
- 25 unit tests for the route layer in
5757
`apps/api/src/__tests__/embedded-llm-downloads-routes.test.ts`:
5858
start happy + missing-userId/modelId + 404 unknown-model, get +
5959
404, percent capping at 100% for over-fetch, percent=0 for pending,
60-
list-for-user, pause ok/false, resume + 404, cancel. Mocks
61-
`@skytwin/db` + the downloader module — no real HTTP / filesystem.
60+
list-for-user, pause ok/false, pause-404, resume + 404, cancel,
61+
cancel-404, plus 7 cross-user-403 tests covering every mutating
62+
endpoint plus the dev-bypass-allowed case. Mocks `@skytwin/db` +
63+
the downloader module — no real HTTP / filesystem.
6264

63-
API total: 477 passing (24 skipped). All 34 packages build clean.
65+
API total: 486 passing (24 skipped). All 34 packages build clean.
66+
67+
### Fixed (post-/review)
68+
69+
Copilot review on PR #247 surfaced 12 substantive findings; all
70+
addressed before merge:
71+
72+
- **IDOR on `POST /downloads/start`**: applied `requireOwnership`
73+
middleware so the request body's `userId` must match the
74+
authenticated user.
75+
- **IDOR on `GET /downloads/:id` and the pause/resume/cancel routes**:
76+
introduced a `loadOwnedDownload(req, res, id)` helper that fetches
77+
the row and rejects with 403 if `req.authenticatedUserId` doesn't
78+
match `row.user_id`. Dev bypass (no `authenticatedUserId`) keeps
79+
current behavior.
80+
- **OOM on multi-GB SHA-256 verify**: replaced `readFile(path)` with a
81+
streaming `createReadStream → hash.update(chunk)` loop in a new
82+
`computeSha256(path)` helper. A 4GB GGUF no longer needs 4GB of
83+
Node heap to verify.
84+
- **DB-level idempotency**: schema changed from a non-unique partial
85+
index to a `UNIQUE` partial index on `(user_id, model_id)` for
86+
non-terminal statuses, so two concurrent `/downloads/start` calls
87+
can't both win past `findActive()`. `startDownload()` now also
88+
catches the `23505` unique-violation that the loser's `INSERT`
89+
raises and re-fetches the surviving row.
90+
- **In-memory race**: `startDownload()` now bails if the row is
91+
already in `inFlight`, preventing two concurrent runners writing
92+
the same `.partial`.
93+
- **Stale progress on Range-ignored**: when the server returns 200 to
94+
a Range request, we now `updateProgress(id, 0)` immediately so the
95+
poll UI doesn't display stale `bytes_downloaded` until the next
96+
1MB flush.
97+
- **`total_bytes` never persisted**: added
98+
`modelDownloadRepository.updateTotalBytes(id, n)` and call it when
99+
Content-Length disagrees with the registry by >1MB. The progress
100+
bar denominator now matches reality.
101+
- **DOM update broke action buttons**: the polling tick was finding
102+
`labelLine.firstElementChild` and replacing it, which clobbered
103+
the size/percent span on first run and the action-button span on
104+
subsequent runs. Switched to stable `data-role` selectors
105+
(`embedded-llm-progress-text`, `embedded-llm-status`) so updates
106+
are scoped.
107+
- **Singleton listener stale closure**: `ensureListener()` no longer
108+
closes over `container` or `userId` from the first mount. The
109+
delegator now re-derives both at click time:
110+
`document.getElementById('embedded-llm-card-target')` for the
111+
container, `localStorage.getItem(KEY_USER_ID)` for the userId.
112+
Same pattern as the other singleton delegators in this codebase.
113+
- **Test auth pattern**: switched from injecting `req.user.id` to
114+
`req.authenticatedUserId` to match production. Added 7 new tests
115+
covering cross-user 403 on every mutating endpoint plus the
116+
dev-bypass-allowed path.
64117

65118
### Why this fits the theme
66119

‎apps/api/src/__tests__/embedded-llm-downloads-routes.test.ts‎

Lines changed: 112 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,19 @@ const SAMPLE_ROW = {
5252
completed_at: null,
5353
};
5454

55-
function buildApp(userId: string | null = USER_ID): Express {
55+
/**
56+
* Build an Express app for tests. Production auth sets
57+
* `req.authenticatedUserId` via `sessionAuth`; the ownership middleware
58+
* compares it against userId in params/body/query (or against the row's
59+
* user_id for `:id`-keyed routes). Pass `null` to simulate dev bypass —
60+
* `requireOwnership` skips the check when `authenticatedUserId` is undefined.
61+
*/
62+
function buildApp(authUserId: string | null = USER_ID): Express {
5663
const app = express();
5764
app.use(express.json());
58-
if (userId !== null) {
65+
if (authUserId !== null) {
5966
app.use((req, _res, next) => {
60-
(req as unknown as { user: { id: string } }).user = { id: userId };
67+
req.authenticatedUserId = authUserId;
6168
next();
6269
});
6370
}
@@ -229,6 +236,7 @@ describe('GET /api/embedded-llm/downloads/user/:userId', () => {
229236

230237
describe('POST /api/embedded-llm/downloads/:id/pause', () => {
231238
it('returns ok=true on successful pause', async () => {
239+
mockRepo.findById.mockResolvedValue(SAMPLE_ROW);
232240
mockPauseDownload.mockResolvedValue(true);
233241
const { status, body } = await req(
234242
buildApp(),
@@ -241,6 +249,7 @@ describe('POST /api/embedded-llm/downloads/:id/pause', () => {
241249
});
242250

243251
it('returns ok=false when row is not pausable', async () => {
252+
mockRepo.findById.mockResolvedValue(SAMPLE_ROW);
244253
mockPauseDownload.mockResolvedValue(false);
245254
const { body } = await req(
246255
buildApp(),
@@ -249,6 +258,17 @@ describe('POST /api/embedded-llm/downloads/:id/pause', () => {
249258
);
250259
expect(body['ok']).toBe(false);
251260
});
261+
262+
it('returns 404 when download not found', async () => {
263+
mockRepo.findById.mockResolvedValue(null);
264+
const { status } = await req(
265+
buildApp(),
266+
'POST',
267+
`/api/embedded-llm/downloads/${DOWNLOAD_ID}/pause`,
268+
);
269+
expect(status).toBe(404);
270+
expect(mockPauseDownload).not.toHaveBeenCalled();
271+
});
252272
});
253273

254274
describe('POST /api/embedded-llm/downloads/:id/resume', () => {
@@ -281,6 +301,7 @@ describe('POST /api/embedded-llm/downloads/:id/resume', () => {
281301

282302
describe('POST /api/embedded-llm/downloads/:id/cancel', () => {
283303
it('returns ok=true on successful cancel', async () => {
304+
mockRepo.findById.mockResolvedValue(SAMPLE_ROW);
284305
mockCancelDownload.mockResolvedValue(true);
285306
const { status, body } = await req(
286307
buildApp(),
@@ -290,4 +311,92 @@ describe('POST /api/embedded-llm/downloads/:id/cancel', () => {
290311
expect(status).toBe(200);
291312
expect(body['ok']).toBe(true);
292313
});
314+
315+
it('returns 404 when download not found', async () => {
316+
mockRepo.findById.mockResolvedValue(null);
317+
const { status } = await req(
318+
buildApp(),
319+
'POST',
320+
`/api/embedded-llm/downloads/${DOWNLOAD_ID}/cancel`,
321+
);
322+
expect(status).toBe(404);
323+
expect(mockCancelDownload).not.toHaveBeenCalled();
324+
});
325+
});
326+
327+
describe('cross-user access is forbidden', () => {
328+
const OTHER_USER_ID = '11111111-2222-3333-4444-555555555555';
329+
330+
it('POST /downloads/start rejects when body userId differs from authenticated user', async () => {
331+
const { status } = await req(
332+
buildApp(USER_ID),
333+
'POST',
334+
'/api/embedded-llm/downloads/start',
335+
{ userId: OTHER_USER_ID, modelId: 'qwen-2.5-3b-q4' },
336+
);
337+
expect(status).toBe(403);
338+
expect(mockStartDownload).not.toHaveBeenCalled();
339+
});
340+
341+
it('GET /downloads/:id rejects when row belongs to another user', async () => {
342+
mockRepo.findById.mockResolvedValue({ ...SAMPLE_ROW, user_id: OTHER_USER_ID });
343+
const { status } = await req(
344+
buildApp(USER_ID),
345+
'GET',
346+
`/api/embedded-llm/downloads/${DOWNLOAD_ID}`,
347+
);
348+
expect(status).toBe(403);
349+
});
350+
351+
it('POST /downloads/:id/pause rejects when row belongs to another user', async () => {
352+
mockRepo.findById.mockResolvedValue({ ...SAMPLE_ROW, user_id: OTHER_USER_ID });
353+
const { status } = await req(
354+
buildApp(USER_ID),
355+
'POST',
356+
`/api/embedded-llm/downloads/${DOWNLOAD_ID}/pause`,
357+
);
358+
expect(status).toBe(403);
359+
expect(mockPauseDownload).not.toHaveBeenCalled();
360+
});
361+
362+
it('POST /downloads/:id/resume rejects when row belongs to another user', async () => {
363+
mockRepo.findById.mockResolvedValue({ ...SAMPLE_ROW, user_id: OTHER_USER_ID });
364+
const { status } = await req(
365+
buildApp(USER_ID),
366+
'POST',
367+
`/api/embedded-llm/downloads/${DOWNLOAD_ID}/resume`,
368+
);
369+
expect(status).toBe(403);
370+
expect(mockStartDownload).not.toHaveBeenCalled();
371+
});
372+
373+
it('POST /downloads/:id/cancel rejects when row belongs to another user', async () => {
374+
mockRepo.findById.mockResolvedValue({ ...SAMPLE_ROW, user_id: OTHER_USER_ID });
375+
const { status } = await req(
376+
buildApp(USER_ID),
377+
'POST',
378+
`/api/embedded-llm/downloads/${DOWNLOAD_ID}/cancel`,
379+
);
380+
expect(status).toBe(403);
381+
expect(mockCancelDownload).not.toHaveBeenCalled();
382+
});
383+
384+
it('GET /downloads/user/:userId rejects when path userId differs from authenticated user', async () => {
385+
const { status } = await req(
386+
buildApp(USER_ID),
387+
'GET',
388+
`/api/embedded-llm/downloads/user/${OTHER_USER_ID}`,
389+
);
390+
expect(status).toBe(403);
391+
});
392+
393+
it('dev bypass (no authenticatedUserId) allows access', async () => {
394+
mockRepo.findById.mockResolvedValue({ ...SAMPLE_ROW, user_id: OTHER_USER_ID });
395+
const { status } = await req(
396+
buildApp(null),
397+
'GET',
398+
`/api/embedded-llm/downloads/${DOWNLOAD_ID}`,
399+
);
400+
expect(status).toBe(200);
401+
});
293402
});

‎apps/api/src/embedded-llm/downloader.ts‎

Lines changed: 54 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { createHash } from 'node:crypto';
22
import {
3+
createReadStream,
34
createWriteStream,
45
existsSync,
56
mkdirSync,
@@ -97,13 +98,34 @@ export async function startDownload(
9798
download = existing;
9899
resumed = existing.bytes_downloaded > 0;
99100
} else {
100-
download = await modelDownloadRepository.create({
101-
userId,
102-
modelId,
103-
targetPath,
104-
totalBytes: model.approxBytes,
105-
sha256Expected: model.sha256,
106-
});
101+
try {
102+
download = await modelDownloadRepository.create({
103+
userId,
104+
modelId,
105+
targetPath,
106+
totalBytes: model.approxBytes,
107+
sha256Expected: model.sha256,
108+
});
109+
} catch (err) {
110+
// The DB-level unique partial index can reject concurrent inserts
111+
// that both passed findActive(). Re-fetch and treat as resumed.
112+
if (isUniqueViolation(err)) {
113+
const refetch = await modelDownloadRepository.findActive(userId, modelId);
114+
if (refetch === null) throw err;
115+
download = refetch;
116+
resumed = refetch.bytes_downloaded > 0;
117+
} else {
118+
throw err;
119+
}
120+
}
121+
}
122+
123+
// If a runner is already executing for this row, skip kicking off a
124+
// second one — two concurrent .partial writers would corrupt the
125+
// file. The active runner will keep streaming and the caller polls
126+
// for progress on the same row.
127+
if (inFlight.has(download.id)) {
128+
return { download, resumed };
107129
}
108130

109131
// Kick off the async transfer. Caller doesn't await this — it polls
@@ -118,6 +140,12 @@ export async function startDownload(
118140
return { download, resumed };
119141
}
120142

143+
function isUniqueViolation(err: unknown): boolean {
144+
if (typeof err !== 'object' || err === null) return false;
145+
const code = (err as { code?: unknown }).code;
146+
return code === '23505';
147+
}
148+
121149
/**
122150
* Pause an in-flight download. Sets the in-memory flag (the streamer
123151
* checks it per-chunk) and updates the DB row to 'paused'. The
@@ -244,6 +272,9 @@ async function runDownload(download: ModelDownloadRow): Promise<void> {
244272
if (existsSync(partialPath)) {
245273
try { unlinkSync(partialPath); } catch { /* best effort */ }
246274
}
275+
// Persist the reset immediately so the polling UI doesn't briefly
276+
// report a stale `bytes_downloaded` until the next 1MB flush.
277+
await modelDownloadRepository.updateProgress(download.id, 0);
247278
}
248279
if (response.body === null) {
249280
inFlight.delete(download.id);
@@ -262,10 +293,7 @@ async function runDownload(download: ModelDownloadRow): Promise<void> {
262293
if (Number.isFinite(len) && len > 0) {
263294
const totalFromServer = response.status === 206 ? resumeFrom + len : len;
264295
if (Math.abs(totalFromServer - download.total_bytes) > 1024 * 1024) {
265-
// Drift > 1MB — update so progress bar isn't lying.
266-
await modelDownloadRepository.setStatus(download.id, 'downloading', {
267-
bytesDownloaded: resumeFrom,
268-
});
296+
await modelDownloadRepository.updateTotalBytes(download.id, totalFromServer);
269297
}
270298
}
271299
}
@@ -321,9 +349,9 @@ async function runDownload(download: ModelDownloadRow): Promise<void> {
321349
const expectedHash = model.sha256.toLowerCase();
322350
const isPlaceholder = /^0+$/.test(expectedHash);
323351
if (!isPlaceholder) {
324-
const fs = await import('node:fs/promises');
325-
const buf = await fs.readFile(partialPath);
326-
const actual = createHash('sha256').update(buf).digest('hex');
352+
// Stream the file through the hash — multi-GB GGUFs would OOM the
353+
// API process if we readFile()'d the whole thing into memory.
354+
const actual = await computeSha256(partialPath);
327355
if (actual !== expectedHash) {
328356
// Corrupt download. Delete the partial — user should retry,
329357
// and we don't want a half-bad GGUF lingering on disk.
@@ -383,3 +411,15 @@ export function ensureDirectory(filePath: string): void {
383411
const dir = dirname(filePath);
384412
if (!existsSync(dir)) mkdirSync(dir, { recursive: true });
385413
}
414+
415+
/**
416+
* Stream the file through a SHA-256 hash. Multi-GB GGUFs would OOM
417+
* the API process if we loaded them into memory whole.
418+
*/
419+
async function computeSha256(path: string): Promise<string> {
420+
const hash = createHash('sha256');
421+
for await (const chunk of createReadStream(path)) {
422+
hash.update(chunk as Buffer);
423+
}
424+
return hash.digest('hex');
425+
}

0 commit comments

Comments
 (0)