Conversation
- Use toJSON() to escape user-provided PR body content - Use toJSON() to escape github.actor username - Properly quote variables in bash heredoc - Fixes Wiz security alert for potential script injection Co-authored-by: Michael Matloka <dev@twixes.com>
|
Cursor Agent can help with this pull request. Just |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 No Changeset FoundThis PR doesn't include a changeset. A changeset (and the release label) is required to release a new version. How to add a changesetRun this command and follow the prompts: pnpm changesetRemember: Never use |
| fi | ||
| env: | ||
| RAW_BODY: ${{ github.event.pull_request.body }} | ||
| RAW_BODY: ${{ toJSON(github.event.pull_request.body) }} |
There was a problem hiding this comment.
toJSON() breaks empty-body detection
toJSON() wraps the value in JSON encoding, so an empty PR body becomes the string "" (two quote characters) and a null body becomes the string null. Neither of these are empty/whitespace-only, so the [[ -z "${FILTERED_BODY//[[:space:]]/}" ]] check on line 24 will never evaluate to true for PRs with no description — the shaming logic is effectively dead.
Additionally, toJSON() collapses multi-line content into a single line with literal \n escape sequences. The sed filter on lines 19–21 was designed to work line-by-line (stripping ##, *, - [ lines). With everything on one line, if the body contains ## or * anywhere, sed deletes the entire body — causing valid descriptions to be incorrectly flagged as empty.
A safer approach that preserves the injection protection while keeping the detection logic working would be to keep the env var indirection (which already prevents injection) but drop toJSON():
| RAW_BODY: ${{ toJSON(github.event.pull_request.body) }} | |
| RAW_BODY: ${{ github.event.pull_request.body }} |
Passing user input through env: and referencing it as $RAW_BODY in the script (rather than inline ${{ }} in run:) is already sufficient to prevent script injection, since the value is set as an environment variable rather than interpolated into the shell script.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/new-pr.yml
Line: 30
Comment:
**`toJSON()` breaks empty-body detection**
`toJSON()` wraps the value in JSON encoding, so an empty PR body becomes the string `""` (two quote characters) and a null body becomes the string `null`. Neither of these are empty/whitespace-only, so the `[[ -z "${FILTERED_BODY//[[:space:]]/}" ]]` check on line 24 will never evaluate to true for PRs with no description — the shaming logic is effectively dead.
Additionally, `toJSON()` collapses multi-line content into a single line with literal `\n` escape sequences. The `sed` filter on lines 19–21 was designed to work line-by-line (stripping `##`, `*`, `- [` lines). With everything on one line, if the body contains `## ` or `*` anywhere, `sed` deletes the **entire** body — causing valid descriptions to be incorrectly flagged as empty.
A safer approach that preserves the injection protection while keeping the detection logic working would be to keep the env var indirection (which already prevents injection) but drop `toJSON()`:
```suggestion
RAW_BODY: ${{ github.event.pull_request.body }}
```
Passing user input through `env:` and referencing it as `$RAW_BODY` in the script (rather than inline `${{ }}` in `run:`) is already sufficient to prevent script injection, since the value is set as an environment variable rather than interpolated into the shell script.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
The previous commit ( In practice, Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: .github/workflows/new-pr.yml
Line: 37-38
Comment:
**Inline `${{ }}` in `run:` reverts security fix**
The previous commit (`61f1fd1`) moved `github.actor` and `github.event.pull_request.number` into `env:` variables to prevent script injection. This commit reverts that by putting `${{ github.actor }}` and `${{ github.event.pull_request.number }}` back inline in the `run:` block.
In practice, `github.actor` is restricted to alphanumeric characters and hyphens by GitHub's username rules, and `pull_request.number` is always an integer, so this isn't exploitable. However, using `env:` indirection is the [recommended best practice](https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-an-intermediate-environment-variable) and was the explicit intent of the security fix. Consider keeping the env var approach for defense-in-depth.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
|
Size Change: -585 B (-0.01%) Total Size: 6.02 MB
ℹ️ View Unchanged
|
Problem
The
new-pr.ymlGitHub workflow was off.Changes
Fix the workflow.