From 4cc75257ff6db420d6b7f1384233278b345fbeb9 Mon Sep 17 00:00:00 2001 From: Bryandero98 Date: Tue, 1 Sep 2026 12:23:27 -0500 Subject: [PATCH] fix: dedupe insightsService's transport onto the shared aiProvider one insightsService.js defined a second callProviderAISimple, behaviorally divergent from the canonical server/services/aiProvider.js one: it classified a non-JSON/blank 200 body as null (surfacing an error, as intended) but a *valid* 200 body with empty content as a successful { text: '' } - the opposite of aiProvider's classification, which treats a whitespace-only completion as a provider error. Both generateThemeAnalysis and refreshCrossDomainNarrative persist their result unconditionally when result.error is unset, so the divergent copy could write an empty narrative/theme over a real cached one on an otherwise-valid-but-content-empty response. Delete the duplicate and route both entry points through the shared aiProvider.callProviderAISimple - same call shape, drop-in compatible. The transport/malformed-body/SSRF-guard test matrix moves to aiProvider.test.js (where the shared implementation now lives); insightsService.test.js keeps only the disk-only-read-path contract, per its own docstring's scope. Fixes atomantic#5617 --- server/services/aiProvider.test.js | 91 ++++++++++++++++++++ server/services/insightsService.js | 70 +--------------- server/services/insightsService.test.js | 106 +++--------------------- 3 files changed, 105 insertions(+), 162 deletions(-) diff --git a/server/services/aiProvider.test.js b/server/services/aiProvider.test.js index 9d6b76c9eb..11a230b93e 100644 --- a/server/services/aiProvider.test.js +++ b/server/services/aiProvider.test.js @@ -363,3 +363,94 @@ describe('callProviderAISimple through the ChatGPT subscription', () => { expect(result).toMatchObject({ text: 'fallback answer' }); }); }); + +// The regression these two guard: a non-JSON/blank 200 body used to return +// { text: '' } from the (now-deleted) insightsService copy of this transport, +// which refreshCrossDomainNarrative / generateThemeAnalysis then persisted over +// narrative.json / themes.json — overwriting the cached result with nothing. +// The shared transport must surface an error instead, so the `if (result.error) +// return` guard at both call sites bails before any write. +describe('callProviderAISimple — malformed / non-2xx responses', () => { + const provider = { id: 'provider-1', name: 'Example Provider', type: 'api', endpoint: 'https://api.example.com/v1' }; + + const respondWithText = (text, { ok = true, status = 200 } = {}) => + vi.stubGlobal('fetch', vi.fn(async () => ({ ok, status, text: async () => text }))); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it('returns an error, not an empty success, on a non-JSON 200 body', async () => { + respondWithText('502 Bad Gateway'); + + const result = await callProviderAISimple(provider, 'model-1', 'prompt'); + + expect(result.text).toBeUndefined(); + expect(result.error).toMatch(/malformed/i); + }); + + it('returns an error on a blank 200 body', async () => { + respondWithText(''); + + const result = await callProviderAISimple(provider, 'model-1', 'prompt'); + + expect(result.error).toMatch(/malformed/i); + }); + + it('returns an error with the status and body on a non-2xx response', async () => { + respondWithText('boom', { ok: false, status: 500 }); + + const result = await callProviderAISimple(provider, 'model-1', 'prompt'); + + expect(result.error).toMatch(/Provider returned 500: boom/); + }); +}); + +describe('callProviderAISimple — endpoint guard (SSRF / key-exfiltration)', () => { + const provider = { id: 'provider-1', name: 'Example Provider', type: 'api', endpoint: 'https://api.example.com/v1' }; + + const respondWithHello = () => vi.stubGlobal('fetch', vi.fn(async () => ({ + ok: true, + status: 200, + text: async () => JSON.stringify({ choices: [{ message: { content: 'hello' } }] }), + }))); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it('blocks a keyed provider pointed at a non-allowlisted endpoint and never calls fetch', async () => { + const fetchSpy = vi.fn(); + vi.stubGlobal('fetch', fetchSpy); + + const result = await callProviderAISimple( + { ...provider, apiKey: 'secret-key', endpoint: 'https://not-an-allowlisted-host.example' }, + 'model-1', 'prompt', + ); + + expect(result.error).toContain('Provider endpoint blocked'); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it('allows a keyed provider on a non-allowlisted host through when allowCustomEndpoint is true', async () => { + respondWithHello(); + + const result = await callProviderAISimple( + { ...provider, apiKey: 'secret-key', endpoint: 'https://not-an-allowlisted-host.example', allowCustomEndpoint: true }, + 'model-1', 'prompt', + ); + + expect(result).toMatchObject({ text: 'hello' }); + }); + + it('allows a keyless provider on a non-allowlisted host through (guard only applies when apiKey is set)', async () => { + respondWithHello(); + + const result = await callProviderAISimple( + { ...provider, endpoint: 'https://not-an-allowlisted-host.example' }, + 'model-1', 'prompt', + ); + + expect(result).toMatchObject({ text: 'hello' }); + }); +}); diff --git a/server/services/insightsService.js b/server/services/insightsService.js index 01c43b409f..256a0b5b1d 100644 --- a/server/services/insightsService.js +++ b/server/services/insightsService.js @@ -19,13 +19,7 @@ import { MARKER_CATEGORIES, CURATED_MARKERS } from '../lib/curatedGenomeMarkers. import { getTasteProfile } from './taste-questionnaire.js'; import { getActiveProvider, getProviderById } from './providers.js'; import { getCorrelationData } from './appleHealthQuery.js'; -import { stripCodeFences, parseLLMJSON } from './aiProvider.js'; -import { fetchWithTimeout } from '../lib/fetchWithTimeout.js'; -import { readResponseJson } from '../lib/readResponseJson.js'; -import { ensureProviderReady as ensureOllamaProviderReady } from './ollamaManager.js'; -import { evaluateSecretEndpoint } from '../lib/aiToolkit/internal/endpointGuard.js'; - -const DEFAULT_AI_TIMEOUT_MS = 300000; +import { stripCodeFences, parseLLMJSON, callProviderAISimple } from './aiProvider.js'; const MARKER_BY_RSID = new Map(CURATED_MARKERS.map(m => [m.rsid, m])); @@ -163,68 +157,6 @@ function getLatestBloodValues(tests) { return latest; } -/** - * Replicate callProviderAISimple pattern from taste-questionnaire.js. - * API-type providers only. Returns { text } on success, { error } on failure. - * Exported for unit testing of the non-JSON-body guard — the public entries that - * reach it (refreshCrossDomainNarrative / generateThemeAnalysis) need the full - * genome/taste/health context mocked, so the guard is exercised directly. - */ -export async function callProviderAISimple(provider, model, prompt, { temperature = 0.3, max_tokens = 1000 } = {}) { - const timeout = provider.timeout || DEFAULT_AI_TIMEOUT_MS; - - if (provider.type === 'api') { - const ready = await ensureOllamaProviderReady(provider).catch((err) => ({ success: false, error: err.message })); - if (!ready.success) { - return { error: `Ollama is not running and PortOS could not start it: ${ready.error || 'unknown error'}` }; - } - - // Never send the API key to an arbitrary/metadata host (SSRF / key - // exfiltration). Keyless local-LLM calls skip this guard entirely. - if (provider.apiKey) { - const guard = evaluateSecretEndpoint(provider.endpoint, { - allowCustomEndpoint: provider.allowCustomEndpoint === true, - }); - if (!guard.allowed) { - return { error: `Provider endpoint blocked: ${guard.reason}` }; - } - } - - const headers = { 'Content-Type': 'application/json' }; - if (provider.apiKey) headers['Authorization'] = `Bearer ${provider.apiKey}`; - - const response = await fetchWithTimeout(`${provider.endpoint}/chat/completions`, { - method: 'POST', - headers, - body: JSON.stringify({ - model, - messages: [{ role: 'user', content: prompt }], - temperature, - max_tokens - }) - }, timeout); - - if (!response.ok) { - const errorText = await response.text().catch(() => 'Unknown error'); - return { error: `Provider returned ${response.status}: ${errorText}` }; - } - - // Sentinel fallback: a non-JSON/blank 200 body must surface as an error, not - // an empty `{ text: '' }` success — both callers persist the result - // (refreshCrossDomainNarrative → narrative.json, generateThemeAnalysis → - // themes.json), so a masqueraded-empty success would overwrite the cached - // narrative/themes with nothing. A valid body (even one with empty content) - // still flows through unchanged. - const data = await readResponseJson(response, { fallback: null, emptyValue: null }); - if (!data) { - return { error: `Provider returned a non-JSON response (${response.status})` }; - } - return { text: data.choices?.[0]?.message?.content || '' }; - } - - return { error: 'Insights analysis requires an API-based provider' }; -} - // ============================================================================= // EXPORTED FUNCTIONS // ============================================================================= diff --git a/server/services/insightsService.test.js b/server/services/insightsService.test.js index ebe5b68eeb..1d68f32f6b 100644 --- a/server/services/insightsService.test.js +++ b/server/services/insightsService.test.js @@ -1,101 +1,21 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -// Mock the provider-readiness check and the HTTP client so the call runs offline. +// Mock the provider-readiness check so any code path that touches it runs offline. vi.mock('./ollamaManager.js', () => ({ ensureProviderReady: vi.fn().mockResolvedValue({ success: true }), })); -vi.mock('../lib/fetchWithTimeout.js', () => ({ - fetchWithTimeout: vi.fn(), -})); - -import { fetchWithTimeout } from '../lib/fetchWithTimeout.js'; -import { mockJsonResponse, mockTextResponse } from '../lib/testHelper.js'; -import { - callProviderAISimple, - getThemeAnalysis, - getCrossDomainNarrative, -} from './insightsService.js'; - -const PROVIDER = { type: 'api', endpoint: 'http://localhost:1234/v1' }; - -describe('insightsService.callProviderAISimple — non-JSON-body guard', () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - // The regression this guards: a non-JSON 200 body used to return { text: '' }, - // which refreshCrossDomainNarrative / generateThemeAnalysis then persisted over - // narrative.json / themes.json — overwriting the cached result with nothing. - // It must surface as { error } so the `if (result.error) return` guard bails - // before any write. - it('returns { error } (not empty success) on a non-JSON 200 body', async () => { - fetchWithTimeout.mockResolvedValue(mockTextResponse('502 Bad Gateway')); - const result = await callProviderAISimple(PROVIDER, 'm', 'prompt'); - expect(result.text).toBeUndefined(); - expect(result.error).toMatch(/non-JSON response/); - }); - - it('returns { error } on a blank 200 body', async () => { - fetchWithTimeout.mockResolvedValue(mockTextResponse('')); - const result = await callProviderAISimple(PROVIDER, 'm', 'prompt'); - expect(result.error).toMatch(/non-JSON response/); - }); - - // A valid body with empty content is a legitimate (if unusual) result and must - // still flow through as { text: '' } — the guard must not conflate valid-empty - // with a parse failure. - it('returns { text: "" } for a valid body with empty content', async () => { - fetchWithTimeout.mockResolvedValue(mockJsonResponse({ choices: [{ message: { content: '' } }] })); - const result = await callProviderAISimple(PROVIDER, 'm', 'prompt'); - expect(result).toEqual({ text: '' }); - }); - - it('returns the content for a valid populated body', async () => { - fetchWithTimeout.mockResolvedValue(mockJsonResponse({ choices: [{ message: { content: 'hello' } }] })); - const result = await callProviderAISimple(PROVIDER, 'm', 'prompt'); - expect(result).toEqual({ text: 'hello' }); - }); - - it('returns { error } with the status code on a non-2xx response', async () => { - fetchWithTimeout.mockResolvedValue(mockTextResponse('boom', { ok: false, status: 500 })); - const result = await callProviderAISimple(PROVIDER, 'm', 'prompt'); - expect(result.error).toMatch(/Provider returned 500: boom/); - }); +// generateThemeAnalysis/refreshCrossDomainNarrative now call the shared +// aiProvider.callProviderAISimple transport (see aiProvider.test.js for its own +// contract coverage) — stub only that export so the disk-only read paths below +// can assert it's never reached, while stripCodeFences/parseLLMJSON (also +// imported from this module) stay real. +vi.mock('./aiProvider.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, callProviderAISimple: vi.fn() }; }); -describe('insightsService.callProviderAISimple — endpoint guard (SSRF / key-exfiltration)', () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - it('blocks a keyed provider pointed at a non-allowlisted endpoint and never calls fetch', async () => { - const result = await callProviderAISimple( - { ...PROVIDER, apiKey: 'secret-key', endpoint: 'https://not-an-allowlisted-host.example' }, - 'm', 'prompt', - ); - expect(result.error).toContain('Provider endpoint blocked'); - expect(fetchWithTimeout).not.toHaveBeenCalled(); - }); - - it('allows a keyed provider on a non-allowlisted host through when allowCustomEndpoint is true', async () => { - fetchWithTimeout.mockResolvedValue(mockJsonResponse({ choices: [{ message: { content: 'hello' } }] })); - const result = await callProviderAISimple( - { ...PROVIDER, apiKey: 'secret-key', endpoint: 'https://not-an-allowlisted-host.example', allowCustomEndpoint: true }, - 'm', 'prompt', - ); - expect(result).toEqual({ text: 'hello' }); - expect(fetchWithTimeout).toHaveBeenCalled(); - }); - - it('allows a keyless provider on a non-allowlisted host through (guard only applies when apiKey is set)', async () => { - fetchWithTimeout.mockResolvedValue(mockJsonResponse({ choices: [{ message: { content: 'hello' } }] })); - const result = await callProviderAISimple( - { ...PROVIDER, endpoint: 'https://not-an-allowlisted-host.example' }, - 'm', 'prompt', - ); - expect(result).toEqual({ text: 'hello' }); - }); -}); +import { callProviderAISimple } from './aiProvider.js'; +import { getThemeAnalysis, getCrossDomainNarrative } from './insightsService.js'; // Enforces the no-cold-bootstrap trigger contract documented at the generation // entry points: the cached-read paths the Insights page mounts with must be @@ -109,13 +29,13 @@ describe('insightsService read paths — disk-only, no provider call', () => { it('getThemeAnalysis performs no provider call (returns not_generated when uncached)', async () => { const result = await getThemeAnalysis(); - expect(fetchWithTimeout).not.toHaveBeenCalled(); + expect(callProviderAISimple).not.toHaveBeenCalled(); expect(result.available === false || result.available === true).toBe(true); }); it('getCrossDomainNarrative performs no provider call (returns not_generated when uncached)', async () => { const result = await getCrossDomainNarrative(); - expect(fetchWithTimeout).not.toHaveBeenCalled(); + expect(callProviderAISimple).not.toHaveBeenCalled(); expect(result.available === false || result.available === true).toBe(true); }); });