Skip to content

Conversation

@gitclonebrian
Copy link

@gitclonebrian gitclonebrian commented Nov 26, 2025

🎟️ Tracking

BRE-1333

📔 Objective

Adjust workflow and app token permissions to align with least privilege best practices.

  • Add empty permission set at workflow level to remove default GITHUB_TOKEN permissions
  • Add empty permission set to setup job as it only runs bash commands
  • Add contents:write to GitHub App tokens in bump_version and cut_branch jobs for git operations
  • Add empty permission set to move_edd_db_scripts job as called workflow declares its own permissions
  • Remove secrets:inherit as called workflow accesses Azure secrets directly

📸 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

- Add empty permission set at workflow level to remove default GITHUB_TOKEN permissions
- Add empty permission set to setup job as it only runs bash commands
- Add contents:write to GitHub App tokens in bump_version and cut_branch jobs for git operations
- Add empty permission set to move_edd_db_scripts job as called workflow declares its own permissions
- Remove secrets:inherit as called workflow accesses Azure secrets directly
@github-actions
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Details672362ba-353c-47c4-9a9b-89229ac940c3

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

@gitclonebrian gitclonebrian marked this pull request as ready for review November 26, 2025 03:55
@gitclonebrian gitclonebrian requested a review from a team as a code owner November 26, 2025 03:55
@claude
Copy link
Contributor

claude bot commented Nov 26, 2025

Claude finished @gitclonebrian's task —— View job


Code Review Complete

This PR implements least privilege permissions for the repository-management workflow. The changes are security-focused and align with GitHub Actions best practices.


Summary of Changes

The PR removes overly broad default permissions and implements granular, scoped permissions:

  • Workflow-level permissions changed from pull-requests: write, contents: write to empty set
  • Empty permissions added to setup job (bash-only operations)
  • GitHub App tokens granted explicit permission-contents: write for git operations
  • move_edd_db_scripts job permissions removed (called workflow declares its own)
  • secrets: inherit removed (called workflow accesses Azure secrets directly)

Critical Issues Found

None. The implementation is correct and secure.


Suggested Improvements

Finding 1: Consider documenting the permission strategy in workflow comments

💭 The workflow now follows least privilege principles, but the rationale isn't immediately clear to future maintainers. Consider adding a comment at the workflow level explaining the permission strategy:

# Permissions strategy: Empty default permissions at workflow level per least privilege.
# Individual jobs request only the minimum permissions needed via GitHub App tokens.
# Azure secrets accessed via OIDC (id-token: write) rather than secrets: inherit.
permissions: {}

Finding 2: GitHub App token usage pattern could be documented

💭 The pattern of using permission-contents: write with GitHub App tokens (lines 91, 215) is correct but non-obvious. The actions/create-github-app-token action uses this parameter to request specific permissions when generating the token. This is superior to granting broad GITHUB_TOKEN permissions but may benefit from a comment:

# Request contents:write permission for this GitHub App token to enable git push operations
permission-contents: write

Good Practices Observed

  • Empty permission sets correctly remove all default GITHUB_TOKEN permissions
  • GitHub App tokens properly scoped to only contents: write for git operations
  • Reusable workflow properly declares its own permissions (verified in _move_edd_db_scripts.yml)
  • OIDC authentication (id-token: write) used for Azure instead of secrets: inherit
  • Setup job correctly has empty permissions (only runs bash commands)

Action Items

No blocking issues - PR is ready to merge

Optional enhancements (non-blocking):

  1. Consider adding explanatory comments per Finding 1 and Finding 2 to aid future maintainers
  2. Verify the workflow runs successfully in CI to confirm permission changes don't break functionality

Security Analysis

Excellent security posture:

  • Implements principle of least privilege throughout
  • Eliminates unnecessary GITHUB_TOKEN permissions at workflow level
  • Scopes permissions to exact operations (only contents: write for git push)
  • Removes secrets: inherit in favor of direct Azure Key Vault access via OIDC
  • Each job declares only the minimum permissions it needs
  • Checkmarx scan passed with no new vulnerabilities

This PR significantly improves the security posture of the workflow by eliminating overly broad permissions.

@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.25%. Comparing base (9fcedd5) to head (9b29579).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6646      +/-   ##
==========================================
- Coverage   53.27%   53.25%   -0.02%     
==========================================
  Files        1906     1906              
  Lines       84955    84955              
  Branches     7639     7639              
==========================================
- Hits        45257    45247      -10     
- Misses      37944    37954      +10     
  Partials     1754     1754              

☔ 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.

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