Address medium issues in SonarQube - #3835
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3835 +/- ##
==========================================
+ Coverage 79.30% 79.31% +0.01%
==========================================
Files 896 896
Lines 67371 67371
Branches 2608 2659 +51
==========================================
+ Hits 53426 53436 +10
+ Misses 13276 13263 -13
- Partials 669 672 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped to security hardening, consistent across workflows/scripts, and don’t introduce detectable functional regressions in the modified logic.
Pull request overview
This PR hardens CI and shell-script downloads to satisfy SonarQube security guidance by (1) preventing Python dependency installs from building sdists during GitHub Actions runs and (2) constraining curl -L redirects to HTTPS-only.
Changes:
- Update
curlinvocations that downloadcacert.pemto enforce HTTPS-only protocol use even when redirects are followed. - Update multiple GitHub Actions workflows to run
uv syncwith--no-build(and install the local project separately) to avoid executing third-party build/setup scripts. - Switch
uv runusage from--frozento--no-syncin these workflows, relying on the prior frozen sync step for reproducibility.
File summaries
| File | Description |
|---|---|
| scripts/release/build_multi_arch.sh | Restricts curl -L protocol to HTTPS when downloading cacert.pem. |
| scripts/linux/openc3_setup.sh | Restricts curl -L protocol to HTTPS when downloading cacert.pem. |
| .github/workflows/tsdb_integration_tests.yml | Uses uv sync --no-build --no-install-project + editable install; runs tests with uv run --no-sync. |
| .github/workflows/python_unit_tests.yml | Uses uv sync --no-build --no-install-project + editable install; runs coverage with uv run --no-sync. |
| .github/workflows/python_lint.yml | Uses uv sync --no-build --no-install-project + editable install; runs ruff with uv run --no-sync. |
| .github/workflows/api_tests.yml | Uses uv sync --no-build --no-install-project + editable install; runs coverage with uv run --no-sync. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Restore the uv sync/run defaults in the api, unit test, and tsdb workflows: those jobs import and execute the locked dependencies under pytest, so blocking build scripts is not a security boundary, and --no-build broke the editable openc3 install outright. Keep the flag in python_lint, where ruff needs no project install, as a tripwire for sdist-only dependencies. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
jmthomas
changed the title
| # the locked dependencies without the openc3 project itself (--no-install-project) | ||
| # and forbids building any source distribution (--no-build). The ruff steps then | ||
| # use --no-sync so they run against exactly what was installed here instead of | ||
| # re-syncing (a re-sync would try to build the skipped project and fail). |
There was a problem hiding this comment.
A re-sync would not fail here: uv run carries no --no-build, so uv run --frozen succeeds and quietly reinstalls openc3, undoing --no-install-project.
--no-sync is still the right flag — just for that reason rather than a build failure.
There was a problem hiding this comment.
We've got a quite a few sonarqube medium issues around omitting --no-build for example. Should these be marked as non-issues or should we have --no-sync and --no-build flags together?
|
I'll keep an eye on these |
What changed
Update .github/workflows/python_lint.yml from to avoid build since we're just doing linting. Currently every package in uv.lock ships a wheel so --no-build is a no-op. If a future package ships a sdist-only then we fail and have to evaluate it.
Add --proto =https to a few curl actions in shell scripts
Why it changed
Various sonarqube warnings. The curl one: "Not enforcing HTTPS here might allow for redirections to insecure websites." is because we add -L to curl which allows redirects.
Testing strategy
Ran setup in the shell script to download the cacert file to verify curl. --no-build is tested through the github actions.