From 567519184ba42ffd2204583327e08091ae3b5120 Mon Sep 17 00:00:00 2001 From: Dudu-0223 Date: Thu, 25 Jun 2026 15:28:15 +0800 Subject: [PATCH] fix: address codex review round 1 - Re-validate redirect targets through validateFetchUrl before following, so a same-origin Location carrying credentials (or a non-http(s)/over-long URL) cannot bypass the transport hygiene a direct request enforces. - Treat only DROPPED bytes as truncation: a body exactly at maxResponseBytes is no longer falsely flagged truncated (which emitted a spurious footer). - Honor the declared response charset: parse the Content-Type charset and decode with it (rejecting unsupported labels as WEB_UNSUPPORTED_CONTENT_TYPE) instead of always assuming UTF-8 and returning replacement characters. - Catalog the web seam vocabulary in docs/core-data-structures/web.md with type-equiv blocks + manifest entries, per the core-data-structures rule. --- docs/core-data-structures/core.md | 1 + docs/core-data-structures/web.md | 119 ++++++++++++++++++ packages/web/web-fetch-local/src/index.ts | 2 +- packages/web/web-fetch-local/src/policy.ts | 26 ++++ packages/web/web-fetch-local/src/provider.ts | 27 ++-- .../web-fetch-local/tests/fetch-local.spec.ts | 42 ++++++- scripts/type-equiv.manifest.json | 12 +- 7 files changed, 218 insertions(+), 11 deletions(-) create mode 100644 docs/core-data-structures/web.md diff --git a/docs/core-data-structures/core.md b/docs/core-data-structures/core.md index 6d20900c93..09058a596a 100644 --- a/docs/core-data-structures/core.md +++ b/docs/core-data-structures/core.md @@ -22,6 +22,7 @@ Everything else is documented on a **sub-page**, not here. The rule that draws t | [bash.md](bash.md) | the bash executor seam: `BashExecRequest`/`Spec`, `BashRunResult`, background `BashTask`s | | [compaction.md](compaction.md) | the compaction seam: the `compact/*` session events, `CompactionResult`, the `CompactService` interface | | [subagent.md](subagent.md) | the subagent seam: the named-provider registry, `SubagentStartRequest`/`Result`/`Run`, the start-time-vs-runtime capability split | +| [web.md](web.md) | the web access seam: `WebSearchRequest`/`Result`, `WebFetchRequest`/`Result`, `WebFetchBody`, provider/capability status, `WebErrorCode` | > Type definitions on this page are pasted **verbatim** from source and drift-checked by `pnpm run verify-type-equiv` (see [development.md](../development.md#documenting-types-verbatim-ts-type-equiv)). Inline JSDoc is omitted for readability; follow the source link for the full contracts. diff --git a/docs/core-data-structures/web.md b/docs/core-data-structures/web.md new file mode 100644 index 0000000000..f0ade276f7 --- /dev/null +++ b/docs/core-data-structures/web.md @@ -0,0 +1,119 @@ +# Web Access + +The web access seam — a [capability seam](../rfc/implemented/architecture/2026-06-24-web-capability-seam.md) that spans **two capabilities** (search and fetch) on one `ctx.web` service, split across packages: interface ([dsh-web](../../packages/web/web), `ctx.web` + the provider registries), implementations ([dsh-web-search-exa](../../packages/web/web-search-exa), [dsh-web-search-perplexity](../../packages/web/web-search-perplexity), [dsh-web-fetch-local](../../packages/web/web-fetch-local)), and consumer ([dsh-tool-web](../../packages/web/tool-web), the `web_search`/`web_fetch` tool schemas). Web is **one optional capability**, not part of the agent-loop spine — so its vocabulary lives here, not in [core.md](core.md). A search-provider swap does not change how the model asks for a query, and a fetch-implementation swap does not change how the model asks for a URL. + +Source: [`packages/web/web/src/types.ts`](../../packages/web/web/src/types.ts) + +## Why one seam for two capabilities + +Search and fetch share no request schema and no business logic, but they are deliberately one `ctx.web` middle layer: one provider-selection policy owner, one abort/error vocabulary, one product-facing "how this harness reaches the web" config surface. The cost is the parallel `searchX`/`fetchX` method pairs on the service; that parallelism is intentional, not a missed extraction. Providers register **capabilities** (a `WebSearchProvider` or `WebFetchProvider`), not tools; the model-facing names, schemas, prompt guidance, and presentation all live in the single `dsh-tool-web` consumer. + +## Search request and result + +The model-facing tool argument is just a `query`; `maxResults` is a consumer-owned bound (`dsh-tool-web`'s `WEB_SEARCH_MAX_RESULTS`, default `8`) passed through the seam and enforced on the way back — if a provider over-returns, the seam truncates `sources[]` and sets `truncated`. + +```ts type-equiv +interface WebSearchRequest { + readonly query: string + /** + * Upper bound on returned sources; the seam truncates to it. Omitted = no + * bound. `dsh-tool-web` always sets it. + */ + readonly maxResults?: number +} +``` + +```ts type-equiv +interface WebSearchResult { + readonly providerId: string + readonly query: string + readonly content?: string + readonly sources: readonly WebSearchSource[] + readonly truncated: boolean +} +``` + +`content` is optional provider-generated answer text (Exa returns none; Perplexity returns a generated answer). `sources[]` is the portable citation surface. A source always has a `url`; `title`/`snippet`/`publishedAt` are optional because not every provider returns them — Perplexity citations may be URL-only, and forcing adapters to invent the rest would make the seam lie. `dsh-tool-web` renders `title ?? hostname(url)`. + +```ts type-equiv +interface WebSearchSource { + readonly url: string + readonly title?: string + readonly snippet?: string + readonly publishedAt?: string +} +``` + +## Fetch request and result + +```ts type-equiv +interface WebFetchRequest { + readonly url: string + readonly timeoutMs?: number +} +``` + +HTTP status is part of the fetched resource state, not automatically a failure: a successful network fetch of a `404`/`500` returns a `WebFetchResult` with the status code and a bounded decoded body. `url` is the final URL after allowed redirects. `WebError` is reserved for failures to safely retrieve or represent the resource. + +```ts type-equiv +interface WebFetchResult { + readonly providerId: string + readonly url: string + readonly statusCode: number + readonly body: WebFetchBody + readonly truncated: boolean +} +``` + +`WebFetchBody` is a **closed** discriminated union owned by `dsh-web` (not a merge-extensible map): the provider decodes the kind and `dsh-tool-web` renders it, so a new kind is a coordinated change across known packages, not a plugin extension. Consumers `switch` on `kind` ending in `default: assertNever(...)`, so adding a kind breaks compilation at every consumer until handled. Each arm stays its own object literal even where fields coincide today, leaving room for arm-specific fields later (a future `pdf` body's `pageCount`). + +```ts type-equiv +type WebFetchBody = + | { readonly kind: 'html'; readonly content: string } + | { readonly kind: 'text'; readonly content: string } +``` + +## Provider and capability status + +A provider's `status()` is a cheap LOCAL check (credential presence, parseable config) and **must not make network calls**. It is an input to selection, not a health system. + +```ts type-equiv +type WebProviderStatus = + | { readonly available: true } + | { readonly available: false; readonly reason: 'missing-credential' | 'misconfigured' } +``` + +The service aggregates provider status into a `WebCapabilityStatus`: whether the capability has a selected usable provider, or the broad category in which selection fails. It carries the winning `providerId` on the available branch but NOT the per-reason payload (the missing id, the ambiguous set) — that branchable detail lives in the thrown `WebError`, the surface callers route on, so the same fact never gets two homes that can disagree. + +```ts type-equiv +type WebCapabilityStatus = + | { readonly available: true; readonly providerId: string } + | { readonly available: false; readonly reason: 'none' | 'configured-missing' | 'configured-unavailable' | 'ambiguous' } +``` + +Selection never depends on registration, config, or HMR order: a capability has an explicit provider id (config `searchProvider`/`fetchProvider`, or the matching env var feeding the same field), or auto-selects when exactly one usable provider is registered; multiple usable providers with no configured id is `ambiguous`, not first-wins. + +## Errors + +`WebError extends HarnessError` ([core.md](core.md) error taxonomy) with a stable `WebErrorCode`. `WEB_DUPLICATE_PROVIDER` is a registration-time programming error (the analogue of `LlmService`'s `DUPLICATE_ADAPTER`); the `WEB_PROVIDER_*` selection codes and the fetch transport codes are execution outcomes. `WEB_PROVIDER_ERROR` is the catch-all for a provider's own failure surfaced through the seam, including network/transport failure (DNS, connection refused, TLS). + +```ts type-equiv +type WebErrorCode = + | 'WEB_PROVIDER_UNAVAILABLE' + | 'WEB_PROVIDER_CONFIGURED_MISSING' + | 'WEB_PROVIDER_CONFIGURED_UNAVAILABLE' + | 'WEB_PROVIDER_AMBIGUOUS' + | 'WEB_DUPLICATE_PROVIDER' + | 'WEB_INVALID_URL' + | 'WEB_BLOCKED_URL' + | 'WEB_REDIRECT_BLOCKED' + | 'WEB_FETCH_TOO_LARGE' + | 'WEB_FETCH_TIMEOUT' + | 'WEB_ABORTED' + | 'WEB_UNSUPPORTED_CONTENT_TYPE' + | 'WEB_PROVIDER_ERROR' +``` + +## The service + +`WebService` (`ctx.web`, defined in [`packages/web/web/src/index.ts`](../../packages/web/web/src/index.ts)) is a provider registry plus a provider-selecting execution surface, close to `LlmService`'s shape: `registerSearchProvider`/`registerFetchProvider` (duplicate ids throw `WEB_DUPLICATE_PROVIDER`, return disposers, emit `web/providers-change`), `searchStatus`/`fetchStatus` (derived, never stored), and `search`/`fetch` (resolve the provider at call time, throw a structured `WebError` when the capability cannot run). Providers issue requests with the platform-native `fetch` (Node 24), mirroring `dsh-llm-deepseek`; the `dsh-web-fetch-local` provider owns safe retrieval (http/https-only, credential rejection, byte/char/timeout/redirect caps, same-origin-only redirects with per-hop re-validation, charset decoding) while `dsh-tool-web` owns presentation (HTML→markdown). SSRF / private-network blocking is deferred (see the RFC) — until it lands, `web_fetch` must not be enabled where it can reach sensitive internal targets. diff --git a/packages/web/web-fetch-local/src/index.ts b/packages/web/web-fetch-local/src/index.ts index 1d57410d68..eb3f8e4143 100644 --- a/packages/web/web-fetch-local/src/index.ts +++ b/packages/web/web-fetch-local/src/index.ts @@ -18,7 +18,7 @@ export { LocalFetchProvider, } from './provider.ts' export type { LocalFetchLimits } from './provider.ts' -export { classifyContentType, isSameOrigin, validateFetchUrl } from './policy.ts' +export { classifyContentType, decoderForCharset, isSameOrigin, parseCharset, validateFetchUrl } from './policy.ts' export type { FetchableKind } from './policy.ts' /** Default `User-Agent`: an explicit product agent, never a browser disguise. */ diff --git a/packages/web/web-fetch-local/src/policy.ts b/packages/web/web-fetch-local/src/policy.ts index 8261c5c1ed..7a76bd1af1 100644 --- a/packages/web/web-fetch-local/src/policy.ts +++ b/packages/web/web-fetch-local/src/policy.ts @@ -57,3 +57,29 @@ export function classifyContentType(contentType: string | null): FetchableKind | if (mime === 'application/json' || mime === 'application/xml' || mime.endsWith('+json') || mime.endsWith('+xml')) return 'text' return undefined } + +/** + * Extract the `charset` parameter from a response `Content-Type`, lower-cased, + * or `undefined` when absent. The provider feeds this label to `TextDecoder` + * so a non-UTF-8 response is decoded with its declared encoding rather than + * silently mangled into replacement characters. + */ +export function parseCharset(contentType: string | null): string | undefined { + const match = /;\s*charset\s*=\s*"?([^";]+)"?/i.exec(contentType ?? '') + return match?.[1]?.trim().toLowerCase() +} + +/** + * Build a `TextDecoder` for the declared charset, falling back to UTF-8 when + * none is declared. Throws {@link WebError} `WEB_UNSUPPORTED_CONTENT_TYPE` when + * the label is present but not a charset `TextDecoder` recognizes — better to + * fail loudly than return mojibake. + */ +export function decoderForCharset(charset: string | undefined): TextDecoder { + if (charset === undefined) return new TextDecoder('utf-8') + try { + return new TextDecoder(charset) + } catch (error: unknown) { + throw new WebError(`unsupported charset "${charset}"`, 'WEB_UNSUPPORTED_CONTENT_TYPE', { cause: error }) + } +} diff --git a/packages/web/web-fetch-local/src/provider.ts b/packages/web/web-fetch-local/src/provider.ts index 0622030af0..c8b99d08b0 100644 --- a/packages/web/web-fetch-local/src/provider.ts +++ b/packages/web/web-fetch-local/src/provider.ts @@ -21,7 +21,7 @@ import { WebError } from '@deepseek-ai/dsh-web' import type { WebFetchBody, WebFetchProvider, WebFetchRequest, WebFetchResult, WebProviderStatus } from '@deepseek-ai/dsh-web' -import { classifyContentType, isSameOrigin, validateFetchUrl } from './policy.ts' +import { classifyContentType, decoderForCharset, isSameOrigin, parseCharset, validateFetchUrl } from './policy.ts' /** Resolved provider limits (the plugin's schemastery Config supplies defaults). */ export interface LocalFetchLimits { @@ -92,14 +92,18 @@ export class LocalFetchProvider implements WebFetchProvider { throw new WebError(`redirect response (HTTP ${response.status}) without a Location header`, 'WEB_PROVIDER_ERROR') } const target = resolveRedirect(location, currentUrl) - if (!isSameOrigin(target, currentUrl)) { + // Re-validate the target against the same transport hygiene a direct + // request gets: a redirect must not be a back door to a credentialed, + // non-http(s), or over-long URL that validateFetchUrl would reject. + const validatedTarget = validateFetchUrl(target.toString(), this.limits.maxUrlLength) + if (!isSameOrigin(validatedTarget, currentUrl)) { throw new WebError( - `cross-origin redirect to ${target.origin} is not followed automatically; retry against that URL directly`, + `cross-origin redirect to ${validatedTarget.origin} is not followed automatically; retry against that URL directly`, 'WEB_REDIRECT_BLOCKED', ) } await response.body?.cancel() - currentUrl = target + currentUrl = validatedTarget continue } @@ -124,14 +128,18 @@ export class LocalFetchProvider implements WebFetchProvider { /** Read, byte-cap, classify, and decode the final response body. */ private async readBody(response: Response, finalUrl: URL): Promise { - const kind = classifyContentType(response.headers.get('content-type')) + const contentType = response.headers.get('content-type') + const kind = classifyContentType(contentType) if (kind === undefined) { await response.body?.cancel() - throw new WebError(`unsupported content type "${response.headers.get('content-type') ?? 'unknown'}"`, 'WEB_UNSUPPORTED_CONTENT_TYPE') + throw new WebError(`unsupported content type "${contentType ?? 'unknown'}"`, 'WEB_UNSUPPORTED_CONTENT_TYPE') } + // Resolve the decoder BEFORE reading the body so an unsupported charset + // fails without consuming the stream. + const decoder = decoderForCharset(parseCharset(contentType)) const { bytes, truncatedByBytes } = await this.readCapped(response) - const decoded = new TextDecoder('utf-8').decode(bytes) + const decoded = decoder.decode(bytes) const truncatedByChars = decoded.length > this.limits.maxBodyChars const content = truncatedByChars ? decoded.slice(0, this.limits.maxBodyChars) : decoded const body: WebFetchBody = kind === 'html' ? { kind: 'html', content } : { kind: 'text', content } @@ -173,7 +181,10 @@ export class LocalFetchProvider implements WebFetchProvider { const { done, value } = await reader.read() if (done) break const remaining = this.limits.maxResponseBytes - total - if (value.byteLength >= remaining) { + // Only DROPPED bytes count as truncation: a chunk that exactly fills the + // remaining capacity keeps all its bytes and we read on to observe EOF, + // so an exactly-at-cap body is not falsely flagged truncated. + if (value.byteLength > remaining) { chunks.push(value.subarray(0, remaining)) total += remaining truncatedByBytes = true diff --git a/packages/web/web-fetch-local/tests/fetch-local.spec.ts b/packages/web/web-fetch-local/tests/fetch-local.spec.ts index 3ca48150e1..54e8de20c0 100644 --- a/packages/web/web-fetch-local/tests/fetch-local.spec.ts +++ b/packages/web/web-fetch-local/tests/fetch-local.spec.ts @@ -3,7 +3,7 @@ import { createServer, type IncomingMessage, type Server, type ServerResponse } import { AddressInfo } from 'node:net' import { Context } from 'cordis' import WebService from '@deepseek-ai/dsh-web' -import { LocalFetchProvider, LOCAL_FETCH_PROVIDER_ID, classifyContentType, isSameOrigin, validateFetchUrl } from '@deepseek-ai/dsh-web-fetch-local' +import { LocalFetchProvider, LOCAL_FETCH_PROVIDER_ID, classifyContentType, decoderForCharset, isSameOrigin, parseCharset, validateFetchUrl } from '@deepseek-ai/dsh-web-fetch-local' import type { LocalFetchLimits } from '@deepseek-ai/dsh-web-fetch-local' import * as fetchPlugin from '@deepseek-ai/dsh-web-fetch-local' @@ -62,6 +62,19 @@ describe('policy helpers', () => { expect(isSameOrigin(new URL('https://a.com'), new URL('https://b.com'))).toBe(false) expect(isSameOrigin(new URL('http://a.com'), new URL('https://a.com'))).toBe(false) }) + + it('parses the charset parameter', () => { + expect(parseCharset('text/html; charset=UTF-8')).toBe('utf-8') + expect(parseCharset('text/plain; charset="iso-8859-1"')).toBe('iso-8859-1') + expect(parseCharset('text/plain')).toBeUndefined() + expect(parseCharset(null)).toBeUndefined() + }) + + it('builds a decoder for a charset and defaults to UTF-8', () => { + expect(decoderForCharset(undefined).encoding).toBe('utf-8') + expect(decoderForCharset('iso-8859-1').encoding).toBe('windows-1252') + expect(() => decoderForCharset('not-a-charset')).toThrow(expect.objectContaining({ code: 'WEB_UNSUPPORTED_CONTENT_TYPE' })) + }) }) describe('LocalFetchProvider success', () => { @@ -109,6 +122,13 @@ describe('LocalFetchProvider caps', () => { expect(result.truncated).toBe(true) }) + it('does not flag a body that exactly fills the byte cap as truncated', async () => { + handler = (_req, res) => { res.writeHead(200, { 'content-type': 'text/plain' }); res.end('abcd') } + const result = await provider({ maxResponseBytes: 4 }).fetch({ url: base }) + expect(result.body.content).toBe('abcd') + expect(result.truncated).toBe(false) + }) + it('truncates a decoded body past the character cap', async () => { handler = (_req, res) => { res.writeHead(200, { 'content-type': 'text/plain' }); res.end('abcdefghij') } const result = await provider({ maxBodyChars: 3 }).fetch({ url: base }) @@ -133,6 +153,19 @@ describe('LocalFetchProvider caps', () => { const result = await provider().fetch({ url: base }) expect(result.body.content).toBe('sized') }) + + it('decodes a non-UTF-8 declared charset', async () => { + // 0xE9 is "é" in ISO-8859-1; decoded as UTF-8 it would be a replacement char. + handler = (_req, res) => { res.writeHead(200, { 'content-type': 'text/plain; charset=iso-8859-1' }); res.end(Buffer.from([0x63, 0x61, 0x66, 0xE9])) } + const result = await provider().fetch({ url: base }) + expect(result.body.content).toBe('café') + }) + + it('rejects an unsupported declared charset', async () => { + handler = (_req, res) => { res.writeHead(200, { 'content-type': 'text/plain; charset=not-a-charset' }); res.end('x') } + await expect(provider().fetch({ url: base })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_UNSUPPORTED_CONTENT_TYPE' })) + }) }) describe('LocalFetchProvider redirects', () => { @@ -152,6 +185,13 @@ describe('LocalFetchProvider redirects', () => { .rejects.toThrow(expect.objectContaining({ code: 'WEB_REDIRECT_BLOCKED' })) }) + it('re-validates a redirect target, rejecting same-origin credentials in the Location', async () => { + const { port } = server.address() as AddressInfo + handler = (_req, res) => { res.writeHead(302, { location: `http://user:pass@127.0.0.1:${port}/` }); res.end() } + await expect(provider().fetch({ url: base })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_BLOCKED_URL' })) + }) + it('rejects exceeding the redirect hop cap', async () => { handler = (req, res) => { const n = Number(new URL(req.url ?? '/', base).searchParams.get('n') ?? '0') diff --git a/scripts/type-equiv.manifest.json b/scripts/type-equiv.manifest.json index 4008a7cc20..f250c5b383 100644 --- a/scripts/type-equiv.manifest.json +++ b/scripts/type-equiv.manifest.json @@ -48,6 +48,16 @@ { "doc": "docs/core-data-structures/subagent.md", "symbol": "SubagentResult", "source": "packages/subagent/subagent/src/types.ts" }, { "doc": "docs/core-data-structures/subagent.md", "symbol": "SubagentStopReasonMap", "source": "packages/subagent/subagent/src/types.ts" }, { "doc": "docs/core-data-structures/subagent.md", "symbol": "SubagentRun", "source": "packages/subagent/subagent/src/types.ts" }, - { "doc": "docs/core-data-structures/subagent.md", "symbol": "SubagentProvider", "source": "packages/subagent/subagent/src/types.ts" } + { "doc": "docs/core-data-structures/subagent.md", "symbol": "SubagentProvider", "source": "packages/subagent/subagent/src/types.ts" }, + + { "doc": "docs/core-data-structures/web.md", "symbol": "WebSearchRequest", "source": "packages/web/web/src/types.ts" }, + { "doc": "docs/core-data-structures/web.md", "symbol": "WebSearchResult", "source": "packages/web/web/src/types.ts" }, + { "doc": "docs/core-data-structures/web.md", "symbol": "WebSearchSource", "source": "packages/web/web/src/types.ts" }, + { "doc": "docs/core-data-structures/web.md", "symbol": "WebFetchRequest", "source": "packages/web/web/src/types.ts" }, + { "doc": "docs/core-data-structures/web.md", "symbol": "WebFetchResult", "source": "packages/web/web/src/types.ts" }, + { "doc": "docs/core-data-structures/web.md", "symbol": "WebFetchBody", "source": "packages/web/web/src/types.ts" }, + { "doc": "docs/core-data-structures/web.md", "symbol": "WebProviderStatus", "source": "packages/web/web/src/types.ts" }, + { "doc": "docs/core-data-structures/web.md", "symbol": "WebCapabilityStatus", "source": "packages/web/web/src/types.ts" }, + { "doc": "docs/core-data-structures/web.md", "symbol": "WebErrorCode", "source": "packages/web/web/src/types.ts" } ] }