fix(fetch): normalize guarded redirect handling (#59121)

* fix(fetch): align guarded redirect rewrites

* fix(fetch): tighten redirect coverage

* fix(fetch): add changelog entry
This commit is contained in:
Agustin Rivera 2026-04-03 15:03:18 -07:00 committed by GitHub
parent 393d8c7606
commit 42ffdf882f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 228 additions and 1 deletions

View File

@ -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.

View File

@ -29,6 +29,11 @@ function getSecondRequestHeaders(fetchImpl: ReturnType<typeof vi.fn>): Headers {
return new Headers(secondInit.headers);
}
function getSecondRequestInit(fetchImpl: ReturnType<typeof vi.fn>): 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

View File

@ -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<GuardedFetchResult> {
const fetcher: FetchLike | undefined = params.fetchImpl ?? globalThis.fetch;
if (!fetcher) {
@ -166,7 +207,7 @@ export async function fetchWithSsrFGuard(params: GuardedFetchOptions): Promise<G
await closeDispatcher(dispatcher ?? undefined);
};
const visited = new Set<string>();
const visited = new Set<string>([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<G
await release(dispatcher);
throw new Error("Redirect loop detected");
}
currentInit = rewriteRedirectInitForMethod({ init: currentInit, status: response.status });
if (nextParsedUrl.origin !== parsedUrl.origin) {
currentInit = retainSafeHeadersForCrossOriginRedirect(currentInit);
}