-
Notifications
You must be signed in to change notification settings - Fork 0
fix(scanner): preserve path contracts and secure control-plane redirects #885
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
020a013
0385f2f
3410e64
47e0e88
eefe2ee
a90d0d9
c815a2a
8818110
95169b6
2744e56
e446bf1
2f56cbe
79e4b60
106a16f
2a6e6a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| ### Changed | ||
|
|
||
| - Reduced repeated scan-root file classification and relative-path allocation in large repository scans while retaining a one-time fallback for standalone `_scan_file` callers. | ||
| - Preserved the public `str | Path` contract, including `str` subclasses, while using allocation-light basename and suffix parsing in language detection. | ||
| - Restricted bearer-authenticated control-plane uploads and redirects to public HTTPS, rejected transport downgrades, and removed sensitive authorization headers from cross-origin redirects. | ||
| - Limited authentication-deferral findings to source comments so executable hardening such as removing `Authorization` headers is not misclassified as deferred authentication work. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -275,10 +275,40 @@ def is_bad_ip(ip) -> bool: | |
|
|
||
|
|
||
| class SafeRedirectHandler(urllib.request.HTTPRedirectHandler): | ||
| """Reject unsafe redirects and prevent cross-origin credential forwarding.""" | ||
|
|
||
| def redirect_request(self, req, fp, code, msg, headers, newurl): | ||
| """Build one safe redirected request with bounded credential scope.""" | ||
| 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) | ||
|
Comment on lines
282
to
+285
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 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검증된 DNS 주소에 연결을 고정하십시오.
🤖 Prompt for AI Agents |
||
| if redirected is None or req is None: | ||
| return redirected | ||
|
|
||
| original = urlparse(req.full_url) | ||
| target = urlparse(newurl) | ||
| 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") | ||
|
Comment on lines
+291
to
+310
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 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
📍 Affects 2 files
🤖 Prompt for AI Agents |
||
| return redirected | ||
|
|
||
|
|
||
| def _send_alert( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,9 +56,9 @@ rules: | |
|
|
||
| - id: todo-skip-auth | ||
| patterns: | ||
| - pattern-regex: '(?i)(?:todo|fixme|hack|temp|temporary).{0,50}(?:auth|security|permission|check|protect)' | ||
| - pattern-regex: '(?i)(?:skip|bypass|disable|remove).{0,30}(?:auth|authentication|authorization|security)' | ||
| - pattern-regex: '(?i)//\s*(?:disable|mock|fake)\s*(?:auth|security)' | ||
| - 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' | ||
|
Comment on lines
+59
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift 블록 주석의 Line 59의 독립된 이 경우 실행 코드에 As per coding guidelines, “Treat AppGuardrail critical/high findings in app code as deploy blockers.” 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| message: | | ||
| Comment suggests authentication or security check was intentionally skipped | ||
| or deferred. This is a common pattern in AI-generated code. Review and ensure | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
줄바꿈 문자열 표기를 복구하십시오.
Line 18-19와 Line 45-48에서
'\n'및"\n"가 실제 줄바꿈으로 분리되었습니다. 현재 예제는 줄바꿈 문자를 인자로 전달하는 코드를 정확히 표시하지 못합니다. 각 예제를 한 줄의 이스케이프된 문자열 리터럴로 작성하십시오.Also applies to: 45-48
🤖 Prompt for AI Agents