Skip to content

fix(orb): revalidate relay DNS before forwarding #1395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JSONbored wants to merge 4 commits into
base: main
Choose a base branch
from
+70 −8
Open

fix(orb): revalidate relay DNS before forwarding #1395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
627c609
fix(orb): revalidate relay DNS before forwarding
JSONbored Jun 25, 2026
33e2528
fix(orb): thread resolveHostname through retryFailedRelays to forward…
JSONbored Jun 26, 2026
e24bc91
Merge branch 'main' into codex/fix-dns-based-blind-ssrf-vulnerability
JSONbored Jun 26, 2026
394aad9
Merge branch 'main' into codex/fix-dns-based-blind-ssrf-vulnerability
JSONbored Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion src/orb/relay.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,36 @@ import { hashToken } from "../auth/security";
import { isSafeHttpUrl } from "../review/content-lane/safe-url";
import { decryptSecret, encryptSecret } from "../utils/crypto";

type RelayDnsResolver = (hostname: string) => Promise<string[]>;

function addressAsUrlHost(address: string): string {
return address.includes(":") ? `[${address.replace(/^\[|\]$/g, "")}]` : address;
}

function resolvedAddressIsSafe(address: string): boolean {
return isSafeHttpUrl(`https://${addressAsUrlHost(address)}/`);
}

async function resolveRelayHostname(hostname: string): Promise<string[]> {
if (/^\d+\.\d+\.\d+\.\d+$/.test(hostname) || hostname.includes(":")) return [hostname];
const answers: string[] = [];
for (const type of ["A", "AAAA"]) {
const res = await fetch(`https://cloudflare-dns.com/dns-query?name=${encodeURIComponent(hostname)}&type=${type}`, { headers: { accept: "application/dns-json" }, signal: AbortSignal.timeout(3_000) });
if (!res.ok) throw new Error("dns_resolution_failed");

@superagent-security superagent-security Bot Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Relay hostname validation leaks internal hostnames to external DNS resolver

Every relay target hostname is sent to Cloudflare's public DoH resolver, leaking internal infrastructure names to a third party.

Use a configurable or local DNS resolver instead of hardcoding cloudflare-dns.com.

AI prompt 
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.

<file name="src/orb/relay.ts">
<violation number="1" location="src/orb/relay.ts:26">
<priority>P2</priority>
<title>Relay hostname validation leaks internal hostnames to external DNS resolver</title>
<evidence>The resolveRelayHostname function sends every relay target hostname to https://cloudflare-dns.com/dns-query via DNS-over-HTTPS. This exposes potentially private or internal infrastructure hostnames to a third-party service (Cloudflare), creating an information-disclosure risk for enterprise webhook endpoints.</evidence>
<recommendation>Replace the hardcoded public DoH endpoint with a configurable or local DNS resolver so internal hostnames are not leaked to external services, or add an allow-list and documentation noting the Cloudflare query behavior.</recommendation>
</violation>
</file>

const json = (await res.json()) as { Answer?: { data?: string; type?: number }[] };
for (const answer of json.Answer ?? []) {
if ((answer.type === 1 || answer.type === 28) && answer.data) answers.push(answer.data);
}
}
return answers;
}

async function relayDestinationIsSafe(raw: string, resolveHostname: RelayDnsResolver): Promise<boolean> {
if (!isSafeHttpUrl(raw)) return false;
const addresses = await resolveHostname(new URL(raw).hostname.toLowerCase());
return addresses.length > 0 && addresses.every(resolvedAddressIsSafe);
}

