fix(workflows): address codeql medium findings#234
Conversation
WalkthroughUpdated three GitHub Actions workflows: added top-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
lerian-studio
added
size/XS
🔍 Lint Analysis
|
🛡️ CodeQL Analysis ResultsLanguages analyzed: ✅ No security issues found. 🔍 View full scan logs | 🛡️ Security tab |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/helm-update-chart.yml:
- Around line 166-167: Update the security rationale comment to avoid
overstating guarantees: change the sentence that claims the ref "cannot be
attacker-controlled" to accurately say that the ref (base_branch) is
caller-controlled but is validated against an allowlist before checkout (e.g.,
via workflow_call validation), so the ref is constrained by that allowlist
rather than inherently non-attacker-controlled; locate and edit the comment
around the base_branch/checkout lines to reflect this precise wording.
- Around line 101-113: The workflow input contract for inputs.base_branch must
match the new validation: update the inputs.base_branch metadata (description
and default) to explicitly list the allowed branches (main, develop, master,
staging) and set the default to one of those allowed values (choose either
develop or main to resolve the current conflict), and/or remove the hard-fail in
the case block if you intend to accept other branches; ensure any external docs
or README that describe base_branch are also updated to reflect the allowed set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9f6f8d1-60a2-4f30-a8e0-9739ed07b909
📒 Files selected for processing (3)
.github/workflows/helm-update-chart.yml.github/workflows/release-notification.yml.github/workflows/release.yml
… branch after checkout
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/helm-update-chart.yml (1)
24-27:⚠️ Potential issue | 🟠 MajorAlign
base_branchinput contract with enforced validation (breaking for callers).Line 25 still documents a
developdefault while Line 27 setsmain, and the input description does not declare the enforced allowlist introduced at Line 101-Line 113. This is now a caller-facing contract mismatch because unsupported branches fail hard.Suggested fix
base_branch: - description: 'Target branch for the PR (default: develop)' + description: 'Target branch for the PR. Allowed: main, develop, master, staging (default: main)' type: string default: 'main'As per coding guidelines: "Flag any removed or renamed input/output as a breaking change for callers".
Also applies to: 101-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helm-update-chart.yml around lines 24 - 27, Update the base_branch input docs to match the actual default and enforcement: change the description to state the default is "main" (not "develop") and explicitly document the allowed branch values enforced by the workflow's allowlist/validation (so callers know which branches will be accepted); ensure the base_branch input definition (the input named base_branch) and its description reflect the enforced allowlist implemented elsewhere in the workflow so this caller-facing contract is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/helm-update-chart.yml:
- Around line 24-27: Update the base_branch input docs to match the actual
default and enforcement: change the description to state the default is "main"
(not "develop") and explicitly document the allowed branch values enforced by
the workflow's allowlist/validation (so callers know which branches will be
accepted); ensure the base_branch input definition (the input named base_branch)
and its description reflect the enforced allowlist implemented elsewhere in the
workflow so this caller-facing contract is consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f19bae32-fa21-48a0-b3f2-dfff5e4c029c
📒 Files selected for processing (1)
.github/workflows/helm-update-chart.yml
…and actual default
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/helm-update-chart.yml:
- Around line 172-178: The checkout step uses git checkout "${BASE_BRANCH}"
which can leave a stale local branch; change it to explicitly use the
remote-tracking ref (origin/${BASE_BRANCH}) so you get the freshly fetched
history—e.g., replace git checkout "${BASE_BRANCH}" with git checkout
"origin/${BASE_BRANCH}" or reset the local branch to the remote via git checkout
-B "${BASE_BRANCH}" "origin/${BASE_BRANCH}" (referencing BASE_BRANCH, git fetch
origin "${BASE_BRANCH}", and git checkout).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 352f5cce-535c-42a5-92e5-fd01d4842a4c
📒 Files selected for processing (1)
.github/workflows/helm-update-chart.yml
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/helm-update-chart.yml:
- Around line 101-113: Add an explicit workflow-level permissions block to
harden the workflow by defaulting to least privilege (set permissions: contents:
read at the top of the workflow) and then elevate permissions only in specific
jobs/steps that need more access; update the workflow YAML to include the
top-level permissions stanza and, if any job needs write or additional scopes,
add a per-job permissions override (for example in the job that performs chart
updates) rather than leaving permissions implicit—this change targets the
workflow containing the "Validate base_branch" step so you can locate the file
and apply the global permissions change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06c87709-4c3b-4bcf-a0e3-4523b4151eb4
📒 Files selected for processing (1)
.github/workflows/helm-update-chart.yml
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/helm-update-chart.yml:
- Around line 104-116: Remove the unnecessary success echo in the base_branch
validation case: in the shell block that reads the BASE_BRANCH env var (the case
... esac for BASE_BRANCH), delete the line that prints "✅ base_branch
'${BASE_BRANCH}' is allowed" so the normal execution path produces no extra
output and only fails via non-zero exit on invalid branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63e75259-610a-41fd-8d60-5cdf3f2fa9f0
📒 Files selected for processing (1)
.github/workflows/helm-update-chart.yml
2 participants
GitHub Actions Shared Workflows
Description
Resolves 5 CodeQL medium findings surfaced on PR #233's scan.
actions/missing-workflow-permissions(4 findings) —release.ymlandrelease-notification.ymldid not constrainGITHUB_TOKEN. Both workflows perform all writes through a dedicated GitHub App token (actions/create-github-app-token), so the defaultGITHUB_TOKENonly needs read access. Added a workflow-levelpermissions: contents: readblock to both files.actions/untrusted-checkout/medium(1 finding) —helm-update-chart.ymlchecks outinputs.base_branch, which CodeQL flagged as potentially untrusted input. Added aValidate base_branchstep that restricts the input to an allowlist (main,develop,master,staging) before the checkout runs. Updated the inline rationale comment accordingly.Affected workflows:
.github/workflows/release.yml.github/workflows/release-notification.yml.github/workflows/helm-update-chart.ymlType of Change
feat: New workflow or new input/output/step in an existing workflowfix: Bug fix in a workflow (incorrect behavior, broken step, wrong condition)perf: Performance improvement (e.g. caching, parallelism, reduced steps)refactor: Internal restructuring with no behavior changedocs: Documentation only (README, docs/, inline comments)ci: Changes to self-CI (workflows under.github/workflows/that run on this repo)chore: Dependency bumps, config updates, maintenancetest: Adding or updating testsBREAKING CHANGE: Callers must update their configuration after this PRBreaking Changes
Callers of
helm-update-chart.ymlthat passbase_branchvalues outside the allowlist (main,develop,master,staging) will now fail with an explicit error instead of silently attempting the checkout. Known callers usemainordevelop, so no migration is expected.Testing
@developor the beta tagCaller repo / workflow run: Validation will come from the CodeQL re-scan triggered on this PR.
Related Issues
Related to #233 — findings surfaced by the CodeQL scan on that PR.
Summary by CodeRabbit