fix: address coderabbit reviews

server/src/routes/webhook.ts

@@ -123,7 +123,7 @@ router.post('/update', requireSignIn, async (req: Request, res: Response) => {
             return res.status(400).json({ ok: false, error: 'Webhook configuration, webhook ID, and robot ID are required' });
         }
 
-        if (webhook.url) {
+        if ('url' in webhook) {
             const urlCheck = await validateWebhookUrl(webhook.url);
             if (!urlCheck.safe) {
                 return res.status(400).json({ ok: false, error: urlCheck.reason || 'Invalid webhook URL' });

server/src/utils/webhook-url.ts

@@ -1,4 +1,7 @@
-import * as dns from 'dns/promises';
+import * as dns from 'dns';
+import * as dnsPromises from 'dns/promises';
+import * as http from 'http';
+import * as https from 'https';
 import * as net from 'net';
 
 export interface WebhookUrlCheck {
@@ -6,6 +9,8 @@ export interface WebhookUrlCheck {
   reason?: string;
 }
 
+const allowPrivateTargets = (): boolean => process.env.ALLOW_PRIVATE_WEBHOOK_URLS === 'true';
+
 /**
  * Blocks addresses that a webhook has no legitimate reason to reach:
  * loopback, private ranges, link-local (including cloud metadata at
@@ -27,18 +32,18 @@ function isPrivateIp(ip: string): boolean {
 
   if (net.isIPv6(ip)) {
     const lower = ip.toLowerCase();
+
     if (lower.startsWith('::ffff:')) {
       const mapped = lower.slice('::ffff:'.length);
       if (net.isIPv4(mapped)) return isPrivateIp(mapped);
       return true;
     }
-    return (
-      lower === '::' ||
-      lower === '::1' ||
-      lower.startsWith('fc') ||
-      lower.startsWith('fd') ||
-      lower.startsWith('fe80')
-    );
+
+    const firstHextet = Number.parseInt(lower.split(':', 1)[0] || '0', 16);
+    const isLinkLocal = (firstHextet & 0xffc0) === 0xfe80;
+    const isUniqueLocal = (firstHextet & 0xfe00) === 0xfc00;
+
+    return lower === '::' || lower === '::1' || isLinkLocal || isUniqueLocal;
   }
 
   return false;
@@ -46,10 +51,15 @@ function isPrivateIp(ip: string): boolean {
 
 /**
  * Validates that a webhook destination is safe to call. Resolves the hostname
- * so that a public name pointing at an internal address is rejected too.
+ * so that a public name pointing at an internal address is rejected too. This
+ * is a fast rejection for the API surface; the authoritative check happens at
+ * connection time in safeLookup.
  */
-export async function validateWebhookUrl(urlString: string): Promise<WebhookUrlCheck> {
-  const allowPrivateTargets = process.env.ALLOW_PRIVATE_WEBHOOK_URLS === 'true';
+export async function validateWebhookUrl(urlString: unknown): Promise<WebhookUrlCheck> {
+  if (typeof urlString !== 'string' || !urlString.trim()) {
+    return { safe: false, reason: 'Webhook URL is required' };
+  }
+
   let parsed: URL;
 
   try {
@@ -62,7 +72,7 @@ export async function validateWebhookUrl(urlString: string): Promise<WebhookUrlC
     return { safe: false, reason: 'Only http and https webhook URLs are allowed' };
   }
 
-  if (allowPrivateTargets) {
+  if (allowPrivateTargets()) {
     return { safe: true };
   }
 
@@ -73,7 +83,7 @@ export async function validateWebhookUrl(urlString: string): Promise<WebhookUrlC
     if (net.isIP(hostname)) {
       addresses = [hostname];
     } else {
-      const results = await dns.lookup(hostname, { all: true });
+      const results = await dnsPromises.lookup(hostname, { all: true });
       addresses = results.map((result) => result.address);
     }
   } catch {
@@ -96,10 +106,61 @@ export async function validateWebhookUrl(urlString: string): Promise<WebhookUrlC
   return { safe: true };
 }
 
+/**
+ * Resolution used by the outbound webhook agents. Validating here rather than
+ * only at request time binds the check to the address the socket actually
+ * connects to, which closes the DNS rebinding window left open by validating
+ * once up front. The hostname is still passed through for TLS and SNI.
+ */
+const safeLookup = (
+  hostname: string,
+  options: dns.LookupAllOptions,
+  callback: (err: NodeJS.ErrnoException | null, addresses: any, family?: number) => void
+): void => {
+  dns.lookup(hostname, { ...options, all: true }, (err, addresses) => {
+    if (err) {
+      callback(err, '', 0);
+      return;
+    }
+
+    const resolved = Array.isArray(addresses) ? addresses : [addresses];
+
+    if (!allowPrivateTargets()) {
+      const blocked = resolved.find((entry) => isPrivateIp(entry.address));
+      if (blocked) {
+        callback(
+          new Error(`Refusing to connect to private or reserved address (${blocked.address})`),
+          '',
+          0
+        );
+        return;
+      }
+    }
+
+    if (resolved.length === 0) {
+      callback(new Error(`Could not resolve ${hostname}`), '', 0);
+      return;
+    }
+
+    if (options && options.all) {
+      callback(null, resolved);
+      return;
+    }
+
+    callback(null, resolved[0].address, resolved[0].family);
+  });
+};
+
+const webhookHttpAgent = new http.Agent({ lookup: safeLookup } as http.AgentOptions);
+const webhookHttpsAgent = new https.Agent({ lookup: safeLookup } as https.AgentOptions);
+
 /**
  * Shared axios options for every outbound webhook call. Redirects are refused
- * so that a validated public URL cannot forward the request to an internal one.
+ * so that a validated public URL cannot forward the request to an internal one,
+ * and the agents re-validate the address at connection time.
  */
 export const WEBHOOK_REQUEST_OPTIONS = {
   maxRedirects: 0,
+  httpAgent: webhookHttpAgent,
+  httpsAgent: webhookHttpsAgent,
 };