fix(scanner): preserve path contracts and secure control-plane redirects - #885
fix(scanner): preserve path contracts and secure control-plane redirects#885seonghobae wants to merge 15 commits into
Conversation
- Hoist `base_path` resolution out of `_scan_file` loop in `cmd_scan` to prevent massive `os.stat` overhead on thousands of scanned files.
- Replace slow `isinstance(..., Path)` checks with `type(...) is not str` in path handling hot paths.
- Avoid string allocations from `replace("\\", "/")` by using `max(rfind("/"), rfind("\\"))` for basename extraction.
- Record optimizations in `.jules/bolt.md`.
- Hoist \`base_path\` resolution out of \`_scan_file\` loop in \`cmd_scan\` to prevent massive \`os.stat\` overhead on thousands of scanned files.
- Replace slow \`isinstance(..., Path)\` checks with \`type(...) is not str\` in path handling hot paths.
- Avoid string allocations from \`replace("\\\\", "/")\` by using \`max(rfind("/"), rfind("\\\\"))\` for basename extraction.
- Record optimizations in \`.jules/bolt.md\`.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough스캔 경로 컨텍스트와 문자열 경로 처리를 최적화했습니다. 제어 평면 업로드와 리디렉션의 HTTPS 및 인증 헤더 정책을 강화했습니다. 인증 지연 탐지를 주석으로 제한하고 회귀 테스트를 추가했습니다. Changes스캔 경로 계산 최적화
제어 평면 및 리디렉션 보안
인증 지연 주석 규칙
Estimated code review effort: 4 (복잡) | ~45분 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scanner/cli/appguardrail.py (1)
1690-1714: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDNS Rebinding SSRF In Bearer-token Delivery (CWE-918): Server-Side Request Forgery (SSRF)
Reachability: External · Exploitability: Difficult
Reachability path
● Entry tests/test_appguardrail.py │ ▼ ● Sink scanner/cli/appguardrail.py검사한 DNS 주소를 실제 연결에 고정하십시오.
_is_safe_url()은 검사 시점에 DNS를 해석하지만,opener.open()은 호스트명을 다시 해석합니다. DNS 재바인딩이 발생하면 공개 IP 검사 후 사설 HTTPS 주소로 bearer token이 전송될 수 있습니다. 검증한 IP를 실제 연결에 고정하거나 연결된 peer IP를 재검증하고,SafeRedirectHandler의 리디렉션에도 동일한 검사를 적용하십시오.🤖 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 `@scanner/cli/appguardrail.py` around lines 1690 - 1714, Update _push_findings and _is_secure_control_plane_url so the DNS-resolved public IP validated by _is_safe_url is pinned or revalidated for the actual opener.open connection, preventing DNS rebinding before bearer-token transmission. Ensure SafeRedirectHandler applies the same public-IP validation to every redirected destination, while preserving the existing HTTPS and public-URL requirements.
🤖 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 @.jules/bolt.md:
- Around line 18-19: Restore the newline literals in the examples at the
referenced sections so they appear as single-line escaped strings, using '\n' or
"\n" rather than embedding an actual line break inside the literal. Preserve the
surrounding .read(), .finditer(content), and line-number recovery guidance
unchanged.
In `@appguardrail_core/controlplane.py`:
- Around line 291-310: Update the redirect handling around has_sensitive_header
and the cross-origin cleanup to inspect redirected.header_items()
case-insensitively, detecting both Authorization and Proxy-Authorization
regardless of urllib’s stored casing, and remove each sensitive header using its
enumerated actual name. In tests/test_bolt_review_regressions.py lines 46-65,
update the regression assertion to inspect header_items() and verify that
neither sensitive header remains after a cross-origin redirect.
- Around line 282-285: Update the redirect handling around _is_safe_url and
redirect_request so validation and the subsequent connection use the same
resolved, verified IP address rather than resolving newurl again. Preserve
rejection of unsafe targets, and ensure redirects cannot reach internal
addresses through DNS rebinding; alternatively enforce an equivalent egress
block for private/internal destinations.
In `@scanner/rules/authz.yml`:
- Around line 59-61: Update the comment-prefix handling in the two relevant
pattern-regex rules so a standalone * prefix only matches within a /* ... */
block comment, preventing executable multiline expressions such as * todo * auth
from producing HIGH findings; alternatively extract comments before applying
these rules. Add regression coverage confirming executable expressions do not
match and /* ... * TODO ... */ block comments do match.
---
Outside diff comments:
In `@scanner/cli/appguardrail.py`:
- Around line 1690-1714: Update _push_findings and _is_secure_control_plane_url
so the DNS-resolved public IP validated by _is_safe_url is pinned or revalidated
for the actual opener.open connection, preventing DNS rebinding before
bearer-token transmission. Ensure SafeRedirectHandler applies the same public-IP
validation to every redirected destination, while preserving the existing HTTPS
and public-URL requirements.
🪄 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: d8fdf032-ab7b-4190-9d5b-8fff096e827b
📒 Files selected for processing (11)
.jules/bolt.mdCHANGELOG.d/877-scan-path-performance.mdappguardrail_core/controlplane.pyappguardrail_core/language.pyscanner/cli/appguardrail.pyscanner/rules/authz.ymltests/test_appguardrail.pytests/test_auth_deferral_comment_rule.pytests/test_bolt_review_regressions.pytests/test_scan_path_context.pytests/test_ssrf_protection.py
| **Action:** Use `.read()` and `.finditer(content)` for the whole file, which pushes the tight iteration loops down to the C-compiled regex engine. Recover line numbers with string `.count(' | ||
| ')` only when a match is found to achieve massive performance gains (~20-30% reduction in scan time on large text corpuses). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
줄바꿈 문자열 표기를 복구하십시오.
Line 18-19와 Line 45-48에서 '\n' 및 "\n"가 실제 줄바꿈으로 분리되었습니다. 현재 예제는 줄바꿈 문자를 인자로 전달하는 코드를 정확히 표시하지 못합니다. 각 예제를 한 줄의 이스케이프된 문자열 리터럴로 작성하십시오.
Also applies to: 45-48
🤖 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 @.jules/bolt.md around lines 18 - 19, Restore the newline literals in the
examples at the referenced sections so they appear as single-line escaped
strings, using '\n' or "\n" rather than embedding an actual line break inside
the literal. Preserve the surrounding .read(), .finditer(content), and
line-number recovery guidance unchanged.
| if not _is_safe_url(newurl): | ||
| raise urllib.error.URLError("Unsafe redirect target") | ||
| return super().redirect_request(req, fp, code, msg, headers, newurl) | ||
|
|
||
| redirected = super().redirect_request(req, fp, code, msg, headers, newurl) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'def _is_safe_url|SafeRedirectHandler|build_opener|opener\.open|socket\.getaddrinfo|create_connection' \
appguardrail_core/controlplane.py scanner/cli/appguardrail.pyRepository: ContextualWisdomLab/appguardrail
Length of output: 6551
SSRF (CWE-918): Server-Side Request Forgery (SSRF)
Reachability: External · Exploitability: Moderate
Reachability path
● Entry
tests/test_bolt_review_regressions.py
│
▼
● Hop
scanner/cli/appguardrail.py:1701
_push_findings: POST normalized findings to a control-plane /api/v1/scans endpoint.
│
▼
● Sink
appguardrail_core/controlplane.py
검증된 DNS 주소에 연결을 고정하십시오.
_is_safe_url(newurl)은 사전 DNS 응답만 검사합니다. 이후 urllib가 호스트 이름을 다시 해석하므로 DNS rebinding으로 내부 주소에 연결할 수 있습니다. 검증된 IP를 실제 연결에 사용하거나 egress 정책으로 내부 주소 연결을 차단하십시오.
🤖 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 `@appguardrail_core/controlplane.py` around lines 282 - 285, Update the
redirect handling around _is_safe_url and redirect_request so validation and the
subsequent connection use the same resolved, verified IP address rather than
resolving newurl again. Preserve rejection of unsafe targets, and ensure
redirects cannot reach internal addresses through DNS rebinding; alternatively
enforce an equivalent egress block for private/internal destinations.
| has_sensitive_header = req.has_header("Authorization") or req.has_header( | ||
| "Proxy-Authorization" | ||
| ) | ||
| if not has_sensitive_header: | ||
| return redirected | ||
| if original.scheme.lower() != "https" or target.scheme.lower() != "https": | ||
| raise urllib.error.URLError("Authenticated redirects require HTTPS") | ||
|
|
||
| def origin(parsed): | ||
| scheme = parsed.scheme.lower() | ||
| port = parsed.port or (443 if scheme == "https" else 80) | ||
| return scheme, (parsed.hostname or "").lower(), port | ||
|
|
||
| try: | ||
| cross_origin = origin(original) != origin(target) | ||
| except ValueError as exc: | ||
| raise urllib.error.URLError("Unsafe redirect target") from exc | ||
| if cross_origin: | ||
| redirected.remove_header("Authorization") | ||
| redirected.remove_header("Proxy-Authorization") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from urllib.request import Request
request = Request(
"https://api.example.com/scans",
headers={"Proxy-Authorization": "Basic secret"},
)
print(request.header_items())
assert not request.has_header("Proxy-Authorization")
assert any(
name.lower() == "proxy-authorization" for name, _ in request.header_items()
)
PYRepository: ContextualWisdomLab/appguardrail
Length of output: 212
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: Internal · Exploitability: Moderate
Reachability path
● Entry
tests/test_bolt_review_regressions.py
│
▼
● Hop
scanner/cli/appguardrail.py:1701
_push_findings: POST normalized findings to a control-plane /api/v1/scans endpoint.
│
▼
● Sink
appguardrail_core/controlplane.py
Proxy-Authorization 헤더를 대소문자와 무관하게 검사하고 제거하십시오.
urllib.request.Request는 헤더를 Proxy-authorization으로 저장합니다. 현재 호출은 해당 헤더를 감지하거나 제거하지 못하므로, 교차 출처 리디렉션이 프록시 자격 증명을 전달할 수 있습니다. header_items()에서 헤더 이름을 소문자로 비교하고, 제거할 때 열거된 실제 헤더 이름을 사용하십시오. 회귀 테스트도 header_items()를 기준으로 두 민감한 헤더가 없는지 검사하십시오.
📍 Affects 2 files
appguardrail_core/controlplane.py#L291-L310(this comment)tests/test_bolt_review_regressions.py#L46-L65
🤖 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 `@appguardrail_core/controlplane.py` around lines 291 - 310, Update the
redirect handling around has_sensitive_header and the cross-origin cleanup to
inspect redirected.header_items() case-insensitively, detecting both
Authorization and Proxy-Authorization regardless of urllib’s stored casing, and
remove each sensitive header using its enumerated actual name. In
tests/test_bolt_review_regressions.py lines 46-65, update the regression
assertion to inspect header_items() and verify that neither sensitive header
remains after a cross-origin redirect.
| - pattern-regex: '(?im)^\s*(?://|#|/\*+|\*)\s*(?:todo|fixme|hack|temp|temporary)\b[^\n]{0,50}\b(?:auth|security|permission|check|protect)\b' | ||
| - pattern-regex: '(?im)^\s*(?://|#|/\*+|\*)\s*(?:skip|bypass|disable|remove)\b[^\n]{0,30}\b(?:auth|authentication|authorization|security)\b' | ||
| - pattern-regex: '(?im)^\s*(?://|#|/\*+|\*)\s*(?:disable|mock|fake)\s+(?:auth|security)\b' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
블록 주석의 * 접두사를 실제 주석 문맥으로 제한하십시오.
Line 59의 독립된 \* 대안은 블록 주석 내부인지 확인하지 않습니다. 예를 들어 유효한 Python 식의 * todo * auth 연속 줄도 todo-skip-auth 규칙에 일치합니다. Line 60도 같은 문제가 있습니다.
이 경우 실행 코드에 HIGH finding이 생성됩니다. * 접두사는 /*로 시작한 블록 내부에서만 허용하거나, 주석을 먼저 추출한 뒤 규칙을 적용하십시오. 실행 가능한 여러 줄 식이 finding을 만들지 않는 회귀 테스트와 /* ... * TODO ... */ 양성 테스트를 추가하십시오.
As per coding guidelines, “Treat AppGuardrail critical/high findings in app code as deploy blockers.”
🤖 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 `@scanner/rules/authz.yml` around lines 59 - 61, Update the comment-prefix
handling in the two relevant pattern-regex rules so a standalone * prefix only
matches within a /* ... */ block comment, preventing executable multiline
expressions such as * todo * auth from producing HIGH findings; alternatively
extract comments before applying these rules. Add regression coverage confirming
executable expressions do not match and /* ... * TODO ... */ block comments do
match.
Source: Coding guidelines
|
@coderabbitai review Please review the exact live head only. Verify scan-root classification and normalized path context are cached once while standalone callers retain the public str|Path contract including subclasses; authenticated uploads require public HTTPS; HTTPS-to-HTTP downgrade is rejected; cross-origin redirects strip Authorization and Proxy-Authorization while same-origin HTTPS preserves credentials; and authentication-deferral detection is comment-scoped. Require current complete tests, 100% configured coverage, Security Process, Security Scan, Semgrep, retention/evidence gates, and zero unresolved threads. Review only; do not update the branch, merge, release, or alter protections. |
|
I will verify the specified scanner behavior, authenticated upload and redirect controls, comment-scoped detection, test and coverage evidence, security gates, Semgrep, retention evidence, and unresolved review threads. I will not modify the branch, merge, release, or alter protections.
|
1 participant
Closed without merge
The reconstructed branch still has four unresolved actionable review findings and a failing exact-head
Testsworkflow. More importantly, its bearer-token transport validates DNS beforeurllibperforms a second resolution, so DNS rebinding can still reach a disallowed destination; cross-originProxy-Authorizationremoval is also not reliably case-insensitive. The authentication-deferral regex can additionally classify executable multiline expressions as comments.Merging a branch that claims redirect hardening while retaining those security gaps would be unsafe. The work has been split into clean, reviewable follow-ups from current
develop:This PR is closed unmerged and must not be used as the implementation base for either follow-up.