Repository navigation
ci: add CodeQL + SonarQube Cloud security scanning - #1
Conversation
Adds the AMMOS scanning pipeline already running on astria_ui: CodeQL -> nasa-scrub translate_results -> fix_sarif_paths.py -> SonarQube Cloud fix_sarif_paths.py is copied verbatim from astria_ui; it repairs the file paths and rule metadata that nasa-scrub degrades, without which every finding imports unlocated and at MEDIUM severity. CodeQL runs with build-mode: none and SonarQube's Java analyzer runs without bytecode, because this project's gov.nasa.jpl.ammos.ids dependencies live in JPL Artifactory and cannot be resolved from a public runner. Both still produce findings, with reduced precision on anything needing resolved types. While the pipeline is validated, the workflow triggers on a push to this branch and pins -Dsonar.branch.name=main, so test runs land on main's analysis instead of creating a second branch in the SonarQube project. Both are flagged in-file for removal once the results look right. Claude-Session: https://claude.ai/code/session_011WeGYwtRQbXf1wJE7YzvYW
Brings this workflow in line with the MMGIS security-scan setup and retires the bootstrap triggers used to validate the pipeline: - push now triggers on main only, not on the feature branch - pull_request triggers on any PR targeting main, from any source branch - concurrency group per ref, cancelling superseded in-progress PR runs but never cancelling a run on main - the job is skipped for PRs from forks, which have no SONAR_TOKEN and would otherwise fail red for a reason the contributor cannot fix Branch attribution is now left to the scanner's GitHub Actions auto-detection: a push to main analyzes as main, and a PR analyzes as a SonarQube pull request, which expires with the PR rather than becoming a long-lived branch in the project. Only workflow_dispatch still pins -Dsonar.branch.name=main, so validating the pipeline by hand cannot leave a stray branch behind. Claude-Session: https://claude.ai/code/session_011WeGYwtRQbXf1wJE7YzvYW
| @@ -0,0 +1,295 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
this seems a bit excessive to commit here but whatever
| # images into DZI tile pyramids. All of our code lives under src/main/java. | ||
| # | ||
| # src/main/java/gov/nist/isg/pyramidio/ is deliberately NOT excluded. It is a | ||
| # local fork of NIST's pyramidio that is compiled into our artifact; the upstream |
There was a problem hiding this comment.
you/your AI is very correct, at least for now:
the JPL-internal
marsandvicariodependencies are not publicly resolvable, so CI cannot compile
| # | ||
| # If this ever runs on a runner with Artifactory access, build first | ||
| # (mvn -B -DskipTests package) and this property will pick up the real classes | ||
| # with no further change. |
There was a problem hiding this comment.
what does "build first" mean exactly? my question is: we do expect to soon have the missing mars.jar and vicario.jar dependencies publicly available, and that should un-break the build of tile_service. At that time, will we need to maintain this file?
There was a problem hiding this comment.
Good question. The answer is now no: nothing needs to be maintained. As of ff89f39/a4e3c26 the Security-Scan workflow decides on every run whether to build. It runs mvn dependency:resolve against the public repositories:
- If everything resolves, CodeQL switches to
build-mode: manual, the workflow runsmvn -DskipTests compile, and SonarQube gets the realtarget/classes. - If anything fails to resolve, it falls back to the current source-only scan.
Today the probe fails on mars. Once mars is published (vicario already is, via #3), scans pick up the full build automatically, and this file and the workflow stay as they are. I also removed the JPL Artifactory references from the comments.
There was a problem hiding this comment.
Correction to my reply above. I said the probe "fails on mars", but I didn't actually see that: I couldn't read the probe step's log, and my local run failed on Maven Central rate limits instead. I also hear mars is being split up and replaced. The mechanism doesn't depend on which artifact is missing. It falls back to source-only whenever any dependency fails to resolve, and that includes network errors or rate limits. The fallback only posts a notice; it doesn't fail the check. I've reworded the comments in 61942d6 so they don't name a specific dependency.
The Security-Scan workflow now probes whether every pom.xml dependency resolves from public repositories. If so, CodeQL runs with build-mode manual and the project is compiled, giving SonarQube real bytecode; if not, it falls back to build-mode none as before. Once mars is published the full build turns on with no change to this pipeline. Also drops the JPL Artifactory references from the comments. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if force_level or 'level' not in result: | ||
| level = (rules[index].get('defaultConfiguration') or {}).get('level') | ||
| if level: | ||
| result['level'] = level |
There was a problem hiding this comment.
🟡 Explicit alert severity lost during restoration
When a CodeQL result overrides its rule's severity, restore_rules replaces the result's level with the rule default. The reference result retains the override, but SonarQube imports the wrong severity.
Learn more
SARIF results can set their own level, overriding the rule's defaultConfiguration.level. The workflow passes the raw report to restore_rules as a reference, but restoration only reads its rules. With force_level=True, apply_rules overwrites every scrubbed result level with the rule default, even when the raw result specified a different level. SonarQube then receives an incorrect alert severity.
Example: A raw result for java/example explicitly has level: error, while its rule defaults to warning. Restoration writes warning into the processed result instead of preserving error.
Recommended fix: Recover explicit result levels from the reference by matching results using a stable identifier such as fingerprints and rule/location, and use rule defaults only for results without an explicit raw level. Avoid relying on array position if scrub can reorder results.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Not changing this here. force_level is deliberate: scrub writes a flat "warning" onto every result, so the scrubbed level can't be trusted, and raw CodeQL results normally inherit their level from the rule. Recovering a true per-result override would mean matching results across the raw and scrubbed reports, which the script's docstring already calls fragile. Also, fix_sarif_paths.py is shared verbatim with astria_ui and mis_rest_service, so any change should land in all three together rather than in this PR.
Split Security-Scan into a codeql job (probe, build, analyze, repair SARIF; no secrets) and a sonarqube job that only checks out the sources, downloads the SARIF and compiled classes as artifacts, and runs the scanner. Build plugins and dependencies never run alongside the token. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
What
Adds the AMMOS code vulnerability scanning pipeline:
Following the AMMOS scanning guide.
.github/workflows/security-scan.yml.github/codeql/codeql-config.yml.github/scripts/fix_sarif_paths.pynasa-scrubdegrades.github/scripts/test_fix_sarif_paths.pysonar-project.propertiesWhy the SARIF repair step exists
nasa-scrub'stranslate_resultsis the step the AMMOS guide prescribes, and it is kept for compliance, but it is lossy. It rewrites CodeQL's workspace-relative URIs to absolute paths and writes the absolute source root intouriBaseId, which stops SonarQube mapping findings to files. It also deletestool.extensions— where CodeQL keeps its real rule metadata — and rebuildstool.driver.rulesas id-only stubs, so every finding imports as MEDIUM regardless of its true severity.fix_sarif_paths.pyrestores both from the pre-scrub report. It is shared verbatim acrossastria_ui,mis_rest_serviceandtile_service— fix it in one, copy to all.Triggers
main— analyzed asmainmain, from any source branch — analyzed as a SonarQube pull request, which expires with the PR and never becomes a long-lived branch in the projectworkflow_dispatch— pins-Dsonar.branch.name=mainso validating the pipeline by hand cannot leave a stray branch behindPRs from forks are skipped: they run without repository secrets, so
SONAR_TOKENwould be empty and the scan would fail red for a reason the contributor cannot fix.A concurrency group per ref supersedes in-progress PR runs when new commits arrive, and never cancels a run on
main.Build mode: automatic
Each run first tries
mvn dependency:resolveagainst public repositories:While any dependency in
pom.xmlis not yet publicly available, the probe falls back to source-only (vicariois now on Maven Central, see #3). Once every dependency is published, scans move to the full build on their own. Nothing in the workflow orsonar-project.propertieshas to change. Note: the fallback is taken on any probe failure, including network errors or Maven Central rate limits, and only posts a notice rather than failing the check.Two jobs, so SONAR_TOKEN never shares a job with Maven
codeql(no secrets;security-events: write): probe, build, CodeQL analyze, scrub +fix_sarif_paths.py. Uploads the SARIF ascodeql-artifacts, plustarget/classesasclasseswhen it built.sonarqube(needs: codeql): checkout, then download those artifacts and run the scanner withSONAR_TOKEN. It never runs Maven or any project code.Verified
A scan of this branch pinned to
mainran green and imported cleanly (single-job version):external_CodeQL(notexternal_java) —driver.namerestoredmainonlyThe PR run with the two-job layout was green:
java_processed.sarifpassed between jobs,1 run(s) successfully imported.Link to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/86093dc594e44c788862ddb10afd3903
Open in Devin Desktop: https://nasa-jpl-demo.devinenterprise.com/desktop/session/86093dc594e44c788862ddb10afd3903?variant=devin
Requested by: @jl-0