fix(security): bound skill URL redirects

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<Response> {
+export async function fetchWithSsrfProtection(
+  url: string,
+  state: { visited: Set<string>; hops: number; chain: string[] } = {
+    visited: new Set<string>(),
+    hops: 0,
+    chain: [],
+  },
+): Promise<Response> {
+  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<Response> {
       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;
 }

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",
+    ]);
+  });
 });