fix(security): preserve SNI for skill URL fetches

src/main/skills/url-validation.ts

@@ -1,4 +1,12 @@
 import { resolve4, resolve6 } from "node:dns/promises";
+import { request, type RequestOptions } from "node:https";
+import type { IncomingHttpHeaders, IncomingMessage } from "node:http";
+import type { LookupFunction } from "node:net";
+
+interface ResolvedIpRecord {
+  address: string;
+  family: 4 | 6;
+}
 
 function isPrivateIPv4(ip: string): boolean {
   const parts = ip.split(".").map(Number);
@@ -62,6 +70,12 @@ export function validateSkillUrl(url: string): void {
  * Resolve DNS and validate that all IPs are public. Returns the resolved IPv4 addresses.
  */
 export async function resolveAndValidateIps(url: string): Promise<string[]> {
+  return (await resolveAndValidateIpRecords(url))
+    .filter((record) => record.family === 4)
+    .map((record) => record.address);
+}
+
+async function resolveAndValidateIpRecords(url: string): Promise<ResolvedIpRecord[]> {
   validateSkillUrl(url);
   const { hostname } = new URL(url);
   const v4: string[] = [];
@@ -69,25 +83,83 @@ export async function resolveAndValidateIps(url: string): Promise<string[]> {
   try { v4.push(...(await resolve4(hostname))); } catch { /* no A records */ }
   try { v6.push(...(await resolve6(hostname))); } catch { /* no AAAA records */ }
   assertPublicIps([...v4, ...v6]);
-  return v4;
+  return [
+    ...v4.map((address): ResolvedIpRecord => ({ address, family: 4 })),
+    ...v6.map((address): ResolvedIpRecord => ({ address, family: 6 })),
+  ];
+}
+
+function buildHeaders(headers: IncomingHttpHeaders): Headers {
+  const out = new Headers();
+  for (const [key, value] of Object.entries(headers)) {
+    if (Array.isArray(value)) {
+      for (const entry of value) out.append(key, entry);
+    } else if (value !== undefined) {
+      out.set(key, value);
+    }
+  }
+  return out;
+}
+
+function readBody(response: IncomingMessage): Promise<Buffer> {
+  return new Promise((resolve, reject) => {
+    const chunks: Buffer[] = [];
+    response.on("data", (chunk: Buffer | string) => {
+      chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk));
+    });
+    response.on("end", () => resolve(Buffer.concat(chunks)));
+    response.on("error", reject);
+  });
+}
+
+export function buildPinnedHttpsRequestOptions(
+  parsed: URL,
+  ip: ResolvedIpRecord,
+): RequestOptions {
+  const lookup: LookupFunction = (_hostname, _options, callback) => {
+    callback(null, ip.address, ip.family);
+  };
+  return {
+    method: "GET",
+    headers: { Host: parsed.host },
+    servername: parsed.hostname,
+    lookup,
+  };
+}
+
+async function fetchHttpsWithResolvedIp(url: string, ip: ResolvedIpRecord): Promise<Response> {
+  const parsed = new URL(url);
+  return new Promise((resolve, reject) => {
+    const req = request(parsed, buildPinnedHttpsRequestOptions(parsed, ip), async (res) => {
+      try {
+        const body = await readBody(res);
+        const arrayBuffer = new ArrayBuffer(body.byteLength);
+        new Uint8Array(arrayBuffer).set(body);
+        resolve(new Response(arrayBuffer, {
+          status: res.statusCode ?? 500,
+          statusText: res.statusMessage,
+          headers: buildHeaders(res.headers),
+        }));
+      } catch (error) {
+        reject(error);
+      }
+    });
+    req.on("error", reject);
+    req.end();
+  });
 }
 
 /**
- * Fetch a URL with SSRF protection: resolve DNS once, validate IPs, then fetch
- * using a resolved IP to prevent DNS rebinding between validation and fetch.
+ * Fetch a URL with SSRF protection: resolve DNS once, validate IPs, then keep
+ * the original hostname for Host/SNI while pinning the socket lookup to a
+ * validated IP.
  */
 export async function fetchWithSsrfProtection(url: string): Promise<Response> {
-  const resolvedIps = await resolveAndValidateIps(url);
+  const resolvedIps = await resolveAndValidateIpRecords(url);
   if (resolvedIps.length === 0) {
-    throw new Error("DNS resolution returned no usable IPv4 addresses");
+    throw new Error("DNS resolution returned no usable addresses");
   }
-  const parsed = new URL(url);
-  const resolvedUrl = new URL(url);
-  resolvedUrl.hostname = resolvedIps[0]!;
-  const response = await fetch(resolvedUrl.toString(), {
-    headers: { Host: parsed.host },
-    redirect: "manual",
-  });
+  const response = await fetchHttpsWithResolvedIp(url, resolvedIps[0]!);
   // Follow redirects manually to prevent redirect-to-private-IP attacks
   if (response.status >= 300 && response.status < 400) {
     const location = response.headers.get("location");

tests/unit/skill-url-validation.test.ts

@@ -0,0 +1,130 @@
+import { EventEmitter } from "node:events";
+import { beforeEach, describe, expect, it, vi } from "vitest";
+import { resolve4, resolve6 } from "node:dns/promises";
+import { request } from "node:https";
+import {
+  buildPinnedHttpsRequestOptions,
+  fetchWithSsrfProtection,
+  validateSkillUrl,
+} from "../../src/main/skills/url-validation";
+
+vi.mock("node:dns/promises", () => ({
+  resolve4: vi.fn(),
+  resolve6: vi.fn(),
+}));
+
+vi.mock("node:https", () => ({
+  request: vi.fn(),
+}));
+
+type RequestCall = {
+  url: URL;
+  options: Parameters<typeof request>[1];
+};
+
+const requestCalls: RequestCall[] = [];
+
+function mockHttpsResponse(input: {
+  statusCode: number;
+  statusMessage?: string;
+  headers?: Record<string, string>;
+  body?: string;
+}) {
+  const requestMock = vi.mocked(request);
+  requestMock.mockImplementationOnce((url, options, callback) => {
+    requestCalls.push({ url: url as URL, options: options as RequestCall["options"] });
+    const req = new EventEmitter() as ReturnType<typeof request>;
+    req.end = vi.fn(() => {
+      const res = new EventEmitter() as EventEmitter & {
+        statusCode: number;
+        statusMessage?: string;
+        headers: Record<string, string>;
+      };
+      res.statusCode = input.statusCode;
+      res.statusMessage = input.statusMessage;
+      res.headers = input.headers ?? {};
+      queueMicrotask(() => {
+        callback?.(res as never);
+        if (input.body) res.emit("data", Buffer.from(input.body));
+        res.emit("end");
+      });
+      return req;
+    }) as never;
+    return req;
+  });
+}
+
+describe("skill URL validation", () => {
+  beforeEach(() => {
+    requestCalls.length = 0;
+    vi.mocked(resolve4).mockReset();
+    vi.mocked(resolve6).mockReset();
+    vi.mocked(request).mockReset();
+  });
+
+  it("pins the socket lookup while preserving Host and SNI", () => {
+    const parsed = new URL("https://example.com/skills/SKILL.md");
+    const options = buildPinnedHttpsRequestOptions(parsed, { address: "93.184.216.34", family: 4 });
+
+    expect(options.servername).toBe("example.com");
+    expect(options.headers).toEqual({ Host: "example.com" });
+
+    let resolved: { address: string; family: number } | null = null;
+    if (typeof options.lookup !== "function") throw new Error("Expected lookup function.");
+    options.lookup("example.com", {}, (error, address, family) => {
+      if (error) throw error;
+      resolved = { address: String(address), family: Number(family) };
+    });
+
+    expect(resolved).toEqual({ address: "93.184.216.34", family: 4 });
+  });
+
+  it("rejects non-HTTPS and private skill import URLs", () => {
+    expect(() => validateSkillUrl("http://example.com/SKILL.md")).toThrow("Only HTTPS URLs are allowed");
+    expect(() => validateSkillUrl("https://127.0.0.1/SKILL.md")).toThrow("Private/reserved IP addresses");
+    expect(() => validateSkillUrl("https://localhost/SKILL.md")).toThrow("Private/reserved IP addresses");
+  });
+
+  it("fetchWithSsrfProtection keeps the original hostname while pinning lookup to validated IP", async () => {
+    vi.mocked(resolve4).mockResolvedValue(["93.184.216.34"]);
+    vi.mocked(resolve6).mockResolvedValue([]);
+    mockHttpsResponse({ statusCode: 200, headers: { "content-type": "text/plain" }, body: "ok" });
+
+    const response = await fetchWithSsrfProtection("https://example.com/skills/SKILL.md");
+
+    expect(response.status).toBe(200);
+    await expect(response.text()).resolves.toBe("ok");
+    expect(requestCalls).toHaveLength(1);
+    expect(requestCalls[0]?.url.hostname).toBe("example.com");
+    expect(requestCalls[0]?.options).toMatchObject({
+      headers: { Host: "example.com" },
+      servername: "example.com",
+    });
+
+    let resolved: { address: string; family: number } | null = null;
+    const lookup = requestCalls[0]?.options?.lookup;
+    if (typeof lookup !== "function") throw new Error("Expected lookup function.");
+    lookup("example.com", {}, (error, address, family) => {
+      if (error) throw error;
+      resolved = { address: String(address), family: Number(family) };
+    });
+    expect(resolved).toEqual({ address: "93.184.216.34", family: 4 });
+  });
+
+  it("revalidates redirects before following them", async () => {
+    vi.mocked(resolve4)
+      .mockResolvedValueOnce(["93.184.216.34"])
+      .mockResolvedValueOnce(["127.0.0.1"]);
+    vi.mocked(resolve6).mockResolvedValue([]);
+    mockHttpsResponse({
+      statusCode: 302,
+      headers: { location: "https://redirect.example/SKILL.md" },
+    });
+
+    await expect(fetchWithSsrfProtection("https://example.com/skills/SKILL.md"))
+      .rejects
+      .toThrow("DNS resolved to private IP 127.0.0.1");
+    expect(requestCalls).toHaveLength(1);
+    expect(requestCalls[0]?.url.hostname).toBe("example.com");
+  });
+});