From 755b90982e4d33d353b8e40b9bf729c425b6a6e4 Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Wed, 6 May 2026 20:11:19 +0300 Subject: [PATCH] fix(sonar-hotspots-pr29): align NOSONAR markers + drop /tmp test fixture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- forgelm/_http.py | 8 ++++++-- tests/test_webhook.py | 28 ++++++++++++++++++++-------- tools/check_site_claims.py | 6 ++++-- 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/forgelm/_http.py b/forgelm/_http.py index b9dd5ad9..787aa919 100644 --- a/forgelm/_http.py +++ b/forgelm/_http.py @@ -198,8 +198,12 @@ def safe_post( # Scheme policy. if parsed.scheme == "http": if not allow_insecure_http: - raise HttpSafetyError( # NOSONAR — error string mentions "http://" by design (operator-facing rejection message) - f"http:// blocked (use https://); url={_mask_netloc(url)}" + # The rejection message intentionally names the blocked scheme so + # operators see WHY the call failed. This is the security guard + # itself, not an insecure call site — Sonar S5332 is the inverse + # of the actual semantic. + raise HttpSafetyError( + f"http:// blocked (use https://); url={_mask_netloc(url)}" # NOSONAR python:S5332 — operator-facing rejection message; this code BLOCKS the insecure scheme ) elif parsed.scheme != "https": raise HttpSafetyError(f"Unsupported URL scheme {parsed.scheme!r}; only http(s) allowed.") diff --git a/tests/test_webhook.py b/tests/test_webhook.py index a7c5ced7..fe71437c 100644 --- a/tests/test_webhook.py +++ b/tests/test_webhook.py @@ -231,12 +231,18 @@ def test_redirect_block(self): kwargs = mock_post.call_args.kwargs assert kwargs["allow_redirects"] is False - def test_http_block(self): - """http:// URLs are rejected unless allow_insecure_http is set.""" + def test_http_block( + self, + ): # NOSONAR python:S5332 — docstring describes the security guard; no insecure call is made + """Verify plain-HTTP URLs are rejected unless allow_insecure_http is set.""" from forgelm._http import HttpSafetyError, safe_post - with pytest.raises(HttpSafetyError, match="http://"): - safe_post("http://example.com/hook", json={}, timeout=10.0) # NOSONAR scheme blocker fixture + with pytest.raises( + HttpSafetyError, match="http://" + ): # NOSONAR python:S5332 — match-regex literal, not an outbound call + safe_post( + "http://example.com/hook", json={}, timeout=10.0 + ) # NOSONAR python:S5332 — scheme blocker fixture; verifies the guard rejects http:// def test_http_allowed_with_opt_in(self): """allow_insecure_http=True (used by webhook) lets http:// through.""" @@ -252,13 +258,15 @@ def test_http_allowed_with_opt_in(self): ) mock_post.assert_called_once() - def test_unsupported_scheme(self): - """ftp:// / file:// / etc. are rejected even with allow_insecure_http.""" + def test_unsupported_scheme( + self, + ): # NOSONAR python:S5332 — docstring names the rejected schemes; no insecure call is made + """Verify non-http(s) schemes (e.g., ftp, file) are rejected even with allow_insecure_http.""" from forgelm._http import HttpSafetyError, safe_post with pytest.raises(HttpSafetyError, match="Unsupported URL scheme"): safe_post( - "ftp://example.com/hook", # NOSONAR scheme blocker fixture (ftp not allowed) + "ftp://example.com/hook", # NOSONAR python:S5332 — scheme blocker fixture; verifies the guard rejects ftp:// json={}, timeout=10.0, allow_insecure_http=True, @@ -416,7 +424,11 @@ def test_notify_awaiting_approval_no_model_weights_in_payload(self, mock_post): notifier.notify_awaiting_approval( run_name="r", - model_path="/tmp/staged", + # Path string is sent as a webhook field; nothing is written to + # the filesystem in this test, so the publicly-writable-tmp + # concern (Sonar S5443) does not apply. Use a project-relative + # placeholder shape to keep the test honest about that. + model_path="./outputs/run-r/final_model.staging", ) call_kwargs = mock_post.call_args diff --git a/tools/check_site_claims.py b/tools/check_site_claims.py index 578ac614..607c50b9 100644 --- a/tools/check_site_claims.py +++ b/tools/check_site_claims.py @@ -237,8 +237,10 @@ def site_template_names() -> set[str]: # Add any standalone slug that appears in the snippet body (e.g. the # bulleted list of templates in 
).
     for match in re.finditer(
-        r"^\s+([a-z][a-z0-9-]+-[a-z0-9-]+)\s{2,}", cleaned, flags=re.MULTILINE
-    ):  # NOSONAR — applied to controlled static HTML, not user input; ReDoS risk is not exploitable here
+        r"^\s+([a-z][a-z0-9-]+-[a-z0-9-]+)\s{2,}",  # NOSONAR python:S5852 — controlled static HTML input, not user-facing; ReDoS not exploitable
+        cleaned,
+        flags=re.MULTILINE,
+    ):
         found.add(match.group(1))
     return found