diff --git a/src/main/skills/url-validation.ts b/src/main/skills/url-validation.ts index 352683f5..f70d6561 100644 --- a/src/main/skills/url-validation.ts +++ b/src/main/skills/url-validation.ts @@ -8,6 +8,8 @@ interface ResolvedIpRecord { family: 4 | 6; } +const MAX_REDIRECT_HOPS = 5; + function isPrivateIPv4(ip: string): boolean { const parts = ip.split(".").map(Number); if (parts.length !== 4 || parts.some((p) => isNaN(p) || p < 0 || p > 255)) return false; @@ -154,7 +156,22 @@ async function fetchHttpsWithResolvedIp(url: string, ip: ResolvedIpRecord): Prom * the original hostname for Host/SNI while pinning the socket lookup to a * validated IP. */ -export async function fetchWithSsrfProtection(url: string): Promise { +export async function fetchWithSsrfProtection( + url: string, + state: { visited: Set; hops: number; chain: string[] } = { + visited: new Set(), + hops: 0, + chain: [], + }, +): Promise { + if (state.visited.has(url)) { + throw new Error(`Redirect loop detected while importing skill: ${[...state.chain, url].join(" -> ")}`); + } + if (state.hops > MAX_REDIRECT_HOPS) { + throw new Error(`Too many redirects while importing skill: ${[...state.chain, url].join(" -> ")}`); + } + state.visited.add(url); + state.chain.push(url); const resolvedIps = await resolveAndValidateIpRecords(url); if (resolvedIps.length === 0) { throw new Error("DNS resolution returned no usable addresses"); @@ -167,8 +184,11 @@ export async function fetchWithSsrfProtection(url: string): Promise { throw new Error(`Redirect ${response.status} with no Location header`); } const redirectUrl = new URL(location, url).toString(); - // Recursively validate the redirect target (prevents redirect to private IP) - return fetchWithSsrfProtection(redirectUrl); + return fetchWithSsrfProtection(redirectUrl, { + visited: state.visited, + hops: state.hops + 1, + chain: state.chain, + }); } return response; } diff --git a/tests/unit/skill-url-validation.test.ts b/tests/unit/skill-url-validation.test.ts index 02525202..b1f6c36c 100644 --- a/tests/unit/skill-url-validation.test.ts +++ b/tests/unit/skill-url-validation.test.ts @@ -127,4 +127,45 @@ describe("skill URL validation", () => { expect(requestCalls).toHaveLength(1); expect(requestCalls[0]?.url.hostname).toBe("example.com"); }); + + it("rejects redirect loops with a concrete chain", async () => { + vi.mocked(resolve4).mockResolvedValue(["93.184.216.34"]); + vi.mocked(resolve6).mockResolvedValue([]); + mockHttpsResponse({ + statusCode: 302, + headers: { location: "https://loop.example/SKILL.md" }, + }); + mockHttpsResponse({ + statusCode: 302, + headers: { location: "https://example.com/skills/SKILL.md" }, + }); + + await expect(fetchWithSsrfProtection("https://example.com/skills/SKILL.md")) + .rejects + .toThrow("Redirect loop detected while importing skill: https://example.com/skills/SKILL.md -> https://loop.example/SKILL.md -> https://example.com/skills/SKILL.md"); + expect(requestCalls.map((call) => call.url.hostname)).toEqual(["example.com", "loop.example"]); + }); + + it("rejects redirect chains beyond the hop limit", async () => { + vi.mocked(resolve4).mockResolvedValue(["93.184.216.34"]); + vi.mocked(resolve6).mockResolvedValue([]); + for (let index = 1; index <= 6; index += 1) { + mockHttpsResponse({ + statusCode: 302, + headers: { location: `https://redirect-${index}.example/SKILL.md` }, + }); + } + + await expect(fetchWithSsrfProtection("https://origin.example/SKILL.md")) + .rejects + .toThrow("Too many redirects while importing skill:"); + expect(requestCalls.map((call) => call.url.hostname)).toEqual([ + "origin.example", + "redirect-1.example", + "redirect-2.example", + "redirect-3.example", + "redirect-4.example", + "redirect-5.example", + ]); + }); });