Reject address forms the platform resolver reads differently

A dotted quad is legal only at the end of a whole address, and octets are
decimal-only: both forms otherwise decode to a public-looking leading word
while the resolver dials loopback. Also fail an empty resolver answer so the
zero failure TTL drops it instead of blackholing the hostname.

Author: RhysSullivan

.changeset/hosted-dns-guard-latency.md

@@ -2,4 +2,4 @@
 "@executor-js/sdk": patch
 ---
 
-Cache hosted outbound DNS guard resolutions, so a proxied request no longer pays a fresh lookup on every hop. Also classifies the IPv6 prefixes that carry an IPv4 destination (IPv4-translatable, 6to4, local-use NAT64) by that destination, and checks every address a hostname resolves to rather than the first.
+Cache hosted outbound DNS guard resolutions, so a proxied request no longer pays a fresh lookup on every hop. Also tightens the outbound guard's address classifiers: IPv6 prefixes that carry an IPv4 destination (IPv4-translatable, 6to4, local-use NAT64) are classified by that destination, every address a hostname resolves to is checked rather than the first, and address forms the platform resolver reads differently from a decimal-only parser (octal octets, a dotted quad in the head of a compressed literal) no longer classify as public.

packages/core/sdk/src/hosted-http-client.test.ts

@@ -246,6 +246,47 @@ describe("hosted outbound HTTP client", () => {
     }),
   );
 
+  // Where the parsers disagree with the platform resolver about what an
+  // address means, the guard reads one destination and the connection dials
+  // another. Both forms below are decoded by the parsers, not rejected by
+  // them, so they reach the classifiers wearing a public-looking leading word
+  // — the exact failure the "cannot classify" test above cannot catch.
+  //
+  // A dotted quad is legal only at the end of the whole address; permitting
+  // one at the end of the head half of a compressed literal puts 0x7f00 in
+  // the leading word, where no prefix or mask matches it. And a leading zero
+  // means octal to inet_aton, so "0177.0.0.1" is 127.0.0.1 to the resolver
+  // and 177.0.0.1 to a decimal-only parser.
+  it.effect("rejects resolved addresses whose syntax the platform reads differently", () =>
+    Effect.gen(function* () {
+      for (const address of [
+        "127.0.0.1::1",
+        "192.168.1.1::",
+        "10.0.0.1::0",
+        "169.254.169.254::1",
+        "0177.0.0.1",
+      ]) {
+        const error = yield* validateHostedOutboundUrl("https://api.example/x", {
+          resolveHostname: async () => [{ address }],
+        }).pipe(Effect.flip);
+
+        expect(error).toMatchObject({
+          _tag: "HostedOutboundRequestBlocked",
+          reason: "Resolved address is local or private",
+        });
+      }
+
+      // Guards against over-blocking: the trailing quad is still legal where
+      // it belongs, so tightening the rule must not reject the v4-compatible
+      // and v4-mapped forms of an ordinary public address.
+      for (const address of ["::93.184.216.34", "::ffff:93.184.216.34"]) {
+        yield* validateHostedOutboundUrl("https://api.example/x", {
+          resolveHostname: async () => [{ address }],
+        });
+      }
+    }),
+  );
+
   // node:dns can hand back a link-local address carrying its interface scope.
   // The zone identifier is not part of the address and must not defeat the
   // classification of the address it is attached to.
@@ -344,6 +385,32 @@ describe("hosted outbound HTTP client", () => {
     }),
   );
 
