fix: address codex review findings on web seam

- search providers (exa/perplexity/deepseek): map the parsed response
  INSIDE the parse try, so a well-formed body of the wrong shape surfaces
  as WEB_PROVIDER_ERROR instead of escaping as a raw TypeError; a WebError
  the mapper throws on purpose is re-thrown untouched
- web-fetch-local: validate numeric limits at plugin construction (positive
  finite caps; non-negative integer maxRedirects) rather than constructing a
  provider with nonsensical values
- web-fetch-local: enforce the redirect budget BEFORE resolving each hop, so
  maxRedirects:N follows exactly N redirects and an over-limit hop reports
  "exceeded the maximum" rather than misdiagnosing a cross-origin block
- drop the stale dsh-tool-web/search and /fetch path aliases (the package no
  longer declares those subpath exports)
- strip trailing EOF blank lines flagged by git diff --check

Each fix carries a regression test.
This commit is contained in:
Dudu-0223
2026-06-29 17:23:11 +08:00
parent b92a3c531a
commit 8395722db5
14 changed files with 157 additions and 21 deletions
+3 -1
View File
@@ -26,9 +26,11 @@ The provider owns **safe resource retrieval**: URL validation, HTTP transport, r
| `maxBodyChars` | `100_000` | Maximum decoded body length in characters. |
| `timeoutMs` | `30_000` | Default fetch timeout. |
| `maxTimeoutMs` | `120_000` | Upper bound for a per-request timeout override. |
| `maxRedirects` | `5` | Maximum same-origin redirect hops. |
| `maxRedirects` | `5` | Maximum same-origin redirect hops (`0` follows none). |
| `userAgent` | `deepseek-harness/…` | `User-Agent` header. |
The numeric limits are validated at plugin construction: every cap except `maxRedirects` must be a positive finite number, and `maxRedirects` must be a non-negative integer. An invalid value throws rather than silently constructing a provider with nonsensical limits.
## Security note
SSRF / private-network protection (blocking private, loopback, link-local, multicast, and otherwise non-public destinations, with DNS-resolve-then-validate and per-hop re-validation) is **deferred** — see the [web capability seam RFC](../../../docs/rfc/implemented/architecture/2026-06-24-web-capability-seam.md). Until it lands, this provider is an SSRF primitive and **must not be enabled** in a deployment that can reach sensitive internal network targets.
+20
View File
@@ -60,10 +60,30 @@ export const Config: z<Config> = z.object({
/** The shape after schemastery applies its defaults to every field. */
type ResolvedConfig = Required<Config>
/** A resource limit (byte/char/length/timeout cap) must be a positive finite number. */
function assertPositiveFinite(name: string, value: number): void {
if (!Number.isFinite(value) || value <= 0) {
throw new Error(`web-fetch-local: ${name} must be a positive finite number`)
}
}
/** The redirect hop cap must be a non-negative integer (0 follows no redirects). */
function assertNonNegativeInteger(name: string, value: number): void {
if (!Number.isInteger(value) || value < 0) {
throw new Error(`web-fetch-local: ${name} must be a non-negative integer`)
}
}
/** Register the local HTTP(S) fetch provider with `ctx.web`. */
export function apply(ctx: Context, config: Config): void {
// schemastery (Config) has already filled every defaulted field.
const resolved = config as ResolvedConfig
assertPositiveFinite('maxUrlLength', resolved.maxUrlLength)
assertPositiveFinite('maxResponseBytes', resolved.maxResponseBytes)
assertPositiveFinite('maxBodyChars', resolved.maxBodyChars)
assertPositiveFinite('timeoutMs', resolved.timeoutMs)
assertPositiveFinite('maxTimeoutMs', resolved.maxTimeoutMs)
assertNonNegativeInteger('maxRedirects', resolved.maxRedirects)
const limits: LocalFetchLimits = {
maxUrlLength: resolved.maxUrlLength,
maxResponseBytes: resolved.maxResponseBytes,
+12 -3
View File
@@ -81,11 +81,21 @@ export class LocalFetchProvider implements WebFetchProvider {
/** Follow same-origin redirects up to the hop cap, then read the final response. */
private async followAndRead(initialUrl: string, controller: AbortController): Promise<WebFetchResult> {
let currentUrl = validateFetchUrl(initialUrl, this.limits.maxUrlLength)
let redirectsFollowed = 0
for (let hop = 0; hop <= this.limits.maxRedirects; hop++) {
for (;;) {
const response = await this.requestOnce(currentUrl, controller)
if (isRedirectStatus(response.status)) {
// The redirect budget is enforced BEFORE this hop's target is resolved
// or origin-checked, so `maxRedirects: N` follows at most N redirects
// exactly: the (N+1)th redirect is refused as "exceeded" regardless of
// where it points (a same-origin/cross-origin distinction on a hop we
// are not allowed to follow would be the wrong diagnosis).
if (redirectsFollowed >= this.limits.maxRedirects) {
await response.body?.cancel()
throw new WebError(`exceeded the maximum of ${this.limits.maxRedirects} redirects`, 'WEB_REDIRECT_BLOCKED')
}
const location = response.headers.get('location')
if (location === null) {
// A redirect status with no Location is not a usable resource. Cancel
@@ -113,13 +123,12 @@ export class LocalFetchProvider implements WebFetchProvider {
}
await response.body?.cancel()
currentUrl = validatedTarget
redirectsFollowed++
continue
}
return await this.readBody(response, currentUrl, controller.signal)
}
throw new WebError(`exceeded the maximum of ${this.limits.maxRedirects} redirects`, 'WEB_REDIRECT_BLOCKED')
}
private async requestOnce(url: URL, controller: AbortController): Promise<Response> {
@@ -203,6 +203,60 @@ describe('LocalFetchProvider redirects', () => {
.rejects.toThrow(expect.objectContaining({ code: 'WEB_REDIRECT_BLOCKED' }))
})
it('follows exactly maxRedirects hops: a chain landing on the Nth redirect succeeds', async () => {
// maxRedirects: 2 → /?n=0 → /?n=1 → /?n=2(200). Exactly 2 redirects + 1
// final = 3 requests; the cap is inclusive of the landing request.
let requests = 0
handler = (req, res) => {
requests++
const n = Number(new URL(req.url ?? '/', base).searchParams.get('n') ?? '0')
if (n >= 2) { res.writeHead(200, { 'content-type': 'text/plain' }); res.end('landed') }
else { res.writeHead(302, { location: `/?n=${n + 1}` }); res.end() }
}
const result = await provider({ maxRedirects: 2 }).fetch({ url: `${base}/?n=0` })
expect(result.body.content).toBe('landed')
expect(requests).toBe(3)
})
it('makes exactly maxRedirects+1 requests before blocking an over-long chain', async () => {
// maxRedirects: 2 on an infinite chain: requests at n=0,1,2 (the 3rd is the
// over-limit redirect, refused before its Location is followed) = 3 total.
let requests = 0
handler = (req, res) => {
requests++
const n = Number(new URL(req.url ?? '/', base).searchParams.get('n') ?? '0')
res.writeHead(302, { location: `/?n=${n + 1}` })
res.end()
}
await expect(provider({ maxRedirects: 2 }).fetch({ url: `${base}/?n=0` }))
.rejects.toThrow(expect.objectContaining({ code: 'WEB_REDIRECT_BLOCKED', message: 'exceeded the maximum of 2 redirects' }))
expect(requests).toBe(3)
})
it('reports an over-limit redirect as "exceeded", not cross-origin, even when the over-limit hop points cross-origin', async () => {
// The redirect budget is checked BEFORE the over-limit hop's target is
// origin-validated, so the diagnosis is "exceeded", not "cross-origin".
handler = (req, res) => {
const n = Number(new URL(req.url ?? '/', base).searchParams.get('n') ?? '0')
const location = n === 0 ? '/?n=1' : 'https://example.com/'
res.writeHead(302, { location })
res.end()
}
await expect(provider({ maxRedirects: 1 }).fetch({ url: `${base}/?n=0` }))
.rejects.toThrow(expect.objectContaining({ code: 'WEB_REDIRECT_BLOCKED', message: 'exceeded the maximum of 1 redirects' }))
})
it('maxRedirects: 0 follows no redirect but still fetches a direct 200', async () => {
handler = (req, res) => {
if (req.url === '/r') { res.writeHead(302, { location: '/done' }); res.end() }
else { res.writeHead(200, { 'content-type': 'text/plain' }); res.end('direct') }
}
await expect(provider({ maxRedirects: 0 }).fetch({ url: `${base}/r` }))
.rejects.toThrow(expect.objectContaining({ code: 'WEB_REDIRECT_BLOCKED' }))
const direct = await provider({ maxRedirects: 0 }).fetch({ url: `${base}/done` })
expect(direct.body.content).toBe('direct')
})
it('treats a redirect without a Location header as a provider error', async () => {
handler = (_req, res) => { res.writeHead(302); res.end() }
await expect(provider().fetch({ url: base }))
@@ -331,4 +385,40 @@ describe('web-fetch-local plugin registration', () => {
it('has no default export (namespace plugin export shape)', () => {
expect('default' in fetchPlugin).toBe(false)
})
it('rejects a non-positive resource limit at construction', async () => {
const ctx = new Context()
await ctx.plugin(WebService, { fetchProvider: LOCAL_FETCH_PROVIDER_ID })
await expect(ctx.plugin(fetchPlugin, { maxResponseBytes: -1 }))
.rejects.toThrow(/maxResponseBytes must be a positive finite number/)
})
it('rejects a zero timeout at construction', async () => {
const ctx = new Context()
await ctx.plugin(WebService, { fetchProvider: LOCAL_FETCH_PROVIDER_ID })
await expect(ctx.plugin(fetchPlugin, { timeoutMs: 0 }))
.rejects.toThrow(/timeoutMs must be a positive finite number/)
})
it('rejects a fractional redirect cap at construction', async () => {
const ctx = new Context()
await ctx.plugin(WebService, { fetchProvider: LOCAL_FETCH_PROVIDER_ID })
await expect(ctx.plugin(fetchPlugin, { maxRedirects: 1.5 }))
.rejects.toThrow(/maxRedirects must be a non-negative integer/)
})
it('rejects a negative redirect cap at construction', async () => {
const ctx = new Context()
await ctx.plugin(WebService, { fetchProvider: LOCAL_FETCH_PROVIDER_ID })
await expect(ctx.plugin(fetchPlugin, { maxRedirects: -1 }))
.rejects.toThrow(/maxRedirects must be a non-negative integer/)
})
it('accepts maxRedirects: 0 (follow no redirects) as valid config', async () => {
const ctx = new Context()
await ctx.plugin(WebService, { fetchProvider: LOCAL_FETCH_PROVIDER_ID })
const fiber = await ctx.plugin(fetchPlugin, { maxRedirects: 0 })
expect(ctx.web.fetchStatus()).toEqual({ available: true, providerId: LOCAL_FETCH_PROVIDER_ID })
await fiber.dispose()
})
})