Skip to content

Commit 0f6d3ad

Browse files
Copilotdalito
Copilot
and
dalito
committed
Fix script injection vulnerabilities in PR checks workflow
- Sanitize pr.user.login to prevent injection (alphanumeric + hyphens only) - Change console.log to use comma-separated values instead of template literals - Add security documentation explaining mitigations - Add security header comment in workflow file - Update README with security references Co-authored-by: dalito <2648874+dalito@users.noreply.github.com>
1 parent 6cc0338 commit 0f6d3ad

3 files changed

Lines changed: 129 additions & 10 deletions

File tree

‎.github/workflows/README-pr-checks.md‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,14 @@ Automated GitHub Actions workflow that provides immediate feedback for:
1515
- PRs from fork's main branch (helpful workflow suggestion)
1616
- Organization account submissions (critical - blocks CI due to GitHub limitation)
1717

18+
**Security**: This workflow uses `pull_request_target` and implements script injection prevention. See `SECURITY.md` for details.
19+
20+
### `SECURITY.md`
21+
Documents security considerations and mitigations implemented in the workflow, including:
22+
- Script injection prevention
23+
- Sanitization of user-controlled data
24+
- Testing procedures with security tools
25+
1826
## What Gets Checked
1927

2028
### 1. Main Branch Submissions
@@ -44,7 +52,7 @@ Automated GitHub Actions workflow that provides immediate feedback for:
4452
1. **Helpful, Not Blocking**: Comments are informational only and don't prevent PR merging (except org accounts which can't work)
4553
2. **Friendly Tone**: Messages are welcoming and educational, not punitive
4654
3. **No Spam**: Comments are only posted once per PR
47-
4. **Secure**: Uses `pull_request_target` to safely work with forks while protecting secrets
55+
4. **Secure**: Uses `pull_request_target` safely with script injection prevention (user-controlled data is sanitized)
4856
5. **Lightweight**: Checks run quickly and don't burden CI resources
4957

5058
## Limitations

