Address review: constrain artifact fetches to the allowlist

Security fixes for the well-known source's fetch path:

- B1 (SSRF / allowlist bypass): validate every fetched URL — index, each
  entry's artifact url, and redirect targets — against the origin allowlist
  and scheme rules via new assertUrlAllowed(). Previously only the configured
  index origin was gated, so an allowlisted-but-malicious publisher could
  point an artifact url at an internal host or off-allowlist CDN. Artifact
  origins (incl. CDNs) must now be on WELL_KNOWN_ALLOWED_ORIGINS.
- B2 (silent off-origin redirects): fetch now uses redirect:"manual" and
  follows hops itself, re-validating each Location (max 5) so a 3xx can't
  escape the allowlist after the initial check.
- N1: the polling update-check now reads the index body through the same
  size-capped reader instead of an unbounded arrayBuffer().
- N5: WELL_KNOWN_MAX_*_MB=0 (a 0-byte cap) now falls back to the default
  instead of failing every fetch.
- N2/N3: clarifying comments on the tar header-size trust and the zip
  symlink-attribute fallback (documented as non-escape).

Tests: assertUrlAllowed unit cases (off-origin, scheme, http-gate) plus sync
integration tests for cross-origin artifact rejection, off-allowlist redirect
rejection, and a followed same-origin redirect. Full suite 225 green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: olaservo

README.md

@@ -37,6 +37,9 @@ GITHUB_ALLOWED_ORGS=acme skilljack-mcp github.com/acme/skills
 
 # Well-known publisher (allowlisted via WELL_KNOWN_ALLOWED_ORIGINS).
 # Each entry's SHA-256 digest is verified against the published index.
+# Every fetched URL — the index, each artifact, and any redirect target — must
+# match an allowlisted origin, so if a publisher serves artifacts from a
+# separate CDN origin, add that origin to the list too (comma-separated).
 WELL_KNOWN_ALLOWED_ORIGINS=https://example.com \
   skilljack-mcp https://example.com/.well-known/agent-skills/
 

src/index.ts

@@ -608,6 +608,8 @@ async function main() {
         cacheDir: wellKnownConfig.cacheDir,
         maxArtifactBytes: wellKnownConfig.maxArtifactBytes,
         maxUnpackedBytes: wellKnownConfig.maxUnpackedBytes,
+        allowedOrigins: wellKnownConfig.allowedOrigins,
+        allowHttp: wellKnownConfig.allowHttp,
       };
 
       const wkResults = await syncAllWellKnown(currentWellKnownSpecs, wkSyncOptions);
