fix(sast): clear false-positive Semgrep findings with documented nosemgrep - #78
fix(sast): clear false-positive Semgrep findings with documented nosemgrep#78seonghobae wants to merge 7 commits into
Conversation
…osemgrep The central Semgrep (multi-language SAST) gate fails on four p/default findings that are false positives already assessed and annotated with `# nosec` by the original code; add the parallel `# nosemgrep` suppressions with justification so the gate passes at base (it currently fails every PR against main): - cost_ledger.py (3x sqlalchemy-execute-raw-query): raw DB-API cursor.execute calls that only interpolate the DB-API placeholder token and fixed column names from the _USAGE_COLUMNS / ATTRIBUTION_DIMENSION_CATALOG constants; every value is passed as a bound parameter tuple, so no user input reaches the SQL text. - orchestrator.py (unverified-ssl-context): ssl._create_unverified_context() is reached only through the explicit, documented, dev-only verify_tls=False opt-out; the default path verifies against the system trust store. Comment-only change (py_compile clean); no runtime behavior changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Msz8Dni5zRaqPatNCmZJZ8
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughSQL·네트워크 보안 경로에 Changes보안 감사 주석
Atheris 퍼징 의존성
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…dings The prior suppressions cleared on multi-line execute() calls (string on its own line) but not on the two single-line findings — cost_ledger.py query() SELECT and orchestrator.py's unverified-context return — where the flagged expression shares its line with the call/return. Move those two suppressions to the immediately-preceding line and use the exact full rule id (...sqlalchemy-execute-raw-query.sqlalchemy-execute-raw-query and ...unverified-ssl-context.unverified-ssl-context) so Semgrep reliably anchors them. Still comment-only; the queries remain parameterized and the SSL opt-out remains the documented, default-secure dev-only path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Msz8Dni5zRaqPatNCmZJZ8
The prior placement suppressed the single-line findings but not the multi-line cur.execute() calls (Semgrep anchors those to the cur.execute( line, not the string argument on the following line), and a third finding was still open: - cost_ledger.py: move both sqlalchemy-execute-raw-query suppressions to the line immediately above each cur.execute( (the anchor Semgrep reports). - orchestrator.py: add a suppression for python...dynamic-urllib-use-detected on _open_provider's urlopen — the request URL is validated/allowlisted by _provider_url and egress to private/reserved addresses is blocked, so it is a false positive. All use the exact full rule id on the preceding line, the form the gate log confirmed it honors (SEMGREP_SUPPRESSED_COUNT rose for those). Comment-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Msz8Dni5zRaqPatNCmZJZ8
![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 headacbf58d2cb735c1df350a770b46605b20e96568d. -
Head SHA:
acbf58d2cb735c1df350a770b46605b20e96568d -
Workflow run: 30558605842
-
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"]
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["Workflow: fuzz.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: fuzz.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Changed file (4 files)"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Changed file (4 files)"]
R2 --> V2["required checks"]
|
atheris 3.0.0 was removed from PyPI (only 3.1.0 remains). The central coverage-evidence "trusted coverage tool image" build preflights every gathered Python lock, so the unresolvable atheris==3.0.0 pin in fuzz/requirements-atheris.txt fails the image build with: ERROR: No matching distribution found for atheris==3.0.0 ##[error]Trusted coverage tool image build failed before PR execution. That fails coverage-evidence for every PR head regardless of the diff, which in turn blocks OpenCode approval. Bump to the drop-in successor 3.1.0 and regenerate fuzz/requirements-atheris.txt hashes via the recorded `uv pip compile ... --generate-hashes` command. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Msz8Dni5zRaqPatNCmZJZ8
…solve The 3.1.0-only pin fixed the central coverage-evidence image build but broke this repo's own required "Atheris coverage-guided" fuzz.yml job: on the CPython 3.11 runner atheris 3.1.0 has no distribution (dropped the cp311 build; candidates end at 3.0.0), while the central coverage-evidence image runs a newer interpreter where only 3.1.0 resolves. No single pin satisfies both. Split by `python_full_version` marker: atheris==3.0.0 ; python_full_version < "3.12" atheris==3.1.0 ; python_full_version >= "3.12" `pip install --require-hashes` evaluates the marker per environment, so the cp311 fuzz job installs 3.0.0 and the newer coverage-evidence image installs 3.1.0 — both hash-pinned. Regenerated via the recorded uv compile command. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Msz8Dni5zRaqPatNCmZJZ8
![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
🤖 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 `@contextual_orchestrator/orchestrator.py`:
- Line 311: Update the nosemgrep suppression comment near _provider_url so it no
longer claims allowlist validation unless that guarantee is enforced. Either
require and apply CONTEXTUAL_ORCHESTRATOR_ALLOWED_PROVIDER_HOSTS in
_validate_provider, or narrow the suppression rationale and scope to the
validation that _provider_url actually performs, while preserving
private/reserved-address blocking.
🪄 Autofix (Beta)
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: 3730b620-34a7-4b83-bd87-7537c0262a9c
📒 Files selected for processing (4)
contextual_orchestrator/cost_ledger.pycontextual_orchestrator/orchestrator.pyfuzz/requirements-atheris.infuzz/requirements-atheris.txt
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 `@fuzz/requirements-atheris.in`:
- Around line 2-5: Update the comment in requirements-atheris.in to remove the
incorrect claim that Atheris 3.0.0 supports only CPython <= 3.11. Keep only the
accurate rationale that 3.1.0 drops the cp311 build, while preserving the
existing interpreter-marker pinning explanation.
🪄 Autofix (Beta)
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: c8106a91-d86a-4f60-9e27-9b8e03290747
📒 Files selected for processing (2)
fuzz/requirements-atheris.infuzz/requirements-atheris.txt
…ression scope Address CodeRabbit review on #78 (comment accuracy only; no behaviour change): - fuzz/requirements-atheris.in: the previous rationale wrongly implied atheris 3.0.0 is CPython <= 3.11 only. Ground truth (PyPI): 3.0.0 ships cp311/cp312/ cp313 wheels but no cp314; 3.1.0 ships cp312/cp313/cp314 wheels and no cp311 wheel or sdist. So cp311 must use 3.0.0 and the wheel-only CPython 3.14 coverage-evidence image must use 3.1.0 — the marker split (boundary 3.12) is unchanged and correct; only the explanation is fixed. - orchestrator.py: the urlopen nosemgrep note claimed _provider_url "allowlists" the URL. It does not — _provider_url only rejects non-http(s)/ local schemes. The SSRF guard is _validate_provider, which blocks egress to private/loopback/link-local/multicast/reserved addresses and enforces the host allowlist only when CONTEXTUAL_ORCHESTRATOR_ALLOWED_PROVIDER_HOSTS is set. Narrowed the suppression rationale to what is actually enforced. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Msz8Dni5zRaqPatNCmZJZ8
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 head6b31913c293792db0a49ea3a3a734845f32b654c. -
Head SHA:
6b31913c293792db0a49ea3a3a734845f32b654c -
Workflow run: 30565945212
-
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 (4 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (4 files)"]
R1 --> V1["required checks"]
The earlier interpreter-marker split did not clear coverage-evidence: the central "trusted coverage tool image" preflight validates every pinned version in a gathered lock against the image interpreter (CPython 3.14) and does NOT skip environment-marker-gated lines, so a marker-gated `atheris==3.0.0` line still failed there (3.0.0 ships no cp314 wheel): Preflighting trusted base Python lock candidate fuzz/requirements-atheris.txt ERROR: No matching distribution found for atheris==3.0.0 (from versions: 3.1.0) ##[error]Trusted coverage tool image build failed before PR execution. Fix: drop the marker split and pin a single atheris==3.1.0 (cp312/cp313/cp314 wheels), and move the Atheris fuzz job from CPython 3.11 to 3.12 since 3.1.0 dropped the cp311 wheel. Now the cp312 fuzz job and the cp314 preflight both resolve the one pinned version. Regenerated the hash lock via the recorded uv command (now --python-version 3.12). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Msz8Dni5zRaqPatNCmZJZ8
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 headd565033a8b894a46f94940ff2856ab00ca4d41f6. -
Head SHA:
d565033a8b894a46f94940ff2856ab00ca4d41f6 -
Workflow run: 30571020475
-
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["Workflow: fuzz.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: fuzz.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Changed file (4 files)"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Changed file (4 files)"]
R2 --> V2["required checks"]
|
The job fails at the trusted-image build, before any PR code runs (run Why the fix on this branch does not clear it:
Net: the gate that must go green to approve/merge preflights a base-branch fuzz-only lock (irrelevant to test coverage) and hard-fails it on cp314 — so this PR stays red until Recommended central fix (in Generated by Claude Code |
|
Closing as superseded by #76. PR #76 contains the same narrowly documented Semgrep suppressions, the stricter non-global provider-address SSRF fix with regression tests, and the Atheris compatibility repair. The central trusted-lock installer invokes pip on the coverage-image interpreter with normal PEP 508 marker evaluation, so #76’s split pin preserves the repository’s Python 3.11 fuzz job while selecting Atheris 3.1.0 on newer coverage interpreters. Keeping both PRs would duplicate the SAST changes and allow the later merge to overwrite the canonical lock strategy. |
3 participants
Summary
The central Semgrep (multi-language SAST) gate fails on
mainwith fourp/defaultfindings that are false positives — they are already assessed and annotated with# nosecby the original code, but Semgrep needs its own# nosemgrepsuppression. This adds narrow, per-line# nosemgrep: <rule-id>suppressions with justification so the gate passes at base (it currently fails every PR againstmain, including #76 and #77).Findings suppressed (with rationale)
contextual_orchestrator/cost_ledger.py— 3×python.sqlalchemy.security.sqlalchemy-execute-raw-query. These are raw DB-APIcursor.execute()calls (SQLite/psycopg, not SQLAlchemy) whose f-strings interpolate only the DB-API placeholder token (?/%s) and fixed column names from the_USAGE_COLUMNS/ATTRIBUTION_DIMENSION_CATALOGconstants. Every value is passed as a bound parameter tuple, so no user input reaches the SQL text — there is no injection surface.contextual_orchestrator/orchestrator.py—python.lang.security.unverified-ssl-context.ssl._create_unverified_context()is reached only through the explicit, documented, dev-onlyverify_tls=Falseopt-out; the default path verifies against the system trust store (or a pinned CA bundle). This is a deliberate, default-secure design decision.Why suppress rather than refactor
The queries are already parameterized and safe; the placeholder token legitimately varies by backend (SQLite
?vs Postgres%s), so the f-string is intentional. The SSL opt-out is a required dev affordance. A narrow# nosemgrep(mirroring the existing# nosec) is the correct governance action for a genuine false positive — it does not disable the rule globally or weaken the gate; Semgrep records these as suppressions and the gate filters only genuinely-suppressed lines.Verification
python -m py_compile contextual_orchestrator/cost_ledger.py contextual_orchestrator/orchestrator.py→ cleanGenerated by Claude Code
Summary by CodeRabbit