‎.github/workflows/SECURITY.md‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
# Security Considerations for PR Checks Workflow
2+
3+
## Overview
4+
5+
The `.github/workflows/pr-checks.yml` workflow uses `pull_request_target` which runs in the context of the base repository with write permissions. This is necessary to post comments on PRs from forks, but requires careful handling of user-controlled data to prevent security vulnerabilities.
6+
7+
## Security Mitigations Implemented
8+
9+
### 1. Script Injection Prevention
10+
11+
**Vulnerability**: User-controlled data (usernames, branch names, PR titles) could contain malicious content that gets executed if interpolated directly into template literals or strings.
12+
13+
**Mitigation**:
14+
- **Username sanitization**: GitHub usernames can only contain alphanumeric characters and hyphens. We sanitize by removing any other characters: `pr.user.login.replace(/[^a-zA-Z0-9-]/g, '')`
15+
- **Console logging**: Changed from template literals to comma-separated arguments: `console.log('Key:', value)` instead of `console.log(\`Key: \${value}\`)`
16+
- **No code execution**: User data is only used in comment bodies (Markdown), not in executable contexts
17+
18+
### 2. Minimal Permissions
19+
20+
The workflow uses the principle of least privilege:
21+
```yaml
22+
permissions:
23+
pull-requests: write # Only for posting comments
24+
contents: read # Only for reading PR metadata
25+
```
26+
27+
### 3. No Code Checkout from Forks
28+
29+
This workflow does NOT checkout code from the PR branch, avoiding the risk of executing malicious code from forks. It only:
30+
- Reads PR metadata via GitHub API
31+
- Posts comments to the PR
32+
33+
### 4. Limited Scope
34+
35+
The workflow only performs these actions:
36+
1. Detect if PR is from main branch (informational)
37+
2. Detect if PR is from organization account (critical)
38+
3. Post comments with guidance
39+
40+
No sensitive operations like deploying, publishing, or modifying code are performed.
41+
42+
## What Data is Safe to Use?
43+
44+
### Safe (GitHub-controlled):
45+
- `context.repo.owner` - Base repository owner
46+
- `context.repo.repo` - Base repository name
47+
- `pr.number` - PR number (numeric)
48+
- `pr.head.repo.owner.type` - Owner type (enum: "User" or "Organization")
49+
50+
### Requires Sanitization (User-controlled):
51+
- `pr.user.login` - Username (sanitize to alphanumeric + hyphens)
52+
- `pr.head.ref` - Branch name (can contain special characters)
53+
- `pr.head.repo.full_name` - Repository name (can be renamed)
54+
- `pr.title` - PR title (arbitrary text)
55+
- `pr.body` - PR description (arbitrary Markdown)
56+
57+
## Testing for Security Issues
58+
59+
### Recommended Tools:
60+
1. **zizmor**: Security linting for GitHub Actions
61+
```bash
62+
pip install zizmor
63+
zizmor .github/workflows/pr-checks.yml
64+
```
65+
66+
2. **actionlint**: General linting for GitHub Actions
67+
```bash
68+
actionlint .github/workflows/pr-checks.yml
69+
```
70+
71+
3. **Manual review**: Check for any use of `${}` with user-controlled data
72+
73+
### Test Cases:
74+
1. PR from user with unusual username (test sanitization)
75+
2. PR from branch with special characters in name
76+
3. PR with malicious content in title/description
77+
78+
## References
79+
80+
- [GitHub Actions Security Hardening](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions)
81+
- [Preventing Script Injection](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/)
82+
- [actions/github-script Security](https://github.com/actions/github-script#passing-inputs-to-the-script)
83+
84+
## Monitoring
85+
86+
Watch for:
87+
- Unexpected workflow failures
88+
- Comments with unusual formatting
89+
- GitHub security advisories related to Actions
90+
91+
## Updates
92+
93+
When modifying this workflow:
94+
1. ✅ Never use `${}` with user-controlled data in template literals
95+
2. ✅ Always sanitize usernames, branch names, and other user inputs
96+
3. ✅ Use console.log with comma-separated values, not template literals
97+
4. ✅ Test with edge cases (special characters, long inputs)
98+
5. ✅ Run security scanning tools before merging

‎.github/workflows/pr-checks.yml‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
# This workflow checks for common PR submission issues
22
# and provides helpful feedback to contributors
3+
#
4+
# Security Note: This workflow uses pull_request_target which runs in the context
5+
# of the base repository, not the fork. User-controlled data (PR titles, descriptions,
6+
# branch names, usernames) must be sanitized before use to prevent script injection.
37

48
name: PR Submission Checks
59

@@ -25,20 +29,23 @@ jobs:
2529
with:
2630
script: |
2731
const pr = context.payload.pull_request;
32+
33+
// Get PR details from context (these are all user-controlled)
2834
const headRef = pr.head.ref;
2935
const headRepo = pr.head.repo.full_name;
3036
const baseRepo = pr.base.repo.full_name;
3137
const isFork = headRepo !== baseRepo;
3238
const isFromMain = headRef === 'main';
3339
const isFromOrg = pr.head.repo.owner.type === 'Organization';
3440
35-
console.log(`PR #${pr.number} details:`);
36-
console.log(` Head branch: ${headRef}`);
37-
console.log(` Head repo: ${headRepo}`);
38-
console.log(` Base repo: ${baseRepo}`);
39-
console.log(` Is fork: ${isFork}`);
40-
console.log(` From main branch: ${isFromMain}`);
41-
console.log(` Owner type: ${pr.head.repo.owner.type}`);
41+
// Log details - no user input in template literals for security
42+
console.log('PR number:', pr.number);
43+
console.log('Head branch:', headRef);
44+
console.log('Head repo:', headRepo);
45+
console.log('Base repo:', baseRepo);
46+
console.log('Is fork:', isFork);
47+
console.log('From main branch:', isFromMain);
48+
console.log('Owner type:', pr.head.repo.owner.type);
4249
4350
core.setOutput('is_fork', isFork);
4451
core.setOutput('is_from_main', isFromMain);
@@ -77,10 +84,13 @@ jobs:
7784
return;
7885
}
7986
87+
// Sanitize username - GitHub usernames can only contain alphanumeric characters and hyphens
88+
const username = pr.user.login.replace(/[^a-zA-Z0-9-]/g, '');
89+
8090
// Post helpful comment
8191
const commentBody = `## ⚠️ Pull Request Submitted from Main Branch
8292
83-
Hi @${pr.user.login}! 👋
93+
Hi @${username}! 👋
8494

8595
Thank you for your contribution to voc4cat!
8696

@@ -148,9 +158,12 @@ For more information, see our [Contributing Guidelines](https://github.com/nfdi4
148158
return;
149159
}
150160
161+
// Sanitize username - GitHub usernames can only contain alphanumeric characters and hyphens
162+
const username = pr.user.login.replace(/[^a-zA-Z0-9-]/g, '');
163+
151164
const commentBody = `## ⚠️ Pull Request from Organization Account
152165
153-
Hi @${pr.user.login}! 👋
166+
Hi @${username}! 👋
154167

155168
We noticed that this pull request comes from an organization account rather than a personal account.
156169

0 commit comments

Comments
 (0)