From 0e4745a44da4c75cd92a266ec93dab11758fc727 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Wed, 4 Nov 2026 17:50:50 +0000 Subject: [PATCH] fix(tool): port upstream webfetch redirect rule checks (anomalyco/opencode#59748) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- packages/opencode/src/tool/webfetch.ts | 63 ++++++++++- packages/opencode/test/tool/webfetch.test.ts | 111 ++++++++++++++++++- 2 files changed, 167 insertions(+), 7 deletions(-) diff --git a/packages/opencode/src/tool/webfetch.ts b/packages/opencode/src/tool/webfetch.ts index 1d2dbe953d..93c5ff886d 100644 --- a/packages/opencode/src/tool/webfetch.ts +++ b/packages/opencode/src/tool/webfetch.ts @@ -3,6 +3,11 @@ import { Tool } from "./tool" import TurndownService from "turndown" import DESCRIPTION from "./webfetch.txt" import { abortAfterAny } from "../util/abort" +// altimate_change start — upstream_fix: check webfetch redirect targets against permission rules (anomalyco/opencode#59748, anomalyco/opencode#61701) +import { PermissionV1 } from "@opencode-ai/core/v1/permission" +import { PermissionEvaluate } from "@/permission/evaluate" +import { Wildcard } from "@opencode-ai/core/util/wildcard" +// altimate_change end const MAX_RESPONSE_SIZE = 5 * 1024 * 1024 // 5MB const DEFAULT_TIMEOUT = 30 * 1000 // 30 seconds @@ -15,6 +20,45 @@ const BROWSER_UA = // Status codes that warrant a retry with a different User-Agent const RETRYABLE_STATUSES = new Set([403, 406]) +// altimate_change start — upstream_fix: check webfetch redirect targets against permission rules (anomalyco/opencode#59748, anomalyco/opencode#61701) +export const MAX_REDIRECTS = 10 +const REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]) + +function assertFetchableUrl(url: URL) { + if (url.protocol !== "http:" && url.protocol !== "https:") throw new Error("URL must use http:// or https://") + // Rules match the URL, so `https://allowed.com@evil.com` would slip past a rule for `https://allowed.com*`. + if (url.username || url.password) throw new Error("URL must not include credentials") +} + +// Follows redirects here instead of in fetch so each new URL is checked against the rules first. +// A target only fails when the rules deny it; the user is not asked again. +async function fetchCheckingRedirects( + url: string, + init: RequestInit, + ruleset: PermissionV1.Ruleset, +): Promise { + let current = url + for (let redirects = 0; ; redirects++) { + const response = await fetch(current, { ...init, redirect: "manual" }) + const location = response.headers.get("location") + if (!REDIRECT_STATUSES.has(response.status) || !location) return response + await response.body?.cancel().catch(() => {}) + if (redirects >= MAX_REDIRECTS) throw new Error(`Too many redirects (more than ${MAX_REDIRECTS})`) + const target = new URL(location, current) + assertFetchableUrl(target) + // The fragment is never sent, and would keep a rule for the exact URL from matching. + target.hash = "" + const rule = PermissionEvaluate.evaluate("webfetch", target.href, ruleset) + if (rule.action === "deny") { + throw new PermissionV1.DeniedError({ + ruleset: ruleset.filter((item) => Wildcard.match("webfetch", item.permission)), + }) + } + current = target.href + } +} +// altimate_change end + // altimate_change start — process-level URL failure cache (#471) // Prevents repeated fetches to URLs that already returned 404/410 in this // process. Cache is module-scope so in daemon mode (`altimate serve`) and @@ -308,11 +352,17 @@ export const WebFetchTool = Tool.define("webfetch", { if (!params.url.startsWith("http://") && !params.url.startsWith("https://")) { throw new Error("URL must start with http:// or https://") } + // altimate_change start — upstream_fix: reject URL credentials and match rules on the requested URL (anomalyco/opencode#61701, anomalyco/opencode#60505) + let parsed: URL try { - new URL(params.url) + parsed = new URL(params.url) } catch { throw new Error(`Invalid URL: "${params.url.slice(0, 200)}" is not a valid URL. Check the format and try again.`) } + assertFetchableUrl(parsed) + parsed.hash = "" + const url = parsed.href + // altimate_change end // Check failure cache — avoid re-fetching URLs that already returned 404/410/451. // Prefix with "(cached failure, Nm ago)" so the agent can tell a cache-hit @@ -328,7 +378,9 @@ export const WebFetchTool = Tool.define("webfetch", { await ctx.ask({ permission: "webfetch", - patterns: [params.url], + // altimate_change start — upstream_fix: match rules on the requested URL (anomalyco/opencode#60505) + patterns: [url], + // altimate_change end always: ["*"], metadata: { url: params.url, @@ -370,13 +422,16 @@ export const WebFetchTool = Tool.define("webfetch", { let arrayBuffer: ArrayBuffer let response: Response try { - response = await fetch(params.url, { signal, headers: honestHeaders }) + // altimate_change start — upstream_fix: check webfetch redirect targets against permission rules (anomalyco/opencode#59748) + const ruleset = ctx.permissions ?? [] + response = await fetchCheckingRedirects(url, { signal, headers: honestHeaders }, ruleset) // Retry with browser UA if the honest UA was rejected if (!response.ok && RETRYABLE_STATUSES.has(response.status)) { await response.body?.cancel().catch(() => {}) - response = await fetch(params.url, { signal, headers: browserHeaders }) + response = await fetchCheckingRedirects(url, { signal, headers: browserHeaders }, ruleset) } + // altimate_change end // altimate_change start — actionable error messages and failure caching (#471) if (!response.ok) { diff --git a/packages/opencode/test/tool/webfetch.test.ts b/packages/opencode/test/tool/webfetch.test.ts index fdf5210b9c..baaa9eb5ee 100644 --- a/packages/opencode/test/tool/webfetch.test.ts +++ b/packages/opencode/test/tool/webfetch.test.ts @@ -1,11 +1,12 @@ import { describe, expect } from "bun:test" -import { Effect, Layer } from "effect" +import { Cause, Effect, Exit, Layer } from "effect" import { FetchHttpClient } from "effect/unstable/http" import { Agent } from "../../src/agent/agent" import { Truncate } from "@/tool/truncate" import { WebFetchTool } from "../../src/tool/webfetch" import { SessionID, MessageID } from "../../src/session/schema" import { Tool } from "@/tool/tool" +import { PermissionV1 } from "@opencode-ai/core/v1/permission" import { testEffect } from "../lib/effect" const it = testEffect(Layer.mergeAll(FetchHttpClient.layer, Truncate.defaultLayer, Agent.defaultLayer)) @@ -31,12 +32,27 @@ const withFetch = ( (server) => Effect.sync(() => server.stop(true)), ) -const exec = Effect.fn("WebFetchToolTest.exec")(function* (args: Tool.InferParameters) { +const exec = Effect.fn("WebFetchToolTest.exec")(function* ( + args: Tool.InferParameters, + // altimate_change start — upstream_fix: redirect rule tests pass the effective ruleset (anomalyco/opencode#59748) + permissions?: PermissionV1.Ruleset, + // altimate_change end +) { const info = yield* WebFetchTool const tool = yield* info.init() - return yield* tool.execute(args, ctx) + return yield* tool.execute(args, permissions ? { ...ctx, permissions } : ctx) }) +// altimate_change start — upstream_fix: redirect rule tests (anomalyco/opencode#59748, anomalyco/opencode#61701) +const failure = (effect: Effect.Effect) => + Effect.gen(function* () { + const exit = yield* Effect.exit(effect) + expect(Exit.isFailure(exit)).toBe(true) + if (Exit.isSuccess(exit)) return "" + return String(Cause.squash(exit.cause)) + }) +// altimate_change end + describe("tool.webfetch", () => { it.instance("returns image responses as file attachments", () => Effect.gen(function* () { @@ -110,4 +126,93 @@ describe("tool.webfetch", () => { }), ), ) + + // altimate_change start — upstream_fix: redirect rule tests (anomalyco/opencode#59748, anomalyco/opencode#61701) + it.instance("follows redirects to targets the rules allow", () => { + const requested: string[] = [] + return withFetch( + (req) => { + const path = new URL(req.url).pathname + requested.push(path) + if (path === "/start") return new Response(null, { status: 302, headers: { location: "/target#frag" } }) + return new Response("arrived", { status: 200, headers: { "content-type": "text/plain" } }) + }, + (url) => + Effect.gen(function* () { + const result = yield* exec({ url: new URL("/start", url).toString(), format: "text" }, [ + { permission: "webfetch", pattern: "*", action: "allow" }, + ]) + expect(result.output).toBe("arrived") + expect(requested).toEqual(["/start", "/target"]) + }), + ) + }) + + it.instance("stops at a redirect target the rules deny", () => { + const requested: string[] = [] + return withFetch( + (req) => { + const path = new URL(req.url).pathname + requested.push(path) + if (path === "/start") return new Response(null, { status: 302, headers: { location: "/secret" } }) + return new Response("secret", { status: 200, headers: { "content-type": "text/plain" } }) + }, + (url) => + Effect.gen(function* () { + const message = yield* failure( + exec({ url: new URL("/start", url).toString(), format: "text" }, [ + { permission: "webfetch", pattern: "*", action: "allow" }, + { permission: "webfetch", pattern: new URL("/secret", url).toString(), action: "deny" }, + ]), + ) + expect(message).toContain("prevents you from using this specific tool call") + expect(requested).toEqual(["/start"]) + }), + ) + }) + + it.instance("rejects redirects to non-HTTP schemes and endless redirect chains", () => + withFetch( + (req) => { + const path = new URL(req.url).pathname + if (path === "/file") return new Response(null, { status: 301, headers: { location: "file:///etc/passwd" } }) + return new Response(null, { status: 307, headers: { location: `${path}x` } }) + }, + (url) => + Effect.gen(function* () { + expect(yield* failure(exec({ url: new URL("/file", url).toString(), format: "text" }))).toContain( + "URL must use http:// or https://", + ) + expect(yield* failure(exec({ url: new URL("/loop", url).toString(), format: "text" }))).toContain( + "Too many redirects", + ) + }), + ), + ) + + it.instance("rejects credentials in the requested URL and in redirect targets", () => { + let requests = 0 + return withFetch( + (req) => { + requests++ + const target = new URL("/next", req.url) + target.username = "user" + return new Response(null, { status: 302, headers: { location: target.toString() } }) + }, + (url) => + Effect.gen(function* () { + const withCredentials = new URL("/start", url) + withCredentials.username = "user" + expect(yield* failure(exec({ url: withCredentials.toString(), format: "text" }))).toContain( + "URL must not include credentials", + ) + expect(requests).toBe(0) + expect(yield* failure(exec({ url: new URL("/start", url).toString(), format: "text" }))).toContain( + "URL must not include credentials", + ) + expect(requests).toBe(1) + }), + ) + }) + // altimate_change end })