Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 148 additions & 0 deletions .github/workflows/pkg-pr-new-comment.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
name: PR Preview Package Comment

on:
workflow_run:
workflows: ["PR Preview Package"]
types: [completed]

permissions:
actions: read
contents: read
issues: write

jobs:
comment:
if: github.event.workflow_run.conclusion == 'success' && github.event.workflow_run.event == 'pull_request'
runs-on: ubuntu-latest
Comment on lines +13 to +16
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


steps:
- name: Check comment payload artifact
id: payload
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1
with:
script: |
const runId = context.payload.workflow_run?.id;
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")
);
Comment on lines +25 to +33
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Artifact listing not paginated

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:

Suggested change
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")
);

core.setOutput("found", found ? "true" : "false");
if (!found) {
core.notice("No comment payload artifact found for this run; skipping comment.");
}

- name: Download comment payload
if: steps.payload.outputs.found == 'true'
uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8
with:
name: pkg-pr-new-comment-payload
repository: ${{ github.repository }}
run-id: ${{ github.event.workflow_run.id }}
github-token: ${{ github.token }}

- name: Comment install command
if: steps.payload.outputs.found == 'true'
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1
with:
script: |
const fs = require("fs");
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})`,
);
}
Comment on lines +66 to +71
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Fork can fabricate the artifact PR number

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.


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,
Comment on lines +54 to +118
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cd /root && find . -name "pkg-pr-new*.yml" -type f 2>/dev/null | head -20

Repository: larksuite/cli

Length of output: 112


🏁 Script executed:

cat -n .github/workflows/pkg-pr-new-comment.yml

Repository: larksuite/cli

Length of output: 6647


🏁 Script executed:

cat -n .github/workflows/pkg-pr-new.yml

Repository: 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:


🏁 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.

].join("\n");

const comments = await github.paginate(github.rest.issues.listComments, {
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: issueNumber,
per_page: 100,
});

const existing = comments.find((comment) =>
comment.user?.login === "github-actions[bot]" &&
typeof comment.body === "string" &&
comment.body.includes("<!-- pkg-pr-new-install-guide -->")
);

if (existing) {
await github.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: existing.id,
body,
});
} else {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: issueNumber,
body,
});
}
71 changes: 71 additions & 0 deletions .github/workflows/pkg-pr-new.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
name: PR Preview Package

on:
pull_request:
types: [opened, synchronize, reopened, ready_for_review]
branches: [main]

permissions:
contents: read

jobs:
publish:
if: github.event.pull_request.draft == false
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4

- uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5
with:
go-version-file: go.mod

- uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4
with:
node-version: lts/*

- name: Build preview package
run: ./scripts/build-pkg-pr-new.sh

- name: Publish to pkg.pr.new
run: npx pkg-pr-new publish --no-compact --json output.json --comment=off ./.pkg-pr-new

- name: Build comment payload
env:
PR_NUMBER: ${{ github.event.pull_request.number }}
SOURCE_REPO: ${{ github.event.pull_request.head.repo.full_name }}
SOURCE_BRANCH: ${{ github.event.pull_request.head.ref }}
run: |
node <<'NODE'
const fs = require("fs");

const output = JSON.parse(fs.readFileSync("output.json", "utf8"));
const url = output?.packages?.[0]?.url;
if (!url) throw new Error("No package URL found in output.json");
if (!url.startsWith("https://pkg.pr.new/")) {
throw new Error(`Unexpected package URL: ${url}`);
}

const pr = Number(process.env.PR_NUMBER);
if (!Number.isInteger(pr) || pr <= 0) {
throw new Error(`Invalid PR_NUMBER: ${process.env.PR_NUMBER}`);
}

const payload = {
pr,
url,
sourceRepo: process.env.SOURCE_REPO || "",
sourceBranch: process.env.SOURCE_BRANCH || "",
};

fs.writeFileSync(
"pkg-pr-new-comment-payload.json",
JSON.stringify(payload),
);
NODE

- name: Upload comment payload
uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4
with:
name: pkg-pr-new-comment-payload
path: pkg-pr-new-comment-payload.json
97 changes: 97 additions & 0 deletions scripts/build-pkg-pr-new.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
#!/usr/bin/env bash
set -euo pipefail

ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
OUT_DIR="$ROOT_DIR/.pkg-pr-new"

cd "$ROOT_DIR"

python3 scripts/fetch_meta.py

rm -rf "$OUT_DIR"
mkdir -p "$OUT_DIR/bin" "$OUT_DIR/scripts"

VERSION="$(node -p "require('./package.json').version")"
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}"
Comment on lines +15 to +17
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.


build_target() {
local goos="$1"
local goarch="$2"
local ext=""
if [[ "$goos" == "windows" ]]; then
ext=".exe"
fi

local output="$OUT_DIR/bin/lark-cli-${goos}-${goarch}${ext}"
echo "Building ${goos}/${goarch} -> ${output}"
CGO_ENABLED=0 GOOS="$goos" GOARCH="$goarch" go build -trimpath -ldflags "$LDFLAGS" -o "$output" ./main.go
}

build_target darwin arm64
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Linux and Windows binaries are never built

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 arm64


cat > "$OUT_DIR/scripts/run.js" <<'RUNJS'
#!/usr/bin/env node
const path = require("path");
const { execFileSync } = require("child_process");

const isWindows = process.platform === "win32";

const platformMap = {
darwin: "darwin",
linux: "linux",
win32: "windows",
};

const archMap = {
x64: "amd64",
arm64: "arm64",
};

const platform = platformMap[process.platform];
const arch = archMap[process.arch];

if (!platform || !arch) {
console.error(`Unsupported platform: ${process.platform}-${process.arch}`);
process.exit(1);
}

const ext = isWindows ? ".exe" : "";
const binary = path.join(__dirname, "..", "bin", `lark-cli-${platform}-${arch}${ext}`);

try {
execFileSync(binary, process.argv.slice(2), { stdio: "inherit" });
} catch (err) {
process.exit(err.status || 1);
}
RUNJS

chmod +x "$OUT_DIR/scripts/run.js"

cat > "$OUT_DIR/package.json" <<EOF_JSON
{
"name": "@larksuite/cli",
"version": "${VERSION}-pr.${SHA}",
"description": "The official CLI for Lark/Feishu open platform (PR preview build)",
"bin": {
"lark-cli": "scripts/run.js"
},
"repository": {
"type": "git",
"url": "git+https://github.com/larksuite/cli.git"
},
"license": "MIT",
"files": [
"bin",
"scripts/run.js",
"CHANGELOG.md",
"LICENSE"
]
}
EOF_JSON

cp CHANGELOG.md "$OUT_DIR/CHANGELOG.md"
cp LICENSE "$OUT_DIR/LICENSE"

echo "Prepared pkg.pr.new package at $OUT_DIR"