Skip to content

Conversation

@pixman20
Copy link
Contributor

@pixman20 pixman20 commented Nov 3, 2025

🎟️ Tracking

BRE-1300

📔 Objective

Adding a permissions block to make the workflow linter pass

📸 Screenshots

⏰ 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

@pixman20 pixman20 marked this pull request as ready for review November 3, 2025 23:11
@pixman20 pixman20 requested a review from a team as a code owner November 3, 2025 23:11
@pixman20 pixman20 requested a review from addisonbeck November 3, 2025 23:11
@pixman20 pixman20 enabled auto-merge (squash) November 3, 2025 23:12
@claude
Copy link

claude bot commented Nov 3, 2025

Claude finished @pixman20's task —— View job


PR Review Complete

Todo List

  • Read the modified workflow file
  • Analyze the git diff to understand changes
  • Review permissions block for security implications
  • Check GitHub Actions best practices compliance
  • Provide comprehensive review feedback

Summary of Changes

This PR adds a permissions block to .github/workflows/delete-old-packages.yml to comply with workflow linter requirements. The changes include:

  1. Added workflow-level permissions: {} (line 8) - denying all permissions by default
  2. Added job-level permissions block to the delete job (lines 18-20):
    • contents: read
    • packages: write

Critical Issues Found

None. The implementation is secure and correct.


Suggested Improvements

📝 Consider adding a comment explaining the permissions model

While the current implementation is correct, it would be helpful for future maintainers to understand the intentional use of the principle of least privilege:

# Deny all permissions by default at workflow level (principle of least privilege)
permissions: {}

defaults:
  run:
    shell: bash

jobs:
  delete:
    name: Cleanup Android SDK
    runs-on: ubuntu-24.04
    # Grant only the minimum permissions required for this job
    permissions:
      contents: read  # Required for actions/checkout if used
      packages: write # Required for deleting package versions

Rationale: This makes the security model explicit and helps reviewers understand that the dual-level permissions are intentional, not accidental.


Good Practices Observed

  • ✅ Implements principle of least privilege by denying all permissions at workflow level
  • ✅ Grants only necessary permissions at job level
  • ✅ Uses specific permission scopes rather than overly broad permissions
  • contents: read is minimal (not write)
  • ✅ Workflow already uses pinned action versions with SHA hashes

Security Analysis

✅ Excellent security posture:

  1. Principle of Least Privilege: The workflow-level permissions: {} ensures that any future jobs added to this workflow will have no permissions by default, requiring explicit permission grants.

  2. Minimal Job Permissions: The delete job only has:

    • contents: read - likely needed if the action internally checks repository context (minimal, read-only access)
    • packages: write - absolutely required for the actions/delete-package-versions action to function
  3. No Excessive Permissions: The permissions are appropriately scoped. No unnecessary permissions like actions: write, security-events: write, or elevated contents: write.

  4. Aligns with GitHub Security Best Practices: This matches GitHub's recommended pattern for workflow security hardening.


Compliance Check

Workflow Linter Compliance: ✅ This change should satisfy workflow linters that enforce explicit permissions declarations, such as:

  • GitHub's internal security scanners
  • Third-party tools like actionlint
  • Security policies requiring explicit permission declarations

Action Items for Author

Optional (non-blocking):

  • Consider adding inline comments to document the permissions model (see suggestion above)

No blocking issues found. This PR is ready to merge from a security and functionality perspective.


@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2025

Logo
Checkmarx One – Scan Summary & Detailsdcafbd54-fb7d-4884-91b0-13008256cc50

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

@codecov
Copy link

codecov bot commented Nov 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.95%. Comparing base (95e329a) to head (242dea8).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #545   +/-   ##
=======================================
  Coverage   78.95%   78.95%           
=======================================
  Files         296      296           
  Lines       30904    30904           
=======================================
  Hits        24400    24400           
  Misses       6504     6504           

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pixman20 pixman20 merged commit 407954d into main Nov 5, 2025
53 checks passed
@pixman20 pixman20 deleted the BRE-1300-verify-all-repositories-pass-the-workflow-linter branch November 5, 2025 20:41
bw-ghapp bot pushed a commit to bitwarden/sdk-swift that referenced this pull request Nov 5, 2025
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.

3 participants