From 42ffdf882f1b44af1a227bef5cb0733ff3815c82 Mon Sep 17 00:00:00 2001 From: Agustin Rivera <31522568+eleqtrizit@users.noreply.github.com> Date: Fri, 3 Apr 2026 15:03:18 -0700 Subject: [PATCH] fix(fetch): normalize guarded redirect handling (#59121) * fix(fetch): align guarded redirect rewrites * fix(fetch): tighten redirect coverage * fix(fetch): add changelog entry --- CHANGELOG.md | 1 + src/infra/net/fetch-guard.ssrf.test.ts | 184 +++++++++++++++++++++++++ src/infra/net/fetch-guard.ts | 44 +++++- 3 files changed, 228 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b72c2b2348d..73a6310d46f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,7 @@ Docs: https://docs.openclaw.ai - Plugins/allowlists: let explicit bundled chat channel enablement bypass `plugins.allow`, while keeping auto-enabled channel activation and startup sidecars behind restrictive allowlists. (#60233) Thanks @dorukardahan. - Allowlist/commands: require owner access for `/allowlist add` and `/allowlist remove` so command-authorized non-owners cannot mutate persisted allowlists. (#59836) Thanks @eleqtrizit. - Control UI/skills: clear stale ClawHub results immediately when the search query changes, so debounced searches cannot keep outdated install targets visible. Related #60134. +- Fetch/redirects: normalize guarded redirect method rewriting and loop detection so SSRF-guarded requests match platform redirect behavior without missing loops back to the original URL. (#59121) Thanks @eleqtrizit. - Discord/ack reactions: keep automatic ACK reaction auth on the active hydrated Discord account so SecretRef-backed and non-default-account reactions stop falling back to stale default config resolution. (#60081) Thanks @FunJim. - Telegram/model switching: render non-default `/model` callback confirmations with HTML formatting so Telegram shows the selected model in bold instead of raw `**...**` markers. (#60042) Thanks @GitZhangChi. - Plugins/update: allow `openclaw plugins update` to use `--dangerously-force-unsafe-install` for built-in dangerous-code false positives during plugin updates. (#60066) Thanks @huntharo. diff --git a/src/infra/net/fetch-guard.ssrf.test.ts b/src/infra/net/fetch-guard.ssrf.test.ts index 73dbc042f82..a63d2c69d80 100644 --- a/src/infra/net/fetch-guard.ssrf.test.ts +++ b/src/infra/net/fetch-guard.ssrf.test.ts @@ -29,6 +29,11 @@ function getSecondRequestHeaders(fetchImpl: ReturnType): Headers { return new Headers(secondInit.headers); } +function getSecondRequestInit(fetchImpl: ReturnType): RequestInit { + const [, secondInit] = fetchImpl.mock.calls[1] as [string, RequestInit]; + return secondInit; +} + async function expectRedirectFailure(params: { url: string; responses: Response[]; @@ -292,6 +297,173 @@ describe("fetchWithSsrFGuard hardening", () => { await result.release(); }); + it("rewrites POST redirects to GET and clears the body for cross-origin 302 responses", async () => { + const lookupFn = createPublicLookup(); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(redirectResponse("https://cdn.example.com/collect")) + .mockResolvedValueOnce(okResponse()); + + const result = await fetchWithSsrFGuard({ + url: "https://api.example.com/login", + fetchImpl, + lookupFn, + init: { + method: "POST", + headers: { + Authorization: "Bearer secret", + "Content-Type": "application/x-www-form-urlencoded", + "Content-Length": "19", + }, + body: "password=hunter2", + }, + }); + + const secondInit = getSecondRequestInit(fetchImpl); + const headers = getSecondRequestHeaders(fetchImpl); + expect(secondInit.method).toBe("GET"); + expect(secondInit.body).toBeUndefined(); + expect(headers.get("authorization")).toBeNull(); + expect(headers.get("content-type")).toBeNull(); + expect(headers.get("content-length")).toBeNull(); + await result.release(); + }); + + it("rewrites same-origin 302 POST redirects to GET and preserves auth headers", async () => { + const lookupFn = createPublicLookup(); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(redirectResponse("https://api.example.com/next")) + .mockResolvedValueOnce(okResponse()); + + const result = await fetchWithSsrFGuard({ + url: "https://api.example.com/login", + fetchImpl, + lookupFn, + init: { + method: "POST", + headers: { + Authorization: "Bearer secret", + "Content-Type": "application/x-www-form-urlencoded", + "Content-Length": "19", + }, + body: "password=hunter2", + }, + }); + + const secondInit = getSecondRequestInit(fetchImpl); + const headers = getSecondRequestHeaders(fetchImpl); + expect(secondInit.method).toBe("GET"); + expect(secondInit.body).toBeUndefined(); + expect(headers.get("authorization")).toBe("Bearer secret"); + expect(headers.get("content-type")).toBeNull(); + expect(headers.get("content-length")).toBeNull(); + await result.release(); + }); + + it("rewrites 303 redirects to GET and clears the body", async () => { + const lookupFn = createPublicLookup(); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + new Response(null, { + status: 303, + headers: { location: "https://api.example.com/final" }, + }), + ) + .mockResolvedValueOnce(okResponse()); + + const result = await fetchWithSsrFGuard({ + url: "https://api.example.com/start", + fetchImpl, + lookupFn, + init: { + method: "PUT", + headers: { + "Content-Type": "application/json", + "Content-Length": "17", + }, + body: '{"secret":"123"}', + }, + }); + + const secondInit = getSecondRequestInit(fetchImpl); + const headers = getSecondRequestHeaders(fetchImpl); + expect(secondInit.method).toBe("GET"); + expect(secondInit.body).toBeUndefined(); + expect(headers.get("content-type")).toBeNull(); + expect(headers.get("content-length")).toBeNull(); + await result.release(); + }); + + it("preserves method and body for 307 redirects", async () => { + const lookupFn = createPublicLookup(); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + new Response(null, { + status: 307, + headers: { location: "https://api.example.com/upload-2" }, + }), + ) + .mockResolvedValueOnce(okResponse()); + + const result = await fetchWithSsrFGuard({ + url: "https://api.example.com/upload", + fetchImpl, + lookupFn, + init: { + method: "POST", + headers: { + "Content-Type": "application/json", + }, + body: '{"secret":"123"}', + }, + }); + + const secondInit = getSecondRequestInit(fetchImpl); + const headers = getSecondRequestHeaders(fetchImpl); + expect(secondInit.method).toBe("POST"); + expect(secondInit.body).toBe('{"secret":"123"}'); + expect(headers.get("content-type")).toBe("application/json"); + await result.release(); + }); + + it("preserves body while stripping auth headers for cross-origin 307 redirects", async () => { + const lookupFn = createPublicLookup(); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + new Response(null, { + status: 307, + headers: { location: "https://cdn.example.com/upload-2" }, + }), + ) + .mockResolvedValueOnce(okResponse()); + + const result = await fetchWithSsrFGuard({ + url: "https://api.example.com/upload", + fetchImpl, + lookupFn, + init: { + method: "POST", + headers: { + Authorization: "Bearer secret", + "Content-Type": "application/json", + }, + body: '{"secret":"123"}', + }, + }); + + const secondInit = getSecondRequestInit(fetchImpl); + const headers = getSecondRequestHeaders(fetchImpl); + expect(secondInit.method).toBe("POST"); + expect(secondInit.body).toBe('{"secret":"123"}'); + expect(headers.get("authorization")).toBeNull(); + expect(headers.get("content-type")).toBe("application/json"); + await result.release(); + }); + it("keeps the exported redirect-header helper functional", () => { const headers = retainSafeHeadersForCrossOriginRedirectHeaders({ Authorization: "Bearer secret", @@ -364,6 +536,18 @@ describe("fetchWithSsrFGuard hardening", () => { }); }); + it("rejects redirect loops that return to the original URL", async () => { + await expectRedirectFailure({ + url: "https://public.example/start", + responses: [ + redirectResponse("https://public.example/next"), + redirectResponse("https://public.example/start"), + ], + expectedError: /redirect loop/i, + lookupFn: createPublicLookup(), + }); + }); + it("blocks URLs that use credentials to obscure a private host", async () => { const fetchImpl = vi.fn(); // http://attacker.com@127.0.0.1:8080/ — URL parser extracts hostname as 127.0.0.1 diff --git a/src/infra/net/fetch-guard.ts b/src/infra/net/fetch-guard.ts index 045bdf6dc25..520c7f361c3 100644 --- a/src/infra/net/fetch-guard.ts +++ b/src/infra/net/fetch-guard.ts @@ -139,6 +139,47 @@ function retainSafeHeadersForCrossOriginRedirect(init?: RequestInit): RequestIni return { ...init, headers: retainSafeRedirectHeaders(init.headers) }; } +function dropBodyHeaders(headers?: HeadersInit): HeadersInit | undefined { + if (!headers) { + return headers; + } + const nextHeaders = new Headers(headers); + nextHeaders.delete("content-encoding"); + nextHeaders.delete("content-language"); + nextHeaders.delete("content-length"); + nextHeaders.delete("content-location"); + nextHeaders.delete("content-type"); + nextHeaders.delete("transfer-encoding"); + return nextHeaders; +} + +function rewriteRedirectInitForMethod(params: { + init?: RequestInit; + status: number; +}): RequestInit | undefined { + const { init, status } = params; + if (!init) { + return init; + } + + const currentMethod = init.method?.toUpperCase() ?? "GET"; + const shouldForceGet = + status === 303 + ? currentMethod !== "GET" && currentMethod !== "HEAD" + : (status === 301 || status === 302) && currentMethod === "POST"; + + if (!shouldForceGet) { + return init; + } + + return { + ...init, + method: "GET", + body: undefined, + headers: dropBodyHeaders(init.headers), + }; +} + export async function fetchWithSsrFGuard(params: GuardedFetchOptions): Promise { const fetcher: FetchLike | undefined = params.fetchImpl ?? globalThis.fetch; if (!fetcher) { @@ -166,7 +207,7 @@ export async function fetchWithSsrFGuard(params: GuardedFetchOptions): Promise(); + const visited = new Set([params.url]); let currentUrl = params.url; let currentInit = params.init ? { ...params.init } : undefined; let redirectCount = 0; @@ -227,6 +268,7 @@ export async function fetchWithSsrFGuard(params: GuardedFetchOptions): Promise