ci: prevent script injection in comment workflow#48
Merged
Conversation
Read PR number and comment body via fs.readFileSync inside actions/github-script instead of piping artifact contents through GITHUB_ENV in shell. The previous 'Setup Environment' step was flagged by GitHub security review for potential injection of attacker-controlled data (PR title) from the workflow_run trigger. Match old comments by github.event.workflow_run.name (workflow YAML, trusted) instead of the user-supplied PR title from the artifact.
レビュアー向けガイド（小さな PR では折りたたみ）レビュアー向けガイドPR コメント用ワークフローをリファクタリングし、 更新後の PR コメント用ワークフローのやり取りのシーケンス図sequenceDiagram
actor GitHub as GitHub
participant WorkflowRun as CI_workflow_run
participant CommentWorkflow as Comment_workflow
participant DownloadArtifacts as Download_artifacts_action
participant GithubScript as Github_script_action
participant GitHubAPI as GitHub_issues_API
GitHub->>WorkflowRun: Execute CI workflow
WorkflowRun-->>GitHub: status, artifacts, conclusion
GitHub-->>CommentWorkflow: Trigger on workflow_run (completed)
CommentWorkflow->>DownloadArtifacts: actions/download-artifact with run-id
DownloadArtifacts-->>CommentWorkflow: pr_number.txt, comment.txt artifacts
CommentWorkflow->>GithubScript: Run actions/github-script
GithubScript->>GithubScript: Read pr_number/pr_number.txt via fs.readFileSync
GithubScript->>GithubScript: Sanitize to numeric prNumber
GithubScript->>GithubScript: Read comment/comment.txt via fs.readFileSync
GithubScript->>GitHubAPI: issues.listComments(owner, repo, issue_number = prNumber)
GitHubAPI-->>GithubScript: Existing PR comments
GithubScript->>GithubScript: Find oldComment where user.type == Bot and body includes WORKFLOW_NAME
GithubScript->>GithubScript: shouldComment = oldComment OR CONCLUSION == failure
alt shouldComment is false
GithubScript-->>CommentWorkflow: Exit without commenting
else shouldComment is true
alt oldComment exists
GithubScript->>GitHubAPI: issues.deleteComment(comment_id = oldComment.id)
GitHubAPI-->>GithubScript: Comment deleted
end
GithubScript->>GitHubAPI: issues.createComment(issue_number = prNumber, body = comment)
GitHubAPI-->>GithubScript: Comment created
end
GithubScript-->>CommentWorkflow: Step completes
CommentWorkflow-->>GitHub: Workflow completed
GitHub Actions ワークフローにおける新しいコメント投稿判定ロジックのフローチャートflowchart TD
A["Start github-script step"] --> B["Read pr_number/pr_number.txt as prNumber and sanitize to digits"]
B --> C["Read comment/comment.txt as comment"]
C --> D["List PR comments via GitHub API using prNumber"]
D --> E["Find oldComment where user.type is Bot and body includes WORKFLOW_NAME"]
E --> F["Compute shouldComment = oldComment exists OR CONCLUSION equals failure"]
F -->|"shouldComment is false"| G["Return without creating a comment"]
F -->|"shouldComment is true"| H{"Does oldComment exist"}
H -->|"Yes"| I["Delete oldComment via issues.deleteComment"]
H -->|"No"| J["Skip deletion"]
I --> K["Create new comment with body comment via issues.createComment"]
J --> K
K --> L["End github-script step"]
ファイル単位の変更内容
Tips やコマンドSourcery とのやり取り
体験のカスタマイズダッシュボード にアクセスして以下を行えます:
ヘルプを得る
Original review guide in EnglishReviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the PR comment workflow to eliminate shell-based environment setup and potential script injection by reading artifacts directly in the github-script step and matching existing comments using the trusted workflow name instead of a user-controlled PR title. Sequence diagram for updated PR comment workflow interactionsequenceDiagram
actor GitHub as GitHub
participant WorkflowRun as CI_workflow_run
participant CommentWorkflow as Comment_workflow
participant DownloadArtifacts as Download_artifacts_action
participant GithubScript as Github_script_action
participant GitHubAPI as GitHub_issues_API
GitHub->>WorkflowRun: Execute CI workflow
WorkflowRun-->>GitHub: status, artifacts, conclusion
GitHub-->>CommentWorkflow: Trigger on workflow_run (completed)
CommentWorkflow->>DownloadArtifacts: actions/download-artifact with run-id
DownloadArtifacts-->>CommentWorkflow: pr_number.txt, comment.txt artifacts
CommentWorkflow->>GithubScript: Run actions/github-script
GithubScript->>GithubScript: Read pr_number/pr_number.txt via fs.readFileSync
GithubScript->>GithubScript: Sanitize to numeric prNumber
GithubScript->>GithubScript: Read comment/comment.txt via fs.readFileSync
GithubScript->>GitHubAPI: issues.listComments(owner, repo, issue_number = prNumber)
GitHubAPI-->>GithubScript: Existing PR comments
GithubScript->>GithubScript: Find oldComment where user.type == Bot and body includes WORKFLOW_NAME
GithubScript->>GithubScript: shouldComment = oldComment OR CONCLUSION == failure
alt shouldComment is false
GithubScript-->>CommentWorkflow: Exit without commenting
else shouldComment is true
alt oldComment exists
GithubScript->>GitHubAPI: issues.deleteComment(comment_id = oldComment.id)
GitHubAPI-->>GithubScript: Comment deleted
end
GithubScript->>GitHubAPI: issues.createComment(issue_number = prNumber, body = comment)
GitHubAPI-->>GithubScript: Comment created
end
GithubScript-->>CommentWorkflow: Step completes
CommentWorkflow-->>GitHub: Workflow completed
Flow diagram for new comment decision logic in GitHub Actions workflowflowchart TD
A["Start github-script step"] --> B["Read pr_number/pr_number.txt as prNumber and sanitize to digits"]
B --> C["Read comment/comment.txt as comment"]
C --> D["List PR comments via GitHub API using prNumber"]
D --> E["Find oldComment where user.type is Bot and body includes WORKFLOW_NAME"]
E --> F["Compute shouldComment = oldComment exists OR CONCLUSION equals failure"]
F -->|"shouldComment is false"| G["Return without creating a comment"]
F -->|"shouldComment is true"| H{"Does oldComment exist"}
H -->|"Yes"| I["Delete oldComment via issues.deleteComment"]
H -->|"No"| J["Skip deletion"]
I --> K["Create new comment with body comment via issues.createComment"]
J --> K
K --> L["End github-script step"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - いくつか高レベルなフィードバックを残しました:
prNumberのパースまわりに簡単なガードを追加することを検討してください（例: 空や数値以外になった場合は例外を投げる、もしくは早期リターンするなど）。そうすることで、無効な issue 番号で GitHub API を呼び出すのではなく、早い段階で失敗させられます。fs.readFileSyncの読み取り失敗を扱うことを検討してもよいかもしれません（例: try/catch で明確なエラーメッセージを出すなど）。そうすることで、成果物が存在しない、あるいは壊れている場合に、ワークフローがより原因を特定しやすい形で失敗するようになります。- 古い bot コメントを見つけるのに
c.body.includes(process.env.WORKFLOW_NAME)に依存する方法は、同じワークフロー名を含むコメントが複数ある場合に脆くなり得ます。置き換える対象のコメントを識別するために、より固有なマーカー（例: 非表示の HTML コメントや固定のプレフィックス）を使うことを検討してください。
AI エージェント向けプロンプト
Please address the comments from this code review:
## Overall Comments
- Consider adding a simple guard around the `prNumber` parsing (e.g., throw or early return if it ends up empty or non-numeric) so we fail fast instead of making a GitHub API call with an invalid issue number.
- It might be worth handling read failures from `fs.readFileSync` (e.g., try/catch with a clear error message) so the workflow fails in a more diagnosable way if the artifacts are missing or malformed.
- Relying on `c.body.includes(process.env.WORKFLOW_NAME)` to find the old bot comment could be brittle if multiple comments contain the same workflow name; consider using a more specific marker (e.g., a hidden HTML comment or a fixed prefix) to identify the comment to replace.もっと役立てるように手助けしてください！各コメントに 👍 または 👎 を押していただけると、そのフィードバックをもとにレビューの質を改善していきます。
Original comment in English
Hey - I've left some high level feedback:
- Consider adding a simple guard around the
prNumberparsing (e.g., throw or early return if it ends up empty or non-numeric) so we fail fast instead of making a GitHub API call with an invalid issue number. - It might be worth handling read failures from
fs.readFileSync(e.g., try/catch with a clear error message) so the workflow fails in a more diagnosable way if the artifacts are missing or malformed. - Relying on
c.body.includes(process.env.WORKFLOW_NAME)to find the old bot comment could be brittle if multiple comments contain the same workflow name; consider using a more specific marker (e.g., a hidden HTML comment or a fixed prefix) to identify the comment to replace.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding a simple guard around the `prNumber` parsing (e.g., throw or early return if it ends up empty or non-numeric) so we fail fast instead of making a GitHub API call with an invalid issue number.
- It might be worth handling read failures from `fs.readFileSync` (e.g., try/catch with a clear error message) so the workflow fails in a more diagnosable way if the artifacts are missing or malformed.
- Relying on `c.body.includes(process.env.WORKFLOW_NAME)` to find the old bot comment could be brittle if multiple comments contain the same workflow name; consider using a more specific marker (e.g., a hidden HTML comment or a fixed prefix) to identify the comment to replace.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
1 participant
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.
Read PR number and comment body via fs.readFileSync inside actions/github-script
instead of piping artifact contents through GITHUB_ENV in shell. The previous
'Setup Environment' step was flagged by GitHub security review for potential
injection of attacker-controlled data (PR title) from the workflow_run trigger.
Match old comments by github.event.workflow_run.name (workflow YAML, trusted)
instead of the user-supplied PR title from the artifact.
Summary by Sourcery
スクリプトインジェクションに対して GitHub Actions のコメント用ワークフローを強化しつつ、プルリクエストへのコメント投稿方法を簡素化します。
CI:
GITHUB_ENVへのシェルコマンドによるエクスポートではなく、actions/github-scriptの中でワークフローアーティファクトから直接プルリクエスト番号とコメント本文を読み取るように変更。workflow_run.nameを使って既存のボットコメントを検出・置換し、コメントを作成する条件分岐ロジックを簡素化。Original summary in English
Summary by Sourcery
Harden the GitHub Actions comment workflow against script injection while simplifying how comments are posted to pull requests.
CI: