security(scanner): detect stored SSRF webhook persistence - #910
Conversation
- /api/v1/webhook 엔드포인트에서 사용자로부터 전달받은 webhook url을 검증 없이 데이터베이스에 저장하는 취약점이 발견되었습니다. - appguardrail_core/controlplane.py에 존재하는 webhook 등록 로직(do_POST)에서 DB 저장(set_webhook) 전 _is_safe_url() 함수로 url 안전성을 검증하도록 수정했습니다. - 검증에 실패할 시 400 에러를 응답합니다. - 이에 대한 테스트 코드를 tests/test_controlplane.py에 추가했습니다.
|
👋 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. |
📝 WalkthroughWalkthrough저장형 SSRF 규칙과 SSRF 메타데이터 정규화를 추가했습니다. 직접 URL 저장과 변수 흐름을 탐지하고, 유효한 검증 경로는 제외하도록 회귀 테스트를 추가했습니다. Changes저장형 SSRF 탐지
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant _scan_file
participant python_stored_ssrf_webhook_url
participant build_rule_metadata
_scan_file->>python_stored_ssrf_webhook_url: Python 파일의 웹훅 URL 저장 흐름 전달
python_stored_ssrf_webhook_url-->>_scan_file: 저장형 SSRF 결과 반환
_scan_file->>build_rule_metadata: 결과 메타데이터 정규화 요청
build_rule_metadata-->>_scan_file: SSRF 분류와 수정 정보 반환
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headd157667dff37f6594f9a589850808fcbc10c1a15. -
Head SHA:
d157667dff37f6594f9a589850808fcbc10c1a15 -
Workflow run: 31300803457
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: test_controlplane.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_controlplane.py"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (5 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (5 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test (4 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (4 files)"]
R2 --> V2["targeted test run"]
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_controlplane_url_types.py (1)
10-13: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win웹훅 엔드포인트 수준의 회귀 테스트를 추가하세요.
현재 테스트는 private helper인
_is_safe_url만 호출합니다. 따라서/api/v1/webhook이 비문자열 URL에 HTTP 400을 반환하는지 확인하지 못합니다. 잘못된 본문이 데이터베이스의webhook_url을 변경하지 않는지도 확인하지 못합니다.
{"url": 123},{"url": true},{"url": {}}, 그리고 mapping이 아닌 JSON 본문을 엔드포인트에 전달하는 테스트를 추가하세요. 각 요청이 HTTP 400을 반환하고 기존 URL을 유지하는지 확인하세요.As per coding guidelines, 웹훅 페이로드 검증은 서버 측 API 경계에서 확인되어야 합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_controlplane_url_types.py` around lines 10 - 13, Extend the webhook endpoint tests beyond the private _is_safe_url helper by sending {"url": 123}, {"url": true}, {"url": {}}, and non-mapping JSON bodies to /api/v1/webhook. Assert each request returns HTTP 400 and verify the persisted webhook_url remains unchanged, ensuring validation occurs at the server API boundary.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@appguardrail_core/controlplane.py`:
- Around line 634-638: Validate the parsed body before accessing it in the
webhook update handler: accept only a mapping and return 400 for list or string
bodies. Validate webhook_url as None, an empty string, or a string accepted by
_is_safe_url; reject all other types, including numbers and booleans, with 400
before calling _is_safe_url or set_webhook.
---
Nitpick comments:
In `@tests/test_controlplane_url_types.py`:
- Around line 10-13: Extend the webhook endpoint tests beyond the private
_is_safe_url helper by sending {"url": 123}, {"url": true}, {"url": {}}, and
non-mapping JSON bodies to /api/v1/webhook. Assert each request returns HTTP 400
and verify the persisted webhook_url remains unchanged, ensuring validation
occurs at the server API boundary.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 52a3e023-f867-4a92-81d8-5c8097de8e02
📒 Files selected for processing (4)
.jules/sentinel.mdappguardrail_core/controlplane.pytests/test_controlplane.pytests/test_controlplane_url_types.py
…hook URL endpoint - /api/v1/webhook 엔드포인트에서 사용자로부터 전달받은 webhook url을 검증 없이 데이터베이스에 저장하는 취약점이 발견되어, 저장 전 _is_safe_url() 함수로 검증하도록 수정했습니다. - _is_safe_url() 내부에서 urllib.parse.urlparse() 사용 전 문자열 타입 확인 로직(isinstance(url, str))을 추가하여 정수형이나 불리언 등 잘못된 타입이 입력될 때 서버 에러(500) 대신 안전하게 차단되도록 수정했습니다. - 검증에 실패할 시 400 에러를 응답하며 관련 테스트 코드를 업데이트/추가했습니다.
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head5ba91311e673813c218922aab245adbd3c86360d. -
Head SHA:
5ba91311e673813c218922aab245adbd3c86360d -
Workflow run: 31315089382
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (5 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (5 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test (4 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (4 files)"]
R2 --> V2["targeted test run"]
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head5ba91311e673813c218922aab245adbd3c86360d. -
Head SHA:
5ba91311e673813c218922aab245adbd3c86360d -
Workflow run: 31316926796
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (5 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (5 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test (4 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (4 files)"]
R2 --> V2["targeted test run"]
|
@coderabbitai review Please complete a current-head security review for exact SHA |
Rate Limit Exceeded
|
seonghobae
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scanner/rules/ssrf.yml`:
- Line 4: Update the SSRF pattern-regex for set_webhook so conditional
validation is not treated as fail-closed: require rejection guards to cover
every execution path, including when the disabled condition is false. Ensure a
safe-url positive guard cannot suppress a later unprotected set_webhook sink
from the same assignment, and add regression cases for both conditional
rejection and positive-guard-then-unprotected-sink flows.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b070017-736b-4ddf-bc9d-72f72bc413ea
📒 Files selected for processing (4)
appguardrail_core/rules.pyscanner/rules/ssrf.ymltests/test_ssrf_rule_accessors.pytests/test_ssrf_rules.py
Avoid evaluating the bounded but expensive flow regex in Python files that cannot persist a webhook. Preserve exact detection semantics while reducing a 1 MiB sink-free scan from seconds to a linear literal check.
Do not let an unrelated conditional rejection or an earlier positive-guarded sink suppress a later unprotected webhook persistence sink. Add exact regression coverage and restack the five-file scanner scope onto current protected develop.
* feat(dashboard): 개선된 파일 업로드 UX를 위한 프록시 버튼 추가 Dashboard의 네이티브 파일 업로드 인풋을 숨기고 스타일 제어가 가능한 프록시 버튼으로 교체하여 UI 일관성을 향상시켰습니다. - 네이티브 `<input type="file">`은 `sr-only` 등을 통해 접근성을 유지한 채 시각적으로만 숨김 처리 - `.tag` 클래스를 활용한 새로운 업로드 프록시 `<button>` 추가 - 단일 페이지 애플리케이션(SPA)에서 동일 파일 연속 선택 시 브라우저가 `change` 이벤트를 무시하는 문제 해결 (JS에서 파일 캡처 후 `value` 초기화) - 관련 UX 학습 내용을 `.jules/palette.md`에 추가 - 변경 사항에 맞게 프론트엔드 UI 컨트랙트 테스트 업데이트 * test(dashboard): lock accessible upload proxy wiring --------- Co-authored-by: seonghobae <8172694+seonghobae@users.noreply.github.com>
Preserve the exact reviewed scanner scope while integrating protected develop after #925.
Merge protected develop into the canonical documentation branch, promote the separately verified webhook prevention and bounded scanner-detection controls, keep PR #911 labelled active, and update executable documentation contracts.
1 participant
Scope
Preserve the unique stored-SSRF static detection work after the runtime webhook boundary fix was integrated through #924.
This exact head intentionally contains only:
python-stored-ssrf-webhook-urlscanner rule;The already-integrated control-plane URL-validation changes and unrelated branch history are excluded from the PR diff.
Evidence
10138195f80a77d1662fe1c8c54e8188979e033e6c6a830383d182ae2e6051944ef61fe68ca8abf0198 passedcompileallandgit diff --check: passedHosted exact-head CI, security, coverage, and independent review evidence must complete before merge.