fix(web-search): harden client response boundary

2026-09-01 03:49:52 +08:00
parent 4a1e5d3121
commit 49de82c486
3 changed files with 355 additions and 16 deletions
--- a/.project-docs/30-worklog/tasks/20260901-web-search-rev01-client-remediation-e4a7c9b2.md
+++ b/.project-docs/30-worklog/tasks/20260901-web-search-rev01-client-remediation-e4a7c9b2.md
@@ -0,0 +1,105 @@
+# Task: REV-01 Client WebSearchClient remediation
+
+## Identity
+
+- Task ID: 20260901-web-search-rev01-client-remediation-e4a7c9b2
+- Mode: Feature
+- Branch: codex/20260901-web-search-rev01-client-remediation-e4a7c9b2-web-search-rev01-client-remediation
+- Worktree: D:\Datas\OthersProjects\makelore-web-search-rev01-client-remediation-e4a7c9b2
+- Base commit: 4a1e5d31213191a0102eab9278dd9887ba8738f4
+- Owner: web-search-rev01-client-remediation
+- Status: Ready for Integration
+
+## Scope
+
+- Remediate the four accepted REV-01 Client Spec-axis findings from the exact
+  coordinator frontier `4a1e5d31213191a0102eab9278dd9887ba8738f4`.
+- Ownership is limited to `electron/services/web-search-client.ts`, its
+  focused Web Search client tests, and this task record. No Server,
+  composition, Pi, Package Store, Renderer, or shared product-scope changes.
+- Preserve the fixed typed route, trusted admission, same logical-operation
+  replay, closed capability envelope, and external PostgreSQL/live OpenAI/
+  production-activation holds.
+
+## Intent And Constraints
+
+- Project Context Loaded:
+  - Task ID: `20260901-web-search-rev01-client-remediation-e4a7c9b2`
+  - Mode: Feature
+  - Branch: `codex/20260901-web-search-rev01-client-remediation-e4a7c9b2-web-search-rev01-client-remediation`
+  - Worktree: `D:\Datas\OthersProjects\makelore-web-search-rev01-client-remediation-e4a7c9b2`
+  - Base commit: `4a1e5d31213191a0102eab9278dd9887ba8738f4`
+  - Other active local tasks: client coordinator, MLW-01/02/03 source tasks,
+    and the read-only REV-01 client/server review tasks. Their records were
+    inspected for scope; no product writer overlaps this remediation.
+  - Overlap/semantic assessment: the four findings are confined to the
+    WebSearchClient response boundary and have one authorized owner. No
+    semantic conflict with server DTO, composition, or Package Store scope.
+- Read: client `AGENTS.md`; complete `maintain-project-docs`, `implement-spec`,
+  and `tdd` skills; project entry files; client architecture/domain/decision,
+  evidence/reflection/commitment/stale indexes; the Web Search design and
+  implementation Spec; REV-01 client Spec and Standards task records.
+- Concurrent Task Gate: Passed. `check_project_docs.py` passed; task_context
+  created the isolated worktree and `status --json` matches this task ID,
+  owner, worktree, branch, and exact base.
+- Planning Gate: Passed. The fixed client review identified exactly four
+  actionable response-boundary findings; the current plan remains within the
+  accepted ownership and does not alter the frozen contract.
+- Implementation plan:
+  1. Add public-seam tests that fail for status/body precedence, streamed
+     response bounds, closed status/receipt/result pairings, and source URL
+     validation.
+  2. Make the smallest WebSearchClient changes to pass each red test: status
+     first with bounded Retry-After, chunked streaming cap, closed receipt and
+     result invariants, and absolute HTTP(S) URL validation without userinfo.
+  3. Run focused and adjacent tests, typecheck, lint, diff/doc gates; then
+     update this record and create one source commit only after a clean handoff.
+
+## Outcome
+
+- Implemented the four accepted WebSearchClient response-boundary fixes:
+  HTTP-status-first 429 handling with bounded Retry-After, a true streamed
+  response cap, closed search/billing/error pairings, and safe bounded source
+  URLs. No file outside the owned client service/test/task-record scope was
+  changed.
+
+## Verification
+
+- Concurrent/Planning gates passed.
+- TDD RED: the new focused suite exposed 8 failures in 16 tests for unsafe
+  URLs, 429 body precedence, chunked buffering, malformed receipts/pairings,
+  and the now-invalid failed-without-error fixture.
+- TDD GREEN: `pnpm exec vitest run tests/unit/web-search-client.test.ts`
+  passed 16/16.
+- Adjacent: `pnpm exec vitest run tests/unit/web-search-client.test.ts
+  tests/unit/web-search-plugin-adapter.test.ts` passed 27/27.
+- `pnpm run typecheck` passed; owned-file ESLint passed with zero errors.
+- Full `pnpm test` passed: 214 files, 1,759 tests passed, 2 skipped; the
+  pressure suite passed 1/1. Full `pnpm run lint:check` passed with zero
+  errors and five pre-existing warnings outside this task's ownership.
+- Task-aware doc drift and source diff checks passed; the final source commit
+  and clean `task_context` handoff are recorded below.
+
+## Handoff
+
+- Source commit: the final task `HEAD` handed off to the coordinator; its
+  sole parent is the exact coordinator frontier
+  `4a1e5d31213191a0102eab9278dd9887ba8738f4`.
+- `task_context complete` passed with `READY_FOR_INTEGRATION`; this source
+  commit is the sole integration candidate.
+
+## Follow-ups
+
+- Coordinator must integrate only the final source commit and trigger a fresh
+  fixed-range Standards/Spec review.
+
+## Promotion Candidates
+
+- Target: client integration coordinator and fresh REV-01 review checkpoint.
+  Proposal: preserve HTTP-status-first semantics, enforce a real 1 MiB stream
+  cap, reject incoherent closed receipts/results, and reject unsafe source URLs.
+  Evidence: REV-01 Client Spec findings and new public-seam regressions.
+  Future impact: prevents malformed responses from overriding server status,
+  unbounded chunked buffering, ungrounded success results, or provider URL
+  leakage. Semantic conflicts: none; human confirmation: not required for this
+  in-scope remediation.
--- a/electron/services/web-search-client.ts
+++ b/electron/services/web-search-client.ts
@@ -12,7 +12,7 @@ const MAX_REQUEST_BYTES = 98_304;
 const MAX_QUERY_LENGTH = 2_000;
 const MAX_ANSWER_LENGTH = 16_000;
 const MAX_SOURCE_TITLE_LENGTH = 240;
