🛡️ Sentinel: [CRITICAL] Fix Server-Side Request Forgery (SSRF) in webhooks

.jules/sentinel.md

@@ -128,3 +128,8 @@
 **Vulnerability:** The backend CSV export for audit logs neutralized `=`, `+`, `-`, and `@` but failed to neutralize `|` (pipe) characters, allowing potential DDE (Dynamic Data Exchange) injection if exported logs were opened in spreadsheet software.
 **Learning:** Spreadsheet formula defenses must cover all command-style prefixes including `|` across all CSV export boundaries, both frontend and backend.
 **Prevention:** Update the sanitization regex in the backend export function to `/^[=+\-@|]/` so that all potentially executable spreadsheet payloads are prefixed with a single quote.
+
+## 2024-05-27 - Server-Side Request Forgery (SSRF) in webhooks
+**Vulnerability:** The POST `/api/orgs/:id/webhooks` handler allowed creating webhooks with internal/private IP addresses (e.g., `127.0.0.1`, `localhost`, `10.x.x.x`), leading to potential SSRF vulnerabilities when the server attempts to deliver events.
+**Learning:** Webhook URLs must be strictly validated not just for proper HTTP(S) formatting, but also to ensure they point to external, publicly routable hostnames/IPs to prevent the application server from attacking internal infrastructure.
+**Prevention:** Implement an `isSafeWebhookUrl` function using the `URL` constructor to normalize inputs and block common internal network patterns (loopback, local, RFC1918) before saving webhook configurations.

server/app.mjs

@@ -729,6 +729,31 @@ app.get('/api/metrics', (c) => {
 });
 
 // ------------------------------------------------------------------- webhooks
+
+function isSafeWebhookUrl(urlStr) {
+  let u;
+  try {
+    u = new URL(urlStr);
+  } catch {
+    return false;
+  }
+  const hn = u.hostname;
+  if (
+    hn === 'localhost' ||
+    hn === '[::1]' ||
+    /^127\.\d+\.\d+\.\d+$/.test(hn) ||
+    /^10\.\d+\.\d+\.\d+$/.test(hn) ||
+    /^192\.168\.\d+\.\d+$/.test(hn) ||
+    /^172\.(1[6-9]|2[0-9]|3[0-1])\.\d+\.\d+$/.test(hn) ||
+    /^169\.254\.\d+\.\d+$/.test(hn) ||
+    /^0\.\d+\.\d+\.\d+$/.test(hn) ||
+    hn === ''
+  ) {
+    return false;
+  }
+  return true;
+}
+
 app.get('/api/orgs/:id/webhooks', requireAuth, (c) => {
   const uid = c.get('user').sub;
   const orgId = c.req.param('id');
@@ -748,6 +773,7 @@ app.post('/api/orgs/:id/webhooks', requireAuth, async (c) => {
   if (!canManage(orgRole(uid, orgId))) return c.json({ error: 'forbidden' }, 403);
   const { url, events } = await c.req.json().catch(() => ({}));
   if (!/^https?:\/\//.test(String(url || ''))) return c.json({ error: 'valid http(s) url required' }, 400);
+  if (!isSafeWebhookUrl(url)) return c.json({ error: 'unsafe webhook url' }, 400);
   const secret = `whsec_${randomBytes(24).toString('base64url')}`;
   const evs = Array.isArray(events) ? events.join(',') : (events || '*');
   const id = rowid(db.prepare('INSERT INTO webhooks(org_id,url,secret,events) VALUES(?,?,?,?)').run(orgId, url, secret, evs));

tests/api/smoke.mjs

@@ -266,7 +266,7 @@ r = await req(`/api/orgs/${orgAId}/export`, { headers: oauth });
 assert.equal(r.status, 403, 'non-owner export → 403');
 
 // ---- Webhooks ----
-r = await req(`/api/orgs/${orgAId}/webhooks`, { method: 'POST', headers: auth, body: body({ url: 'http://127.0.0.1:9/hook', events: ['project.update'] }) });
+r = await req(`/api/orgs/${orgAId}/webhooks`, { method: 'POST', headers: auth, body: body({ url: 'http://example.com:9/hook', events: ['project.update'] }) });
 assert.equal(r.status, 200, 'create webhook');
 const wh = await r.json();
 assert.ok(wh.secret.startsWith('whsec_'), 'webhook secret returned once');