Skip to content

Conversation

@vvolkgang
Copy link
Member

🎟️ Tracking

PM-28029

📔 Objective

Address SonarCube and zizmor findings, following similar work in bitwarden/android#6151

  • Removed unused dispatch-and-download action.
  • Removed secrets: inherit from _version.yml jobs.
  • Set default value for Patch version when repo doesn't have a tag.

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@vvolkgang vvolkgang requested a review from a team as a code owner November 10, 2025 21:18
@vvolkgang vvolkgang enabled auto-merge (squash) November 10, 2025 21:18
@claude
Copy link

claude bot commented Nov 10, 2025

Claude finished @vvolkgang's task —— View job


Code Review Complete

This PR addresses security and code quality findings from SonarCloud and zizmor by removing unused code and fixing workflow security issues.

Summary

The changes improve workflow security and remove dead code. The default value fix for _PATCH_VERSION is correct.


Findings

Finding 1 💭 (:thought_balloon:): Outdated documentation reference in _version.yml

The distinct_id input descriptions at lines 20 and 43 still reference "dispatch-and-download.yml" which has been deleted. While this doesn't break functionality, it may confuse future maintainers.

Context
distinct_id:
  description: "Unique ID for this dispatch, used by dispatch-and-download.yml"

The action file has been removed, but these descriptions remain.


Finding 2 ⚠️ (:warning:): Incomplete removal of secrets: inherit

The PR removes secrets: inherit from the version job calls in ci-bwa.yml:62 and ci-bwpm.yml:64, but secrets: inherit still exists in other jobs within the same files:

  • ci-bwa.yml:76 (build-manual job)
  • ci-bwa.yml:98 (build-public job)
  • ci-bwpm.yml:79 (build-manual job)
  • ci-bwpm.yml:106 (build-public job)

If the security concern identified by zizmor applies to workflow calls that don't need secrets, these additional instances should also be reviewed and potentially removed if those jobs don't require inherited secrets.


Action Items

  1. Update documentation (Finding 1): Consider updating or removing the distinct_id parameter descriptions that reference the deleted action file.

  2. Review remaining secrets: inherit (Finding 2): Verify whether the remaining secrets: inherit declarations in the build jobs are necessary. If they're not needed for security reasons similar to the version job, consider removing them as well.


latest_tag_version=$(git tag -l --sort=-creatordate | grep "$APP_CODENAME" | head -n 1)
if [[ -z "$latest_tag_version" ]]; then
version_name="${current_year}.${current_month}.${_PATCH_VERSION}"
version_name="${current_year}.${current_month}.${_PATCH_VERSION:-0}"
Copy link

Choose a reason for hiding this comment

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

💭 Note: The distinct_id parameter descriptions at lines 20 and 43 still reference "dispatch-and-download.yml" which was deleted. Consider updating those descriptions in a follow-up to avoid confusion.

base_version_number: 30
version_name: ${{ inputs.version-name }}
version_number: ${{ inputs.version-number }}
patch_version: ${{ inputs.patch_version && '999' || '' }}
Copy link

Choose a reason for hiding this comment

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

⚠️ Good removal here for the version job. However, secrets: inherit still exists in other jobs in this file (lines 76 and 98). If the zizmor finding indicates that secrets shouldn't be inherited when not needed, those may also warrant review.

base_version_number: 2300
version_name: ${{ inputs.version-name }}
version_number: ${{ inputs.version-number }}
patch_version: ${{ inputs.patch_version && '999' || '' }}
Copy link

Choose a reason for hiding this comment

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

⚠️ Good removal here for the version job. However, secrets: inherit still exists in other jobs in this file (lines 79 and 106). If the zizmor finding indicates that secrets shouldn't be inherited when not needed, those may also warrant review.

@github-actions
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Detailsdc8770d0-759f-4bce-af1c-a41d55766a15

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Nov 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.77%. Comparing base (ca6c90d) to head (86619b4).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2124      +/-   ##
==========================================
- Coverage   85.22%   83.77%   -1.45%     
==========================================
  Files        1708     1971     +263     
  Lines      145420   160924   +15504     
==========================================
+ Hits       123940   134822   +10882     
- Misses      21480    26102    +4622     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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