+  // An empty answer is a failure, not a successful "no addresses". Only the
+  // error channel takes the zero failure TTL, so caching it as a success would
+  // stamp the full 60s window on one transient empty answer and blackhole the
+  // hostname for its duration — something the uncached code could never do.
+  it("re-resolves after a resolver returns no addresses", async () => {
+    let attempts = 0;
+    const hostedFetch = makeHostedFetch({
+      fetch: async () => new Response("ok"),
+      resolveHostname: async () => {
+        attempts++;
+        return attempts === 1 ? [] : [{ address: "93.184.216.34" }];
+      },
+    });
+
+    await expect(hostedFetch("https://api.example/x")).rejects.toMatchObject({
+      _tag: "HostedOutboundRequestBlocked",
+      reason: "Hostname did not resolve to an address",
+    });
+
+    // The second request must reach the resolver again rather than replaying
+    // the empty answer out of the cache.
+    const recovered = await hostedFetch("https://api.example/x");
+    expect(recovered.status).toBe(200);
+    expect(attempts).toBe(2);
+  });
+
   it.effect("rejects IPv4-mapped IPv6 URLs for local and private networks", () =>
     Effect.gen(function* () {
       for (const url of [
@@ -620,12 +687,18 @@ describe("hosted outbound HTTP client", () => {
         await gate.promise;
         return [{ address: "93.184.216.34" }];
       },
+      // A zero TTL is what makes the count mean what this test says it means.
+      // Under the default 60s window a settled entry is reusable, so
+      // `resolutions === 1` is also satisfied by the second request simply
+      // arriving after the first finished and reading the cache — the test
+      // would keep passing while no longer testing in-flight sharing at all.
+      // With no settled entry to find, one resolution can only be a join.
+      dnsCacheTtlMillis: 0,
     });
 
     // The second request is issued only once the first is provably inside the
-    // resolver, so a single resolution can only mean it joined the in-flight
-    // lookup. Firing both at once and counting would also pass if the second
-    // simply arrived after the first settled and hit the TTL cache.
+    // resolver, then given a turn of the event loop to reach the cache before
+    // the gate opens: issuance order alone does not put it there.
     const first = hostedFetch("https://api.example/one");
     await entered.promise;
     const second = hostedFetch("https://api.example/two");
@@ -641,13 +714,11 @@ describe("hosted outbound HTTP client", () => {
   // point: before the shared lookup was detached from the requesting fiber it
   // failed with an untyped "All fibers interrupted without error".
   it("keeps concurrent requests alive when one caller aborts", async () => {
-    let resolutions = 0;
     const gate = Promise.withResolvers<void>();
     const entered = Promise.withResolvers<void>();
     const hostedFetch = makeHostedFetch({
       fetch: async () => new Response("ok"),
       resolveHostname: async () => {
-        resolutions++;
         entered.resolve();
         await gate.promise;
         return [{ address: "93.184.216.34" }];
@@ -667,8 +738,14 @@ describe("hosted outbound HTTP client", () => {
     // Asserting the shape, not merely that it threw: the regression this test
     // exists to exclude also throws, so a bare toThrow() would accept it.
     await expect(aborted).rejects.toMatchObject({ name: "AbortError" });
+    // The survivor completing is the assertion. Its 200 does not prove it was
+    // still waiting on the shared lookup — under the default TTL a caller that
+    // arrived after the first resolution settled would read the cache and get
+    // the same 200 — so no resolution count is asserted here. That the lookup
+    // survives an abort at all is what fails without forkDetach: the survivor
+    // rejects with "All fibers interrupted without error". In-flight sharing
+    // itself is pinned by the zero-TTL test above.
     expect((await survivor).status).toBe(200);
-    expect(resolutions).toBe(1);
   });
 
   // The guard's verdict is decided by the failure's cause, not by whether the

packages/core/sdk/src/hosted-http-client.ts

@@ -34,26 +34,38 @@ export interface HostedHttpClientOptions {
   readonly dnsResolutionTimeoutMillis?: number;
 }
 
+// Octets are decimal-only and leading zeros are rejected. inet_aton reads a
+// leading zero as octal, so accepting "0177.0.0.1" here would classify it as
+// 177.0.0.1 (public) while the platform resolver dials 127.0.0.1 — the guard
+// and the connection would disagree about the destination. WHATWG normalizes
+// these forms, so the URL path never produces one, but a resolver answer
+// reaches isAllowedResolvedAddress unnormalized.
 const parseIpv4 = (hostname: string): readonly [number, number, number, number] | null => {
   const parts = hostname.split(".");
   if (parts.length !== 4) return null;
   const parsed: number[] = [];
   for (const part of parts) {
-    if (!/^\d+$/.test(part)) return null;
+    if (!/^(?:0|[1-9]\d*)$/.test(part)) return null;
     const value = Number(part);
     if (!Number.isInteger(value) || value < 0 || value > 255) return null;
     parsed.push(value);
   }
   return parsed as [number, number, number, number];
 };
 
-const parseIpv6Groups = (text: string): number[] | null => {
+// `allowTrailingQuad` is the caller's statement that this segment ends the
+// whole address, not merely that it ends its own half. Deciding it from the
+// segment alone would legalize a quad at the end of the head half of a
+// compressed literal: "127.0.0.1::1" would parse, land 0x7f00 in the leading
+// word where no prefix or mask matches it, and be classified public.
+const parseIpv6Groups = (text: string, allowTrailingQuad: boolean): number[] | null => {
   if (text === "") return [];
   const parts = text.split(":");
   const groups: number[] = [];
   for (const [index, part] of parts.entries()) {
     // A trailing dotted quad is the only place IPv4 syntax is legal.
     if (index === parts.length - 1 && part.includes(".")) {
+      if (!allowTrailingQuad) return null;
       const dotted = parseIpv4(part);
       if (!dotted) return null;
       groups.push((dotted[0] << 8) | dotted[1], (dotted[2] << 8) | dotted[3]);
@@ -78,11 +90,12 @@ const parseIpv6 = (hostname: string): ReadonlyArray<number> | null => {
   const [head, tail, ...extra] = hostname.replace(/%.*$/, "").split("::");
   if (extra.length > 0) return null;
 
-  const high = parseIpv6Groups(head);
+  // The head half ends the address only when there is no "::" after it.
+  const high = parseIpv6Groups(head, tail === undefined);
   if (high === null) return null;
   if (tail === undefined) return high.length === 8 ? high : null;
 
-  const low = parseIpv6Groups(tail);
+  const low = parseIpv6Groups(tail, true);
   if (low === null) return null;
   const elided = 8 - high.length - low.length;
   if (elided < 1) return null;
@@ -193,10 +206,15 @@ const isAllowedResolvedAddress = (address: string): boolean => {
 const isAddressLiteral = (normalized: string): boolean =>
   parseIpv4(normalized) !== null || normalized.includes(":");
 
+// `empty` separates "the resolver answered, with nothing" from "the resolver
+// did not answer". Both fail closed and both must stay out of the success
+// cache, but they are different operator diagnoses, so the block reason keeps
+// them apart rather than collapsing into one message.
 class HostedHostnameResolutionFailed extends Schema.TaggedErrorClass<HostedHostnameResolutionFailed>()(
   "HostedHostnameResolutionFailed",
   {
     hostname: Schema.String,
+    empty: Schema.Boolean,
   },
 ) {}
 
@@ -210,13 +228,24 @@ type GuardResolver = (
   hostname: string,
 ) => Effect.Effect<ReadonlyArray<HostedResolvedAddress>, HostedHostnameResolutionFailed>;
 
+// An empty answer becomes a failure at the resolver boundary, not at the call
+// site, so it reaches every caller the same way: the cached path needs it in
+// the error channel for the zero failure TTL to drop it, and putting it here
+// rather than inside the cache keeps the uncached path — the exported
+// validateHostedOutboundUrl — from having a second copy of the rule.
 const toGuardResolver =
   (resolve: HostedHostnameResolver): GuardResolver =>
   (hostname) =>
     Effect.tryPromise({
       try: (signal) => resolve(hostname, signal),
-      catch: () => new HostedHostnameResolutionFailed({ hostname }),
-    });
+      catch: () => new HostedHostnameResolutionFailed({ hostname, empty: false }),
+    }).pipe(
+      Effect.flatMap((addresses) =>
+        addresses.length === 0
+          ? Effect.fail(new HostedHostnameResolutionFailed({ hostname, empty: true }))
+          : Effect.succeed(addresses),
+      ),
+    );
 
 const DNS_CACHE_TTL_MILLIS = 60_000;
 const DNS_CACHE_CAPACITY = 256;
@@ -261,12 +290,18 @@ const withResolutionCache = (
   // the request instead is a worse outcome than the wait. Callers that will
   // not wait that long abort their own fetch, which the detached lookup
   // survives on behalf of everyone else waiting on it.
+  //
+  // An empty answer is already a failure by the time it reaches the cache —
+  // toGuardResolver raises it — so the zero failure TTL drops it. Were it a
+  // success, one transient empty answer would stamp the full TTL on "no
+  // addresses" and blackhole the hostname for the rest of the window; the
+  // uncached code re-resolved every request and never could.
   const bounded: GuardResolver = (hostname) =>
     Effect.timeoutOrElse(resolve(hostname), {
       duration: Duration.millis(
         options.dnsResolutionTimeoutMillis ?? DNS_RESOLUTION_TIMEOUT_MILLIS,
       ),
-      orElse: () => Effect.fail(new HostedHostnameResolutionFailed({ hostname })),
+      orElse: () => Effect.fail(new HostedHostnameResolutionFailed({ hostname, empty: false })),
     });
   const cache = Effect.runSync(
     Cache.makeWith(bounded, {
@@ -321,24 +356,22 @@ const validateOutboundUrl = (
     }
 
     if (!options.allowLocalNetwork && !isAddressLiteral(normalizedHostname)) {
+      // The empty answer is already a failure by the time it arrives here —
+      // the cached lookup raises it, so it takes the zero failure TTL instead
+      // of being retained as a successful "no addresses" for the full window.
       const addresses = yield* resolve(normalizedHostname).pipe(
-        Effect.catchTag("HostedHostnameResolutionFailed", () =>
+        Effect.catchTag("HostedHostnameResolutionFailed", (error) =>
           Effect.fail(
             new HostedOutboundRequestBlocked({
               url: value,
-              reason: "Hostname could not be resolved",
+              reason: error.empty
+                ? "Hostname did not resolve to an address"
+                : "Hostname could not be resolved",
             }),
           ),
         ),
       );
 
-      if (addresses.length === 0) {
-        return yield* new HostedOutboundRequestBlocked({
-          url: value,
-          reason: "Hostname did not resolve to an address",
-        });
-      }
-
       for (const { address } of addresses) {
         if (!isAllowedResolvedAddress(address)) {
           return yield* new HostedOutboundRequestBlocked({