Cleanup of workflow files for Zizmor changes

[mandreko-bitwarden]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/VULN-285

📔 Objective

Includes cleanup of workflow files to be compliant with zizmor and bwwl.

⏰ 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

[github-actions]

Checkmarx One – Scan Summary & Details – 0e7af06b-1cc1-44c7-9589-f1d066806403

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

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[withinfocus]

Very quick pass through most of the content and I focused on the new Action itself.

[withinfocus]

Looking good to me -- might want to sync with BRE before a full review with them.

[fntyler]

Overall looks good, just had a few ⛏️ s. Another set of 👀 from a BRE member would probably be worth it.

[claude]

Claude finished @mandreko-bitwarden's task —— View job

---

PR Review: Workflow Cleanup for Zizmor Compliance

Summary

This PR successfully implements workflow cleanup changes for zizmor and bwwl (Bitwarden Workflow Linter) compliance. The changes primarily focus on:

1. Security improvements: Adding persist-credentials: false to checkout actions
2. Shell script hardening: Proper quoting of variables and arithmetic evaluation
3. Code formatting: Prettier-driven formatting updates across YAML files
4. Quote consistency: Converting single quotes to double quotes in YAML

Overall Assessment: The PR is in good shape with most issues addressed. However, there is one critical issue that needs immediate attention before merging.

---

Critical Issues

❌ 1. Missing Environment Variables in check-run.yml

File: .github/workflows/check-run.yml:32-34

Issue: The environment variable definitions for GITHUB_TRIGGERING_ACTOR and GITHUB_ACTOR were removed in commit 83a3732, but the shell script still references them.

Problem: While github.triggering_actor and github.actor are available as GitHub context variables in workflow expressions, they are NOT automatically available as shell environment variables. The current code will print empty values in the error message.

Current code:

- name: Check user permission
  if: |
    steps.get-permission.outputs.require-result == 'false'
    && github.triggering_actor != 'bw-ghapp[bot]'
  env:
    USER_PERMISSIONS: ${{ steps.get-permission.outputs.user-permission }}
  run: |
    echo "User ${GITHUB_TRIGGERING_ACTOR} does not have the necessary access for this repository."
    echo "Current permission level is ${USER_PERMISSIONS}."
    echo "Job originally triggered by ${GITHUB_ACTOR}."
    exit 1

Fix Required:

- name: Check user permission
  if: |
    steps.get-permission.outputs.require-result == 'false'
    && github.triggering_actor != 'bw-ghapp[bot]'
  env:
    GITHUB_TRIGGERING_ACTOR: ${{ github.triggering_actor }}
    GITHUB_ACTOR: ${{ github.actor }}
    USER_PERMISSIONS: ${{ steps.get-permission.outputs.user-permission }}
  run: |
    echo "User ${GITHUB_TRIGGERING_ACTOR} does not have the necessary access for this repository."
    echo "Current permission level is ${USER_PERMISSIONS}."
    echo "Job originally triggered by ${GITHUB_ACTOR}."
    exit 1

Note: The author's comment suggests this is a false positive because these are GitHub runner variables. However, this is incorrect. GitHub context variables like github.triggering_actor are only available in workflow expressions (the ${{ }} syntax), not as shell environment variables. They must be explicitly passed to the env block to be available in shell scripts.

---

Suggested Improvements

🎨 1. Error Handling in _ephemeral_environment_manager.yml

File: .github/workflows/_ephemeral_environment_manager.yml:193

Current code:

APP_NAME=$(argocd app list -o name | grep "${PR_NUMBER}")

Issue: If no ArgoCD application matches the PR number, grep will fail and cause the workflow to exit unexpectedly without a clear error message.

Suggested improvement:

APP_NAME=$(argocd app list -o name | grep "${PR_NUMBER}" || true)

if [ -z "$APP_NAME" ]; then
  echo "::error::No ArgoCD application found for PR ${PR_NUMBER}"
  exit 1
fi

This provides clearer error messages when the expected application doesn't exist.

🎨 2. Multiple ArgoCD Application Matches

File: .github/workflows/_ephemeral_environment_manager.yml:193

Issue: If multiple ArgoCD applications contain the same PR number in their names, grep could return multiple lines, causing subsequent commands to fail.

Suggested improvement:

APP_NAME=$(argocd app list -o name | grep "${PR_NUMBER}" | head -1)

This ensures only one match is used, making the behavior more predictable.

---

Status of Previously Identified Issues

✅ Fixed: Syntax Error in _ephemeral_environment_manager.yml

Previous Issue: Line 193 had mismatched braces ${PR_NUMBER})

Status: RESOLVED - The current code correctly uses "${PR_NUMBER}" with proper quoting and matching braces.

---

Good Practices Observed

• Security hardening: Consistent use of persist-credentials: false across checkout actions
• Shell variable quoting: Proper quoting of variables (e.g., "$VERSION", "${PR_NUMBER}")
• Arithmetic evaluation: Improved use of [[ $tag_count -gt 1 ]] for integer comparisons in _version.yml:67
• Error handling: Good use of set -euo pipefail in _sonar.yml:106 for safer bash scripts
• Workflow linter compliance: All changes align with bwwl rules (permissions, pinned actions, naming conventions)
• Consistent formatting: Prettier has been applied consistently across all YAML files

---

Additional Observations

Shell Script Improvements

The PR includes several good shell scripting improvements:

1. Integer arithmetic: Using [[ ]] instead of [ ] for better integer comparisons (.github/workflows/_version.yml:67)
2. Variable quoting: Consistent use of double quotes around variable expansions to prevent word splitting
3. Echo formatting: Using echo -e for formatted output in _version.yml:83

Formatting Changes

The prettier formatting has made several spacing improvements:

• Consistent indentation in YAML arrays
• Proper spacing around colons and values
• Standardized quote usage (double quotes preferred)

Security Considerations

The persist-credentials: false additions are important security improvements that prevent accidentally leaking credentials in subsequent steps, particularly important for public repositories or workflows that execute untrusted code.

---

Action Items for Author

Required Before Merge:

1. Fix the critical environment variable issue in .github/workflows/check-run.yml:29-30 by restoring the GITHUB_TRIGGERING_ACTOR and GITHUB_ACTOR environment variable definitions

Recommended Improvements:

2. Consider adding error handling for the ArgoCD grep command in _ephemeral_environment_manager.yml:193
3. Consider adding head -1 to prevent multiple match issues in the same file

---

Changes Since Last Review

New in this review (since Claude's last review on 2025-10-21):

• Confirmed the critical environment variable issue in check-run.yml is still present and needs fixing
• Verified all other previously identified issues have been resolved
• All CI checks are passing or in progress (11 passed, 2 in progress, 0 failed)
• Workflow linter validation is passing

Previously identified issues that remain:

• ❌ Missing environment variables in check-run.yml (still unfixed from previous review)

---

Conclusion

This PR makes valuable security and code quality improvements to align with zizmor and bwwl requirements. The changes are well-structured and follow best practices for GitHub Actions workflows.

Recommendation: Request changes - The critical environment variable issue in check-run.yml must be fixed before merging, as it will cause error messages to display empty values when permission checks fail.

Once the critical issue is resolved, this PR will be ready to merge.

---

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud