From 01b1260aa44bbd9767ff9a6b19456dc1f7e1ab1f Mon Sep 17 00:00:00 2001 From: unknown Date: Tue, 19 May 2026 09:56:59 +0800 Subject: [PATCH] fix(senseaudio): address PR #2065 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three non-blocking review notes from @PerishCode on PR #2065: 1. Drop the dead /api/byok-image/:id route. The PR description claimed it was "legacy fallback for old chat history" but that storage layout never existed on main, so the route can only ever 400 or 404 — never 200. Removed the handler, the isSafeByokImageId export, the unused createReadStream / stat / path / Request / Response imports, and the two byok-image regression tests. 2. Add rejectProxyPluginContext guard to the senseaudio proxy handler so it matches the invariant the other five proxy paths already enforce (plugin runs must go through /api/runs for snapshot pinning). Extended the existing "API fallback rejects plugin runs" describe to also cover /api/proxy/senseaudio/stream with the 409 PLUGIN_REQUIRES_DAEMON expectation. 3. Wrap the secondary image / video downloads (the URLs the SenseAudio gateway hands back in /v1/image/sync .url and /v1/video/status .video_url) in validateBaseUrlResolved so a malicious gateway can't point us at 169.254.169.254 (AWS / Azure metadata) or RFC1918 hosts via the response payload. Also passed `redirect: 'error'` on both fetches to match the SSRF posture the primary proxy fetch already uses. The new assertExternalAssetUrl helper lives next to executeGenerateImage so future tool downloads can reuse it. Tests: 120/120 daemon tests pass; guard + typecheck green. --- apps/daemon/src/byok-tools.ts | 48 ++++++++++++++++++-------- apps/daemon/src/chat-routes.ts | 35 ++----------------- apps/daemon/tests/byok-tools.test.ts | 22 ------------ apps/daemon/tests/proxy-routes.test.ts | 16 +-------- 4 files changed, 37 insertions(+), 84 deletions(-) diff --git a/apps/daemon/src/byok-tools.ts b/apps/daemon/src/byok-tools.ts index 6f3e895fea..e7bcf72705 100644 --- a/apps/daemon/src/byok-tools.ts +++ b/apps/daemon/src/byok-tools.ts @@ -17,10 +17,33 @@ import path from 'node:path'; import { writeFile } from 'node:fs/promises'; import { randomBytes } from 'node:crypto'; +import { validateBaseUrlResolved } from './connectionTest.js'; import { resolveProviderConfig } from './media-config.js'; import { IMAGE_MODELS } from './media-models.js'; import { ensureProject } from './projects.js'; +// SSRF guard for URLs the SenseAudio gateway hands back. The primary +// /v1/image/sync and /v1/video/create calls go through the user's +// configured base URL which is already validated upstream, but the +// returned `url` / `video_url` is attacker-controllable: a malicious +// gateway could point us at http://169.254.169.254/... (AWS metadata) +// or RFC1918 hosts to exfiltrate creds via secondary fetch. Resolve +// the host through validateBaseUrlResolved and refuse non-loopback +// internal addresses before downloading bytes. +async function assertExternalAssetUrl(rawUrl: string): Promise<{ ok: true } | { ok: false; error: string }> { + if (!rawUrl) return { ok: false, error: 'empty download url' }; + const validated = await validateBaseUrlResolved(rawUrl); + if (validated.error || !validated.parsed) { + return { + ok: false, + error: validated.forbidden + ? `senseaudio returned a blocked download url (${validated.error ?? 'internal address'})` + : `senseaudio returned an invalid download url: ${validated.error ?? 'unknown reason'}`, + }; + } + return { ok: true }; +} + // SenseAudio image model allowlist — derived from the shared media-models // registry so adding a new SenseAudio image model in one place (media-models) // auto-extends the BYOK tool param enum, the Settings dropdown, and the @@ -333,9 +356,12 @@ export async function executeGenerateImage( }; } + const imageUrlCheck = await assertExternalAssetUrl(imageUrl); + if (!imageUrlCheck.ok) return { ok: false, error: imageUrlCheck.error }; + let bytes: Buffer; try { - const imgResp = await fetch(imageUrl); + const imgResp = await fetch(imageUrl, { redirect: 'error' }); if (!imgResp.ok) { return { ok: false, error: `image download ${imgResp.status}` }; } @@ -560,9 +586,15 @@ export async function executeGenerateVideo( } // Step 3: download the mp4 bytes and persist into the project folder. + // Re-validate the returned URL through validateBaseUrlResolved so a + // malicious gateway can't point us at 169.254.169.254 (AWS / Azure + // metadata service) or RFC1918 hosts via the response payload. + const videoUrlCheck = await assertExternalAssetUrl(videoUrl); + if (!videoUrlCheck.ok) return { ok: false, error: videoUrlCheck.error }; + let bytes: Buffer; try { - const videoResp = await fetch(videoUrl); + const videoResp = await fetch(videoUrl, { redirect: 'error' }); if (!videoResp.ok) { return { ok: false, error: `video download ${videoResp.status}` }; } @@ -586,15 +618,3 @@ export async function executeGenerateVideo( }; } -/** - * Validate that a path segment is safe for filesystem lookup. The - * /api/byok-image/:id route uses this to refuse anything with slashes, - * `..`, NUL, control chars, or backslashes — i.e. anything that could - * escape . Tests assert it rejects every traversal - * variant we could think of. - */ -export function isSafeByokImageId(id: string): boolean { - if (typeof id !== 'string' || !id) return false; - if (id.length > 128) return false; - return /^[A-Za-z0-9._-]+$/.test(id) && !id.startsWith('.') && !id.includes('..'); -} diff --git a/apps/daemon/src/chat-routes.ts b/apps/daemon/src/chat-routes.ts index b0356bb5e2..aff821c8a1 100644 --- a/apps/daemon/src/chat-routes.ts +++ b/apps/daemon/src/chat-routes.ts @@ -1,7 +1,4 @@ -import path from 'node:path'; -import { createReadStream } from 'node:fs'; -import { stat } from 'node:fs/promises'; -import type { Express, Request, Response } from 'express'; +import type { Express } from 'express'; import type { RouteDeps } from './server-context.js'; import { newInsertId } from './analytics.js'; import { seedProviderIfMissing } from './media-config.js'; @@ -9,7 +6,6 @@ import { BYOK_SENSEAUDIO_TOOLS, executeGenerateImage, executeGenerateVideo, - isSafeByokImageId, isSenseAudioImageModel, type BYOKToolContext, } from './byok-tools.js'; @@ -1185,34 +1181,6 @@ export function registerChatRoutes(app: Express, ctx: RegisterChatRoutesDeps) { } }); - // Serve PNG bytes produced by the BYOK generate_image tool. The - // SenseAudio chat handler writes images to - // /byok-images/.png; this endpoint streams them - // back so the chat UI can render the URL the model embedded in - // markdown. isSafeByokImageId rejects any path segment that could - // escape the directory — see byok-tools.ts for the regex. 404 covers - // both unknown ids and the "directory does not exist yet" case - // (which happens on a fresh install before the first tool call). - app.get('/api/byok-image/:id', async (req: Request, res: Response) => { - const id = String(req.params.id || ''); - if (!isSafeByokImageId(id)) { - return sendApiError(res, 400, 'BAD_REQUEST', 'invalid byok image id'); - } - const filePath = path.join(ctx.paths.RUNTIME_DATA_DIR, 'byok-images', id); - try { - const info = await stat(filePath); - if (!info.isFile()) { - return sendApiError(res, 404, 'NOT_FOUND', 'byok image not found'); - } - } catch { - return sendApiError(res, 404, 'NOT_FOUND', 'byok image not found'); - } - res.setHeader('Content-Type', 'image/png'); - res.setHeader('Cache-Control', 'public, max-age=3600'); - res.setHeader('X-Content-Type-Options', 'nosniff'); - createReadStream(filePath).pipe(res); - }); - // SenseAudio chat completions. Wire-compatible with OpenAI (POST // /v1/chat/completions, Bearer auth, SSE `data: {...}` + `data: [DONE]`) // plus a daemon-side tool loop: the handler injects an OpenAI @@ -1237,6 +1205,7 @@ export function registerChatRoutes(app: Express, ctx: RegisterChatRoutesDeps) { app.post('/api/proxy/senseaudio/stream', async (req, res) => { const proxyBody = req.body || {}; + if (rejectProxyPluginContext(proxyBody, res)) return; const { baseUrl, apiKey, diff --git a/apps/daemon/tests/byok-tools.test.ts b/apps/daemon/tests/byok-tools.test.ts index 93a8689b07..815dc6e674 100644 --- a/apps/daemon/tests/byok-tools.test.ts +++ b/apps/daemon/tests/byok-tools.test.ts @@ -7,30 +7,8 @@ import { BYOK_SENSEAUDIO_TOOLS, executeGenerateImage, executeGenerateVideo, - isSafeByokImageId, } from '../src/byok-tools.js'; -describe('isSafeByokImageId', () => { - it('accepts ordinary id-shaped filenames', () => { - expect(isSafeByokImageId('abc123.png')).toBe(true); - expect(isSafeByokImageId('lq8h2k-abcd1234.png')).toBe(true); - expect(isSafeByokImageId('A1_B2.png')).toBe(true); - }); - - it('rejects directory traversal and unsafe characters', () => { - expect(isSafeByokImageId('../etc/passwd')).toBe(false); - expect(isSafeByokImageId('..png')).toBe(false); - expect(isSafeByokImageId('a/b.png')).toBe(false); - expect(isSafeByokImageId('a\\b.png')).toBe(false); - expect(isSafeByokImageId('.hidden.png')).toBe(false); - expect(isSafeByokImageId('with space.png')).toBe(false); - expect(isSafeByokImageId('a\0b.png')).toBe(false); - expect(isSafeByokImageId('a\nb.png')).toBe(false); - expect(isSafeByokImageId('')).toBe(false); - expect(isSafeByokImageId('a'.repeat(200))).toBe(false); - }); -}); - describe('BYOK_SENSEAUDIO_TOOLS', () => { it('exports an OpenAI-shaped generate_image tool definition', () => { const tool = BYOK_SENSEAUDIO_TOOLS.find( diff --git a/apps/daemon/tests/proxy-routes.test.ts b/apps/daemon/tests/proxy-routes.test.ts index 3528bf622e..1a1ee1f707 100644 --- a/apps/daemon/tests/proxy-routes.test.ts +++ b/apps/daemon/tests/proxy-routes.test.ts @@ -1014,21 +1014,6 @@ describe('API proxy routes', () => { expect(fetchMock).not.toHaveBeenCalled(); }); - // Legacy /api/byok-image/:id route — kept around so chat history - // links from before the migration to project-scoped storage still - // resolve. New tool calls write to project folders; these tests - // pin the legacy surface so the deprecated route continues to - // reject path traversal and 404 cleanly on misses. - it('rejects unsafe byok-image ids with 400', async () => { - const res = await realFetch(`${baseUrl}/api/byok-image/..%2Fpasswd`); - expect(res.status).toBe(400); - }); - - it('returns 404 for unknown byok-image ids', async () => { - const res = await realFetch(`${baseUrl}/api/byok-image/this-id-does-not-exist.png`); - expect(res.status).toBe(404); - }); - // Plan §3.A4 / spec §11.8 (e2e-7): the API-fallback proxy paths must // never carry plugin context. The web sidecar's fallback mode bypasses // the daemon snapshot bus, so any pluginId / appliedPluginSnapshotId in @@ -1040,6 +1025,7 @@ describe('API proxy routes', () => { '/api/proxy/openai/stream', '/api/proxy/azure/stream', '/api/proxy/google/stream', + '/api/proxy/senseaudio/stream', ]; for (const path of proxies) {