diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 17f338fe..ef4b6c47 100644 --- a/.jules/sentinel.md +++ b/.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. diff --git a/server/app.mjs b/server/app.mjs index c432a84f..ebea4cd5 100644 --- a/server/app.mjs +++ b/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)); diff --git a/tests/api/smoke.mjs b/tests/api/smoke.mjs index e536b908..69d3d87d 100644 --- a/tests/api/smoke.mjs +++ b/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');