// The events a brokered container needs to review/act on. Installation-lifecycle + other Orb-internal events are
// deliberately NOT forwarded (the container runs under the CENTRAL Orb App, not its own, so it must not treat
// those as its own installation state).
Expand Down Expand Up @@ -104,7 +134,7 @@ export async function storeRelayFailure(
* Each row gets up to RELAY_RETRY_MAX_ATTEMPTS (5) retries within a 1-hour TTL; on success or expiry the row
* is removed. Never throws — a bad DB row or a persistently-down container is dropped (with an alertable log,
* below) after exhaustion. */
export async function retryFailedRelays(env: Env, opts?: { fetchImpl?: typeof fetch }): Promise<void> {
export async function retryFailedRelays(env: Env, opts?: { fetchImpl?: typeof fetch; resolveHostname?: RelayDnsResolver }): Promise<void> {
// Prune rows whose TTL has elapsed or whose attempt budget is exhausted.
const pruned = await env.DB
.prepare("DELETE FROM orb_relay_failures WHERE expires_at < datetime('now') OR attempts >= ?")
Expand All @@ -129,6 +159,7 @@ export async function retryFailedRelays(env: Env, opts?: { fetchImpl?: typeof fe
env,
{ eventName: row.event_name, installationId: row.installation_id, deliveryId: row.delivery_id, rawBody: row.raw_body },
opts?.fetchImpl,
opts?.resolveHostname,
);
if (result === "forwarded" || result === "skipped") {
await env.DB.prepare("DELETE FROM orb_relay_failures WHERE delivery_id = ?").bind(row.delivery_id).run();
Expand All @@ -154,6 +185,7 @@ export async function forwardOrbEvent(
env: Env,
args: { eventName: string; installationId: number | null | undefined; deliveryId: string; rawBody: string },
fetchImpl: typeof fetch = fetch,
resolveHostname: RelayDnsResolver = resolveRelayHostname,
): Promise<"forwarded" | "skipped" | "failed"> {
if (!args.installationId || !RELAY_FORWARD_EVENTS.has(args.eventName)) return "skipped";
const row = await env.DB
Expand All @@ -162,6 +194,7 @@ export async function forwardOrbEvent(
.first<{ relay_url: string; relay_secret_enc: string; relay_secret_iv: string; relay_secret_salt: string | null }>();
if (!row || !env.TOKEN_ENCRYPTION_SECRET) return "skipped";
try {
if (!(await relayDestinationIsSafe(row.relay_url, resolveHostname))) return "failed";

@superagent-security superagent-security Bot Jun 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: DNS revalidation uses DoH but fetch still resolves hostname, leaving TOCTOU rebinding risk

DoH IP check does not prevent rebinding because fetchImpl re-resolves the hostname at connection time.

Connect to the validated resolved IP directly, preserving Host header for TLS, instead of passing the hostname to fetch.

AI prompt 
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.

<file name="src/orb/relay.ts">
<violation number="1" location="src/orb/relay.ts:184">
<priority>P1</priority>
<title>DNS revalidation uses DoH but fetch still resolves hostname, leaving TOCTOU rebinding risk</title>
<evidence>The relayDestinationIsSafe call resolves the hostname via Cloudflare DoH and checks returned IPs, but the subsequent fetchImpl(row.relay_url, ...) still passes the original hostname string, causing the platform's DNS resolver to re-resolve it at connection time. An attacker controlling a relay hostname can serve a safe public IP to Cloudflare's DoH endpoint while returning a private/loopback IP to the Cloudflare Worker runtime resolver, bypassing the safety check.</evidence>
<recommendation>After DNS validation, connect to the resolved IP directly (e.g., reconstruct the request URL using the validated IP and original port/path, and set the Host header to the original hostname for TLS SNI/certificate verification), or use a fetch implementation that enforces the pre-resolved IP.</recommendation>
</violation>
</file>

const secret = await decryptSecret(row.relay_secret_enc, row.relay_secret_iv, env.TOKEN_ENCRYPTION_SECRET, row.relay_secret_salt);
const signature = await relaySignature(secret, args.rawBody);
const res = await fetchImpl(row.relay_url, {
Expand Down
43 changes: 36 additions & 7 deletions test/integration/orb-relay.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { describe, expect, it } from "vitest";
import { afterEach, describe, expect, it, vi } from "vitest";
import { createApp } from "../../src/api/routes";
import { issueOrbEnrollment } from "../../src/orb/broker";
import { forwardOrbEvent, registerOrbRelay, relaySignature, relayVerify, retryFailedRelays, storeRelayFailure } from "../../src/orb/relay";
import { createTestEnv, type TestD1Database } from "../helpers/d1";

afterEach(() => vi.unstubAllGlobals());

const db = (e: Env) => e.DB as unknown as TestD1Database;
const seedInstall = (e: Env, id: number, cols: Record<string, string | number | null> = {}) => {
const all: Record<string, string | number | null> = { installation_id: id, registered: 1, ...cols };
Expand Down Expand Up @@ -103,6 +105,7 @@ describe("relaySignature", () => {
});

describe("forwardOrbEvent", () => {
const publicDns = async () => ["203.0.113.10"];
const capture = (resp: Response) => {
const calls: { url: string; init?: RequestInit | undefined }[] = [];
const fetchImpl = ((u: RequestInfo | URL, init?: RequestInit) => {
Expand All @@ -127,7 +130,7 @@ describe("forwardOrbEvent", () => {
await registerOrbRelay(e, secret, "https://c.example/v1/orb/relay");
const { fetchImpl, calls } = capture(new Response("ok"));
const body = '{"action":"opened","number":7}';
expect(await forwardOrbEvent(e, { eventName: "pull_request", installationId: 800, deliveryId: "del-1", rawBody: body }, fetchImpl)).toBe("forwarded");
expect(await forwardOrbEvent(e, { eventName: "pull_request", installationId: 800, deliveryId: "del-1", rawBody: body }, fetchImpl, publicDns)).toBe("forwarded");
expect(calls[0]?.url).toBe("https://c.example/v1/orb/relay");
const h = calls[0]?.init?.headers as Record<string, string>;
expect(h["x-github-event"]).toBe("pull_request");
Expand All @@ -136,12 +139,38 @@ describe("forwardOrbEvent", () => {
expect(calls[0]?.init?.body).toBe(body);
});

it("revalidates the relay DNS destination at forward time before posting", async () => {
const e = brokeredEnv();
const secret = await enroll(e, 805);
await registerOrbRelay(e, secret, "https://dns-checked.example/v1/orb/relay");
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
const url = new URL(String(input));
const type = url.searchParams.get("type");
return Response.json({ Answer: type === "A" ? [{ type: 1, data: "203.0.113.20" }] : [{ type: 28, data: "2001:db8::20" }] });
});
const { fetchImpl, calls } = capture(new Response("ok"));
expect(await forwardOrbEvent(e, { eventName: "pull_request", installationId: 805, deliveryId: "d", rawBody: "{}" }, fetchImpl)).toBe("forwarded");
expect(calls[0]?.url).toBe("https://dns-checked.example/v1/orb/relay");
});

it("returns FAILED (never throws) on a non-ok response or a thrown fetch — the Orb 202 always stands", async () => {
const e = brokeredEnv();
const secret = await enroll(e, 802);
await registerOrbRelay(e, secret, "https://c.example/v1/orb/relay");
expect(await forwardOrbEvent(e, { eventName: "pull_request", installationId: 802, deliveryId: "d", rawBody: "{}" }, (() => Promise.resolve(new Response("no", { status: 503 }))) as typeof fetch)).toBe("failed");
expect(await forwardOrbEvent(e, { eventName: "pull_request", installationId: 802, deliveryId: "d", rawBody: "{}" }, (() => Promise.reject(new Error("down"))) as typeof fetch)).toBe("failed");
expect(await forwardOrbEvent(e, { eventName: "pull_request", installationId: 802, deliveryId: "d", rawBody: "{}" }, (() => Promise.resolve(new Response("no", { status: 503 }))) as typeof fetch, publicDns)).toBe("failed");
expect(await forwardOrbEvent(e, { eventName: "pull_request", installationId: 802, deliveryId: "d", rawBody: "{}" }, (() => Promise.reject(new Error("down"))) as typeof fetch, publicDns)).toBe("failed");
});

it("rejects a registered relay destination that resolves to loopback/private or has no DNS answers", async () => {
const e = brokeredEnv();
const secret = await enroll(e, 804);
await registerOrbRelay(e, secret, "https://rebinding.example/v1/orb/relay");
const { fetchImpl, calls } = capture(new Response("ok"));
const args = { eventName: "pull_request", installationId: 804, deliveryId: "d", rawBody: "{}" };
expect(await forwardOrbEvent(e, args, fetchImpl, async () => ["127.0.0.1"])).toBe("failed");
expect(await forwardOrbEvent(e, args, fetchImpl, async () => ["10.0.0.5"])).toBe("failed");
expect(await forwardOrbEvent(e, args, fetchImpl, async () => [])).toBe("failed");
expect(calls).toHaveLength(0);
});

it("SKIPS when the server's encryption secret is gone (can't decrypt the stored secret)", async () => {
Expand Down Expand Up @@ -225,7 +254,7 @@ describe("retryFailedRelays", () => {
await registerOrbRelay(e, secret, "https://c.example/v1/orb/relay");
await storeRelayFailure(e, { deliveryId: "retry-ok", eventName: "pull_request", installationId: 9100, rawBody: "{}" });
const fetchOk = (() => Promise.resolve(new Response("ok", { status: 200 }))) as typeof fetch;
await retryFailedRelays(e, { fetchImpl: fetchOk });
await retryFailedRelays(e, { fetchImpl: fetchOk, resolveHostname: async () => ["203.0.113.10"] });
const row = await db(e).prepare("SELECT delivery_id FROM orb_relay_failures WHERE delivery_id='retry-ok'").first();
expect(row ?? null).toBeNull(); // row removed on success
});
Expand All @@ -236,7 +265,7 @@ describe("retryFailedRelays", () => {
await registerOrbRelay(e, secret, "https://c.example/v1/orb/relay");
await storeRelayFailure(e, { deliveryId: "retry-fail", eventName: "pull_request", installationId: 9101, rawBody: "{}" });
const fetchFail = (() => Promise.resolve(new Response("bad", { status: 503 }))) as typeof fetch;
await retryFailedRelays(e, { fetchImpl: fetchFail });
await retryFailedRelays(e, { fetchImpl: fetchFail, resolveHostname: async () => ["203.0.113.10"] });
const row = await db(e).prepare("SELECT attempts FROM orb_relay_failures WHERE delivery_id='retry-fail'").first<{ attempts: number }>();
expect(row?.attempts).toBe(1);
});
Expand Down Expand Up @@ -271,7 +300,7 @@ describe("retryFailedRelays", () => {
return new Response("bad", { status: 503 });
}) as typeof fetch;

await retryFailedRelays(e, { fetchImpl: fetchFail });
await retryFailedRelays(e, { fetchImpl: fetchFail, resolveHostname: async () => ["203.0.113.10"] });

expect(calls).toBe(25);
expect(maxActive).toBeLessThanOrEqual(5);
Expand Down
Loading