-const MAX_SOURCE_URL_LENGTH = 4_096;
+const MAX_SOURCE_URL_LENGTH = 2_048;
 const MAX_SEARCH_QUERY_LENGTH = 500;
 const MAX_RETRY_AFTER_SECONDS = 86_400;
 const DECIMAL = /^(?:0|[1-9]\d*)\.\d{2}$/u;
@@ -36,6 +36,17 @@ const KNOWN_ERROR_CODES = new Set([
  'web_search_result_invalid',
  'web_search_submission_unknown',
 ]);
+const FAILED_ERROR_CODES = new Set([
+  'plugin_reservation_expired',
+  'plugin_reservation_unavailable',
+  'web_search_provider_rejected',
+  'web_search_rate_limited',
+  'web_search_request_invalid',
+]);
+const SUBMISSION_UNKNOWN_ERROR_CODES = new Set([
+  'web_search_result_invalid',
+  'web_search_submission_unknown',
+]);

 type FetchImplementation = typeof fetch;
 type AccessTokenGetter = typeof getValidWorksSquareAccessToken;
@@ -163,9 +174,12 @@ function billing(value: unknown): WebSearchServerBillingReceipt {
    || !['reserved', 'dispatched', 'settled', 'released', 'expired', 'pending_review', 'refunded'].includes(status)
    || typeof reservedPoints !== 'string'
    || !DECIMAL.test(reservedPoints)
-    || (value.actual_points !== null && value.actual_points !== undefined && actualPoints === undefined)
+    || (value.actual_points !== null && value.actual_points !== undefined
+      && (actualPoints === undefined || !DECIMAL.test(actualPoints)))
    || (value.usage_amount !== null && value.usage_amount !== undefined && usageAmount === undefined)
+    || (usageAmount !== undefined && usageAmount !== 1)
    || (['settled', 'refunded'].includes(status) && actualPoints === undefined)
+    || (!['settled', 'refunded'].includes(status) && actualPoints !== undefined)
    || value.unit !== 'search_request') {
    throw new WebSearchClientError('plugin_backend_invalid', 502, false, 'Web Search billing receipt is invalid');
  }
