๐ก๏ธ Sentinel: [MEDIUM] ์นํ URL ์์ฑ ์ SSRF ์ทจ์ฝ์ ์์ - #612
๐ก๏ธ Sentinel: [MEDIUM] ์นํ
URL ์์ฑ ์ SSRF ์ทจ์ฝ์ ์์ #612seonghobae wants to merge 1 commit 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 58 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/hook', events: ['project.update'] }) }); |
There was a problem hiding this comment.
๐ก Webhook test now depends on external network
The webhook test posts to http://example.com/hook instead of a port that refuses instantly, so the failure-and-retry assertions now depend on an outside host. If outbound traffic is dropped, the fetch stalls until the 3s abort, past the 900ms wait, and the attempt-2 retry is never recorded, failing the test. If the host answers 2xx, no retry is scheduled and the assertions fail too.
Prompt for agents
The webhook delivery assertions at tests/api/smoke.mjs:289-296 require the delivery to fail (ok===0) and be retried to attempt 2 within a 900ms wait. Previously the test used http://127.0.0.1:9/hook, which produces an instant, deterministic connection refusal on the loopback interface. The SSRF fix in POST /api/orgs/:id/webhooks now blocks 127.0.0.1, so the test was switched to http://example.com/hook. This makes the failure/retry assertions depend on outbound internet reachability, on example.com returning a non-2xx status, and on the response arriving fast enough that the retry (scheduled 500ms after the first failure) lands inside the 900ms window. In a sandboxed CI environment that drops outbound packets, the fetch will hang until the 3s AbortController timeout in sendWebhook, so the first delivery is not recorded until ~3s and no attempt-2 row exists at 900ms. Consider using a URL that is guaranteed to fail fast yet is not on the SSRF blocklist โ e.g. a reserved TEST-NET address like http://192.0.2.1:9/hook (RFC 5737, unroutable, fast to fail) or a .invalid domain like http://nonexistent.invalid/hook that fails DNS instantly โ so the test stays deterministic and offline-safe.
Was this helpful? React with ๐ or ๐ to provide feedback.
| try { | ||
| const parsedUrl = new URL(url); | ||
| const hostname = parsedUrl.hostname.toLowerCase(); | ||
| if (['localhost', '127.0.0.1', '::1', '169.254.169.254', '0.0.0.0'].includes(hostname)) { | ||
| return c.json({ error: 'internal webhook destinations are not allowed' }, 400); | ||
| } | ||
| } catch { | ||
| return c.json({ error: 'valid http(s) url required' }, 400); | ||
| } |
There was a problem hiding this comment.
๐จ Webhook SSRF denylist is incomplete and bypassable
The check rejects only five literal hostnames. It misses private ranges (10/8, 172.16/12, 192.168/16), the rest of loopback and link-local, IPv6 internal ranges, and alternate IP encodings (decimal, octal, hex). Because only the submitted hostname is checked, a public DNS name resolving to an internal IP, or a public host that redirects inward, still reaches internal targetsโfetch in sendWebhook follows redirects unvalidated (server/app.mjs:106).
Was this helpful? React with ๐ or ๐ to provide feedback.
|
Closing path: this PR is not a safe SSRF repair and is superseded by the existing #551 owner lane, PR #588 ( Fresh exact-head review of #612 at
#588 already owns the causal boundary with public-HTTPS registration, IPv4/IPv6 special-use rejection, A/AAAA validation, per-attempt DNS re-resolution, validated-address socket pinning, TLS hostname preservation, redirect refusal, bounded transport behavior, legacy-row handling, and offline-realistic regressions. Its exact source includes a dedicated There is no unique buyer/security behavior in #612 worth preserving: |
Understood. Acknowledging that this work is now obsolete and superseded by PR #588, and stopping work on this task. |
1 participant
๐จ Severity: MEDIUM
๐ก Vulnerability: The application allows users to configure a webhook URL via
/api/orgs/:id/webhooks. Previously, it did not check if the provided URL pointed to internal IP addresses or cloud metadata services. SincesendWebhookblindly issues afetchrequest to that URL, an attacker could use this endpoint to perform a Server-Side Request Forgery (SSRF) attack.๐ฏ Impact: An attacker could probe internal networks, access cloud metadata services (e.g.,
169.254.169.254), or interact with internal APIs running on the server or localhost.๐ง Fix: Added validation in the
POST /api/orgs/:id/webhooksendpoint to parse the provided URL and explicitly rejectlocalhost,127.0.0.1,::1,169.254.169.254, and0.0.0.0. Updated the smoke tests to useexample.cominstead of127.0.0.1to accommodate this new restriction.โ Verification: Run
npm run test:apito verify the tests still pass and the smoke test completes successfully. Attempting to addhttp://localhost/hookvia the API should now return a 400 Bad Request.PR created automatically by Jules for task 9438641368313520909 started by @seonghobae