Update sync-issue-labels-compute.yml#12
Conversation
Signed-off-by: manishdait24-bot <manishdait24@gmail.com>
Up to standards ✅🟢 Issues
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced a two-stage workflow with a single Changes
Sequence DiagramsequenceDiagram
participant WF as GitHub Workflow
participant Sync as Sync Job
participant API as GitHub REST API
participant Issues as GitHub Issues API
WF->>Sync: start `sync` job
Sync->>API: get PR metadata & body
API-->>Sync: PR author, body, existing PR labels
Sync->>Sync: if author is bot -> set `skip=true` and exit
alt not skipped
Sync->>Sync: parse body for `fix/close/resolve ... #<number>`
Sync->>API: fetch referenced issue details (loop)
API-->>Sync: issue labels or error (errors logged, continue)
Sync->>Sync: accumulate & filter labels -> compute `newLabels`
alt has_new_labels
Sync->>Issues: call `issues.addLabels` with `newLabels`
Issues-->>Sync: labels applied response
end
Sync->>Sync: write `labels.json` and upload artifact
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82cb4698-5686-4ce2-bf31-8675ad17f4c7
📒 Files selected for processing (1)
.github/workflows/sync-issue-labels-compute.yml
Signed-off-by: manishdait24-bot <manishdait24@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (4)
.github/workflows/sync-issue-labels-compute.yml (4)
35-35:⚠️ Potential issue | 🔴 CriticalThird-party actions must be pinned to full commit SHAs.
Per coding guidelines, all third-party actions require immutable SHA pinning. The following still use mutable version tags:
Line Action Current 35 step-security/harden-runner@v246 actions/github-script@v7109 actions/upload-artifact@v4123 actions/github-script@v7#!/bin/bash # Fetch latest commit SHAs for pinning echo "=== step-security/harden-runner v2 ===" gh api repos/step-security/harden-runner/git/ref/tags/v2 --jq '.object.sha' 2>/dev/null || echo "Tag not found directly, check releases" echo "=== actions/github-script v7 ===" gh api repos/actions/github-script/git/ref/tags/v7 --jq '.object.sha' 2>/dev/null || echo "Tag not found directly, check releases" echo "=== actions/upload-artifact v4 ===" gh api repos/actions/upload-artifact/git/ref/tags/v4 --jq '.object.sha' 2>/dev/null || echo "Tag not found directly, check releases"Also applies to: 46-46, 109-109, 123-123
45-45:⚠️ Potential issue | 🟠 MajorFork PR detection is hardcoded, breaking safety checks.
IS_FORK_PRis hardcoded to'false'instead of being derived from PR metadata. This defeats downstream logic that relies on fork detection for security.Compute fork status from
prDatainside the script:const isForkPr = prData.head.repo?.fork === true || prData.head.repo?.full_name !== prData.base.repo?.full_name; core.setOutput('is_fork_pr', String(isForkPr));
93-105:⚠️ Potential issue | 🔴 CriticalCommand injection via untrusted label names.
The shell script directly interpolates
${{ steps.compute.outputs.labels }}which contains user-controlled issue label names. GitHub expression interpolation happens before shell parsing, so an attacker with label-creation permission could craft malicious label names to execute arbitrary commands.🔒 Proposed fix using environment variables and jq
- name: Write labels artifact if: steps.compute.outputs.has_labels == 'true' + env: + LABELS_JSON: ${{ steps.compute.outputs.labels }} + PR_NUMBER: ${{ steps.compute.outputs.pr_number }} + IS_FORK_PR: ${{ steps.compute.outputs.is_fork_pr }} + DRY_RUN: ${{ steps.compute.outputs.dry_run }} run: | - echo '${{ steps.compute.outputs.labels }}' > labels_only.json - echo "{" > labels.json - echo " \"pr_number\": \"${{ steps.compute.outputs.pr_number }}\"," >> labels.json - echo " \"labels\": $(cat labels_only.json)," >> labels.json - echo " \"is_fork_pr\": ${{ steps.compute.outputs.is_fork_pr }}," >> labels.json - echo " \"dry_run\": \"${{ steps.compute.outputs.dry_run }}\"," >> labels.json - echo " \"source_event\": \"workflow_dispatch\"" >> labels.json - echo "}" >> labels.json + jq -n \ + --arg pr_number "$PR_NUMBER" \ + --argjson labels "$LABELS_JSON" \ + --argjson is_fork_pr "$IS_FORK_PR" \ + --arg dry_run "$DRY_RUN" \ + '{pr_number: $pr_number, labels: $labels, is_fork_pr: $is_fork_pr, dry_run: $dry_run, source_event: "workflow_dispatch"}' \ + > labels.json
82-82: 🧹 Nitpick | 🔵 TrivialSilent error handling hides failure details.
The catch block discards the actual error, making debugging difficult. Log the error message and status code.
- } catch (e) { console.log(`Error fetching #${num}`); } + } catch (e) { + const status = e.status || 'unknown'; + console.log(`Skipping #${num}: ${e.message || e} (status: ${status})`); + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a3678f4-f95f-470f-8bbb-549995f9f247
📒 Files selected for processing (1)
.github/workflows/sync-issue-labels-compute.yml
Refactor workflow to sync linked issue labels, update job names, and improve label handling. Signed-off-by: Manish Dait <90558243+manishdait@users.noreply.github.com>
Signed-off-by: manishdait24-bot <manishdait24@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
.github/workflows/sync-issue-labels-compute.yml (3)
26-37:⚠️ Potential issue | 🟠 MajorSkip fork PRs before reaching the write step.
On
pull_requestruns from forks, GitHub downgrades requestedGITHUB_TOKENwrite scopes to read-only unless the repository explicitly enables sending write tokens, so this workflow will fail inApply Labelsinstead of cleanly skipping external PRs. (docs.github.com)🛡️ Proposed fix
const prNumber = context.payload.pull_request.number; + const isForkPr = + context.payload.pull_request.head.repo.full_name !== + context.payload.pull_request.base.repo.full_name; console.log(`--- Processing PR #${prNumber} ---`); + if (isForkPr) { + console.log('Skipping fork PR: label writes are not allowed on pull_request from forks.'); + core.setOutput('has_new_labels', 'false'); + core.setOutput('skip', 'true'); + return; + } + const { data: prData } = await github.rest.pulls.get({ owner: context.repo.owner, repo: context.repo.repo, pull_number: prNumber });As per coding guidelines, workflows must behave safely when executed from forks.
68-70:⚠️ Potential issue | 🟠 MajorDon't treat every fetch error as a harmless miss.
A 404 is safe to skip, but 401/403/429/5xx means label discovery failed and this run can still go green with missing labels. Re-throw non-404 errors; if you want best-effort resiliency,
actions/github-scriptalso supports built-in retries for transient API failures. (github.com)🔁 Proposed fix
- } catch (e) { - console.log(`Error fetching labels for issue #${num}: ${e.message}`); + } catch (e) { + if (e.status === 404) { + console.log(`Skipping missing issue #${num}.`); + continue; + } + throw e; }As per coding guidelines, log key decisions and early exits — avoid silent skips.
92-95:⚠️ Potential issue | 🔴 CriticalPass
labelsthroughenv, not${{ }}inside the script.
actions/github-scriptevaluates workflow expressions before the JavaScript function body is parsed and explicitly recommends using environment variables instead. Here, label names are untrusted input, so a backtick or${...}in a label can break this template literal or inject code beforeJSON.parseruns. (github.com)🔒 Proposed fix
- name: Apply Labels if: steps.compute.outputs.skip == 'false' && steps.compute.outputs.has_new_labels == 'true' uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 + env: + LABELS_JSON: ${{ steps.compute.outputs.labels }} with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | - const labels = JSON.parse(`${{ steps.compute.outputs.labels }}`); + const labels = JSON.parse(process.env.LABELS_JSON); const prNumber = context.payload.pull_request.number;As per coding guidelines, treat all GitHub event data as potentially untrusted input: issue titles, bodies, comments, labels, usernames, branch names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38c8057e-837c-46dc-8786-6eed8b5402bb
📒 Files selected for processing (1)
.github/workflows/sync-issue-labels-compute.yml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/sync-issue-labels-compute.yml (2)
33-37:⚠️ Potential issue | 🟠 MajorSkip fork PRs before the workflow reaches the label-write path.
On
pull_request, fork-originated runs do not get a token that can mutate PR state. As written, a fork PR with linked issue labels will fall through toApply Labelsand fail the job instead of exiting cleanly.🔧 Proposed fix
const prAuthor = prData.user.login; + const isForkPr = + context.payload.pull_request.head.repo.full_name !== + `${context.repo.owner}/${context.repo.repo}`; + + if (isForkPr) { + console.log('Skipping fork PR: label sync is read-only on pull_request runs from forks.'); + core.setOutput('has_new_labels', 'false'); + core.setOutput('skip', 'true'); + return; + } + if (/\[bot\]$/i.test(prAuthor) || /dependabot/i.test(prAuthor)) { console.log(`Skipping bot-authored PR (Author: ${prAuthor})`); core.setOutput('skip', 'true');For GitHub Actions `pull_request` events from forks, are `GITHUB_TOKEN` write permissions downgraded to read-only, and what is the recommended pattern for workflows that would otherwise mutate PR state?As per coding guidelines, "Workflows must behave safely when executed from forks."
93-93:⚠️ Potential issue | 🔴 CriticalDo not splice untrusted label JSON into the
github-scriptsource.
steps.compute.outputs.labelsis built from issue label names, so it is untrusted text. Injecting it into a JS template literal means a label containing a backtick or${...}can break parsing or change what executes.🔧 Proposed fix
- name: Apply Labels if: steps.compute.outputs.skip == 'false' && steps.compute.outputs.has_new_labels == 'true' uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 + env: + LABELS_JSON: ${{ steps.compute.outputs.labels }} with: script: | - const labels = JSON.parse(`${{ steps.compute.outputs.labels }}`); + const labels = JSON.parse(process.env.LABELS_JSON || '[]'); const prNumber = context.payload.pull_request.number;In `actions/github-script`, are `${{ ... }}` expressions rendered before the JavaScript runs, and what is the safe way to pass JSON outputs containing untrusted characters into the script?As per coding guidelines, "Treat all GitHub event data as potentially untrusted input: issue titles, bodies, comments, labels, usernames, branch names."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b508778c-8318-42a0-8acd-f5a3b51ff776
📒 Files selected for processing (1)
.github/workflows/sync-issue-labels-compute.yml
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: manishdait24-bot <manishdait24@gmail.com>
Signed-off-by: Manish Dait <90558243+manishdait@users.noreply.github.com>
Signed-off-by: Manish Dait <90558243+manishdait@users.noreply.github.com>
Signed-off-by: manishdait24-bot <manishdait24@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/sync-issue-labels-compute.yml (3)
7-10:⚠️ Potential issue | 🟠 MajorReduce this token to the permissions the job actually uses.
This job only reads PR/issue metadata and uploads an artifact.
pull-requests: writeis broader than needed here, andcontents: readappears unused.What minimum `GITHUB_TOKEN` permissions are required for GitHub REST `pulls.get`, `issues.get`, and `actions/upload-artifact@v4` in GitHub Actions?As per coding guidelines, "
permissions:must be explicitly declared and scoped to the minimum needed. Flag over-permissioned or under-permissioned workflows, and any reliance on broad default permissions."
57-70:⚠️ Potential issue | 🟠 MajorDon't turn GitHub API failures into silent label loss.
The catch block continues on every
issues.geterror. That means auth, rate-limit, or 5xx failures can be reported as “no labels found,” and the workflow still succeeds with incomplete results. Only swallow expected misses like 404; rethrow the rest.What HTTP status codes does GitHub REST `GET /repos/{owner}/{repo}/issues/{issue_number}` return for not-found versus permission, rate-limit, and server errors?🔧 Suggested change
- } catch (e) { - console.log(`Error fetching labels for issue #${num}: ${e.message}`); + } catch (e) { + if (e.status === 404) { + console.log(`Skipping #${num}: issue not found`); + continue; + } + throw e; }
102-102:⚠️ Potential issue | 🔴 CriticalPin
actions/upload-artifactto a full commit SHA.This step reintroduces a mutable
@v4tag while the other actions in this workflow are SHA-pinned. That regresses the workflow's immutability guarantees.Does GitHub recommend pinning Actions like `actions/upload-artifact` to full commit SHAs instead of version tags, and what commit SHA currently backs `actions/upload-artifact` v4?As per coding guidelines, "All third-party actions must be pinned to full commit SHAs, consistent with other workflows in this repository."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43ebe537-cc28-4ec9-8978-8dabf0b3f932
📒 Files selected for processing (1)
.github/workflows/sync-issue-labels-compute.yml
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <90558243+manishdait@users.noreply.github.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <90558243+manishdait@users.noreply.github.com>
Signed-off-by: Manish Dait <90558243+manishdait@users.noreply.github.com>
github-actions
Bot
added
automated
broken-markdown-links
enhancement
manishdait
force-pushed
the
main
branch
2 times, most recently
from
d7cfc56 to
7571b15
Compare
2 participants
testpr
fixes #7