Implement SARIF 2.1.0 conversion for AquaSec scan results #9
Conversation
Co-authored-by: tmikula-dev <[email protected]>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the
WalkthroughThis PR implements Step 3—SARIF conversion—adding a SarifConvertor class that transforms AquaSec JSON findings into SARIF 2.1.0 format. Changes include new conversion logic, utility functions, main workflow integration, comprehensive test coverage, and documentation updates. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Workflow
participant Main as main.py
participant Fetcher as ScanFetcher
participant Convertor as SarifConvertor
participant FileIO as File I/O
participant Output as Action Output
Client->>Main: Run with auth/config
Main->>Fetcher: fetch_findings()
Fetcher-->>Main: findings_json (dict)
Main->>Convertor: SarifConvertor(findings_json)
Main->>Convertor: convert_to_sarif()
Convertor->>Convertor: Build rules (map severity, truncate text)
Convertor->>Convertor: Build findings (link rules, add locations)
Convertor-->>Main: sarif_dict
Main->>FileIO: open & write SARIF JSON
FileIO-->>Main: file written
Main->>Output: set_action_output(aquasec_sarif_file, path)
Output-->>Client: SARIF path exposed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@DEVELOPER.md`:
- Line 111: Update the example documentation lines that reference the SARIF
converter to use the correct path including the model subdirectory: replace
occurrences of "src/sarif_convertor.py" with "src/model/sarif_convertor.py"
(references relate to the SARIF converter file sarif_convertor.py mentioned in
DEVELOPER.md), and ensure the same fix is applied to all instances flagged (the
examples at the three noted locations).
- Around line 210-215: Update the example pytest commands in DEVELOPER.md to
reference the correct test file paths under the model subdirectory; replace
occurrences of "tests/test_sarif_convertor.py" with
"tests/model/test_sarif_convertor.py" (and the node id example accordingly) so
the shown commands (e.g., the full-file run and the node-id run "pytest
tests/model/test_sarif_convertor.py::test_convert_to_sarif_returns_valid_structure
-q") point to the actual test locations.
🧹 Nitpick comments (6)
.github/workflows/check_python.yml (1)
142-145: Minor inconsistency: missing step name for Python setup.Other jobs in this workflow use
name: Set up Pythonfor the setup-python step. Consider adding a name for consistency.Suggested fix
- - uses: actions/setup-python@83679a892e2d95755f2dac6acb0bfd1e9ac5d548 + - name: Set up Python + uses: actions/setup-python@83679a892e2d95755f2dac6acb0bfd1e9ac5d548 with: python-version: '3.14' cache: 'pip'main.py (1)
53-53: Redundant exception object inlogging.exceptioncalls.
logging.exceptionautomatically includes exception information in the log output, makingstr(e)redundant.Suggested fix
except (ValueError, RequestException) as e: - logger.exception("Authentication failed: %s", str(e)) + logger.exception("Authentication failed.") sys.exit(1)except (ValueError, RequestException) as e: - logger.exception("Fetching scan results failed: %s", str(e)) + logger.exception("Fetching scan results failed.") sys.exit(1)except IOError as e: - logger.exception("Failed to convert and write SARIF file: %s", str(e)) + logger.exception("Failed to convert and write SARIF file.") sys.exit(1)Also applies to: 60-60, 72-72
tests/model/test_sarif_convertor.py (1)
264-288: Good coverage of helper methods. Consider adding edge case tests for unknown severity values.The tests for
_map_severity_to_level,_map_severity_to_score, and_get_severity_tagcover the defined severity levels (1-4), but don't verify the default/fallback behavior for out-of-range orNonevalues.🧪 Optional: Add tests for edge case severity values
def test_map_severity_to_level_handles_unknown(): assert "note" == SarifConvertor._map_severity_to_level(0) assert "note" == SarifConvertor._map_severity_to_level(-1) assert "error" == SarifConvertor._map_severity_to_level(5) # >= 3 def test_map_severity_to_score_handles_unknown(): assert "2.0" == SarifConvertor._map_severity_to_score(0) assert "2.0" == SarifConvertor._map_severity_to_score(99) def test_get_severity_tag_handles_unknown(): assert "UNKNOWN" == SarifConvertor._get_severity_tag(0) assert "UNKNOWN" == SarifConvertor._get_severity_tag(99)src/model/sarif_convertor.py (3)
139-152: Empty strings in help text will produce blank lines.When
cweorremediationare empty, the conditional expressions produce empty strings that remain inhelp_text_parts, resulting in blank lines in the final help text.♻️ Optional: Filter empty strings before joining
help_text_parts = [ f"Misconfiguration {rule_id}", f"Type: {category}", f"Severity: {severity_tag}", f"Check: {title}", f"Message: {message}", - f"CWE: {cwe}" if cwe else "", - f"Remediation: {remediation}" if remediation else "", ] + if cwe: + help_text_parts.append(f"CWE: {cwe}") + if remediation: + help_text_parts.append(f"Remediation: {remediation}") if references: help_text_parts.append(f"Link: [{rule_id}]({references[0]})") - help_text = "\n".join(help_text_parts) + help_text = "\n".join(part for part in help_text_parts if part)
292-292: Minor typo: "SARF" should be "SARIF".📝 Fix typo
- # Build SARF structure + # Build SARIF structure
117-118: Inconsistentstr()casting forrule_idtruncation.In
_build_rule, therule_idis truncated without casting tostrfirst (line 118), whereas inconvert_to_sarif(lines 275, 288), it's explicitly cast withstr(rule_id). For consistency and defensive coding, consider applying the same pattern here.♻️ Add str() cast for consistency
rule_id = finding.get("avd_id", SARIF_PLACEHOLDER) - rule_id = self._truncate_text(rule_id, RULE_ID_MAX_LENGTH) + rule_id = self._truncate_text(str(rule_id), RULE_ID_MAX_LENGTH)Apply the same change in
_build_sarif_findingat line 185:rule_id = finding_json.get("avd_id", SARIF_PLACEHOLDER) - rule_id = self._truncate_text(rule_id, RULE_ID_MAX_LENGTH) + rule_id = self._truncate_text(str(rule_id), RULE_ID_MAX_LENGTH)
tmikula-dev
left a comment
tmikula-dev
left a comment
There was a problem hiding this comment.
This PR was refactored and is approved by myself for other human review.
Overview
Adds SARIF conversion pipeline to enable GitHub Security tab integration. AquaSec findings are now converted to SARIF 2.1.0 format with proper severity mapping and field truncation per spec.
Release Notes
sarif-fileoutput for GitHub code scanning integrationRelated
Closes #8
Implementation
src/model/convertor.pyConvertorclass handles AquaSec→SARIF transformationmain.pyaquasec_scan.sarifto working directorysarif_fileoutputaction.ymlsarif-fileoutputsrc/utils/constants.pyExample SARIF output:
{ "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json", "version": "2.1.0", "runs": [{ "tool": { "driver": { "name": "AquaSec", "rules": [{"id": "CVE-2021-12345", ...}] } }, "results": [{ "ruleId": "CVE-2021-12345", "level": "error", "message": {"text": "SQL injection - Use parameterized queries"}, "locations": [{"physicalLocation": {"artifactLocation": {"uri": "src/db.py"}}}] }] }] }Original prompt
This pull request was created from Copilot chat.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.