Skip to content

Conversation

@gitclonebrian
Copy link

@gitclonebrian gitclonebrian commented Nov 21, 2025

🎟️ Tracking

BRE-1333

📔 Objective

Added contents:write permission to app token generation step to limit scope of token.
Successfully tested workflow here.

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • 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

@codecov
Copy link

codecov bot commented Nov 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 6.87%. Comparing base (99655a0) to head (23d0a72).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #929   +/-   ##
=====================================
  Coverage   6.87%   6.87%           
=====================================
  Files         67      67           
  Lines       2791    2791           
  Branches     481     481           
=====================================
  Hits         192     192           
  Misses      2585    2585           
  Partials      14      14           

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 21, 2025

Logo
Checkmarx One – Scan Summary & Detailsec6dda68-10e1-4f8e-8034-434307d24432

New Issues (2)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
HIGH CVE-2025-12725 Npm-electron-39.1.0
detailsRecommended version: 39.2.0
Description: Out of bounds read in WebGPU in Google Chrome on Android prior to 142.0.7444.137 allowed a remote attacker to perform an Out-of-Bounds memory write...
Attack Vector: NETWORK
Attack Complexity: LOW

ID: QaoZvAfUMXLVtJ7KOjsjam5knRSWl3siP6YlAkp3CiY%3D
Vulnerable Package
HIGH CVE-2025-12726 Npm-electron-39.1.0
detailsRecommended version: 39.2.0
Description: Inappropriate implementation in Views in Google Chrome on Windows prior to 142.0.7444.137 allowed a remote attacker who had compromised the rendere...
Attack Vector: NETWORK
Attack Complexity: HIGH

ID: UMkeWqWthNXV%2FxJQ0rKPA1CG%2FiHici7t7x3ps4x6%2FIs%3D
Vulnerable Package

@gitclonebrian gitclonebrian force-pushed the BRE-1333/workflow-token-perms_directory-connector branch from c8ae77c to 23d0a72 Compare November 21, 2025 16:35
@gitclonebrian gitclonebrian marked this pull request as ready for review November 21, 2025 17:07
@gitclonebrian gitclonebrian requested a review from a team as a code owner November 21, 2025 17:07
@gitclonebrian gitclonebrian requested a review from r-tome November 21, 2025 17:07
@claude
Copy link

claude bot commented Nov 21, 2025

Claude finished @gitclonebrian's task —— View job


Review Complete

Summary of Changes

This PR adds the permission-contents: write parameter to the GitHub App token generation step in the version-bump workflow. This explicitly scopes the generated token to only have write access to repository contents, following the principle of least privilege.

Findings

Finding 1: 💭 PR description could better explain the "why" behind this change

Suggested improvements

The current objective states "Added contents:write permission to app token generation step to limit scope of token" but doesn't explain what the token scope was before or why limiting it is beneficial.

Consider expanding to something like:

## 📔 Objective
Added explicit `permission-contents: write` parameter to the GitHub App token generation step. This follows the principle of least privilege by requesting only the specific permission needed for version bump commits, rather than inheriting all permissions available to the GitHub App. This reduces the blast radius if the token were to be compromised during workflow execution.

Finding 2: ⚠️ Inconsistency between job-level and token-level permissions (.github/workflows/version-bump.yml:18,50)

The job has permissions: contents: write at line 18, and now the token generation also has permission-contents: write at line 50. While this aligns both permission scopes, it's worth verifying:

  1. Does the actions/create-github-app-token action respect the job-level permissions as an upper bound?
  2. Is specifying permission-contents: write on the token actually more restrictive than the default GitHub App installation permissions?

According to the action's documentation, the permission-* inputs request specific permissions from the App. If the App doesn't have those permissions granted during installation, the workflow will fail. This is correct security posture.

Finding 3: 🎨 Consider documenting the security model in workflow comments

Adding a brief comment above the token generation step would help future maintainers understand the security considerations:

# Generate a scoped GitHub App token with minimal permissions.
# Only requests 'contents:write' permission needed for version bump commits.
- name: Generate GH App token
  uses: actions/create-github-app-token@df432ceedc7162793a195dd1713ff69aefc7379e # v2.0.6

Good Practices Observed

  • Explicit permission scoping follows least privilege principle
  • Workflow tested before PR submission
  • Pinned action to specific commit SHA for security

Action Items

  1. Consider enhancing PR description to explain the security benefit (Finding 1)
  2. Verify the permission model understanding is correct (Finding 2)
  3. Optional: Add inline comment documenting security model (Finding 3)

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