*: harden update bazel files workflow permissions#67464
*: harden update bazel files workflow permissions#67464ti-chi-bot[bot] merged 11 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-triage-completed
release-note-none
|
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:
📝 WalkthroughWalkthroughThe changes introduce a new "Generate Bazel Files" workflow that automatically generates Bazel configuration patches on pull requests, while refactoring the "Update Bazel Files" workflow to consume these artifacts via Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request Event
participant GenWF as Generate Bazel Files<br/>Workflow
participant Artifact as Artifact Storage
participant UpdateWF as Update Bazel Files<br/>Workflow
participant Repo as Repository
PR->>GenWF: trigger (opened/synchronize/reopened)
GenWF->>GenWF: checkout PR head
GenWF->>GenWF: run bazel_prepare
GenWF->>GenWF: generate bazel.patch
GenWF->>Artifact: upload bazel-files artifact
GenWF-->>UpdateWF: workflow_run trigger<br/>(on success)
UpdateWF->>UpdateWF: resolve PR metadata
UpdateWF->>UpdateWF: validate stale check
UpdateWF->>Artifact: download bazel-files artifact
UpdateWF->>UpdateWF: apply patch to PR branch
UpdateWF->>Repo: commit & push changes<br/>(via PUSH_TOKEN)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
ti-chi-bot
bot
removed
do-not-merge/needs-linked-issue
do-not-merge/needs-triage-completed
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/update-bazel-files.yml (1)
63-76: Explicit commit/push logic is cleaner than auto-commit and properly scopes file changes.The pathspecs correctly match only generated Bazel files, consistent with
tools/check/check-bazel-prepare.sh. Minor suggestions:
Optional: Mask the token explicitly — While GitHub Actions auto-masks secrets, adding
echo "::add-mask::${PUSH_TOKEN}"at the start provides defense-in-depth if git errors expose the URL.The
-uflag inset -euo pipefailwill cause the script to fail if any variable is unset, which is good defensive coding. Thepipefailoption is harmless but unnecessary since there are no pipelines.🛡️ Optional: Explicit token masking
run: | set -euo pipefail + echo "::add-mask::${PUSH_TOKEN}" if git diff --quiet -- DEPS.bzl ':(glob)**/*.bazel' ':(glob)**/*.bzl'; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/update-bazel-files.yml around lines 63 - 76, Add explicit masking of the secret and remove the unnecessary pipefail note: update the workflow step that runs the commit/push shell (the block containing "set -euo pipefail", the git remote set-url using ${PUSH_TOKEN}, and git push) to call the GitHub Actions mask command for the token (echo "::add-mask::${PUSH_TOKEN}") early in the step so the token is explicitly masked, and keep the defensive "set -euo" behavior (you may drop mentioning pipefail if desired) while leaving the existing git add/commit/push logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/update-bazel-files.yml:
- Around line 63-76: Add explicit masking of the secret and remove the
unnecessary pipefail note: update the workflow step that runs the commit/push
shell (the block containing "set -euo pipefail", the git remote set-url using
${PUSH_TOKEN}, and git push) to call the GitHub Actions mask command for the
token (echo "::add-mask::${PUSH_TOKEN}") early in the step so the token is
explicitly masked, and keep the defensive "set -euo" behavior (you may drop
mentioning pipefail if desired) while leaving the existing git add/commit/push
logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e64a6a8c-819c-437a-bbbc-db0e3d6daf69
📒 Files selected for processing (2)
.github/actions/tidb_build/action.yml.github/workflows/update-bazel-files.yml
There was a problem hiding this comment.
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/update-bazel-files.yml:
- Around line 22-23: The workflow's privileged job currently only checks
repository equality via the existing if condition
(github.event.pull_request.head.repo.full_name == github.repository) but lacks a
membership gate; update the job's if to also require an allowed author
association by checking github.event.pull_request.author_association (e.g., only
allow MEMBER or OWNER) so that pull_request_target cannot run for external
collaborators—locate the if: expression in
.github/workflows/update-bazel-files.yml and add the additional
author_association check to the boolean expression to restrict execution to org
members/owners.
- Around line 61-64: The current no-op check uses the literal command git diff
--quiet ... which ignores untracked files and therefore misses newly created
*.bazel/*.bzl; update the check so it also detects untracked Bazel files (those
produced by make bazel_prepare) — e.g., replace the single git diff test with a
combined check that returns non-empty when either git diff detects changes or
git status/ls-files shows untracked **/*.bazel/**/*.bzl files (or equivalently
use git status --porcelain --untracked-files=all filtered for *.bazel and
*.bzl); ensure the updated condition references the existing git diff --quiet
test and also queries untracked files so the workflow proceeds to the later git
add -A step when new generated Bazel files exist.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 282ffd3b-6ff7-49e7-a532-2b4fbc1d0c21
📒 Files selected for processing (1)
.github/workflows/update-bazel-files.yml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67464 +/- ##
================================================
- Coverage 77.8017% 77.5755% -0.2262%
================================================
Files 2023 1943 -80
Lines 556530 545147 -11383
================================================
- Hits 432990 422901 -10089
- Misses 121794 122239 +445
+ Partials 1746 7 -1739
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
.github/workflows/update-bazel-files.yml (2)
110-113:⚠️ Potential issue | 🟠 MajorThe early-exit check still ignores newly created Bazel files.
Line 110 uses
git diff --quiet, which does not report untracked files. Ifapply-bazel-files.shcreates a newBUILD.bazelor.bzl, this branch exits early and Line 117 never stages it.Based on learnings: `make bazel_prepare` output for `**/*.bazel`/`**/*.bzl` must be included when structural changes generate Bazel metadata.🧩 Suggested fix
- if git -C pr diff --quiet -- DEPS.bzl ':(glob)**/*.bazel' ':(glob)**/*.bzl'; then + if ! git -C pr status --porcelain --untracked-files=all -- DEPS.bzl ':(glob)**/*.bazel' ':(glob)**/*.bzl' | grep -q .; then echo "No generated Bazel changes to commit." exit 0 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/update-bazel-files.yml around lines 110 - 113, The early-exit uses git diff --quiet which ignores untracked new Bazel files (so newly created BUILD.bazel/.bzl from apply-bazel-files.sh are missed); update the check in update-bazel-files.yml to detect both tracked diffs and untracked files by querying the pr repo for untracked/modified files (e.g., use git -C pr status --porcelain or git -C pr ls-files -o --exclude-standard) and match those results against the patterns DEPS.bzl, **/*.bazel and **/*.bzl (or run make bazel_prepare and include its output), then only exit early when no matches are found so new generated Bazel files are correctly staged.
25-58:⚠️ Potential issue | 🔴 CriticalThe write-capable path is still reachable by
COLLABORATORauthors.This step resolves PR metadata, but it never records
.author_association, and the laterif:guards on Lines 69-101 still only checksame_repo/stale. Same-repo PRs from outside collaborators can still hit the checkout/apply/push path, which is the privileged case this hardening is supposed to close.Also applies to: 69-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/update-bazel-files.yml around lines 25 - 58, The step "Resolve PR metadata" doesn't capture the PR's .author_association, so later guards (same_repo/stale) allow same-repo PRs authored by COLLABORATOR to reach the write-capable checkout/apply/push path; update the "Resolve PR metadata" shell block to query the PR for '.author_association' (using the same gh api call pattern that produces pr_number/head_repo/head_ref/pr_head_sha) and write it to GITHUB_OUTPUT as author_association, and then ensure the later if: logic (the step(s) that check same_repo/stale) also checks author_association and only allows the privileged write path for trusted associations (e.g., OWNER or MEMBER) while treating COLLABORATOR and other associations as untrusted.
🤖 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/scripts/apply-bazel-files.sh:
- Around line 37-40: The script currently rejects symlinks in the artifact
(files_root) but not in the destination checkout; add a defensive check before
any copy/remove that iterates the destination path components under target_root
(use the same variables referenced in the diff: target_root and path) and fail
if target_root or any ancestor or final component of "${target_root}/${path}" is
a symlink (test -L or equivalent), ensuring the cp/rm blocks (the section around
the current cp/rm loop that uses target_root/path) never operate on symlinked
targets; apply the same check for the other cp/rm block mentioned (the 69-82
area) so both artifact application and cleanup reject symlinked destination
components.
In @.github/scripts/package-bazel-files.sh:
- Around line 20-24: The script currently creates the artifact directory and
empties the manifest (mkdir -p "${files_root}" and : > "${manifest_path}")
before collecting git file lists, causing the new manifest to appear as an
untracked file; move the two operations that create/initialize "${files_root}"
and "${manifest_path}" to after the mapfile lines that populate tracked_changes
and untracked_changes (i.e., run the git scans first using mapfile -t
tracked_changes ... and mapfile -t untracked_changes ..., then create the
directory and manifest) so the manifest won't be picked up as an untracked file.
In @.github/workflows/update-bazel-files.yml:
- Around line 52-57: The job currently verifies pr_head_sha vs run_head_sha but
later checks out the mutable branch ref (head_ref); change the checkout step(s)
that use head_ref to instead perform a checkout of the verified SHA
(pr_head_sha) so the workspace matches the validated commit, and retain head_ref
only as the remote push target (e.g. push to HEAD:${HEAD_REF}); update both
occurrences referenced in the workflow (the earlier checkout and the later
checkout/push block around the second occurrence) to use pr_head_sha for
checkout and head_ref for the push target.
---
Duplicate comments:
In @.github/workflows/update-bazel-files.yml:
- Around line 110-113: The early-exit uses git diff --quiet which ignores
untracked new Bazel files (so newly created BUILD.bazel/.bzl from
apply-bazel-files.sh are missed); update the check in update-bazel-files.yml to
detect both tracked diffs and untracked files by querying the pr repo for
untracked/modified files (e.g., use git -C pr status --porcelain or git -C pr
ls-files -o --exclude-standard) and match those results against the patterns
DEPS.bzl, **/*.bazel and **/*.bzl (or run make bazel_prepare and include its
output), then only exit early when no matches are found so new generated Bazel
files are correctly staged.
- Around line 25-58: The step "Resolve PR metadata" doesn't capture the PR's
.author_association, so later guards (same_repo/stale) allow same-repo PRs
authored by COLLABORATOR to reach the write-capable checkout/apply/push path;
update the "Resolve PR metadata" shell block to query the PR for
'.author_association' (using the same gh api call pattern that produces
pr_number/head_repo/head_ref/pr_head_sha) and write it to GITHUB_OUTPUT as
author_association, and then ensure the later if: logic (the step(s) that check
same_repo/stale) also checks author_association and only allows the privileged
write path for trusted associations (e.g., OWNER or MEMBER) while treating
COLLABORATOR and other associations as untrusted.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f0d902c9-4758-4843-a7e7-d646bcea868d
📒 Files selected for processing (4)
.github/scripts/apply-bazel-files.sh.github/scripts/package-bazel-files.sh.github/workflows/generate-bazel-files.yml.github/workflows/update-bazel-files.yml
| mkdir -p "${files_root}" | ||
| : > "${manifest_path}" | ||
|
|
||
| mapfile -t tracked_changes < <(git diff --name-only --no-renames HEAD -- .) | ||
| mapfile -t untracked_changes < <(git ls-files --others --exclude-standard) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git check-ignore -v bazel-artifact bazel-artifact/manifest.tsv || true
fd -HI '.gitignore' -x rg -n 'bazel-artifact' {}
if [[ -f .git/info/exclude ]]; then
rg -n 'bazel-artifact' .git/info/exclude || true
fiRepository: pingcap/tidb
Length of output: 142
Move artifact directory creation after the Git file scans.
Lines 20-21 create bazel-artifact/manifest.tsv before Line 24 runs git ls-files --others --exclude-standard. The bazel-artifact/ directory is not ignored by .gitignore rules, so the newly created manifest file will be picked up as an untracked file and unnecessarily rejected on Lines 36-39. Reorder the operations to create the artifact directory and manifest after collecting tracked and untracked changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/scripts/package-bazel-files.sh around lines 20 - 24, The script
currently creates the artifact directory and empties the manifest (mkdir -p
"${files_root}" and : > "${manifest_path}") before collecting git file lists,
causing the new manifest to appear as an untracked file; move the two operations
that create/initialize "${files_root}" and "${manifest_path}" to after the
mapfile lines that populate tracked_changes and untracked_changes (i.e., run the
git scans first using mapfile -t tracked_changes ... and mapfile -t
untracked_changes ..., then create the directory and manifest) so the manifest
won't be picked up as an untracked file.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/update-bazel-files.yml (1)
71-80:⚠️ Potential issue | 🟠 MajorTOCTOU: checkout uses mutable branch ref after SHA validation.
The stale check at lines 52-55 validates that
pr_head_sha == run_head_sha, but line 76 checks outhead_ref(the branch name). If the branch advances between the stale check and checkout, this job applies an artifact built from an older commit to a newer checkout.Consider checking out by SHA instead:
Suggested fix
- name: Checkout PR branch if: steps.pr.outputs.same_repo == 'true' && steps.pr.outputs.stale != 'true' uses: actions/checkout@v6 with: repository: ${{ steps.pr.outputs.head_repo }} - ref: ${{ steps.pr.outputs.head_ref }} + ref: ${{ github.event.workflow_run.head_sha }} path: pr fetch-depth: 0 token: ${{ github.token }} persist-credentials: falseThis ensures the checkout matches the exact commit the artifact was built from. The push step can still target
HEAD:${HEAD_REF}since git push allows specifying the target ref independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/update-bazel-files.yml around lines 71 - 80, The checkout step currently uses the mutable branch ref (uses: actions/checkout@v6 with ref: ${{ steps.pr.outputs.head_ref }}), which can change after the stale check; update it to check out the verified commit SHA instead (use the PR head SHA output, e.g. ${{ steps.pr.outputs.head_sha }} or the exact variable emitted by the pr step) so the workspace matches the exact commit validated earlier; keep repository, fetch-depth, token and persist-credentials as-is and continue to allow the later push to target HEAD:${HEAD_REF} if needed.
🧹 Nitpick comments (1)
.github/scripts/apply-bazel-files.sh (1)
39-46: Patch generation inconsistency: renames could be generated but rejected.The patch generation in
package-bazel-files.sh(lines 54–58) doesn't include--no-renames, while this script rejects rename/copy operations (lines 39–46). Ifmake bazel_preparetriggers a rename within the tracked Bazel files (DEPS.bzl, *.bazel, *.bzl), the patch could include that rename operation, only to be rejected here during application.Consider adding
--no-renamesto thegit diffcommand inpackage-bazel-files.sh(lines 54–58) to prevent renaming operations from being generated in the first place, ensuring consistency with the rejection logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/apply-bazel-files.sh around lines 39 - 46, The apply-bazel-files.sh script's case that rejects lines matching " rename " or " copy " conflicts with how patches are generated in package-bazel-files.sh; update the git diff invocation in package-bazel-files.sh (the command that produces the patch summary) to include --no-renames so rename/copy operations are never produced, ensuring consistency with the rejection logic in apply-bazel-files.sh (the case block matching " rename "*|" copy "*).
🤖 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/update-bazel-files.yml:
- Around line 71-80: The checkout step currently uses the mutable branch ref
(uses: actions/checkout@v6 with ref: ${{ steps.pr.outputs.head_ref }}), which
can change after the stale check; update it to check out the verified commit SHA
instead (use the PR head SHA output, e.g. ${{ steps.pr.outputs.head_sha }} or
the exact variable emitted by the pr step) so the workspace matches the exact
commit validated earlier; keep repository, fetch-depth, token and
persist-credentials as-is and continue to allow the later push to target
HEAD:${HEAD_REF} if needed.
---
Nitpick comments:
In @.github/scripts/apply-bazel-files.sh:
- Around line 39-46: The apply-bazel-files.sh script's case that rejects lines
matching " rename " or " copy " conflicts with how patches are generated in
package-bazel-files.sh; update the git diff invocation in package-bazel-files.sh
(the command that produces the patch summary) to include --no-renames so
rename/copy operations are never produced, ensuring consistency with the
rejection logic in apply-bazel-files.sh (the case block matching " rename "*|"
copy "*).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb2c8e6b-b885-4c85-a3eb-d55224b708a3
📒 Files selected for processing (4)
.github/scripts/apply-bazel-files.sh.github/scripts/package-bazel-files.sh.github/workflows/generate-bazel-files.yml.github/workflows/update-bazel-files.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/scripts/package-bazel-files.sh
ti-chi-bot
bot
added
size/L
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/update-bazel-files.yml (1)
63-72:⚠️ Potential issue | 🟠 MajorCheckout still uses mutable
head_refinstead of verified SHA.The stale check verifies
pr_head_sha == run_head_sha, but the checkout useshead_ref(the branch name). If the branch advances between the stale check and checkout, the workspace could diverge from what was validated. Use the verified SHA for checkout.🔒 Proposed fix: checkout by SHA
- name: Checkout PR branch if: steps.pr.outputs.same_repo == 'true' && steps.pr.outputs.stale != 'true' uses: actions/checkout@v6 with: repository: ${{ steps.pr.outputs.head_repo }} - ref: ${{ steps.pr.outputs.head_ref }} + ref: ${{ github.event.workflow_run.head_sha }} path: pr fetch-depth: 0 token: ${{ github.token }} persist-credentials: falseThen in the commit step, push to the branch name:
- git -C pr push "https://x-access-token:${PUSH_TOKEN}@github.com/${TARGET_REPOSITORY}.git" "HEAD:${HEAD_REF}" + git -C pr push "https://x-access-token:${PUSH_TOKEN}@github.com/${TARGET_REPOSITORY}.git" "HEAD:refs/heads/${HEAD_REF}"Note: You'll also need to output
run_head_shafrom the metadata step if not already available, or usegithub.event.workflow_run.head_shadirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/update-bazel-files.yml around lines 63 - 72, The checkout step currently uses the mutable branch name (steps/pr.outputs.head_ref) which can advance after the stale check; update the Checkout PR branch action to use the verified commit SHA (use the validated value such as pr_head_sha or run_head_sha / github.event.workflow_run.head_sha) as the with: ref so the workspace matches the validated commit (keep repository as steps/pr.outputs.head_repo and fetch-depth: 0); also ensure the later commit/push step still targets the branch name (steps/pr.outputs.head_ref) when pushing changes back.
🧹 Nitpick comments (3)
.github/workflows/update-bazel-files.yml (2)
20-22: Membership gate may still be warranted for defense-in-depth.While the architecture is now safer (no PR code execution), this workflow still has
contents: writeand pushes to the PR branch. A collaborator with write access to a same-repo branch could potentially craft a malicious artifact in the generate workflow. Consider adding an author association check as defense-in-depth.However, since the generate workflow runs on
pull_request(notpull_request_target), it cannot access secrets and runs in a sandboxed context. The patch is validated before application. The risk is significantly reduced compared to the previous architecture.🛡️ Optional: Add membership gate for defense-in-depth
This is optional given the improved architecture, but could be added if stricter controls are desired:
if: > github.event.workflow_run.conclusion == 'success' && github.event.workflow_run.event == 'pull_request' && github.event.workflow_run.actor.type == 'User'Note: Full membership checking would require an API call since
workflow_rundoesn't exposeauthor_associationdirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/update-bazel-files.yml around lines 20 - 22, Update the workflow run condition in .github/workflows/update-bazel-files.yml to add an author/actor check as a defense-in-depth: modify the existing if expression that currently uses github.event.workflow_run.conclusion and event to also require github.event.workflow_run.actor.type == 'User' (or call the GitHub API to validate author_association for stricter membership checking) so only runs triggered by genuine user actors or verified member associations will allow the workflow to push to the PR branch.
146-149: Consider using--force-with-leasefor safer push.The current push could overwrite concurrent commits. While the stale check mitigates this, using
--force-with-leasewould provide an additional safety net.🛡️ Proposed: Use force-with-lease
- git -C pr push "https://x-access-token:${PUSH_TOKEN}@github.com/${TARGET_REPOSITORY}.git" "HEAD:${HEAD_REF}" + git -C pr push --force-with-lease "https://x-access-token:${PUSH_TOKEN}@github.com/${TARGET_REPOSITORY}.git" "HEAD:${HEAD_REF}"This ensures the push fails if the remote ref has changed since checkout, preventing accidental overwrites of concurrent updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/update-bazel-files.yml around lines 146 - 149, Update the git push invocation to use a safe forced push variant: replace the current git -C pr push "https://x-access-token:${PUSH_TOKEN}@github.com/${TARGET_REPOSITORY}.git" "HEAD:${HEAD_REF}" with a push that uses --force-with-lease so the push will fail if the remote ref changed unexpectedly; keep the same remote URL and ref substitution but append --force-with-lease (optionally with the target ref) to ensure concurrent remote updates are not overwritten..github/workflows/generate-bazel-files.yml (1)
36-45: Potential issue:sedmodifications may fail silently on missing patterns.The
sed -icommands delete lines containing specific patterns. If those patterns don't exist in the files,sedsucceeds silently. This is likely intentional, but consider adding a comment explaining the purpose of these removals (stripping cache/service configuration for the unprivileged context).Also,
unset CIshould come before any commands that might read it—placement looks correct here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/generate-bazel-files.yml around lines 36 - 45, Add a short explanatory comment above the sed commands in the "Run Bazel Prepare" run block clarifying that the sed -i '/bazel-cache/d' DEPS.bzl and sed -i '/ats.apps.svc/d' WORKSPACE lines intentionally strip cache/service configuration for an unprivileged CI runner (and that sed succeeds silently if patterns are absent); keep the existing unset CI placement as-is. Reference the exact run block containing the sed invocations and the unset CI line so the comment sits immediately before those commands.
🤖 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/update-bazel-files.yml:
- Around line 63-72: The checkout step currently uses the mutable branch name
(steps/pr.outputs.head_ref) which can advance after the stale check; update the
Checkout PR branch action to use the verified commit SHA (use the validated
value such as pr_head_sha or run_head_sha / github.event.workflow_run.head_sha)
as the with: ref so the workspace matches the validated commit (keep repository
as steps/pr.outputs.head_repo and fetch-depth: 0); also ensure the later
commit/push step still targets the branch name (steps/pr.outputs.head_ref) when
pushing changes back.
---
Nitpick comments:
In @.github/workflows/generate-bazel-files.yml:
- Around line 36-45: Add a short explanatory comment above the sed commands in
the "Run Bazel Prepare" run block clarifying that the sed -i '/bazel-cache/d'
DEPS.bzl and sed -i '/ats.apps.svc/d' WORKSPACE lines intentionally strip
cache/service configuration for an unprivileged CI runner (and that sed succeeds
silently if patterns are absent); keep the existing unset CI placement as-is.
Reference the exact run block containing the sed invocations and the unset CI
line so the comment sits immediately before those commands.
In @.github/workflows/update-bazel-files.yml:
- Around line 20-22: Update the workflow run condition in
.github/workflows/update-bazel-files.yml to add an author/actor check as a
defense-in-depth: modify the existing if expression that currently uses
github.event.workflow_run.conclusion and event to also require
github.event.workflow_run.actor.type == 'User' (or call the GitHub API to
validate author_association for stricter membership checking) so only runs
triggered by genuine user actors or verified member associations will allow the
workflow to push to the PR branch.
- Around line 146-149: Update the git push invocation to use a safe forced push
variant: replace the current git -C pr push
"https://x-access-token:${PUSH_TOKEN}@github.com/${TARGET_REPOSITORY}.git"
"HEAD:${HEAD_REF}" with a push that uses --force-with-lease so the push will
fail if the remote ref changed unexpectedly; keep the same remote URL and ref
substitution but append --force-with-lease (optionally with the target ref) to
ensure concurrent remote updates are not overwritten.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f8633229-25cc-4a23-9845-1655f534b76d
📒 Files selected for processing (2)
.github/workflows/generate-bazel-files.yml.github/workflows/update-bazel-files.yml
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
|
(we have retelled some AI comments internally. Due to security consideration it's not publicly exposed) |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, lance6716, xhebox, YangKeao The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
1 similar comment
|
/retest |
5 participants
What problem does this PR solve?
Issue Number: ref #66999
Problem Summary:
update-bazel-files.ymlusedpull_request_targetto check out PR code and run repository-local build logic in a privileged context. That allowed untrusted PR changes to influence what ran with write access, and the workflow also passed more secret material than it needed.What changed and how does it work?
generate-bazel-files.ymlonpull_requestto runmake bazel_preparewithout write permissions or repository secretstidb_buildin the generate workflow to a trustedpingcap/tidbcommit instead of using a local action path from the PR workspacegit patch(bazel.patch) instead of copying generated files directlybazel_preparechanges unexpected paths outsideDEPS.bzl,**/*.bazel, and**/*.bzlupdate-bazel-files.ymlto aworkflow_runconsumer that downloads the patch, validates the touched paths and patch summary inline, and applies it to the PR branch without executing PR-local build scriptspersist-credentials: false, use onlygithub.token, and stop passingGCP_SA_KEYgithub.tokenis scoped topingcap/tidbTi Chi Robot <ti-community-prow-bot@tidb.io>Check List
Tests
Manual test steps:
git diff --check.ruby -e 'require "yaml"; YAML.load_file(".github/workflows/generate-bazel-files.yml"); YAML.load_file(".github/workflows/update-bazel-files.yml")'.make lint.Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.