๐ก๏ธ Sentinel: [CRITICAL] Fix Server-Side Request Forgery (SSRF) in webhooks - #627
๐ก๏ธ Sentinel: [CRITICAL] Fix Server-Side Request Forgery (SSRF) in webhooks#627seonghobae wants to merge 2 commits into
Conversation
|
๐ Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a ๐ emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reachedNext included review available in 10 minutes. View limit detailsLimit details: Youโve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: โ๏ธ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ๐ Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| // ---- 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'] }) }); |
There was a problem hiding this comment.
๐ด Webhook delivery test breaks on switch to remote host
The webhook URL was changed from http://127.0.0.1:9/hook to http://example.com:9/hook. The local address failed instantly with connection-refused, so both delivery attempts recorded within the 900ms wait; the external host silently drops port 9, so sendWebhook hangs until its 3000ms abort. At 900ms no delivery is recorded, so the dels.length >= 2 and attempt === 2 assertions fail.
Prompt for agents
The webhook delivery test at tests/api/smoke.mjs was changed to use http://example.com:9/hook to satisfy the new isSafeWebhookUrl SSRF check, which now rejects 127.0.0.1. The problem: the downstream assertions (lines 290-296) rely on both delivery attempts failing fast and being recorded within a 900ms wait. Against 127.0.0.1:9 the connection was refused immediately, but example.com:9 is an external host that silently drops the connection, so sendWebhook (server/app.mjs:102) will hang until its 3000ms AbortController timeout on each attempt. With attempt 1 recording at ~3s and attempt 2 at ~6.5s, the 900ms wait elapses before any delivery row exists, so dels.length >= 2 and attempt === 2 fail; the test also now depends on real outbound network/DNS. Consider using a URL that passes isSafeWebhookUrl (public/external-looking) but still yields a fast, deterministic connection failure โ e.g. a hostname that resolves but with a port that produces an immediate refusal, or a domain guaranteed not to resolve โ or increase the wait to exceed the retry timeouts. Ensure the fix keeps the test deterministic and offline-safe.
Was this helpful? React with ๐ or ๐ to provide feedback.
| 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; | ||
| } |
There was a problem hiding this comment.
๐จ Webhook SSRF filter bypassable via IP encodings and DNS
isSafeWebhookUrl rejects only dotted-decimal private ranges, localhost, and [::1], matching the raw hostname string rather than the resolved address. Integer/hex IPs (http://2130706433/), non-[::1] IPv6 loopback and IPv4-mapped forms, and any public hostname whose DNS resolves to an internal or 169.254.169.254 metadata address all pass, and sendWebhook then fetches them server-side.
Was this helpful? React with ๐ or ๐ to provide feedback.
|
Superseded by #588. That PR is the single SSRF remediation path and covers encoded IPs, IPv4/IPv6 special-use ranges, DNS rebinding, pinned connections, and the deterministic delivery tests identified in this PR. |
1 participant
๐จ ์ฌ๊ฐ๋
CRITICAL
๐ก ์ทจ์ฝ์
์นํ ์์ฑ API (
POST /api/orgs/:id/webhooks)์์ ์ ๋ ฅ๋ URL์ด ๋ด๋ถ ๋คํธ์ํฌ(์:127.0.0.1,localhost,10.x.x.x๋ฑ)๋ฅผ ๊ฐ๋ฆฌํค๋์ง ๊ฒ์ฆํ์ง ์์ SSRF(Server-Side Request Forgery) ์ทจ์ฝ์ ์ด ์กด์ฌํ์ต๋๋ค.๐ฏ ์ํฅ
์ ์์ ์ธ ์ฌ์ฉ์๊ฐ ๋ด๋ถ ๋คํธ์ํฌ์ ์๋น์ค(์: ํด๋ผ์ฐ๋ ๋ฉํ๋ฐ์ดํฐ API, ๋ก์ปฌ ํธ์คํธ์ ๋ฐ์ธ๋ฉ๋ ๊ด๋ฆฌ์ ๋๊ตฌ ๋ฑ)์ HTTP ์์ฒญ์ ๋ณด๋ด ์ ๋ณด๋ฅผ ํ์ทจํ๊ฑฐ๋ ์ธ๊ฐ๋์ง ์์ ์์ ์ ์ํํ ์ ์์ต๋๋ค.
๐ง ์์ ์ฌํญ
isSafeWebhookUrlํจ์๋ฅผ ์ถ๊ฐํ์ฌ URL์ ํ์ฑํ๊ณ ,URL๊ฐ์ฒด๊ฐ ์ ๊ทํํhostname์ ๊ฒ์ฌํ์ฌ ๋ก์ปฌํธ์คํธ, IPv4 ์ฌ์ค ๋์ญ, IPv6 ๋ฃจํ๋ฐฑ ์ฃผ์([::1]) ๋ฑ ๋ด๋ถ๋ง IP๋ฅผ ๊ฑฐ๋ถํ๋๋ก ๋ณ๊ฒฝํ์ต๋๋ค.isSafeWebhookUrl์ ํต๊ณผํ๋์ง ํจ๊ป ๊ฒ์ฌํฉ๋๋ค.โ ํ์ธ ๋ฐฉ๋ฒ
npm run test:api๋ฅผ ์คํํ์ฌ ์ฐ๊ด๋ ํ ์คํธ๊ฐ ํต๊ณผํ๋์ง ํ์ธํฉ๋๋ค. (ํ ์คํธ ๋ด์ ๋ก์ปฌํธ์คํธ ์นํ ์example.com์ผ๋ก ์์ ํ์ฌ ๋ฐ์ํ์ต๋๋ค.)http://127.0.0.1/์ ์๋ํ์ฌ 400 Bad Request๊ฐ ๋ฐํ๋๋์ง ํ์ธํฉ๋๋ค.PR created automatically by Jules for task 3209121968036812837 started by @seonghobae