From 49de82c4860fd0b377070279b6411237dc6b9564 Mon Sep 17 00:00:00 2001 From: brother7 <7brother7@gmail.com> Date: Tue, 1 Sep 2026 03:49:52 +0800 Subject: [PATCH] fix(web-search): harden client response boundary --- ...earch-rev01-client-remediation-e4a7c9b2.md | 105 ++++++++++++ electron/services/web-search-client.ts | 114 +++++++++++-- tests/unit/web-search-client.test.ts | 152 +++++++++++++++++- 3 files changed, 355 insertions(+), 16 deletions(-) create mode 100644 .project-docs/30-worklog/tasks/20260901-web-search-rev01-client-remediation-e4a7c9b2.md diff --git 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 new file mode 100644 index 0000000..f6de240 --- /dev/null +++ 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. diff --git a/electron/services/web-search-client.ts b/electron/services/web-search-client.ts index 36a4981..83da079 100644 --- 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 { 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 { + 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, diff --git a/tests/unit/web-search-client.test.ts b/tests/unit/web-search-client.test.ts index 89a1722..1a6bbfe 100644 --- 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 = {}) { 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().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().mockResolvedValue(jsonResponse(search({ + sources: [{ title: 'Source', url }], + }))), + getAccessToken: vi.fn(async () => 'token') as never, + }); + + await expect(client.search(input())).rejects.toMatchObject({ + 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().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().mockResolvedValue(response), + getAccessToken: vi.fn(async () => 'token') as never, + }); + + await expect(client.search(input())).rejects.toMatchObject({ + 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().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({ + 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().mockResolvedValue(response), + getAccessToken: vi.fn(async () => 'token') as never, + }); + + await expect(client.search(input())).rejects.toMatchObject({ + 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().mockResolvedValue(jsonResponse(payload)), + getAccessToken: vi.fn(async () => 'token') as never, + }); + + await expect(client.search(input())).rejects.toMatchObject({ + 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().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', }); });