Task/improve validation of web fetch response (#8362)

* Improve validation of web_fetch tool response

* Update changelog

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- Improved the validation of the response of the `web_fetch` tool in the `FetchService`
 - Deprecated the `api/v1/admin/user`, `api/v1/user/:id` and `api/v1/user/:id/access-token` endpoints in favor of the `api/v1/users` endpoints
 - Upgraded `chart.js` from version `4.5.1` to `4.6.1`
 

apps/api/src/helper/web-fetch.helper.spec.ts

@@ -0,0 +1,112 @@
+import { getWebFetchBody } from './web-fetch.helper';
+
+const FAILURE_MESSAGE =
+  '{"url":"https://www.example.com/data.json","status":"failed","error":"HTTP 405: Method Not Allowed","_failure_reason":"http_error","http_status":405}';
+
+const BODY = '{"items":[{"id":1,"name":"Example"}],"total":1}';
+
+describe('getWebFetchBody', () => {
+  describe('with a JSON route', () => {
+    it.each([
+      ['the raw body', BODY],
+      ['leading line breaks', `\n\n\n\n${BODY}`],
+      ['a code fence', `\n\n\`\`\`json\n${BODY}\n\`\`\``],
+      ['an intro text', `I'll fetch that URL for you.\n\n${BODY}`]
+    ])('should get the body from %s', (_, candidate) => {
+      expect(getWebFetchBody({ candidate, isJson: true })).toBe(BODY);
+    });
+
+    it('should get an array', () => {
+      expect(
+        getWebFetchBody({ candidate: 'Result:\n[{"a":1}]', isJson: true })
+      ).toBe('[{"a":1}]');
+    });
+
+    it.each([
+      ['the raw message', FAILURE_MESSAGE],
+      [
+        'a text before the message',
+        `The fetch failed — no response body was returned. The tool reported:\n\n${FAILURE_MESSAGE}`
+      ],
+      [
+        'a text around the message',
+        `I'll fetch that URL for you.\n\nThe fetch did not succeed, so there is no response body to return. The exact tool result was:\n\n${FAILURE_MESSAGE}\n\nHTTP 405 (Method Not Allowed) means the server rejected the request method used by the fetch tool for this endpoint.`
+      ],
+      [
+        'a formatted message',
+        'I\'ll fetch that URL for you.\n\n{\n  "url": "https://www.example.com/data.json",\n  "status": "failed",\n  "error": "The engine returned no content for this URL.",\n  "_failure_reason": "empty_result"\n}'
+      ]
+    ])('should reject a failure message: %s', (_, candidate) => {
+      expect(getWebFetchBody({ candidate, isJson: true })).toBeUndefined();
+    });
+
+    it.each([
+      ['an empty text', ' \n '],
+      ['a number', '42'],
+      ['a string', '"Example"'],
+      ['null', 'null'],
+      [
+        'a text without JSON',
+        '`AccessDenied`Access Denied\n\n<Error>\n\n<Code>AccessDenied</Code>\n\n<Message>Access Denied</Message>\n\n...\n\n</Error>'
+      ],
+      [
+        'a failure in plain words',
+        'The fetch did not succeed, so there is no response body to return.\n\n- Status: failed\n- HTTP status: 403 (Forbidden)'
+      ],
+      [
+        'a body with broken JSON',
+        '{"items":{"a":{"id":1,"value":0.1},"b\\":{\\"id\\":2,\\"value\\":0.2},"c":{"id":3,"value":0.3}}}'
+      ],
+      ['two JSON objects', `${BODY}\n\n${BODY}`],
+      ['a citation marker', 'The fetch failed with HTTP 403 [1].'],
+      ['an empty array after a text', 'No data was returned []'],
+      ['an object after a citation marker', 'Sources [1]: {"a":1}'],
+      ['an object after a text in brackets', `[Note] ${BODY}`],
+      [
+        'an array in a broken object',
+        '{"name":"bad\\q","items":[{"id":1},{"id":2}],"total":2}'
+      ],
+      ['an object in a broken array', '[{"a":1},]'],
+      [
+        'a false tool call',
+        '\n\n{"url":"https://www.example.com/data.json"}</parameter>'
+      ]
+    ])('should reject %s', (_, candidate) => {
+      expect(getWebFetchBody({ candidate, isJson: true })).toBeUndefined();
+    });
+
+    it('should not reject an object which has only some keys of a failure message', () => {
+      const candidate = '{"id":1,"status":"failed"}';
+
+      expect(getWebFetchBody({ candidate, isJson: true })).toBe(candidate);
+    });
+  });
+
+  describe('with a route for other content', () => {
+    it.each([
+      ['a text', 'Plain text'],
+      ['HTML with brackets', '<script>if (a) { b(); }</script>'],
+      ['JSON', BODY]
+    ])('should get %s without a change', (_, candidate) => {
+      expect(getWebFetchBody({ candidate, isJson: false })).toBe(candidate);
+    });
+
+    it('should remove the white space around the body', () => {
+      expect(getWebFetchBody({ candidate: ' \nBody\n ', isJson: false })).toBe(
+        'Body'
+      );
+    });
+
+    it('should reject a failure message', () => {
+      expect(
+        getWebFetchBody({ candidate: FAILURE_MESSAGE, isJson: false })
+      ).toBeUndefined();
+    });
+
+    it('should reject an empty text', () => {
+      expect(
+        getWebFetchBody({ candidate: ' \n ', isJson: false })
+      ).toBeUndefined();
+    });
+  });
+});

apps/api/src/helper/web-fetch.helper.ts

@@ -0,0 +1,70 @@
+/**
+ * Gets the response body from a candidate of the `web_fetch` tool. The model
+ * retypes the body, so that JSON can have code fences or text around it. Gives
+ * back `undefined` if the candidate has no usable body.
+ */
+export function getWebFetchBody({
+  candidate,
+  isJson
+}: {
+  candidate: string;
+  isJson: boolean;
+}): string | undefined {
+  const body = candidate.trim();
+
+  if (!body) {
+    return undefined;
+  }
+
+  const json = extractJson(body);
+
+  if (isWebFetchFailureMessage(json?.value)) {
+    return undefined;
+  }
+
+  if (!isJson) {
+    return body;
+  }
+
+  return json?.isWhole ? json.text : undefined;
+}
+
+/**
+ * Extracts the JSON object or array from a text. The JSON must start at the
+ * beginning of a line and goes to the last matching closing bracket, so that a
+ * bracket in a sentence gives no result. `isWhole` tells if only a closing code
+ * fence follows the JSON.
+ */
+function extractJson(text: string) {
+  const start = text.search(/^[[{]/m);
+
+  if (start < 0) {
+    return undefined;
+  }
+
+  const end = text.lastIndexOf(text[start] === '{' ? '}' : ']') + 1;
+  const jsonText = text.slice(start, end);
+
+  try {
+    return {
+      isWhole: /^(```)?$/.test(text.slice(end).trim()),
+      text: jsonText,
+      value: JSON.parse(jsonText) as unknown
+    };
+  } catch {
+    return undefined;
+  }
+}
+
+/**
+ * Checks for the result of a failed fetch, which the model can give back as if
+ * it was the response body:
+ * { "url": "…", "status": "failed", "error": "HTTP 405: Method Not Allowed" }
+ */
+function isWebFetchFailureMessage(value: unknown) {
+  const { error, status, url } = (value ?? {}) as Record<string, unknown>;
+
+  return (
+    status === 'failed' && typeof error === 'string' && typeof url === 'string'
+  );
+}

apps/api/src/services/fetch/fetch.service.spec.ts

@@ -0,0 +1,259 @@
+import { PropertyService } from '@ghostfolio/api/services/property/property.service';
+import {
+  PROPERTY_API_KEY_OPENROUTER,
+  PROPERTY_OPENROUTER_ENGINE_WEB_FETCH,
+  PROPERTY_OPENROUTER_MODEL_WEB_FETCH,
+  PROPERTY_WEB_FETCH_ROUTES
+} from '@ghostfolio/common/config';
+
+import { Logger } from '@nestjs/common';
+import { createOpenRouter } from '@openrouter/ai-sdk-provider';
+import { generateText } from 'ai';
+
+import { FetchService } from './fetch.service';
+
+// The packages are ECMAScript modules, which Jest cannot load
+jest.mock('@openrouter/ai-sdk-provider', () => {
+  return { createOpenRouter: jest.fn() };
+});
+jest.mock('ai', () => {
+  return {
+    generateText: jest.fn(),
+    jsonSchema: jest.fn(),
+    tool: jest.fn((options) => {
+      return options;
+    })
+  };
+});
+
+const BODY = '{"items":[{"id":1,"name":"Example"}],"total":1}';
+
+const FAILURE_MESSAGE =
+  '{"url":"https://www.example.com/data.json","status":"failed","error":"HTTP 405: Method Not Allowed","http_status":405}';
+
+const URL_WITH_ROUTE = 'https://www.example.com/data.json';
+
+describe('FetchService', () => {
+  let directFetch: jest.SpyInstance;
+  let fetchService: FetchService;
+  let properties: Record<string, unknown>;
+
+  const mockGenerateText = (result: {
+    finishReason?: string;
+    sources?: unknown[];
+    text?: string;
+  }) => {
+    (generateText as jest.Mock).mockResolvedValue({
+      finishReason: 'stop',
+      sources: [],
+      text: '',
+      ...result
+    });
+  };
+
+  beforeEach(async () => {
+    jest.clearAllMocks();
+
+    (createOpenRouter as jest.Mock).mockReturnValue({
+      chat: jest.fn((model: string) => {
+        return model;
+      })
+    });
+
+    directFetch = jest
+      .spyOn(globalThis, 'fetch')
+      .mockResolvedValue(new Response('direct'));
+
+    properties = {
+      [PROPERTY_API_KEY_OPENROUTER]: 'api-key',
+      [PROPERTY_OPENROUTER_ENGINE_WEB_FETCH]: 'parallel',
+      [PROPERTY_OPENROUTER_MODEL_WEB_FETCH]: 'model',
+      [PROPERTY_WEB_FETCH_ROUTES]: [
+        {
+          domain: 'example.com',
+          responseContentType: 'application/json'
+        },
+        { domain: 'example.org' }
+      ]
+    };
+
+    fetchService = new FetchService({
+      getByKey: jest.fn((key: string) => {
+        return Promise.resolve(properties[key]);
+      })
+    } as unknown as PropertyService);
+
+    await fetchService.onModuleInit();
+  });
+
+  afterEach(() => {
+    directFetch.mockRestore();
+  });
+
+  it('should get the body from the text of the model', async () => {
+    mockGenerateText({ text: `\`\`\`json\n${BODY}\n\`\`\`` });
+
+    const response = await fetchService.fetch(URL_WITH_ROUTE);
+
+    await expect(response.text()).resolves.toBe(BODY);
+
+    expect(response.headers.get('content-type')).toBe('application/json');
+    expect(directFetch).not.toHaveBeenCalled();
+  });
+
+  it('should get the body from the content of a source first', async () => {
+    mockGenerateText({
+      sources: [{ providerMetadata: { openrouter: { content: BODY } } }],
+      text: '{"text":true}'
+    });
+
+    const response = await fetchService.fetch(URL_WITH_ROUTE);
+
+    await expect(response.text()).resolves.toBe(BODY);
+  });
+
+  it('should use the text of the model after a source with a failure message', async () => {
+    mockGenerateText({
+      sources: [
+        { providerMetadata: { openrouter: { content: FAILURE_MESSAGE } } }
+      ],
+      text: BODY
+    });
+
+    const response = await fetchService.fetch(URL_WITH_ROUTE);
+
+    await expect(response.text()).resolves.toBe(BODY);
+  });
+
+  it('should get the body from the content of a source on a truncated text', async () => {
+    mockGenerateText({
+      finishReason: 'length',
+      sources: [{ providerMetadata: { openrouter: { content: BODY } } }],
+      text: '{"items":['
+    });
+
+    const response = await fetchService.fetch(URL_WITH_ROUTE);
+
+    await expect(response.text()).resolves.toBe(BODY);
+
+    expect(directFetch).not.toHaveBeenCalled();
+  });
+
+  it('should log the failure message of a source after an empty text', async () => {
+    const debug = jest.spyOn(Logger.prototype, 'debug');
+
+    mockGenerateText({
+      sources: [
+        { providerMetadata: { openrouter: { content: FAILURE_MESSAGE } } }
+      ],
+      text: ''
+    });
+
+    await fetchService.fetch(URL_WITH_ROUTE);
+
+    expect(debug.mock.calls.join()).toContain('HTTP 405');
+
+    debug.mockRestore();
+  });
+
+  it('should redact the API keys in the log of a rejected body', async () => {
+    const debug = jest.spyOn(Logger.prototype, 'debug');
+
+    mockGenerateText({
+      text: '{"url":"https://www.example.com/data.json?apikey=secret&api_token=secret","status":"failed","error":"HTTP 405"}'
+    });
+
+    await fetchService.fetch(URL_WITH_ROUTE);
+
+    const logs = debug.mock.calls.join();
+
+    expect(logs).toContain('apikey=*******');
+    expect(logs).toContain('api_token=*******');
+    expect(logs).not.toContain('secret');
+
+    debug.mockRestore();
+  });
+
+  it('should call the model with temperature 0 and the configured engine', async () => {
+    mockGenerateText({ text: BODY });
+
+    await fetchService.fetch(URL_WITH_ROUTE);
+
+    expect(generateText).toHaveBeenCalledWith(
+      expect.objectContaining({
+        model: 'model',
+        temperature: 0,
+        tools: {
+          web_fetch: expect.objectContaining({
+            args: { parameters: { engine: 'parallel' } }
+          })
+        }
+      })
+    );
+  });
+
+  it.each([
+    ['a failure message', { text: FAILURE_MESSAGE }],
+    ['a text without JSON', { text: 'The fetch did not succeed.' }],
+    ['an empty text', { text: '' }],
+    ['a truncated body', { finishReason: 'length', text: BODY }],
+    ['a content filter', { finishReason: 'content-filter', text: BODY }]
+  ])('should use the direct request on %s', async (_, result) => {
+    mockGenerateText(result);
+
+    const response = await fetchService.fetch(URL_WITH_ROUTE);
+
+    await expect(response.text()).resolves.toBe('direct');
+
+    expect(directFetch).toHaveBeenCalledTimes(1);
+  });
+
+  it('should use the direct request on an error of the model', async () => {
+    (generateText as jest.Mock).mockRejectedValue(new Error('Timeout'));
+
+    const response = await fetchService.fetch(URL_WITH_ROUTE);
+
+    await expect(response.text()).resolves.toBe('direct');
+  });
+
+  it('should get a body which is not JSON for a route without a content type', async () => {
+    mockGenerateText({ text: 'Plain text' });
+
+    const response = await fetchService.fetch('https://example.org/page');
+
+    await expect(response.text()).resolves.toBe('Plain text');
+
+    expect(directFetch).not.toHaveBeenCalled();
+  });
+
+  it('should reject a failure message for a route without a content type', async () => {
+    mockGenerateText({ text: FAILURE_MESSAGE });
+
+    const response = await fetchService.fetch('https://example.org/page');
+
+    await expect(response.text()).resolves.toBe('direct');
+  });
+
+  it('should not use the model without an API key', async () => {
+    delete properties[PROPERTY_API_KEY_OPENROUTER];
+
+    await fetchService.fetch(URL_WITH_ROUTE);
+
+    expect(generateText).not.toHaveBeenCalled();
+    expect(directFetch).toHaveBeenCalledTimes(1);
+  });
+
+  it('should not use the model for a request other than GET', async () => {
+    await fetchService.fetch(URL_WITH_ROUTE, { method: 'POST' });
+
+    expect(generateText).not.toHaveBeenCalled();
+    expect(directFetch).toHaveBeenCalledTimes(1);
+  });
+
+  it('should not use the model for a domain without a route', async () => {
+    await fetchService.fetch('https://www.ghostfol.io');
+
+    expect(generateText).not.toHaveBeenCalled();
+    expect(directFetch).toHaveBeenCalledTimes(1);
+  });
+});

apps/api/src/services/fetch/fetch.service.ts

@@ -1,4 +1,5 @@
 import { redactPaths } from '@ghostfolio/api/helper/object.helper';
+import { getWebFetchBody } from '@ghostfolio/api/helper/web-fetch.helper';
 import { PropertyService } from '@ghostfolio/api/services/property/property.service';
 import {
   DEFAULT_OPENROUTER_ENGINE_WEB_FETCH,
@@ -119,13 +120,14 @@ export class FetchService implements OnModuleInit {
     try {
       const openRouterService = createOpenRouter({ apiKey: openRouterApiKey });
 
-      const { sources, text } = await generateText({
+      const { finishReason, sources, text } = await generateText({
         model: openRouterService.chat(model),
         prompt: [
           'You have access to a web_fetch tool. You MUST call it to retrieve the URL below, do not answer from prior knowledge.',
           'Return the fetched response body exactly as received: raw body only, no commentary, no Markdown, and no code fences.',
           `URL: ${url}`
         ].join('\n'),
+        temperature: 0,
         timeout: FetchService.WEB_FETCH_TIMEOUT,
         tools: {
           // Provider-executed tool: lets OpenRouter perform the actual web
@@ -149,13 +151,23 @@ export class FetchService implements OnModuleInit {
         }
       });
 
+      if (finishReason !== 'stop') {
+        this.logger.warn(
+          `Web fetch tool stopped with the reason "${finishReason}" for ${this.redactUrl(url)}`
+        );
+      }
+
       const candidates = [
         ...(sources ?? []).map((source) => {
           return source.providerMetadata?.openrouter?.content;
         }),
-        text
+        ...(finishReason === 'stop' ? [text] : [])
       ];
 
+      const isJson =
+        webFetchRoute.responseContentType?.includes('application/json') ??
+        false;
+
       let rejectedBody: string | undefined;
 
       for (const candidate of candidates) {
@@ -163,21 +175,13 @@ export class FetchService implements OnModuleInit {
           continue;
         }
 
-        const body = candidate.trim();
+        const body = getWebFetchBody({ candidate, isJson });
 
         if (!body) {
-          continue;
-        }
-
-        if (webFetchRoute.responseContentType?.includes('application/json')) {
-          try {
-            JSON.parse(body);
-          } catch {
-            rejectedBody = body;
+          rejectedBody = candidate.trim() || rejectedBody;
 
           continue;
         }
-        }
 
         this.logger.debug(`Routed ${this.redactUrl(url)} via web fetch tool`);
 
@@ -264,7 +268,16 @@ export class FetchService implements OnModuleInit {
       return '(empty)';
     }
 
-    const singleLineBody = body.replace(/\s+/g, ' ').trim();
+    const singleLineBody = body
+      .replace(
+        new RegExp(
+          `(${FetchService.REDACTED_QUERY_PARAM_NAMES.join('|')})=[^&\\s"']+`,
+          'gi'
+        ),
+        '$1=*******'
+      )
+      .replace(/\s+/g, ' ')
+      .trim();
 
     return singleLineBody.length > FetchService.BODY_PREVIEW_LENGTH
       ? `${singleLineBody.slice(0, FetchService.BODY_PREVIEW_LENGTH)}…`