ci: make pkg.pr.new comment flow fork-safe#170
Merged
kongenpei merged 5 commits intolarksuite:mainfrom Apr 1, 2026
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughProducer workflow writes a JSON artifact with {pr, url, sourceRepo, sourceBranch}. A new consumer workflow triggers on the producer's successful run for PRs, downloads and validates the artifact, then creates or updates a bot comment containing the install guide marker on the PR. Changes
Sequence Diagram(s)sequenceDiagram
participant Producer as "Producer CI\n(pkg-pr-new)"
participant GH as "GitHub Actions\n(workflow_run)"
participant Consumer as "Comment CI\n(pkg-pr-new-comment)"
participant API as "GitHub API\n(issues/comments)"
Producer->>GH: complete workflow_run (success, PR)
GH->>Consumer: trigger workflow with run metadata
Consumer->>Consumer: list artifacts & download payload
Consumer->>Consumer: validate pr, url, sourceRepo/sourceBranch
Consumer->>API: list PR comments (paginated)
API-->>Consumer: return comments
alt marker found
Consumer->>API: update existing comment (contains marker)
else not found
Consumer->>API: create new bot comment with install/skill content
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
kongenpei
force-pushed
the
codex/pkg-pr-new-fork-safe-comment
branch
from
08dfd07 to
1347997
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/pkg-pr-new-comment.yml (2)
11-12: Trim token scope to least privilege.
pull-requests: writeappears unnecessary here since the workflow usesgithub.rest.issues.*comment APIs. Keeping onlyissues: writereduces write surface.🤖 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 11 - 12, Remove the unnecessary write scope to follow least-privilege: delete the "pull-requests: write" entry and keep only "issues: write" in the workflow permissions block (the keys shown as pull-requests: write and issues: write) since the job only calls github.rest.issues.* comment APIs.
37-39: Harden URL validation beyondstartsWith.
startsWith("https://pkg.pr.new/")is too permissive for a trusted write path. Parse withnew URL(url), require exact protocol/host, and reject whitespace/control characters before composing the install command.🔒 Suggested fix
- if (!url || !url.startsWith("https://pkg.pr.new/")) { - throw new Error(`Invalid package URL in payload: ${url}`); - } + let parsed; + try { + parsed = new URL(url); + } catch { + throw new Error(`Invalid package URL in payload: ${url}`); + } + if ( + parsed.protocol !== "https:" || + parsed.hostname !== "pkg.pr.new" || + /\s/.test(url) + ) { + throw new Error(`Invalid package URL in payload: ${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 37 - 39, Replace the brittle startsWith check for the payload URL with strict parsing using new URL(url): validate that the parsed URL.protocol === "https:" and parsed URL.host === "pkg.pr.new", ensure the original url has no leading/trailing whitespace or control characters (e.g. url.trim() === url and no control chars via a regex), and throw the same error if any check fails; apply these checks before composing or executing the install command so only exactly "https" + host "pkg.pr.new" URLs are accepted.
🤖 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 33-36: The workflow currently derives issueNumber from the
untrusted artifact payload.pr; update the logic to read the trusted PR number
from github.event.workflow_run.pull_requests[0].number (e.g. set issueNumber =
parseInt(process.env.GITHUB_EVENT_WORKFLOW_RUN_PULL_REQUEST_NUMBER) or access
event JSON directly) and only optionally compare it to payload.pr for sanity
checking; locate the block that reads payload (payload, url, issueNumber) and
replace assignment of issueNumber with the trusted workflow_run value (and if
you keep payload.pr, add an assert or warning if they differ).
---
Nitpick comments:
In @.github/workflows/pkg-pr-new-comment.yml:
- Around line 11-12: Remove the unnecessary write scope to follow
least-privilege: delete the "pull-requests: write" entry and keep only "issues:
write" in the workflow permissions block (the keys shown as pull-requests: write
and issues: write) since the job only calls github.rest.issues.* comment APIs.
- Around line 37-39: Replace the brittle startsWith check for the payload URL
with strict parsing using new URL(url): validate that the parsed URL.protocol
=== "https:" and parsed URL.host === "pkg.pr.new", ensure the original url has
no leading/trailing whitespace or control characters (e.g. url.trim() === url
and no control chars via a regex), and throw the same error if any check fails;
apply these checks before composing or executing the install command so only
exactly "https" + host "pkg.pr.new" URLs are accepted.
🪄 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: 72444251-968a-491c-b78c-ddff79aa6c97
📒 Files selected for processing (2)
.github/workflows/pkg-pr-new-comment.yml.github/workflows/pkg-pr-new.yml
Greptile SummaryThis PR splits the Key design decisions:
Confidence Score: 5/5Safe to merge — the fork-safe split is implemented correctly, all payload fields are re-validated in the trusted context, and the primary goal (posting comments from fork PRs) is verified to work by the author's own test run. No new P0 or P1 issues were found in this review. The PR correctly handles the empty No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Fork as Fork PR
participant UntrustedWF as pkg-pr-new.yml<br/>(pull_request, no write perms)
participant Artifacts as GitHub Artifacts
participant TrustedWF as pkg-pr-new-comment.yml<br/>(workflow_run, write perms)
participant PR as PR Comment
Fork->>UntrustedWF: push / sync PR
UntrustedWF->>UntrustedWF: build + publish to pkg.pr.new
UntrustedWF->>UntrustedWF: validate URL & PR number
UntrustedWF->>Artifacts: upload pkg-pr-new-comment-payload.json
UntrustedWF-->>TrustedWF: workflow_run completed (success)
TrustedWF->>Artifacts: listWorkflowRunArtifacts (actions:read)
Artifacts-->>TrustedWF: artifact found?
alt artifact found
TrustedWF->>Artifacts: download-artifact (run-id scoped)
Artifacts-->>TrustedWF: pkg-pr-new-comment-payload.json
TrustedWF->>TrustedWF: validate PR#, URL, sourceRepo/Branch
TrustedWF->>PR: upsert comment (pull-requests:write)
else not found
TrustedWF->>TrustedWF: core.notice + skip
end
Reviews (5): Last reviewed commit: "ci: allow PR comment workflow to write p..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/pkg-pr-new-comment.yml (1)
65-71:⚠️ Potential issue | 🟠 MajorDo not default to untrusted
payload.prforissueNumber.At Line 65, the trusted workflow still targets comments using artifact-controlled
payloadPrwhenworkflow_run.pull_requestsis empty. That keeps the write target partially untrusted in the privileged context.🔧 Suggested hardening
- const issueNumber = payloadPr; const runPrNumber = context.payload.workflow_run?.pull_requests?.[0]?.number; - if (Number.isInteger(runPrNumber) && runPrNumber !== issueNumber) { + let issueNumber = runPrNumber; + if (!Number.isInteger(issueNumber)) { + const headSha = context.payload.workflow_run?.head_sha; + if (typeof headSha !== "string" || headSha.length === 0) { + throw new Error("Unable to determine trusted PR number: missing workflow_run.head_sha"); + } + const prs = await github.paginate( + github.rest.repos.listPullRequestsAssociatedWithCommit, + { + owner: context.repo.owner, + repo: context.repo.repo, + commit_sha: headSha, + per_page: 100, + }, + ); + if (prs.length !== 1 || !Number.isInteger(prs[0]?.number)) { + throw new Error(`Unable to determine a unique trusted PR for head SHA: ${headSha}`); + } + issueNumber = prs[0].number; + } + if (payloadPr !== issueNumber) { throw new Error( `PR number mismatch between workflow_run (${runPrNumber}) and artifact payload (${issueNumber})`, ); }🤖 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 65 - 71, The code currently assigns the write target from the untrusted payloadPr (const issueNumber = payloadPr) even when workflow_run.pull_requests is empty; change this to trust workflow_run first: compute runPrNumber from context.payload.workflow_run?.pull_requests?.[0]?.number and, if Number.isInteger(runPrNumber), set issueNumber = runPrNumber; otherwise do NOT fall back to payloadPr—fail early or skip the privileged action (throw an Error or return) so privileged code never uses artifact-controlled payloadPr; update references to issueNumber accordingly (variables: issueNumber, payloadPr, runPrNumber, context.payload.workflow_run?.pull_requests).
🤖 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/pkg-pr-new-comment.yml:
- Around line 65-71: The code currently assigns the write target from the
untrusted payloadPr (const issueNumber = payloadPr) even when
workflow_run.pull_requests is empty; change this to trust workflow_run first:
compute runPrNumber from
context.payload.workflow_run?.pull_requests?.[0]?.number and, if
Number.isInteger(runPrNumber), set issueNumber = runPrNumber; otherwise do NOT
fall back to payloadPr—fail early or skip the privileged action (throw an Error
or return) so privileged code never uses artifact-controlled payloadPr; update
references to issueNumber accordingly (variables: issueNumber, payloadPr,
runPrNumber, context.payload.workflow_run?.pull_requests).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63c3676e-cd4b-4247-b640-ab773108dc34
📒 Files selected for processing (1)
.github/workflows/pkg-pr-new-comment.yml
liangshuo-1
approved these changes
Apr 1, 2026
tuxedomm
pushed a commit
that referenced
this pull request
Apr 3, 2026
* ci: make pkg.pr.new comment flow fork-safe * ci: harden trusted comment workflow inputs * ci: skip comment steps when payload artifact is missing * ci: use artifact PR number when workflow_run pull_requests is empty * ci: allow PR comment workflow to write pull requests --------- Co-authored-by: kongenpei <kongenpei@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
pkg.pr.newflow into two workflows:pull_requestworkflow builds/publishes and uploads a small comment payload artifactworkflow_runworkflow downloads payload and upserts the PR install guide commentactions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c(v8)actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02(v4)<!-- pkg-pr-new-install-guide -->) to update in place.Why
Fork PRs run with restricted token on
pull_request, so comment write fails with403 Resource not accessible by integration.This change moves write operations to trusted context while keeping PR code execution in untrusted context.
Security model
pull_requestworkflow.pkg.pr.newURL prefix + integer PR number).Verification
origin/mainand resolved workflow conflict.Summary by CodeRabbit
New Features
Chores