@@ -185,12 +199,35 @@ function source(value: unknown): WebSearchSource {
  }
  const title = boundedText(value.title, MAX_SOURCE_TITLE_LENGTH);
  const url = boundedText(value.url, MAX_SOURCE_URL_LENGTH);
-  if (!title || !url) {
+  if (!title || !url || !safeSourceUrl(url)) {
    throw new WebSearchClientError('plugin_backend_invalid', 502, false, 'Web Search source is invalid');
  }
  return { title, url };
 }

+function safeSourceUrl(value: string): boolean {
+  if (!/^https?:\/\//iu.test(value)) return false;
+  let parsed: URL;
+  try {
+    parsed = new URL(value);
+  } catch {
+    return false;
+  }
+  if ((parsed.protocol !== 'http:' && parsed.protocol !== 'https:')
+    || !parsed.hostname
+    || parsed.username.length > 0
+    || parsed.password.length > 0) {
+    return false;
+  }
+  const schemeSeparator = value.indexOf('//');
+  const authorityEnd = value.slice(schemeSeparator + 2).search(/[/?#]/u);
+  const authority = value.slice(
+    schemeSeparator + 2,
+    authorityEnd < 0 ? value.length : schemeSeparator + 2 + authorityEnd,
+  );
+  return !authority.includes('@');
+}
+
 function read(value: unknown, request: WebSearchRequest): WebSearchRead {
  if (!isRecord(value) || !exactKeys(
    value,
@@ -218,6 +255,7 @@ function read(value: unknown, request: WebSearchRequest): WebSearchRead {
  const parsedErrorCode = value.error_code === undefined || value.error_code === null
    ? null
    : errorCode(value.error_code);
+  const parsedBilling = billing(value.billing);
  if (!executionId || !releaseId || !logicalOperationId || !status
    || releaseId !== request.releaseId || logicalOperationId !== request.logicalOperationId
    || (value.answer !== undefined && value.answer !== null && answer === null)
@@ -225,7 +263,17 @@ function read(value: unknown, request: WebSearchRequest): WebSearchRead {
    || !Array.isArray(rawSearchQueries) || rawSearchQueries.length > 8
    || (value.error_code !== undefined && value.error_code !== null && parsedErrorCode === null)
    || (value.retry_after_seconds !== null && value.retry_after_seconds !== undefined && retryAfter === undefined)
-    || (retryAfter !== undefined && (status !== 'failed' || parsedErrorCode !== 'web_search_rate_limited'))) {
+    || (retryAfter !== undefined && (status !== 'failed' || parsedErrorCode !== 'web_search_rate_limited'))
+    || ((status === 'reserved' && parsedBilling.status !== 'reserved')
+      || (status === 'dispatched' && parsedBilling.status !== 'dispatched')
+      || (status === 'succeeded' && !['settled', 'refunded'].includes(parsedBilling.status))
+      || (status === 'failed' && !['released', 'expired'].includes(parsedBilling.status))
+      || (status === 'submission_unknown' && parsedBilling.status !== 'pending_review')
+      || (status === 'pending_review' && parsedBilling.status !== 'pending_review'))
+    || ((status === 'succeeded' || status === 'pending_review') && parsedBilling.usage_amount !== 1)
+    || (status === 'failed' && (parsedErrorCode === null || !FAILED_ERROR_CODES.has(parsedErrorCode)))
+    || (status === 'submission_unknown'
+      && (parsedErrorCode === null || !SUBMISSION_UNKNOWN_ERROR_CODES.has(parsedErrorCode)))) {
    throw new WebSearchClientError('plugin_backend_invalid', 502, false, 'Web Search response is invalid');
  }
  const sources = rawSources.map(source);
@@ -234,7 +282,7 @@ function read(value: unknown, request: WebSearchRequest): WebSearchRead {
    throw new WebSearchClientError('plugin_backend_invalid', 502, false, 'Web Search response is invalid');
  }
  if (status === 'succeeded' || status === 'pending_review') {
-    if (!answer || (status === 'pending_review' && parsedErrorCode !== null)) {
+    if (!answer || sources.length === 0 || parsedErrorCode !== null) {
      throw new WebSearchClientError('plugin_backend_invalid', 502, false, 'Web Search result is incomplete');
    }
  } else if (answer !== null || sources.length > 0 || searchQueries.length > 0) {
@@ -254,7 +302,7 @@ function read(value: unknown, request: WebSearchRequest): WebSearchRead {
    searchQueries: searchQueries as string[],
    errorCode: parsedErrorCode,
    ...(retryAfter === undefined ? {} : { retryAfterSeconds: retryAfter }),
-    billing: billing(value.billing),
+    billing: parsedBilling,
  };
 }

@@ -270,11 +318,39 @@ async function readBoundedJson(response: Response): Promise<unknown> {
    await response.body?.cancel().catch(() => undefined);
    throw new WebSearchClientError('plugin_backend_response_too_large', 502, false, 'Web Search response exceeds its bound');
  }
-  const bytes = new Uint8Array(await response.arrayBuffer());
-  if (bytes.byteLength > MAX_JSON_BYTES) {
-    throw new WebSearchClientError('plugin_backend_response_too_large', 502, false, 'Web Search response exceeds its bound');
+  if (!response.body) return null;
+  const reader = response.body.getReader();
+  const chunks: Uint8Array[] = [];
+  let length = 0;
+  try {
+    while (true) {
+      const { done, value } = await reader.read();
+      if (done) break;
+      if (!(value instanceof Uint8Array)) {
+        throw new WebSearchClientError('plugin_backend_invalid', 502, false, 'Web Search response is invalid');
+      }
+      length += value.byteLength;
+      if (length > MAX_JSON_BYTES) {
+        await reader.cancel().catch(() => undefined);
+        throw new WebSearchClientError(
+          'plugin_backend_response_too_large',
+          502,
+          false,
+          'Web Search response exceeds its bound',
+        );
+      }
+      chunks.push(new Uint8Array(value));
+    }
+  } finally {
+    reader.releaseLock();
+  }
+  if (length === 0) return null;
+  const bytes = new Uint8Array(length);
+  let offset = 0;
+  for (const chunk of chunks) {
+    bytes.set(chunk, offset);
+    offset += chunk.byteLength;
  }
-  if (bytes.byteLength === 0) return null;
  try {
    return JSON.parse(Buffer.from(bytes).toString('utf8')) as unknown;
  } catch {
@@ -356,15 +432,25 @@ function domainMessage(code: string, status: number): string {
 }

 async function domainError(response: Response): Promise<WebSearchClientError> {
+  const retryAfter = response.status === 429 ? retryAfterHeader(response) : undefined;
+  if (response.status === 429) {
+    await response.body?.cancel().catch(() => undefined);
+    return new WebSearchClientError(
+      'web_search_rate_limited',
+      429,
+      false,
+      domainMessage('web_search_rate_limited', 429),
+      retryAfter,
+    );
+  }
  let payload: unknown = null;
  try {
    payload = await readBoundedJson(response);
-  } catch (error) {
-    if (error instanceof WebSearchClientError) return error;
+  } catch {
+    // Preserve the HTTP-status fallback when the bounded error body is unusable.
  }
  const detail = isRecord(payload) && isRecord(payload.detail) ? payload.detail : null;
-  const code = errorCode(detail?.error_code) ?? (response.status === 429 ? 'web_search_rate_limited' : 'plugin_backend_unavailable');
-  const retryAfter = response.status === 429 ? retryAfterHeader(response) : undefined;
+  const code = errorCode(detail?.error_code) ?? 'plugin_backend_unavailable';
  const retryable = response.status >= 500 && response.status !== 401;
  return new WebSearchClientError(
    code,
--- a/tests/unit/web-search-client.test.ts
+++ b/tests/unit/web-search-client.test.ts
@@ -10,6 +10,7 @@ const RELEASE_ID = '22222222-2222-4222-8222-222222222222';
 const ADMISSION_ID = 'admission-a';
 const PROJECT_ID = '33333333-3333-4333-8333-333333333333';
 const LOGICAL_OPERATION_ID = 'pi:run-a:resource-a';
+const MAX_JSON_BYTES = 1_048_576;

 function search(overrides: Record<string, unknown> = {}) {
  return {
@@ -126,7 +127,7 @@ describe('WebSearchClient', () => {
  });

  it('matches the frozen server source title and URL bounds exactly', async () => {
-    const maximumUrl = `https://example.test/${'x'.repeat(4_096 - 'https://example.test/'.length)}`;
+    const maximumUrl = `https://example.test/${'x'.repeat(2_048 - 'https://example.test/'.length)}`;
    await expect(new WebSearchClient({
      fetchImpl: vi.fn<typeof fetch>().mockResolvedValue(jsonResponse(search({
        sources: [{ title: 't'.repeat(240), url: maximumUrl }],
@@ -146,6 +147,25 @@ describe('WebSearchClient', () => {
    });
  });

+  it.each([
+    'http://user:password@example.test/source',
+    'javascript:alert(1)',
+    '//example.test/source',
+    'https:example.test/source',
+    'https://example.test/' + 'x'.repeat(2_049 - 'https://example.test/'.length),
+  ])('rejects an unsafe or overlong source URL: %s', async (url) => {
+    const client = new WebSearchClient({
+      fetchImpl: vi.fn<typeof fetch>().mockResolvedValue(jsonResponse(search({
+        sources: [{ title: 'Source', url }],
+      }))),
+      getAccessToken: vi.fn(async () => 'token') as never,
+    });
+
+    await expect(client.search(input())).rejects.toMatchObject<WebSearchClientError>({
+      code: 'plugin_backend_invalid', status: 502, retryable: false,
+    });
+  });
+
  it('preserves only a bounded Retry-After for a known rate limit', async () => {
    const fetchImpl = vi.fn<typeof fetch>().mockResolvedValue(jsonResponse(
      { detail: { error_code: 'web_search_rate_limited', message: 'try later' } },
@@ -162,6 +182,38 @@ describe('WebSearchClient', () => {
    });
  });

+  it('uses HTTP 429 as the authoritative non-retryable rate-limit result', async () => {
+    const responses = [
+      new Response('{malformed', {
+        status: 429,
+        headers: { 'retry-after': '31' },
+      }),
+      new Response('x'.repeat(MAX_JSON_BYTES + 1), {
+        status: 429,
+        headers: { 'retry-after': '31' },
+      }),
+      jsonResponse(
+        { detail: { error_code: 'plugin_provider_unavailable', message: 'private' } },
+        429,
+        { 'retry-after': '31' },
+      ),
+    ];
+
+    for (const response of responses) {
+      const client = new WebSearchClient({
+        fetchImpl: vi.fn<typeof fetch>().mockResolvedValue(response),
+        getAccessToken: vi.fn(async () => 'token') as never,
+      });
+
+      await expect(client.search(input())).rejects.toMatchObject<WebSearchClientError>({
+        code: 'web_search_rate_limited',
+        status: 429,
+        retryable: false,
+        retryAfterSeconds: 31,
+      });
+    }
+  });
+
  it('projects a server-side rate-limit result and does not trust Retry-After on other statuses', async () => {
    const rateLimited = vi.fn<typeof fetch>().mockResolvedValue(jsonResponse(search({
      status: 'failed',
@@ -207,6 +259,101 @@ describe('WebSearchClient', () => {
    });
  });

+  it('bounds a chunked response before buffering the complete body', async () => {
+    let cancelled = false;
+    let chunks = 0;
+    const stream = new ReadableStream<Uint8Array>({
+      pull(controller) {
+        controller.enqueue(new Uint8Array(chunks++ === 0 ? MAX_JSON_BYTES : 1));
+      },
+      cancel() {
+        cancelled = true;
+      },
+    });
+    const response = new Response(stream, { status: 200 });
+    const arrayBuffer = vi.spyOn(response, 'arrayBuffer');
+    const client = new WebSearchClient({
+      fetchImpl: vi.fn<typeof fetch>().mockResolvedValue(response),
+      getAccessToken: vi.fn(async () => 'token') as never,
+    });
+
+    await expect(client.search(input())).rejects.toMatchObject<WebSearchClientError>({
+      code: 'plugin_backend_response_too_large', status: 502, retryable: false,
+    });
+    expect(arrayBuffer).not.toHaveBeenCalled();
+    expect(cancelled).toBe(true);
+  });
+
+  it('rejects incoherent search, billing, and error pairings', async () => {
+    const invalidPayloads = [
+      search({
+        billing: {
+          mode: 'platform_metered', status: 'settled', reserved_points: '1.00',
+          actual_points: '1', usage_amount: 1, unit: 'search_request',
+        },
+      }),
+      search({
+        billing: {
+          mode: 'platform_metered', status: 'settled', reserved_points: '1.00',
+          actual_points: '1.000', usage_amount: 1, unit: 'search_request',
+        },
+      }),
+      search({
+        billing: {
+          mode: 'platform_metered', status: 'settled', reserved_points: '1.00',
+          actual_points: '1.00', usage_amount: 2, unit: 'search_request',
+        },
+      }),
+      search({
+        billing: {
+          mode: 'platform_metered', status: 'settled', reserved_points: '1.00',
+          actual_points: '1.00', unit: 'search_request',
+        },
+      }),
+      search({ sources: [] }),
+      search({
+        status: 'pending_review',
+        billing: {
+          mode: 'platform_metered', status: 'pending_review', reserved_points: '1.00',
+          usage_amount: 1, unit: 'search_request',
+        },
+        sources: [],
+      }),
+      search({
+        billing: {
+          mode: 'platform_metered', status: 'released', reserved_points: '1.00',
+          unit: 'search_request',
+        },
+      }),
+      search({
+        status: 'failed', answer: null, sources: [], search_queries: [], error_code: null,
+        billing: {
+          mode: 'platform_metered', status: 'released', reserved_points: '1.00',
+          usage_amount: 1, unit: 'search_request',
+        },
+      }),
+      search({
+        status: 'submission_unknown', answer: null, sources: [], search_queries: [],
+        error_code: 'web_search_rate_limited',
+        billing: {
+          mode: 'platform_metered', status: 'pending_review', reserved_points: '1.00',
+          usage_amount: 1, unit: 'search_request',
+        },
+      }),
+    ];
+
+    for (const payload of invalidPayloads) {
+      const client = new WebSearchClient({
+        fetchImpl: vi.fn<typeof fetch>().mockResolvedValue(jsonResponse(payload)),
+        getAccessToken: vi.fn(async () => 'token') as never,
+      });
+
+      await expect(client.search(input())).rejects.toMatchObject<WebSearchClientError>({
+        code: 'plugin_backend_invalid', status: 502, retryable: false,
+      });
+    }
+  });
+
  it('accepts omitted nullable/default response fields while keeping the object closed', async () => {
    const payload = search();
    delete payload.answer;
@@ -215,6 +362,7 @@ describe('WebSearchClient', () => {
    delete payload.error_code;
    delete payload.retry_after_seconds;
    payload.status = 'failed';
+    payload.error_code = 'web_search_request_invalid';
    payload.billing = {
      mode: 'platform_metered', status: 'released', reserved_points: '1.00', unit: 'search_request',
    };
@@ -222,7 +370,7 @@ describe('WebSearchClient', () => {
      fetchImpl: vi.fn<typeof fetch>().mockResolvedValue(jsonResponse(payload)),
      getAccessToken: vi.fn(async () => 'token') as never,
    }).search(input())).resolves.toMatchObject({
-      status: 'failed', answer: null, sources: [], searchQueries: [], errorCode: null,
+      status: 'failed', answer: null, sources: [], searchQueries: [], errorCode: 'web_search_request_invalid',
    });
  });