Repository navigation
Conversation
Maffooch
left a comment
There was a problem hiding this comment.
Adversarial review: feat(sonarqube): title REST API JSON findings with the issue message
Verdict: request-changes
Reviewed at: a90b644 Profile: code-change (parser)
Repo rules followed: origin/dev .claude/skills/review-pr/SKILL.md, .claude/skills/defectdojo-parser/SKILL.md (dedup-change section), plus the generic review-pr skill
Attempts to break it
- Upgrade reimport, "SonarQube Scan" default dedup. Imported
issue_16115.jsonwith the pre-PR title (get_titlepatched torule_key), then reimported the same file with the PR code,close_old_findings=True. Result:new=1 closed=1. The BUG finding (id 9,Java:S2259_AZb-Issue-16115-3) was mitigated and re-created as id 10 with the message title. Same run onfindings_over_api.json:new=2 closed=3; the BUG and CODE_SMELL findings (ids 12, 13) were closed and re-created (ids 16, 17). Blocker.
Cause: the parser never setscwefor BUG, CODE_SMELL or hotspot findings, socwestays at the model default 0 (dojo/finding/models.py:121).HASHCODE_ALLOWS_NULL_CWE["SonarQube Scan"]isFalse(dojo/settings/settings.dist.py:1534), socompute_hash_codefalls back to the legacy hash (dojo/finding/models.py:858-862), which hashestitle + cwe + line + file_path + description(dojo/finding/models.py:899-900). Changing the title changeshash_codefor every BUG, CODE_SMELL and hotspot finding that has a non-empty message. - Control: fresh install, no upgrade. Import then reimport the same file, both with PR code.
new=0 closed=0 untouched=3. No break; the problem is only for data imported before the upgrade. - Upgrade reimport, "SonarQube Scan detailed". Same as attempt 1 with the detailed scan type (
DEDUPE_ALGO_UNIQUE_ID_FROM_TOOL,settings.dist.py:1723).new=0 closed=0. No break. Old titles are kept on matched findings. - Missing or null
message. Parsed a REST API JSON doc with the key absent and with"message": null, for BUG, VULNERABILITY, CODE_SMELL and a hotspot. Both PR and origin/dev raiseTypeError("**message:** " + messageatsonarqube_restapi_json.py:47, and"Category: CWE-" in messageat line 80). Reproduces on base: follow-up. Note that the newget_titlefallback comment ("when the report carries no message") is only reachable for"", never forNone. - Whitespace-only message (
" "). PR titles the finding" "(visually blank). Base gaver:1_k1. Introduced by the PR but low impact. Nit: usemessage.strip()in the emptiness check. - Length boundaries. 511-char message gives a 511-char title; 512 chars gives 510 (507 +
...). Both fitFinding.titlemax_length=511(dojo/finding/models.py:105) and matchdojo/tools/api_sonarqube/importer.py:187. No break. - Hostile input. Message
<script>x</script>, RLO mark, Hebrew, emoji: title kept verbatim and saved; output escaping is the template's job, same as SonarQube API Import. Message with\nproduces a multi-line title. No break; nit at most. - The tests themselves. The new test
test_parse_json_file_issue_16115_title_from_messageand the 9 changed assertions all assert the new titles, so they fail on base. No test covers the reimport/dedup behaviour that attempt 1 shows.
Blocking
dojo/tools/sonarqube/sonarqube_restapi_json.py:54,:208,:262(BUG, CODE_SMELL, hotspot branches): the title change altershash_codefor existing findings because these findings havecwe=0and "SonarQube Scan" falls back to the legacy, title-based hash. On the first reimport after upgrade, every such finding with a message is closed and re-created as a new finding (reproduced, attempt 1). Product-level dedup against older tests breaks the same way.docs/content/supported_tools/parsers/file/sonarqube.md(new "Finding Title" section) and the PR description state that the title is not part of the dedup hash and that only alegacydedup override is affected. That is wrong for the default config. Per the parser skill's dedup-change rule, this needs: an accurate upgrade note indocs/content/releases/os_upgrading/for the release it ships in, the PR description corrected, and ideally a design that does not churn existing findings (for example keep the old title on the hash path, setcwe-independent hashing, or make the new title opt-in).manage.py dedupe --hash_code_onlydoes not help here because the stored findings still carry the old title.
Nice to have
sonarqube_restapi_json.py:284: treat whitespace-only messages as empty (if not message or not message.strip()).sonarqube_restapi_json.py:282-283: the comment implies a missing message falls back torule_key, but a missing/null message crashes earlier at line 47 / 80 (pre-existing). Either guardmessagewithor ""at theissue.get("message")sites or fix the comment. Follow-up if left out of this PR.- Finding title-casing on save turns
"boolean"into"Boolean"inside the message (seen in attempt 1, id 16). Same as SonarQube API Import today; not this PR's to fix. - PR has no milestone set (repo checklist). Not set by the reviewer.
Verified hands-on
manage.py test unittests.tools.test_sonarqube_parserplus a local upgrade test (not part of the PR), in a compose stack on the PR head: 27 tests OK.- Upgrade-simulation reimports for "SonarQube Scan" (two fixtures) and "SonarQube Scan detailed" (attempts 1 to 3).
- Edge-case parsing on PR head and on origin/dev (attempts 4 to 7).
uvx [email protected] checkon the two changed Python files: all checks passed.scripts/check_migration_leaves.py: single leaf (no migrations in the PR).
Checked only statically
- Rendering of message titles in the UI (escaping of markup, RTL, newlines).
- Hotspot upgrade path: same
cwe=0legacy-hash path as BUG/CODE_SMELL by reading the code; the hands-on run covered BUG and CODE_SMELL only.
Still needs verification
- Required CI has not run: on the head SHA,
Ruff Linter,Unit tests,Migration Graphand the otherpull_requestworkflows areaction_required(fork workflows awaiting maintainer approval). They must be approved and pass before merge.
"SonarQube Scan" imports of /api/issues/search and /api/hotspots/search JSON titled findings as <rule>_<key>, while "SonarQube API Import" uses the SonarQube message. Use the message here too (truncated like the API importer), falling back to <rule>_<key> when the message is empty. The title is not part of the SonarQube Scan dedup hash, so existing findings still match on reimport and keep their old title. Closes DefectDojo#16115
SonarQube Scan falls back to the title-based legacy hash for findings without a CWE, so changing their title closed and re-created existing findings on reimport. Keep rule_key titles there and use the message only in SonarQube Scan detailed, which deduplicates on unique_id_from_tool. Treat missing, null and whitespace-only messages as empty instead of crashing.
016a892 to
6692470
Compare
|
Thanks for the thorough review. The blocker was right: for bugs, code smells and hotspots without a CWE, the hash falls back to the title-based legacy hash. I went with the opt-in route, in 6692470:
The 24 SonarQube parser tests pass, and the two new tests fail on |
|
This pull request contains multiple low-severity findings where user 'cvas22' modified files in sensitive codepaths without being on the allowed authors list. These changes affect various Python and HTML files within the 'dojo' directory but are not blocking.
Configured Sensitive Codepath Modified by Non-Allowed Author in
|
| Vulnerability | Configured Sensitive Codepath Modified by Non-Allowed Author |
|---|---|
| Description | File 'dojo/api_v2/exception_handler.py' matches configured sensitive codepath pattern 'dojo/api_v2/*.py' and was modified by 'cvas22' (commit fa7811f) who is not in the allowed authors list. |
Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/importers/base_importer.py (drs_5e2a07d6)
| Vulnerability | Configured Sensitive Codepath Modified by Non-Allowed Author |
|---|---|
| Description | File 'dojo/importers/base_importer.py' matches configured sensitive codepath pattern 'dojo/importers/*.py' and was modified by 'cvas22' (commit fa7811f) who is not in the allowed authors list. |
Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/importers/default_reimporter.py (drs_367e8d96)
| Vulnerability | Configured Sensitive Codepath Modified by Non-Allowed Author |
|---|---|
| Description | File 'dojo/importers/default_reimporter.py' matches configured sensitive codepath pattern 'dojo/importers/*.py' and was modified by 'cvas22' (commit fa7811f) who is not in the allowed authors list. |
Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/importers/import_lock.py (drs_9dae1cb9)
| Vulnerability | Configured Sensitive Codepath Modified by Non-Allowed Author |
|---|---|
| Description | File 'dojo/importers/import_lock.py' matches configured sensitive codepath pattern 'dojo/importers/*.py' and was modified by 'cvas22' (commit fa7811f) who is not in the allowed authors list. |
Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/tasks.py (drs_c78256ae)
| Vulnerability | Configured Sensitive Codepath Modified by Non-Allowed Author |
|---|---|
| Description | File 'dojo/tasks.py' matches configured sensitive codepath pattern 'dojo/tasks.py' and was modified by 'cvas22' (commit fa7811f) who is not in the allowed authors list. |
Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/templates/dojo/filter_js_snippet.html (drs_9b9bb00d)
| Vulnerability | Configured Sensitive Codepath Modified by Non-Allowed Author |
|---|---|
| Description | File 'dojo/templates/dojo/filter_js_snippet.html' matches configured sensitive codepath pattern 'dojo/templates/**/*.html' and was modified by 'cvas22' (commit fa7811f) who is not in the allowed authors list. |
Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/templates/dojo/findings_list_snippet.html (drs_f49776ff)
| Vulnerability | Configured Sensitive Codepath Modified by Non-Allowed Author |
|---|---|
| Description | File 'dojo/templates/dojo/findings_list_snippet.html' matches configured sensitive codepath pattern 'dojo/templates/**/*.html' and was modified by 'cvas22' (commit fa7811f) who is not in the allowed authors list. |
Configured Sensitive Codepath Modified by Non-Allowed Author in dojo/templates/dojo/view_test.html (drs_41690f8e)
| Vulnerability | Configured Sensitive Codepath Modified by Non-Allowed Author |
|---|---|
| Description | File 'dojo/templates/dojo/view_test.html' matches configured sensitive codepath pattern 'dojo/templates/**/*.html' and was modified by 'cvas22' (commit fa7811f) who is not in the allowed authors list. |
Comment to provide feedback on these findings.
Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]
Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing
All finding details can be found in the DryRun Security Dashboard.
Closes #16115.
Summary
When you import SonarQube REST API JSON (
/api/issues/search,/api/hotspots/search, or a zip of those files), each finding is titled<rule>_<key>, for exampletypescript:S1533_fjoiewfjoweifjoihugu-. SonarQube API Import titles the same issues with SonarQube'smessage. This PR gives SonarQube Scan detailed the message titles, and leaves SonarQube Scan as it is:message. A message longer than 511 characters is cut to 507 characters plus..., the same rule asdojo/tools/api_sonarqube/importer.py.messageis missing,null, empty or only whitespace, the title falls back to<rule>_<key>.rule,keyandmessageare still in the description, andunique_id_from_toolis unchanged.nullmessageno longer raisesTypeError. This crash also happens ondevtoday.Why only detailed mode
"SonarQube Scan" hashes on
cwe,severityandfile_path. The parser leavescweat 0 for bugs, code smells and hotspots, andHASHCODE_ALLOWS_NULL_CWE["SonarQube Scan"]isFalse, so for those findings the hash falls back to the legacy hash, which includes the title. Changing their title there would close and re-create every such finding on the first reimport after upgrading (see the review). So that mode keeps<rule>_<key>, and its hashes don't change."SonarQube Scan detailed" deduplicates on
unique_id_from_tool(the SonarQubekey), so the title isn't part of the match. On reimport, findings imported before the upgrade still match and keep their old title; new findings get the message title.Changes
dojo/tools/sonarqube/sonarqube_restapi_json.py: a newget_title()helper, used for all four finding types, returns the message only in detailed mode. Eachmessageread now defaults to"".unittests/tools/test_sonarqube_parser.py: two new tests, both failing ondev:test_detailed_parse_json_file_issue_16115_title_from_messagecovers a normal message, truncation, and the empty, whitespace, missing andnullfallbacks, for issues and hotspots.test_parse_json_file_issue_16115_keeps_rule_key_titlechecks that "SonarQube Scan" keeps<rule>_<key>for every type.The existing title assertions are unchanged.
unittests/scans/sonarqube/issue_16115.json: a small fabricated fixture.docs/content/supported_tools/parsers/file/sonarqube.md: a new "Finding Title" section explaining the behavior of each mode.No upgrade note: "SonarQube Scan" titles and hashes don't change, and detailed mode matches on
unique_id_from_tool.Test results
unittests.tools.test_sonarqube_parser: 24 tests OK in Docker.uvx [email protected] checkon the changed Python files: passed.dev.