@@ -776,6 +778,8 @@ async function main() {
           cacheDir: freshWellKnownConfig.cacheDir,
           maxArtifactBytes: freshWellKnownConfig.maxArtifactBytes,
           maxUnpackedBytes: freshWellKnownConfig.maxUnpackedBytes,
+          allowedOrigins: freshWellKnownConfig.allowedOrigins,
+          allowHttp: freshWellKnownConfig.allowHttp,
         };
         const wkResults = await syncAllWellKnown(allowedWellKnownSpecs, wkSyncOptions);
         for (const result of wkResults) {
@@ -861,6 +865,8 @@ async function main() {
       cacheDir: wellKnownConfig.cacheDir,
       maxArtifactBytes: wellKnownConfig.maxArtifactBytes,
       maxUnpackedBytes: wellKnownConfig.maxUnpackedBytes,
+      allowedOrigins: wellKnownConfig.allowedOrigins,
+      allowHttp: wellKnownConfig.allowHttp,
     };
 
     wellKnownPollingManager = createWellKnownPollingManager(

src/well-known-config.test.ts

@@ -4,6 +4,7 @@ import {
   isWellKnownUrl,
   parseWellKnownUrl,
   isOriginAllowed,
+  assertUrlAllowed,
   getWellKnownCacheKey,
   getWellKnownCachePath,
   getWellKnownIndexUrl,
@@ -152,6 +153,46 @@ describe("isOriginAllowed", () => {
   });
 });
 
+describe("assertUrlAllowed", () => {
+  const allowed = { allowedOrigins: ["https://example.com"], allowHttp: false };
+
+  it("accepts an https url whose origin is allowlisted", () => {
+    expect(() =>
+      assertUrlAllowed("https://example.com/skills/a/SKILL.md", allowed)
+    ).not.toThrow();
+  });
+
+  it("rejects an off-allowlist origin (SSRF / cross-origin artifact)", () => {
+    expect(() =>
+      assertUrlAllowed("https://169.254.169.254/latest/meta-data", allowed)
+    ).toThrow(/not in WELL_KNOWN_ALLOWED_ORIGINS/);
+    expect(() =>
+      assertUrlAllowed("https://cdn.example.com/a.tgz", allowed)
+    ).toThrow(/not in WELL_KNOWN_ALLOWED_ORIGINS/);
+  });
+
+  it("rejects http when allowHttp is false, even if origin matches", () => {
+    expect(() =>
+      assertUrlAllowed("http://example.com/a", allowed)
+    ).toThrow(/disallowed scheme/);
+  });
+
+  it("permits http only when allowHttp is set and origin matches", () => {
+    const cfg = { allowedOrigins: ["http://example.com"], allowHttp: true };
+    expect(() => assertUrlAllowed("http://example.com/a", cfg)).not.toThrow();
+    // still origin-gated
+    expect(() => assertUrlAllowed("http://evil.com/a", cfg)).toThrow(
+      /not in WELL_KNOWN_ALLOWED_ORIGINS/
+    );
+  });
+
+  it("rejects non-http(s) schemes (file:, etc.)", () => {
+    expect(() => assertUrlAllowed("file:///etc/passwd", allowed)).toThrow(
+      /disallowed scheme/
+    );
+  });
+});
+
 describe("getWellKnownCacheKey", () => {
   it("produces a unique key per host + base path", () => {
     const a = getWellKnownCacheKey({

src/well-known-config.ts

@@ -144,6 +144,51 @@ export function isOriginAllowed(spec: WellKnownSpec, config: WellKnownConfig): b
   return config.allowedOrigins.some((allowed) => allowed.toLowerCase() === target);
 }
 
+/**
+ * Validate that an arbitrary URL is safe to fetch: it must use an allowed
+ * scheme (https, or http only when allowHttp is set) and its origin must be on
+ * the allowlist. Throws with a descriptive message otherwise, returning the
+ * parsed URL on success.
+ *
+ * This is the trust boundary for every network request the well-known source
+ * makes — not just the configured index origin, but each skill entry's artifact
+ * `url` and any redirect target. Without it, an allowlisted-but-malicious
+ * publisher could point `url` at an internal endpoint (SSRF) or an off-allowlist
+ * host, bypassing both the origin allowlist and the http scheme gate.
+ */
+export function assertUrlAllowed(
+  rawUrl: string,
+  config: Pick<WellKnownConfig, "allowedOrigins" | "allowHttp">,
+  label = "URL"
+): URL {
+  let parsed: URL;
+  try {
+    parsed = new URL(rawUrl);
+  } catch {
+    throw new Error(`${label} is not a valid absolute URL: "${rawUrl}"`);
+  }
+
+  const isHttps = parsed.protocol === "https:";
+  const isAllowedHttp = config.allowHttp && parsed.protocol === "http:";
+  if (!isHttps && !isAllowedHttp) {
+    throw new Error(
+      `${label} uses disallowed scheme "${parsed.protocol}": ${rawUrl}`
+    );
+  }
+
+  const origin = `${parsed.protocol}//${parsed.host}`;
+  const allowed = config.allowedOrigins.some(
+    (o) => o.toLowerCase() === origin.toLowerCase()
+  );
+  if (!allowed) {
+    throw new Error(
+      `${label} origin "${origin}" is not in WELL_KNOWN_ALLOWED_ORIGINS: ${rawUrl}`
+    );
+  }
+
+  return parsed;
+}
+
 /**
  * Parse a comma-separated list from an environment variable.
  */
@@ -181,12 +226,21 @@ function loadAllowedOriginsFromConfig(): string[] {
 }
 
 /**
- * Parse a positive integer environment variable, falling back to a default.
+ * Parse a non-negative integer environment variable, falling back to a default.
+ *
+ * `minValue` sets the smallest accepted value: size caps pass `1` so that
+ * `WELL_KNOWN_MAX_ARTIFACT_MB=0` (a 0-byte cap that would fail every fetch)
+ * falls back to the default instead of silently disabling the feature. The
+ * poll interval keeps `minValue = 0`, where `0` meaningfully disables polling.
  */
-function parseIntEnv(value: string | undefined, fallback: number): number {
+function parseIntEnv(
+  value: string | undefined,
+  fallback: number,
+  minValue = 0
+): number {
   if (value === undefined) return fallback;
   const parsed = parseInt(value, 10);
-  if (Number.isNaN(parsed) || parsed < 0) return fallback;
+  if (Number.isNaN(parsed) || parsed < minValue) return fallback;
   return parsed;
 }
 
@@ -215,8 +269,10 @@ export function getWellKnownConfig(): WellKnownConfig {
     ? path.join(cacheBase, "well-known")
     : DEFAULT_CACHE_DIR;
 
-  const maxArtifactMb = parseIntEnv(process.env.WELL_KNOWN_MAX_ARTIFACT_MB, DEFAULT_MAX_ARTIFACT_MB);
-  const maxUnpackedMb = parseIntEnv(process.env.WELL_KNOWN_MAX_UNPACKED_MB, DEFAULT_MAX_UNPACKED_MB);
+  // minValue 1: a 0-byte cap is a footgun (fails every fetch), so fall back to
+  // the default rather than silently disabling downloads.
+  const maxArtifactMb = parseIntEnv(process.env.WELL_KNOWN_MAX_ARTIFACT_MB, DEFAULT_MAX_ARTIFACT_MB, 1);
+  const maxUnpackedMb = parseIntEnv(process.env.WELL_KNOWN_MAX_UNPACKED_MB, DEFAULT_MAX_UNPACKED_MB, 1);
 
   return {
     pollIntervalMs: parseIntEnv(process.env.WELL_KNOWN_POLL_INTERVAL_MS, DEFAULT_POLL_INTERVAL_MS),

src/well-known-polling.test.ts

@@ -76,7 +76,13 @@ describe("createWellKnownPollingManager", () => {
     server = await startServer();
     cacheDir = fs.mkdtempSync(path.join(os.tmpdir(), "wkpoll-"));
     spec = { origin: server.baseUrl, basePath: "/.well-known/agent-skills" };
-    options = { cacheDir, maxArtifactBytes: 1024 * 1024, maxUnpackedBytes: 1024 * 1024 };
+    options = {
+      cacheDir,
+      maxArtifactBytes: 1024 * 1024,
+      maxUnpackedBytes: 1024 * 1024,
+      allowedOrigins: [server.baseUrl],
+      allowHttp: true,
+    };
   });
 
   afterEach(async () => {

src/well-known-sync.test.ts

@@ -39,6 +39,8 @@ interface RouteResponse {
   contentType?: string;
   body: Buffer | string;
   delay?: number;
+  /** Extra response headers (e.g. Location for redirects). */
+  headers?: Record<string, string>;
 }
 
 interface FixtureServer {
@@ -59,6 +61,7 @@ async function startFixtureServer(): Promise<FixtureServer> {
     res.writeHead(route.status ?? 200, {
       "Content-Type": route.contentType ?? "application/octet-stream",
       "Content-Length": String(body.byteLength),
+      ...route.headers,
     });
     if (route.delay) {
       setTimeout(() => res.end(body), route.delay);
@@ -235,6 +238,8 @@ describe("syncWellKnown", () => {
       cacheDir,
       maxArtifactBytes: 1024 * 1024,
       maxUnpackedBytes: 1024 * 1024,
+      allowedOrigins: [server.baseUrl],
+      allowHttp: true,
     };
   });
 
@@ -324,6 +329,62 @@ describe("syncWellKnown", () => {
     expect(result.skillsSynced).toEqual([]);
   });
 
+  it("rejects an artifact url whose origin is not on the allowlist (SSRF guard)", async () => {
+    const body = makeSkillMd("evil", "x");
+    // Absolute cross-origin url — a would-be SSRF target. It is allowlist-gated
+    // and never fetched, even though its digest would match.
+    const offOrigin = "http://169.254.169.254/latest/meta-data/SKILL.md";
+    setIndex([
+      { name: "evil", type: "skill-md", description: "evil", url: offOrigin, digest: digestOf(body) },
+    ]);
+
+    const result = await syncWellKnown(spec, options);
+
+    expect(result.error).toBeUndefined();
+    expect(result.skillsSynced).toEqual([]);
+    expect(result.skillErrors.evil).toMatch(/not in WELL_KNOWN_ALLOWED_ORIGINS/);
+  });
+
+  it("rejects a redirect that leaves the allowlist", async () => {
+    const body = makeSkillMd("redir", "x");
+    const { url, digest } = setSkillMd("redir", body);
+    // Same-origin artifact url, but the server 302s it off-allowlist.
+    server.routes.set(url, {
+      status: 302,
+      body: "",
+      headers: { Location: "http://169.254.169.254/evil" },
+    });
+    setIndex([
+      { name: "redir", type: "skill-md", description: "redir", url, digest },
+    ]);
+
+    const result = await syncWellKnown(spec, options);
+
+    expect(result.error).toBeUndefined();
+    expect(result.skillsSynced).toEqual([]);
+    expect(result.skillErrors.redir).toMatch(/not in WELL_KNOWN_ALLOWED_ORIGINS/);
+  }, 15000);
+
+  it("follows a same-origin redirect that stays on the allowlist", async () => {
+    const body = makeSkillMd("hop", "x");
+    const finalPath = "/.well-known/agent-skills/hop/real-SKILL.md";
+    server.routes.set(finalPath, { contentType: "text/markdown", body });
+    const startPath = "/.well-known/agent-skills/hop/SKILL.md";
+    server.routes.set(startPath, {
+      status: 302,
+      body: "",
+      headers: { Location: finalPath },
+    });
+    setIndex([
+      { name: "hop", type: "skill-md", description: "hop", url: startPath, digest: digestOf(body) },
+    ]);
+
+    const result = await syncWellKnown(spec, options);
+
+    expect(result.skillErrors).toEqual({});
+    expect(result.skillsSynced).toEqual(["hop"]);
+  });
+
   it("warns and skips entries with unknown type, but other entries still load", async () => {
     const body = makeSkillMd("good", "ok");
     const { url, digest } = setSkillMd("good", body);