๐ก๏ธ Sentinel: [HIGH] Fix empty hostname SSRF bypass - #1068
๐ก๏ธ Sentinel: [HIGH] Fix empty hostname SSRF bypass#1068seonghobae wants to merge 92 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
๐ WalkthroughWalkthrough๋ URL ๊ฒ์ฆ๊ธฐ๊ฐ ๋น ํธ์คํธ๋ช ์ DNS ํด์ ์ ์ ๊ฑฐ๋ถํฉ๋๋ค. DNS ํด์ ์คํจ๋ ๊ฑฐ๋ถํฉ๋๋ค. HIGH ๋ฑ๊ธ SSRF fail-open ํ์ง ๊ท์น๊ณผ ๋ณด์ ํฝ์ค์ฒ๋ฅผ ์ถ๊ฐํ์ต๋๋ค. ๊ท์น์ ๋ฐํ์๊ณผ ์์ธ ์ฒ๋ฆฌ ๊ฒฝ๋ก๋ฅผ ํ๊ท ํ ์คํธ๋ก ๊ฒ์ฆํฉ๋๋ค. Changes๋น ํธ์คํธ๋ช SSRF ์ฐจ๋จ
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: ๐ก Moderate ยท up to The runtime SSRF boundary now rejects empty and unresolved hosts, but the packaged detector can still miss documented vulnerable patterns and one regression test is DNS-dependent. The detector gap should be resolved before merge. ๐ฅ Pre-merge checks | โ 4 | โ 1โ Failed checks (1 warning)
โ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 67.24% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 58 functions across 10 files. (2 skipped: 2 unsupported.)
โจ Finishing Touches ๐ก 1๐ Generate docstrings ๐ก
๐งช Generate unit tests (beta)
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 |
2026-09-07 exact-head successor consolidationGenerated PR #1128 was re-fetched at exact head Verified complete carryover of every valid #1128 delta:
Therefore #1128 can be retired only as a verified complete-successor carryover, not as a claim that its finding was invalid. No commit or valid requirement is discarded. Current acceptance remains blocked: eight repository-local workflows are exact-head success, but CodeQL PR run 33996585942 has |
ํ์ธํ์ต๋๋ค. PR #1128 ์ ์ ํจํ ๋ณ๊ฒฝ์ฌํญ์ด ๋ณธ PR์ ๋ชจ๋ ๋ฐ์๋์ด ํตํฉ๋ ๊ฒ์ผ๋ก ์ดํดํ์ต๋๋ค. ํฅํ ๋ชจ๋ ์์ ์์ ์ฝ๋์ ๋ณํฉ ์ด๋ ฅ ๋ฐ ์ธ์ฆ๋ ํ์ (CodeQL)์ ๊ฒฐ๊ณผ๋ฅผ ์๊ฒฉํ๊ฒ ์ค์ํ๊ฒ ์ต๋๋ค. ์น์ธ์ด ์ด๋ฃจ์ด์ง ๋๊น์ง ๋๊ธฐํ๊ฒ ์ต๋๋ค. |
|
Re-fetched the concurrent exact head |
ํ์ธํ์ต๋๋ค. ๋ฐํ์/ํ์ง๊ธฐ/ํ ์คํธ ์ฐจ์ด๊ฐ ์ฑ๊ณต์ ์ผ๋ก ์ ์ง๋์์ผ๋ฉฐ ํ์ฌ ์์ค์ ์ด๋ ํ ํดํ๋ ๋ฐ์ํ์ง ์์ ์ํ์์ ์ธ์งํ์ต๋๋ค. CodeQL ๋ฐ ๋๋จธ์ง ๋ณด์ ๊ฒ์ฌ๋ค์ด ์๋ฃ๋๊ธฐ๋ฅผ ๋๊ธฐํ๊ณ ์๊ฒ ์ต๋๋ค. |
Summary: - Snapshot 19:03 UTC records Draft #1161 `a92e936` stacked on #1158. - Named secrets in MCP env/args/command fail closed. - Jules #1159 empty-host SSRF stays Draft under #1068. Rationale: - #999 is the single writer of the product-technical gap baseline. - #1099 remaining surfaces stay on stacked successors, not Close. Tests: - documentation-only; detector evidence lives on #1161 (1929/1929)
|
Canonical succession update from generated #1159. Fresh live authority before this mutation: protected I preserved the one valid stronger regression shape introduced while repairing #1159 by adding The new contract runs against both validators and proves #1159 now has no valid semantic production delta that is stronger than this owner lineage. Retire it only after verifying this exact successor head retains the new test plus the existing runtime/detector/fixture/TRACEABILITY corpus; do not transfer #1159 predecessor checks/reviews. New exact-head hosted checks/review remain authoritative. |
ํ์ธํ์ต๋๋ค. PR #1159์ ์ ํจํ ๋ณ๊ฒฝ์ฌํญ(ํ
์คํธ ๋ณด๊ฐ ๋ฑ)์ด ๋ชจ๋ ํ์ฌ HEAD( |
Summary: - Snapshot 20:04 UTC records Draft #1165 `5790559` stacked on #1164. - Setuid/setgid and world-writable executable/hook modes fail closed. - #1068 live head `2379b37` is another empty Strix retrigger. Rationale: - #999 is the single writer of the product-technical gap baseline. - #1099 remaining surfaces stay on stacked successors, not Close. Tests: - documentation-only; detector evidence lives on #1165 (2035/2035)
Security objective
Close the empty-host / unresolved-host SSRF validation weakness at its causal runtime boundary and preserve the defect class as executable AppGuardrail detection evidence.
Protected predecessor logic could normalize a missing hostname to
"", let it reach DNS resolution, ignoresocket.gaierror, and return success. This branch rejects missing hosts before DNS/IP validation and fails closed when a public destination cannot be established.Runtime + detector contract
appguardrail_core.controlplane._is_safe_urlis the causal webhook/storage/redirect boundary; CLI_is_safe_urlis defense in depth.python-ssrf-empty-host-fail-openkeeps bounded production_scan_fileevidence for direct/empty-string-normalized hosts, multiline definitions, annotated assignments, dominating vs conditional guards, nonempty fallbacks, tuple DNS exceptions, diagnostic fallthrough, falsy/raise termination, and reviewed truthy success forms.RED โ GREEN lineage
62df0db1a831985fc34dbdc3565cfa2688facc98โ deterministic redirect evidence5b79be8144f0444535ab12290851b7f8afe28538;38bde3b74ec0ed606e884b9620273f60285c641cโ438c5ae5e607fe03bdeece44e4ab30dd55e8b67b;cb89d786f31ecf47dd24cda590c485ede2717194โ8c06fe741a4c63d43495e536d06f0f2419a3109f.Intervening-delta repair and successor consolidation
Earlier verified descendant
3c015e2e76d9df7cb20e5577c310fc3f2567519dretained the reviewed detector grammar and the credential-only hostless fixture used to consolidate generated PR #1103.A later three-commit descendant ending at
c5874eff879ee988d4b650bc2a952d3dae9a04dawas re-read rather than treated as a race. Relative to3c015e2...it removedtests/test_ssrf_empty_host_credentials_contract.pyandtests/test_ssrf_empty_host_detector_edge_contract.py, narrowed the detector regex, and modified the general SSRF regression. The deleted contracts and narrowed detector were valid repair findings.Normal descendants restored the reviewed detector and both lost regression surfaces without force/rebase:
a31d91dbc43282655a5e8d9ca2550d64352ab7f7restores the reviewed detector grammar;5be43e8b45657b400866255dfe3d59539501ac19restores the hostless credential contract and additionally inherits generated PR ๐ก๏ธ Sentinel: [CRITICAL] ๋น ํธ์คํธ๋ช ์ ํตํ SSRF ๋ฐฉ์ด ์ฐํ ์ทจ์ฝ์ ์์ ย #1113's uniquehttp://user@/fixture;bce54900be6794a0281e944c6816420a34622b13restores the executable multiline/annotated/truthy-return detector edge contracts.Fresh compare
3c015e2...bce5490is ahead 6 / behind 0. Its only effective differences are the preserved deterministictests/test_ssrf_protection.pyrefinement and the expanded hostless-credential regression; the detector and deleted edge contract are restored to the reviewed semantics.Generated PR #1113's runtime guard is already present here for both control-plane and CLI. Its
http://,http://user@, andhttp://user@/behavioral evidence is now fully represented on this canonical lineage. Its generated.jules/sentinel.mdrepository-wide doctrine is not an independent product/security contract and is not required for succession.Exact authority
develop@e71d37e7c58118e6764c96ab7c4492fe33eed6f82379b37f05b12af8e22990965d42da2e69b9c611sentinel/fix-empty-hostname-ssrf-14253541902387870366Exact head
2379b37...is a source-neutral descendant and adds no product/security delta. All eight repository workflows are terminal success on this exact head. CodeQL PR run34155024470successfully dispatched exact-head Python and Actions analysis, then failed closed atVERDICT_STATE=pending; this is neither a source failure nor GREEN. No qualifying current-head approval exists. One outdated detector thread remains open until an independent current-head review confirms the bounded truthy-return repair. Predecessor evidence does not transfer.Merge boundary
Not merge-ready. Require unchanged exact-head terminal-success applicable checks, qualifying independent current-head review, and ordinary protected-branch acceptance. No self-approval, stale evidence, force update, detector waiver, source-neutral retrigger, administrator bypass, or required-check weakening.
Generated PR #1128 complete carryover
Fresh comparison at
4a76b955ecc6e767e137ac15e82b83a2af148386verified that this lane completely carries #1128's current hostless rejection andhttp:///http://user@regression obligations for both validators and the control-plane API. #1128's body-mentionedtests/test_hostless_url_admission.pyis absent from its current four-file patch; its generated doctrine is not an independent product/security delta. #1128 may therefore be retired only as verified complete successor carryover, while this Draft remains the single writer.The current head commit is a source-neutral workflow-retrigger descendant and adds no product/security delta. This checkpoint does not reuse the retrigger as proof and creates no further no-op commit; future transient workflow retries should use the GitHub rerun API.