Skip to content

feat: include DM001 findings in SARIF output#888

Open
rksharma-owg wants to merge 2 commits into
OWASP:mainfrom
rksharma-owg:codex/fix-sarif-maintenance
Open

feat: include DM001 findings in SARIF output#888
rksharma-owg wants to merge 2 commits into
OWASP:mainfrom
rksharma-owg:codex/fix-sarif-maintenance

Conversation

@rksharma-owg

Copy link
Copy Markdown

Problem\n

audit\n\n- Fix issue #881: maintenance findings from --check-maintenance were omitted from SARIF output, so Code Scanning could miss DM001 risk signals.\n\n## Root cause\n- write-outputs only passed vulnerabilities and override findings into writeSarifReport.\n- buildSarifOutput only converted vulnerability and override findings.\n\n## Fix\n- Added SARIF support for maintenance findings using DM001 rules.\n- Wired maintenanceFindings to writeSarifReport and writeOutputs.\n- Added unit tests for maintenance SARIF transformation and write-output call wiring.\n\n## Validation\n- Targeted unit tests are added in tests/sarif.test.ts, tests/output/maintenance-findings-sarif.test.ts, and tests/write-outputs.test.ts.\n- Full test run is currently blocked by pre-existing repo typing issue in src/utils/network.ts during build (Property 'preconnect' is missing in type ...), unrelated to this change.

@rksharma-owg
rksharma-owg requested a review from sonukapoor as a code owner July 25, 2026 01:21
@rksharma-owg
rksharma-owg force-pushed the codex/fix-sarif-maintenance branch 2 times, most recently from ead6012 to 598ccae Compare July 25, 2026 01:39

@sonukapoor sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rksharma-owg - #881 is a real gap (SARIF was the one output surface still missing the DM001 findings), and threading maintenanceFindings through to buildSarifOutput mirrors the existing JSON path correctly.

A few things before merge:

  1. DM001 ends up registered twice in the SARIF: once in tool.driver.rules (via the ruleMap injection in sarif.ts) and again as an orphaned copy in tool.extensions[] with diverging metadata, and buildMaintenanceSarifComponent() gets called twice. The existing override-findings-sarif.ts uses the extension-component pattern deliberately (there is a comment explaining why Code Scanning needs it) - could you pick one registration path and match that precedent so DM001 is defined once?

  2. There is no test with both override and maintenance findings present at once - that is the one case where the extensions-array ordering matters (override hardcodes toolComponent.index: 0). Worth adding.

  3. Same override commit as your other PRs - please drop the overrides: brace-expansion + jest bump (already handled via the baseline in #897) and rebase.

The core fix is sound; these are about matching the existing SARIF pattern and dropping the unrelated bits.

@rksharma-owg
rksharma-owg force-pushed the codex/fix-sarif-maintenance branch from 598ccae to b730754 Compare July 26, 2026 19:31
@rksharma-owg

Copy link
Copy Markdown
Author

Thanks for the review. I rebased onto current main, removed the unrelated dependency commit, and changed DM001 to use only the maintenance extension component. Maintenance results now carry the matching toolComponent.index; when override and maintenance findings are both present, their references point to extension indices 0 and 1 respectively. I added mixed-result coverage for that ordering.

Validation:

  • SARIF and output focused tests — 37/37 passed
  • git diff --check — passed

The local full build still encounters the unrelated typeof fetch/preconnect error tracked in #885 and #886. CI is running on commit b730754.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants