temp: verify fork-safe pkg.pr.new comment workflow#171
temp: verify fork-safe pkg.pr.new comment workflow#171kongenpei wants to merge 17 commits intolarksuite:mainfrom
Conversation
feat: add build date in lark-cli -v output
This reverts commit e32883b.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds two GitHub Actions workflows and a build script to build a PR-preview package, publish it to pkg.pr.new, emit a payload artifact with package metadata, and post or update an install-guide comment on the originating pull request after the publish completes. Changes
Sequence Diagram(s)sequenceDiagram
actor PR as Pull Request
participant GH as GitHub Actions
participant Build as Build Script
participant Publish as pkg.pr.new
participant Artifact as Artifact Store
participant Comment as Comment Workflow
PR->>GH: trigger `pkg-pr-new.yml` on PR events
GH->>Build: run `scripts/build-pkg-pr-new.sh`
Build->>Build: produce binaries, launcher, package dir
GH->>Publish: `npx pkg-pr-new publish` (package)
Publish-->>GH: returns package `url`
GH->>GH: validate url & PR, write payload (`pkg-pr-new-comment-payload.json`)
GH->>Artifact: upload `pkg-pr-new-comment-payload`
Publish-->>GH: completes workflow_run (success)
Artifact->>Comment: workflow_run triggers `pkg-pr-new-comment.yml`
Comment->>Artifact: download payload artifact
Comment->>Comment: validate payload fields and URL
Comment->>PR: post or update bot comment with install guide
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
⚔️ Resolve merge conflicts
Comment |
Greptile SummaryThis temporary PR introduces a fork-safe
Confidence Score: 4/5Not safe to merge in current state — the published preview package is broken for all platforms except macOS Apple Silicon, and the fork-PR-number trust issue remains unresolved. Three previously flagged P1 issues (fork artifact fabrication, unpaginated artifact listing, darwin/amd64 missing binary) are unresolved, and a new P1 finding (Linux and Windows binaries not built at all) makes the preview package non-functional for the vast majority of users. Score kept at 4 rather than lower because the workflow split architecture is sound and the PR is explicitly temporary.
Important Files Changed
Sequence DiagramsequenceDiagram
participant Fork as Fork / PR Branch
participant GH as GitHub Actions
participant Artifact as Artifact Store
participant TrustedWF as pkg-pr-new-comment.yml (base branch, trusted)
participant PkgPrNew as pkg.pr.new CDN
participant PR as Pull Request Comment
Fork->>GH: push / pull_request event
GH->>GH: pkg-pr-new.yml triggered (runs in fork context, contents:read only)
GH->>GH: build-pkg-pr-new.sh (cross-compile darwin/arm64 only)
GH->>PkgPrNew: npx pkg-pr-new publish
PkgPrNew-->>GH: output.json (package URL)
GH->>GH: Build comment payload JSON {pr, url, sourceRepo, sourceBranch}
GH->>Artifact: Upload pkg-pr-new-comment-payload
Artifact-->>TrustedWF: workflow_run completed event
TrustedWF->>Artifact: listWorkflowRunArtifacts (per_page:100, no pagination)
TrustedWF->>Artifact: Download pkg-pr-new-comment-payload
TrustedWF->>TrustedWF: Validate pr / url / cross-check runPrNumber (empty for forks)
TrustedWF->>PR: createComment / updateComment on issue_number = payload.pr
Reviews (3): Last reviewed commit: "ci: limit pkg-pr-new build to darwin arm..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const runPrNumber = context.payload.workflow_run?.pull_requests?.[0]?.number; | ||
| if (Number.isInteger(runPrNumber) && runPrNumber !== issueNumber) { | ||
| throw new Error( | ||
| `PR number mismatch between workflow_run (${runPrNumber}) and artifact payload (${issueNumber})`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
For fork-originated PRs, context.payload.workflow_run.pull_requests is intentionally empty (GitHub omits it for security). This means runPrNumber is undefined, the mismatch check is never entered, and the pr field from the artifact is accepted unconditionally.
Because pkg-pr-new.yml runs in the fork's context for pull_request events, a contributor can modify that workflow file and upload an artifact with an arbitrary pr value — any positive integer will pass the Number.isInteger guard. The trusted comment workflow (pkg-pr-new-comment.yml, which always runs from the base branch) will then dutifully post or update a comment on that fabricated PR number.
A safer approach is to resolve the PR number inside the comment workflow itself, using workflow_run's head_sha to look it up via the GitHub API rather than trusting the untrusted artifact payload:
// Example: resolve PR number from head_sha instead of the artifact
const headSha = context.payload.workflow_run.head_sha;
const prs = await github.rest.pulls.list({
owner: context.repo.owner,
repo: context.repo.repo,
head: `${context.payload.workflow_run.head_repository.full_name.split("/")[0]}:${context.payload.workflow_run.head_branch}`,
state: "open",
per_page: 10,
});
const pr = prs.data.find(p => p.head.sha === headSha);
if (!pr) throw new Error("Could not resolve PR for workflow run");
const issueNumber = pr.number;Until this is addressed, the fork-safety claim for PR number integrity does not fully hold.
| const { data } = await github.rest.actions.listWorkflowRunArtifacts({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| run_id: runId, | ||
| per_page: 100, | ||
| }); | ||
| const found = Boolean( | ||
| data.artifacts?.some((artifact) => artifact.name === "pkg-pr-new-comment-payload") | ||
| ); |
There was a problem hiding this comment.
listWorkflowRunArtifacts is called with per_page: 100 but without pagination. If the triggering workflow run accumulates more than 100 artifacts, the pkg-pr-new-comment-payload artifact could be missed, causing the workflow to silently skip posting the comment. Using github.paginate here (just as it is used for listComments below) would make this robust:
| const { data } = await github.rest.actions.listWorkflowRunArtifacts({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| run_id: runId, | |
| per_page: 100, | |
| }); | |
| const found = Boolean( | |
| data.artifacts?.some((artifact) => artifact.name === "pkg-pr-new-comment-payload") | |
| ); | |
| const allArtifacts = await github.paginate( | |
| github.rest.actions.listWorkflowRunArtifacts, | |
| { | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| run_id: runId, | |
| per_page: 100, | |
| } | |
| ); | |
| const found = Boolean( | |
| allArtifacts?.some((artifact) => artifact.name === "pkg-pr-new-comment-payload") | |
| ); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/pkg-pr-new.yml (1)
30-31: Pin thepkg-pr-newCLI version.Line 31 resolves whatever
pkg-pr-newversion npm serves that day, so preview publishes stop being reproducible and an upstream regression can break this workflow without any repo change. Once this is more than a temporary validation flow, pin an exact version or vendor it as a dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pkg-pr-new.yml around lines 30 - 31, The workflow currently runs an unpinned npx command ("npx pkg-pr-new publish --no-compact --json output.json --comment=off ./.pkg-pr-new") which can pull changing upstream versions; update the step to pin an exact pkg-pr-new version (e.g., use npx pkg-pr-new@<version> publish ...) or vendor it as a repo dependency (add to devDependencies and invoke the local binary) so the publish step is reproducible and stable.
🤖 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/pkg-pr-new-comment.yml:
- Around line 13-16: The job "comment" can run concurrently for the same PR and
race when writing comments; add a concurrency gate to this job to serialize runs
for the same PR/source branch (use the job name "comment" and place a
concurrency block on that job) by creating a concurrency key that includes the
PR/branch identifier (e.g., github.event.pull_request.number or github.head_ref)
and enabling cancel-in-progress: true so older runs are canceled and only the
latest run can post/update the preview comment.
- Around line 54-118: The current check uses
context.payload.workflow_run?.pull_requests (runPrNumber) which is always
undefined, so a malicious artifact can specify any payloadPr; instead retrieve
the trusted PR(s) for the workflow run by calling the GitHub API for the run's
commit SHA (use context.payload.workflow_run?.head_sha) and compare those PR
numbers to payloadPr (use Octokit method like
repos.listPullRequestsAssociatedWithCommit); if no workflow_run/head_sha or no
matching PR is found, throw an error; update the logic around payloadPr,
runPrNumber and the mismatch check to use the API-derived PR number(s) before
proceeding to post the comment.
In `@scripts/build-pkg-pr-new.sh`:
- Around line 15-17: The script sets DATE to a full RFC3339 timestamp but
internal/build.Date is documented and expected as YYYY-MM-DD; change
DATE="$(date -u +%Y-%m-%d)" and keep LDFLAGS using -X
github.com/larksuite/cli/internal/build.Date=${DATE} (leave -X for build.Version
and build.Date intact) so the injected build.Date is date-only; alternatively if
you intend a timestamp, rename/document internal/build.Date to indicate it's a
timestamp and update references accordingly.
---
Nitpick comments:
In @.github/workflows/pkg-pr-new.yml:
- Around line 30-31: The workflow currently runs an unpinned npx command ("npx
pkg-pr-new publish --no-compact --json output.json --comment=off ./.pkg-pr-new")
which can pull changing upstream versions; update the step to pin an exact
pkg-pr-new version (e.g., use npx pkg-pr-new@<version> publish ...) or vendor it
as a repo dependency (add to devDependencies and invoke the local binary) so the
publish step is reproducible and stable.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7802a3e2-be2f-48c8-ac50-e8ef4bb8466b
📒 Files selected for processing (3)
.github/workflows/pkg-pr-new-comment.yml.github/workflows/pkg-pr-new.ymlscripts/build-pkg-pr-new.sh
| jobs: | ||
| comment: | ||
| if: github.event.workflow_run.conclusion == 'success' && github.event.workflow_run.event == 'pull_request' | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Serialize comment runs for the same PR/source branch.
Every successful preview run triggers this job. Without a concurrency gate or a “latest run only” check, two close pushes can race: one run can create a duplicate marker comment, or an older run can overwrite the newer install URL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pkg-pr-new-comment.yml around lines 13 - 16, The job
"comment" can run concurrently for the same PR and race when writing comments;
add a concurrency gate to this job to serialize runs for the same PR/source
branch (use the job name "comment" and place a concurrency block on that job) by
creating a concurrency key that includes the PR/branch identifier (e.g.,
github.event.pull_request.number or github.head_ref) and enabling
cancel-in-progress: true so older runs are canceled and only the latest run can
post/update the preview comment.
| const payload = JSON.parse(fs.readFileSync("pkg-pr-new-comment-payload.json", "utf8")); | ||
| const url = payload?.url; | ||
| const payloadPr = payload?.pr; | ||
| const sourceRepo = payload?.sourceRepo; | ||
| const sourceBranch = payload?.sourceBranch; | ||
| if (!Number.isInteger(payloadPr)) { | ||
| throw new Error(`Invalid PR number in artifact payload: ${payloadPr}`); | ||
| } | ||
| if (payloadPr <= 0) { | ||
| throw new Error(`Invalid PR number in artifact payload: ${payloadPr}`); | ||
| } | ||
| const issueNumber = payloadPr; | ||
| const runPrNumber = context.payload.workflow_run?.pull_requests?.[0]?.number; | ||
| if (Number.isInteger(runPrNumber) && runPrNumber !== issueNumber) { | ||
| throw new Error( | ||
| `PR number mismatch between workflow_run (${runPrNumber}) and artifact payload (${issueNumber})`, | ||
| ); | ||
| } | ||
|
|
||
| if (typeof url !== "string" || url.trim() !== url || /[\u0000-\u001F\u007F]/.test(url)) { | ||
| throw new Error(`Invalid package URL in payload: ${url}`); | ||
| } | ||
| let parsedUrl; | ||
| try { | ||
| parsedUrl = new URL(url); | ||
| } catch { | ||
| throw new Error(`Invalid package URL in payload: ${url}`); | ||
| } | ||
| if (parsedUrl.protocol !== "https:" || parsedUrl.hostname !== "pkg.pr.new") { | ||
| throw new Error(`Invalid package URL in payload: ${url}`); | ||
| } | ||
|
|
||
| const safeRepoPattern = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; | ||
| const safeBranchPattern = /^[A-Za-z0-9._\/-]+$/; | ||
| const hasSkillSource = | ||
| typeof sourceRepo === "string" && | ||
| typeof sourceBranch === "string" && | ||
| safeRepoPattern.test(sourceRepo) && | ||
| safeBranchPattern.test(sourceBranch); | ||
| const skillSection = hasSkillSource | ||
| ? [ | ||
| "", | ||
| "### 🧩 Skill update", | ||
| "", | ||
| "```bash", | ||
| `npx skills add ${sourceRepo}#${sourceBranch} -y -g`, | ||
| "```", | ||
| ] | ||
| : [ | ||
| "", | ||
| "### 🧩 Skill update", | ||
| "", | ||
| "_Unavailable for this PR because source repo/branch metadata is missing._", | ||
| ]; | ||
|
|
||
| const body = [ | ||
| "<!-- pkg-pr-new-install-guide -->", | ||
| "## 🚀 PR Preview Install Guide", | ||
| "", | ||
| "### 🧰 CLI update", | ||
| "", | ||
| "```bash", | ||
| `npm i -g ${url}`, | ||
| "```", | ||
| ...skillSection, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /root && find . -name "pkg-pr-new*.yml" -type f 2>/dev/null | head -20Repository: larksuite/cli
Length of output: 112
🏁 Script executed:
cat -n .github/workflows/pkg-pr-new-comment.ymlRepository: larksuite/cli
Length of output: 6647
🏁 Script executed:
cat -n .github/workflows/pkg-pr-new.ymlRepository: larksuite/cli
Length of output: 2770
🌐 Web query:
GitHub Actions workflow_run context pull_requests array documentation
💡 Result:
In GitHub Actions, when a workflow is triggered by the workflow_run event (e.g., on: workflow_run), the github context provides access to information about the triggering workflow run via github.event.workflow_run. This is an object containing details about the completed (or requested) workflow run, such as its ID, name, status, conclusion, head branch, head SHA, event that triggered it (e.g., "push"), actor, run number, and URL. There is no pull_requests array directly in the workflow_run context or github.event.workflow_run object, based on official documentation and webhook payload examples. The workflow_run event payload primarily describes the workflow run itself, not a list of associated pull requests. If the triggering workflow run was from a pull_request event, you can infer this from github.event.workflow_run.event == 'pull_request', and access details of that original pull request via the nested github.event.workflow_run.head_commit or by using the GitHub API with the workflow run's details (e.g., fetch the pull request associated with the head SHA if applicable). However, no direct pull_requests array exists in the context. To inspect the full structure in your workflow, dump the context: - name: Dump github.event run: echo '${{ toJSON(github.event) }}' Official sources confirm github.event mirrors the webhook payload for the event, and workflow_run payload samples (e.g., from webhook docs and third-party examples matching official structure) show no pull_requests array.
Citations:
- 1: https://docs.github.com/en/actions/reference/accessing-contextual-information-about-workflow-runs
- 2: https://docs.github.com/en/actions/reference/contexts-reference
- 3: https://docs.github.com/actions/using-workflows/events-that-trigger-workflows
- 4: https://docs.github.com/en/actions/reference/events-that-trigger-workflows
- 5: https://docs.github.com/en/webhooks/webhook-events-and-payloads
- 6: https://www.magicbell.com/workflows/github/workflow-run-completed
- 7: https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/trigger-a-workflow
🏁 Script executed:
rg "pull_requests" .github/workflows/Repository: larksuite/cli
Length of output: 187
🏁 Script executed:
rg "head_sha\|head_commit" .github/workflows/Repository: larksuite/cli
Length of output: 39
Do not use untrusted artifact payload to determine the PR to comment on.
The PR validation at line 67–71 is non-functional: context.payload.workflow_run?.pull_requests does not exist in the workflow_run context per GitHub Actions documentation, so runPrNumber is always undefined and the validation never executes. This allows a malicious fork that edits .github/workflows/pkg-pr-new.yml to set an arbitrary pr number in the artifact, causing the privileged job to comment on any PR with attacker-controlled install commands.
Resolve the PR from trusted workflow_run data (e.g., by looking up the PR associated with workflow_run.head_sha via the GitHub API) and verify it matches before commenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pkg-pr-new-comment.yml around lines 54 - 118, The current
check uses context.payload.workflow_run?.pull_requests (runPrNumber) which is
always undefined, so a malicious artifact can specify any payloadPr; instead
retrieve the trusted PR(s) for the workflow run by calling the GitHub API for
the run's commit SHA (use context.payload.workflow_run?.head_sha) and compare
those PR numbers to payloadPr (use Octokit method like
repos.listPullRequestsAssociatedWithCommit); if no workflow_run/head_sha or no
matching PR is found, throw an error; update the logic around payloadPr,
runPrNumber and the mismatch check to use the API-derived PR number(s) before
proceeding to post the comment.
| DATE="$(date -u +%Y-%m-%dT%H:%M:%SZ)" | ||
| SHA="$(git rev-parse --short HEAD)" | ||
| LDFLAGS="-s -w -X github.com/larksuite/cli/internal/build.Version=${VERSION}-${SHA} -X github.com/larksuite/cli/internal/build.Date=${DATE}" |
There was a problem hiding this comment.
Keep internal/build.Date date-only.
internal/build/build.go documents Date as YYYY-MM-DD, but Lines 15-17 now inject a full YYYY-MM-DDTHH:MM:SSZ timestamp. Anything parsing build.Date as a date-only string will drift here. Use a date-only value, or rename/document the field as a timestamp instead.
Minimal fix
-DATE="$(date -u +%Y-%m-%dT%H:%M:%SZ)"
+DATE="$(date -u +%Y-%m-%d)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DATE="$(date -u +%Y-%m-%dT%H:%M:%SZ)" | |
| SHA="$(git rev-parse --short HEAD)" | |
| LDFLAGS="-s -w -X github.com/larksuite/cli/internal/build.Version=${VERSION}-${SHA} -X github.com/larksuite/cli/internal/build.Date=${DATE}" | |
| DATE="$(date -u +%Y-%m-%d)" | |
| SHA="$(git rev-parse --short HEAD)" | |
| LDFLAGS="-s -w -X github.com/larksuite/cli/internal/build.Version=${VERSION}-${SHA} -X github.com/larksuite/cli/internal/build.Date=${DATE}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build-pkg-pr-new.sh` around lines 15 - 17, The script sets DATE to a
full RFC3339 timestamp but internal/build.Date is documented and expected as
YYYY-MM-DD; change DATE="$(date -u +%Y-%m-%d)" and keep LDFLAGS using -X
github.com/larksuite/cli/internal/build.Date=${DATE} (leave -X for build.Version
and build.Date intact) so the injected build.Date is date-only; alternatively if
you intend a timestamp, rename/document internal/build.Date to indicate it's a
timestamp and update references accordingly.
| CGO_ENABLED=0 GOOS="$goos" GOARCH="$goarch" go build -trimpath -ldflags "$LDFLAGS" -o "$output" ./main.go | ||
| } | ||
|
|
||
| build_target darwin arm64 |
There was a problem hiding this comment.
The build script calls build_target exactly once — darwin arm64. The run.js launcher, however, advertises support for five platform/arch combinations:
process.platform |
process.arch |
Expected binary |
|---|---|---|
linux |
x64 |
lark-cli-linux-amd64 |
linux |
arm64 |
lark-cli-linux-arm64 |
win32 |
x64 |
lark-cli-windows-amd64.exe |
win32 |
arm64 |
lark-cli-windows-arm64.exe |
darwin |
x64 |
lark-cli-darwin-amd64 |
None of these are built. Because both the platform string (linux, windows) and the arch string (amd64, arm64) appear as valid keys in platformMap / archMap, the "unsupported platform" guard on lines 55-58 of run.js never fires. The binary path is constructed successfully, execFileSync throws ENOENT, and the catch block silently exits with code 1 — no helpful message is shown to the user.
This means the published preview package is fully broken for every platform except macOS Apple Silicon. The CI runner itself (ubuntu-latest) would fail if it tried to consume the package. Add the missing build_target calls:
build_target linux amd64
build_target linux arm64
build_target darwin arm64
build_target windows amd64
build_target windows arm641 participant
Temporary PR for validating pkg.pr.new comment workflow behavior from fork main.\n\nIncludes:\n- split trusted comment workflow_run flow\n- artifact-gated comment execution\n- fork-safe PR number source from artifact payload\n\nWill close after verification.
Summary by CodeRabbit