Repository navigation
fix: address 7 Sonar security hotspots flagged on PR #29 - #36
Conversation
Address all 7 Security Hotspots flagged by SonarCloud on PR #29 (`development → main` v0.5.5 consolidation). Each is a verified false positive — the code is the security guard itself, a test fixture verifying the guard rejects insecure schemes, or a string argument that is never written to disk. The fix is a combination of moving existing NOSONAR markers onto the literal violation line (Sonar's per-line NOSONAR parser is strict) and replacing one `/tmp/`-shaped string fixture with a project-relative path. ## Hotspot-by-hotspot disposition ### #1 — `tools/check_site_claims.py:240` — S5852 ReDoS (MEDIUM) The regex `r"^\s+([a-z][a-z0-9-]+-[a-z0-9-]+)\s{2,}"` runs against *controlled static HTML* the build emits — never user input. The backtracking risk is unreachable. The existing NOSONAR was on the trailing line (after the closing `)`); Sonar didn't apply it to line 240. Move the NOSONAR onto the regex literal line and tag the rule explicitly (`# NOSONAR python:S5852`). ### #2-3 — `forgelm/_http.py:202` — S5332 HTTP insecure (LOW × 2) This is the rejection f-string of the `safe_post` security guard itself: `f"http:// blocked (use https://); url={_mask_netloc(url)}"`. Naming the blocked scheme is intentional so operators see WHY the call failed. Move the NOSONAR onto the f-string line + tag `python:S5332` + add a paragraph-level comment explaining the guard semantics. ### #4 — `tests/test_webhook.py:235` — S5332 (LOW, docstring) Sonar flagged the literal `http://` in a docstring describing the test that verifies http:// IS BLOCKED. Tag the test method with `# NOSONAR python:S5332` and rephrase the docstring to lead with "Verify plain-HTTP URLs are rejected…" so the security intent is inverted-explicit. ### #5 — `tests/test_webhook.py:238` — S5332 (LOW, regex match) `pytest.raises(HttpSafetyError, match="http://")` — the literal is a regex pattern matching the rejection error message, not an outbound call. Tag with `# NOSONAR python:S5332`. ### #6 — `tests/test_webhook.py:256` — S5332 (LOW, ftp docstring) Same shape as #4 but for the `test_unsupported_scheme` docstring that names ftp / file as the rejected schemes. Tag the method + rephrase docstring. ### #7 — `tests/test_webhook.py:419` — S5443 publicly-writable dir `model_path="/tmp/staged"` — a string fixture passed to `notify_awaiting_approval`. Nothing is written to disk; the test asserts the webhook payload schema. Sonar's S5443 fires on the `/tmp/` literal regardless. Replace with the project-relative shape `./outputs/run-r/final_model.staging` (the actual production shape), which is honest about what `model_path` looks like in real deployments and side-steps the false positive without needing a NOSONAR. Inline comment documents why no filesystem write happens. ## Verification All 5 strict CI guards green at HEAD: | Check | Result | |---|---| | `ruff format --check .` + `ruff check .` | ✅ clean | | `check_bilingual_parity --strict` | ✅ 39/39 | | `check_anchor_resolution --strict` | ✅ 264/264 | | `check_cli_help_consistency --strict` | ✅ 425/425 | | `check_site_claims --strict` | ✅ artefacts/templates/GPU/version | | `check_field_descriptions --strict` | ✅ | | `pytest --no-cov -q tests/` | ✅ 1436 passed / 14 skipped | | `forgelm --config config_template.yaml --dry-run` | ✅ exit 0 | `tests/test_webhook.py` (32 cases) and `tests/test_check_site_claims.py` (9 cases) both pass under the new NOSONAR + tmp-path-replacement state. ## Note for the SonarCloud review pass Security Hotspots normally use SonarCloud's "Mark as Safe" review workflow rather than `# NOSONAR` (which is for issues). Even after these in-code clarifications, the maintainer may still need to mark each of the 7 entries as Safe in the SonarCloud UI. The in-code comments above document the rationale so the UI review is a 2-minute audit rather than a re-derivation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
Reviewer's GuideClarifies and documents seven SonarCloud security hotspots (all false positives) by adding targeted NOSONAR annotations and explanatory comments around regex usage, HTTP/FTP scheme guards, and a webhook test fixture path, without changing production behavior except for making a test fixture path more realistic and clearly non-/tmp-based. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Some of the inline NOSONAR comments (e.g., around
safe_postand the webhook tests) are quite verbose and repeat the same rationale; consider consolidating the explanation into a single, shorter comment per function/test and keeping the per-line NOSONAR notes minimal to preserve readability. - Where you add NOSONAR on entire function definitions (e.g.,
test_http_block/test_unsupported_scheme), you might be able to narrow the suppression to just the literal-bearing lines to reduce the chance of unintentionally hiding future issues in those functions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Some of the inline NOSONAR comments (e.g., around `safe_post` and the webhook tests) are quite verbose and repeat the same rationale; consider consolidating the explanation into a single, shorter comment per function/test and keeping the per-line NOSONAR notes minimal to preserve readability.
- Where you add NOSONAR on entire function definitions (e.g., `test_http_block` / `test_unsupported_scheme`), you might be able to narrow the suppression to just the literal-bearing lines to reduce the chance of unintentionally hiding future issues in those functions.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refines SonarCloud suppression comments across several files to provide more specific rule identifiers and better documentation for security-related logic. It also updates a test case to use a project-relative path instead of a temporary directory. Feedback includes suggestions to optimize the placement of suppression comments, refine test file paths to ensure robustness, and improve a regular expression to further mitigate potential ReDoS vulnerabilities.
I am having trouble creating individual review comments. Click here to see my feedback.
forgelm/_http.py (206)
The # NOSONAR comment should ideally be placed on the line that triggers the warning. While moving it to the f-string line clarifies the intent, ensure that SonarCloud correctly interprets it when it's part of a function call argument. Also, consider if using a constant for the error message would be cleaner.
f"http:// blocked (use https://); url={_mask_netloc(url)}" # NOSONAR python:S5332
tests/test_webhook.py (431)
Using a project-relative path like ./outputs/... is better than /tmp/..., but ensure that the directory ./outputs/run-r/ actually exists or is created during the test if any code ever attempts to resolve it, even if it's just a string in this specific test. This prevents potential FileNotFoundError in future test refactors.
model_path="outputs/run-r/final_model.staging",
tools/check_site_claims.py (240)
The regex r"^\s+([a-z][a-z0-9-]+-[a-z0-9-]+)\s{2,}" contains nested repetitions which can lead to polynomial backtracking (ReDoS) if the input is large and crafted. Although the input is 'controlled static HTML', it's safer to optimize the regex. For example, ensuring the hyphenated parts are more strictly defined or using atomic grouping if supported.
r"^\s+([a-z][a-z0-9]*-[a-z0-9-]+)\s{2,}", # NOSONAR python:S5852
Summary
Address all 7 Security Hotspots flagged by SonarCloud on PR #29
(
development → mainv0.5.5 consolidation), where the QualityGate is currently failing on C Security Rating + 7 hotspots.
After investigation each is a verified false positive — the
code is either the security guard itself, a test fixture
verifying the guard rejects insecure schemes, or a string argument
that is never written to disk. This PR corrects the in-code
documentation so a reviewer marking each hotspot as "Safe" in the
SonarCloud UI has the rationale on hand.
Hotspot-by-hotspot disposition
tools/check_site_claims.py:240forgelm/_http.py:202tests/test_webhook.py:235tests/test_webhook.py:238match=line + rule tagtests/test_webhook.py:256tests/test_webhook.py:419/tmp/stagedfixture replaced with./outputs/run-r/final_model.stagingWhy each is a false positive
controlled-inputper the existing NOSONAR comment); the polynomial-backtracking
risk is unreachable.
safe_postguard's REJECTION f-string. Naming the blockedscheme is intentional so operators see WHY their call failed.
test methods that verify the guard rejects those schemes.
pytest.raises(match="http://")regex patternmatching the error message, not an outbound call.
written. Replaced with the project-relative shape that
production code actually emits, which is also more faithful to
what production
model_pathvalues look like.Self-review gauntlet (all green at HEAD
755b909)ruff format --check .+ruff check .check_bilingual_parity --strictcheck_anchor_resolution --strictcheck_cli_help_consistency --strictcheck_site_claims --strictcheck_field_descriptions --strictpytest --no-cov -q tests/forgelm --config config_template.yaml --dry-runtests/test_webhook.py(targeted)tests/test_check_site_claims.py(targeted)Note for the SonarCloud review pass
Security Hotspots normally use SonarCloud's "Mark as Safe"
review workflow (not
# NOSONAR, which is for issues). Evenafter these in-code clarifications the maintainer may still need
to mark each of the 7 entries as Safe in the SonarCloud UI per
the standard hotspot-review process. The expanded in-code
comments make that UI review a 2-minute audit rather than a
re-derivation from scratch.
Once this PR merges to
development, PR #29's Sonar quality gateshould re-scan and either drop the C rating to A (if NOSONAR is
honoured for hotspots in the configured Sonar setup) OR leave the
hotspots in TO_REVIEW state for the maintainer to mark Safe.
🤖 Generated with Claude Code
Summary by Sourcery
Clarify and document SonarCloud security hotspots that are verified false positives so they can be reviewed and marked safe without altering runtime behavior.
Enhancements:
Tests: