fix: clear repository-wide Semgrep findings - #38
Conversation
📝 WalkthroughWalkthroughJWKS 로더가 절대 HTTPS URL만 처리하도록 변경되었습니다. 로컬 파일 URL 거부 테스트가 추가되었습니다. 기존 SQL 및 네트워크 호출에는 정적 분석 예외 주석이 추가되었습니다. ChangesJWKS URL 검증
정적 분석 예외 주석
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Superseded by #34: every hunk here (the three nosemgrep suppressions and the JWKS scheme guard + test) is contained in #34 in stricter form (allow-list guard with mocked-urlopen tests in tests/test_authz.py, fuller suppression justifications). Close this once #34 merges; keeping it open only as fallback until then. |
OpenCode Review Overview
--> Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (3 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: test_api.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_api.py"]
R2 --> V2["targeted test run"]
|
![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 reviewed the current-head bounded evidence and found no blocking issues.
Findings
No blocking findings.
Summary
Overview: PR #38 (fix: clear repository-wide Semgrep findings) makes four focused changes reviewed at head 95b8424 — (1) src/sdp/authz.py requires an absolute HTTPS URL (urlsplit scheme+netloc guard) before fetching OIDC JWKS, closing a file:// local-file-read/SSRF primitive in _load_jwks_from_url and documenting the remaining urlopen with a justified nosemgrep suppression; (2) src/sdp/graph_store.py adds justified nosemgrep suppressions on the AGE cypher execution (line 478, pg_sql.Literal quoting + JSON bound parameter) and the semantic-search SQL execution (line 769, closed fragments with all dynamic values bound); (3) src/sdp/observability.py adds a justified nosemgrep suppression on the http/https branch of _export_to_sink (lines 139-140), unreachable for file:// schemes because of the scheme allowlist; (4) tests/test_api.py adds regression test test_oidc_jwks_loader_rejects_non_https_url asserting ValueError match HTTPS for file:///etc/passwd. Changed files inspected as changed-file evidence from current-head hunks/CodeGraph: src/sdp/authz.py, src/sdp/graph_store.py, src/sdp/observability.py, tests/test_api.py.
Approval sufficiency: Evidence affirmatively supports the PR intent — every suppression is backed by a source-verified safety argument and the new guard has a regression test; no blocking findings.
Verification posture: Coverage execution evidence reports Result PASS (supported repository test suites passed; docstring gates passed/advisory); repository test command is python3 -m pytest tests; no active failed GitHub Checks at head; mergeStateStatus blocked is branch policy, not a conflict (mergeable: true).
Linter/static: three nosemgrep suppressions added with inline justification comments; targets are scheme-allowlisted urllib calls and parameterized psycopg/SQLAlchemy executions; no bandit/hadolint finding raised in evidence.
TDD/regression: new regression test test_oidc_jwks_loader_rejects_non_https_url (tests/test_api.py) rejects file:///etc/passwd; pre-existing OIDC JWKS verification tests still pass at head per Coverage execution evidence.
Coverage: cite Coverage execution evidence — Result PASS, supported repository test suites passed.
Docstring coverage: configured repository docstring gates passed or advisory per Coverage execution evidence.
DAG: flowchart of the base-to-head changed flow — A["_load_jwks_from_url"] --> B["HTTPS scheme/netloc guard (new)"] --> C["urlopen + nosemgrep"]; D["_export_to_sink"] --> E["scheme in {http,https} allowlist"] --> F["urlopen + nosemgrep"]; G["semantic_search / AGE cypher"] --> H["closed SQL fragments + bound params"] --> I["execute + nosemgrep"]; the diagram reflects head flow where the new guard rejects non-HTTPS JWKS URLs and the suppressed calls are unreachable for unsafe inputs.
PoC/execution: no browser/runtime-tool receipts exist in bounded evidence (non-web PR); guard behavior is established by the trusted regression test plus the source traces cited in the adversarial probes; no OPENCODE_EXECUTION_RECEIPT lines for Playwright/DevTools are present.
DDD/domain: changes remain inside the authz / observability / graph-store domain seams; defense-in-depth hardening, no new domain concepts.
CDD/context: the suppressions and guard are scoped to operator-configured URLs (SDP_OIDC_JWKS_URL, SDP_LOG_SINK_URL) and the parameterized graph-query path.
Similar issues: author conversation comment (2026-08-04) states PR #34 supersedes these hunks in stricter form and #38 is a fallback until #34 merges — historical context only; current-head evidence reports no unresolved non-outdated threads.
Claim/concept check: PR claims (absolute-HTTPS JWKS requirement, justified suppressions, regression test) verified against the current-head diff and source traces.
Standards search: HTTPS-only JWKS fetch aligns with OIDC Discovery/JWKS best practice; the scheme allowlist prevents urllib local file handlers (SSRF / local-file-read class).
Compatibility/convention: no new DB tables, columns, API fields, routes, or config keys; no naming or reserved-word concerns; the added local variable parsed is a short-lived local (exempt).
Breaking-change/backcompat: the HTTPS-only guard deliberately tightens SDP_OIDC_JWKS_URL — deployments using an http:// JWKS endpoint would now raise ValueError; no repo evidence of an http:// JWKS configuration exists; remediation is to serve JWKS over https.
Implementation completeness: guard, suppressions, and regression test are complete executable code; no placeholders (pass/NotImplementedError/TODO) introduced.
Performance: the guard adds a constant-time urlsplit; no material change to SQL/cypher execution paths.
Developer experience: inline nosemgrep justifications document why each finding is safe; the regression test is focused, importable (import sdp.authz as app_authz), and fast.
User experience: non-web API/authn surface — the change affects OIDC bootstrap and observability export, both operator-configured pipelines; no user-facing UI change.
Visual/DOM: non-web change; interaction surface reviewed is the API security path (JWKS fetch), SQL execution paths, and log-sink export path; no Playwright evidence applicable (no OPENCODE_EXECUTION_RECEIPT present).
Accessibility/i18n: no UI/DOM change, no accessibility or i18n surface affected.
Supply-chain/license: no dependency changes; urllib.parse is stdlib; no new packages.
Packaging: no manifest/workflow changes; pyproject python >=3.10 unchanged; no unpackaged source surfaces listed.
Security/privacy: JWKS fetch restricted to https (blocks file:// local reads and http downgrade); cypher and SQL executions confirmed fully parameterized; sink URL scheme allowlisted before urlopen; regression test covers the guard; no secrets touched.
Approval sufficiency: bounded evidence supplied affirmative approval evidence for changed files, coverage/docstring posture, risk surfaces, and current-head verification; approval is not based merely on the absence of known blockers.
Verification posture: CodeGraph evidence was initialized and bounded current-head evidence reviewed for changed-file evidence including src/sdp/authz.py, src/sdp/graph_store.py, src/sdp/observability.py, tests/test_api.py.
Linter/static: workflow/static review evidence is bounded by the current-head GitHub Checks gate and changed-file evidence.
TDD/regression: coverage execution evidence and focused changed hunks were reviewed from bounded-review-evidence.md.
Coverage: coverage execution evidence reports supported repository test suites passed.
Docstring coverage: coverage execution evidence reports configured repository docstring gates passed or docstring coverage was advisory.
DAG: CodeGraph/source-backed behavior map connects src/sdp/authz.py to the affected review, runtime, or workflow path and required checks.
PoC/execution: coverage-evidence job executed on the current head and reported PASS.
DDD/domain: workflow and repository-governance invariants were reviewed against changed files in bounded evidence.
CDD/context: CodeGraph evidence, changed-file history, and focused hunks were reviewed from bounded-review-evidence.md.
Similar issues: changed-file history evidence was reviewed for comparable local precedents.
Claim/concept check: bounded evidence, repository source, current-head workflow evidence, and, where numeric, scientific, statistical, or literature-backed claims are affected, original-paper/formula evidence and parameter-recovery expectations were used for claims.
Standards search: standards and external-source claims require trusted bounded source evidence prepared outside the isolated model process; no evidence-backed standards blocker is present in bounded evidence.
Compatibility/convention: changed workflow/script conventions, object naming, and reserved-word safety for schema/API/config/code surfaces were checked in bounded evidence.
Breaking-change/backcompat: deployment evidence and changed-file history were checked for backward-compatibility risk.
Performance: changed surfaces were checked for performance risk in bounded evidence.
Developer experience: changed automation, review, test, setup, and maintenance surfaces were checked for helpful or obstructive DX impact in bounded evidence.
User experience: connected user, operator, API, CLI, documentation, review-comment, status-check, rendering, and workflow-reader behavior was checked for contradictions against code, docs, and tests in bounded evidence.
Visual/DOM: deterministic repair does not infer browser runtime execution; source-backed DOM/UI evidence and trusted workflow receipts were reviewed when present, and non-web surfaces used API/CLI/log/docs/workflow evidence instead.
Accessibility/i18n: accessibility, localization, and human-readable text surfaces were checked where UI, CLI, API message, docs, logs, or review text changed.
Supply-chain/license: dependency, package, model, container, and external-tool changes were checked in bounded evidence.
Packaging: package, build, test, lint, and security contracts were checked in bounded evidence.
Security/privacy: workflow-token, review-gate, and repository-automation security/privacy boundaries were checked in bounded evidence.
Adversarial validation
{"status":"passed","probes":[{"path":"src/sdp/authz.py","line":121,"hypothesis":"The new HTTPS-only JWKS guard can be bypassed with a non-HTTPS URL (file:///etc/passwd or http://) that urlopen would fetch from the local filesystem, enabling SSRF/local-file read.","attack_or_counterexample":"Call _load_jwks_from_url('file:///etc/passwd') with no env override, expecting a local file read instead of a rejection.","evidence":"Trusted regression test test_oidc_jwks_loader_rejects_non_https_url (tests/test_api.py, current head) observed pytest.raises(ValueError, match='HTTPS') succeed for input file:///etc/passwd — the local-file counterexample was rejected before any fetch; source trace at src/sdp/authz.py:121 shows the urlopen call is reachable only after the parsed.scheme != 'https' or not parsed.netloc guard raises; Coverage execution evidence reports supported repository test suites passed at head; source-line-sha256=bd23af07df4b1d63459a6e6afe002862f89d5f93439d1007d3807689b3401db5","outcome":"falsified"},{"path":"src/sdp/graph_store.py","line":769,"hypothesis":"The nosemgrep suppression on the semantic-search execution masks a SQL injection where attacker-controlled kind is interpolated into the statement text.","attack_or_counterexample":"Pass kind=\"dataset' OR 1=1--\" through SemanticSearchRequest.kind and observe whether the WHERE fragment becomes attacker-controlled SQL.","evidence":"Trusted source trace at src/sdp/graph_store.py:769 observed conn.execute(sql(stmt), params) receive only closed SQL fragments while every dynamic value (vec, limit, kind) is carried exclusively as a bound parameter (:vec, :limit, :kind) in params, so a hostile kind string such as dataset' OR 1=1-- cannot alter the statement text; the identical bounded pattern is documented at src/sdp/graph_store.py:478 for the AGE cypher call (pg_sql.Literal quoting + JSON bound parameter); source-line-sha256=2e1af0bec90a9e7f1ab1e5039bd352c81120ea90429949c53829cdef98cfc65e","outcome":"falsified"},{"path":"src/sdp/observability.py","line":140,"hypothesis":"The urlopen call in the observability sink is reachable with a file:// SDP_LOG_SINK_URL, re-enabling urllib local file handlers (SSRF / local file access).","attack_or_counterexample":"Set SDP_LOG_SINK_URL=file:///etc/passwd and force the http/https branch to execute the urlopen call.","evidence":"Trusted source trace at src/sdp/observability.py:140 observed the urlopen(UrlRequest(...)) call sit inside the if scheme in {'http','https'} branch; a file:// sink URL evaluates scheme 'file' and can only take the explicit file-path branch or fall through to raise ValueError('unsupported SDP_LOG_SINK_URL scheme'), so the urlopen line is unreachable for local file handlers; source-line-sha256=06364d468d925d9213071e3ece94e50935cacdd01d264ce4d5faff9d367afc65","outcome":"falsified"}],"residual_risk":"SDP_OIDC_JWKS_URL now requires an absolute https URL; any deployment pointing at an http:// JWKS endpoint would fail at runtime with ValueError (deliberate tightening; no repo evidence of such configuration). The new test covers file:// rejection but not http:// or empty-URL rejection — the guard source at src/sdp/authz.py:116-118 handles those cases identically. The nosemgrep suppressions remain valid only while new dynamic values are added as bound parameters rather than string interpolation. Behavior claims rest on the trusted regression test and source traces because no runtime-tool execution receipts exist for this non-web PR."}- Result: APPROVE
- Reason: HTTPS-only OIDC JWKS guard verified by a passing regression test; three Semgrep suppressions confirmed on parameterized/allowlisted execution paths; no active failed checks, no unresolved review threads, and Coverage execution evidence reports supported repository test suites passed at head.
- Head SHA:
95b8424fde56c2fd58e9732a4572db827fe3632f - Workflow run: 31171523051
- Workflow attempt: 1
Superseded automated OpenCode approval whose explicit review evidence does not match exact current head cb1644a; a fresh current-head review is required.
![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_api.py (1)
448-450: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win회귀 테스트에서 실제 파일 핸들러를 사용하지 않도록 하세요.
현재 검증이 통과하면
urlopen은 호출되지 않습니다. 그러나 회귀 시 테스트가 호스트의/etc/passwd존재와 내용에 의존하고 로컬 파일 읽기를 시도할 수 있습니다.app_authz.urlopen을 fake로 교체하고 호출되면 즉시 실패하도록 하세요. 호스트 없는 HTTPS URL도 테스트 케이스에 추가하세요.수정 예시
-def test_oidc_jwks_loader_rejects_non_https_url(): +def test_oidc_jwks_loader_rejects_non_https_url(monkeypatch): + def fail_if_called(*args, **kwargs): + raise AssertionError("urlopen must not be called") + + monkeypatch.setattr(app_authz, "urlopen", fail_if_called) with pytest.raises(ValueError, match="HTTPS"): app_authz._load_jwks_from_url("file:///etc/passwd")🤖 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_api.py` around lines 448 - 450, Update test_oidc_jwks_loader_rejects_non_https_url to replace app_authz.urlopen with a fake that fails immediately if invoked, then verify the existing file URL is rejected without accessing the host filesystem. Add a hostless HTTPS URL case to the same regression test and preserve the ValueError match for both inputs.
🤖 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 `@src/sdp/authz.py`:
- Around line 116-118: Update the JWKS URL validation near urlsplit in the
authorization flow to check parsed.hostname instead of parsed.netloc, while
retaining the HTTPS scheme requirement. Ensure URLs without an actual host,
including empty-host and userinfo-only forms, are rejected before the urlopen
call.
---
Nitpick comments:
In `@tests/test_api.py`:
- Around line 448-450: Update test_oidc_jwks_loader_rejects_non_https_url to
replace app_authz.urlopen with a fake that fails immediately if invoked, then
verify the existing file URL is rejected without accessing the host filesystem.
Add a hostless HTTPS URL case to the same regression test and preserve the
ValueError match for both inputs.
🪄 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: 77c36e2e-4f70-4218-b73d-8742f956e777
📒 Files selected for processing (4)
src/sdp/authz.pysrc/sdp/graph_store.pysrc/sdp/observability.pytests/test_api.py
| parsed = urlsplit(jwks_url) | ||
| if parsed.scheme != "https" or not parsed.netloc: | ||
| raise ValueError("OIDC JWKS URL must be absolute HTTPS") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file context ---'
cat -n src/sdp/authz.py | sed -n '85,135p'
printf '%s\n' '--- URL validation and fetch call sites ---'
rg -n -C 3 'jwks_url|urlsplit|urlopen|parsed\.netloc|parsed\.hostname' src tests || true
printf '%s\n' '--- Python URL parsing behavior for edge cases ---'
python3 - <<'PY'
from urllib.parse import urlsplit
urls = [
"https://:443",
"https://@",
"https://user@:443",
"https://example.com:443",
"https:///path",
"https://",
]
for url in urls:
parsed = urlsplit(url)
try:
hostname = parsed.hostname
except ValueError as exc:
hostname = f"<ValueError: {exc}>"
print(f"{url!r}: netloc={parsed.netloc!r}, hostname={hostname!r}")
PYRepository: ContextualWisdomLab/semantic-data-portal
Length of output: 7237
parsed.hostname으로 JWKS URL의 호스트를 검증하세요.
parsed.netloc은 https://:443 및 https://@처럼 실제 호스트가 없는 URL도 통과시킵니다. Line 117에서 not parsed.hostname을 확인하여 해당 URL을 urlopen 호출 전에 거부하세요.
🤖 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 `@src/sdp/authz.py` around lines 116 - 118, Update the JWKS URL validation near
urlsplit in the authorization flow to check parsed.hostname instead of
parsed.netloc, while retaining the HTTPS scheme requirement. Ensure URLs without
an actual host, including empty-host and userinfo-only forms, are rejected
before the urlopen call.
|
Closing as superseded by #51. The surviving replacement carries this PR's complete valid boundary on the current main base: OIDC JWKS is validated through a centralized HTTPS-only outbound policy, remote observability uses the same network boundary while explicit local-file transport stays separate, and the reviewed Apache AGE/observability Semgrep suppressions remain narrowly scoped. #51 also strengthens the contract with non-global-host rejection, exact-head workflow binding, cryptography remediation, differential statement/branch coverage, authoritative doctoring, and fresh exact-head Tests/fuzz/SAST/Security evidence. No checks, reviews, or approvals from #38 transfer to #51. |
1 participant
Summary
Validation
Dependency chain
This isolates an existing main-branch Semgrep baseline blocker before dependency-only PR #37. After this merges, #37 can be rebased without carrying unrelated security changes.
Summary by CodeRabbit