Skip to content

Commit 4a91920

Browse files
justin808claude
andcommitted
Fix CI comment trigger security and reliability issues
This addresses critical security and reliability issues identified in the initial implementation of the /run-full-ci comment trigger. **Security Fixes:** - Add permission check to restrict command to users with write access - Prevents resource abuse from external contributors - Posts informative message to unauthorized users **Reliability Improvements:** - Consolidate workflow triggers with better error handling - Post detailed results comment listing succeeded/failed workflows - Fail job if any workflows fail to trigger - Add concurrency controls to prevent duplicate runs per PR - Improve comment matching to require command at line start **Workflow Updates:** - Update main.yml, examples.yml to run minimum deps on workflow_dispatch - Update Pro workflows to honor workflow_dispatch events - Skip welcome comment for bot PRs (dependabot, renovate) **Documentation:** - Document security controls and access restrictions - Document concurrency protection - Explain workflow_dispatch behavior These changes ensure the feature is secure, reliable, and provides clear feedback when workflows fail to trigger. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 2f3942e commit 4a91920

File tree

7 files changed

+131
-73
lines changed

7 files changed

+131
-73
lines changed

.github/README.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,20 @@ By default, PRs run a subset of CI jobs to provide fast feedback:
3535

3636
This is intentional to keep PR feedback loops fast. However, before merging, you should verify compatibility across all supported versions. The `/run-full-ci` command makes this easy without waiting for the PR to be merged to master.
3737

38+
### Security & Access Control
39+
40+
**Only repository collaborators with write access can trigger full CI runs.** This prevents:
41+
42+
- Resource abuse from external contributors
43+
- Unauthorized access to Pro package tests
44+
- Potential DoS attacks via repeated CI runs
45+
46+
If an unauthorized user attempts to use `/run-full-ci`, they'll receive a message explaining the restriction.
47+
48+
### Concurrency Protection
49+
50+
Multiple `/run-full-ci` comments on the same PR will cancel in-progress runs to prevent resource waste and duplicate results.
51+
3852
## Testing Comment-Triggered Workflows
3953

4054
**Important**: Comment-triggered workflows (`issue_comment` event) only execute from the **default branch** (master). This creates a chicken-and-egg problem when developing workflow changes.

.github/workflows/examples.yml

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,17 +38,20 @@ jobs:
3838

3939
examples:
4040
needs: detect-changes
41-
if: github.ref == 'refs/heads/master' || needs.detect-changes.outputs.run_generators == 'true'
41+
if: github.ref == 'refs/heads/master' || github.event_name == 'workflow_dispatch' || needs.detect-changes.outputs.run_generators == 'true'
4242
strategy:
4343
fail-fast: false
4444
matrix:
4545
include:
4646
# Always run: Latest versions (fast feedback on PRs)
4747
- ruby-version: '3.4'
4848
dependency-level: 'latest'
49-
# Master only: Minimum supported versions (full coverage)
49+
# Master and workflow_dispatch: Minimum supported versions (full coverage)
5050
- ruby-version: '3.2'
5151
dependency-level: 'minimum'
52+
# On PRs (not workflow_dispatch), only run latest versions
53+
exclude:
54+
- ${{ github.event_name != 'workflow_dispatch' && github.ref != 'refs/heads/master' && matrix.dependency-level == 'minimum' || null }}
5255
env:
5356
SKIP_YARN_COREPACK_CHECK: 0
5457
BUNDLE_FROZEN: ${{ matrix.dependency-level == 'minimum' && 'false' || 'true' }}

.github/workflows/main.yml

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38,20 +38,23 @@ jobs:
3838

3939
build-dummy-app-webpack-test-bundles:
4040
needs: detect-changes
41-
# Run on master OR when tests needed on PR (but skip minimum deps on PR)
41+
# Run on master, workflow_dispatch, OR when tests needed on PR
4242
if: |
43-
(github.ref == 'refs/heads/master' || needs.detect-changes.outputs.run_dummy_tests == 'true')
43+
(github.ref == 'refs/heads/master' || github.event_name == 'workflow_dispatch' || needs.detect-changes.outputs.run_dummy_tests == 'true')
4444
strategy:
4545
matrix:
4646
include:
4747
# Always run: Latest versions (fast feedback on PRs)
4848
- ruby-version: '3.4'
4949
node-version: '22'
5050
dependency-level: 'latest'
51-
# Master only: Minimum supported versions (full coverage)
51+
# Master and workflow_dispatch: Minimum supported versions (full coverage)
5252
- ruby-version: '3.2'
5353
node-version: '20'
5454
dependency-level: 'minimum'
55+
# On PRs (not workflow_dispatch), only run latest versions
56+
exclude:
57+
- ${{ github.event_name != 'workflow_dispatch' && github.ref != 'refs/heads/master' && matrix.dependency-level == 'minimum' || null }}
5558
runs-on: ubuntu-22.04
5659
steps:
5760
- uses: actions/checkout@v4
@@ -122,9 +125,9 @@ jobs:
122125

123126
dummy-app-integration-tests:
124127
needs: [detect-changes, build-dummy-app-webpack-test-bundles]
125-
# Run on master OR when tests needed on PR (but skip minimum deps on PR)
128+
# Run on master, workflow_dispatch, OR when tests needed on PR
126129
if: |
127-
(github.ref == 'refs/heads/master' || needs.detect-changes.outputs.run_dummy_tests == 'true')
130+
(github.ref == 'refs/heads/master' || github.event_name == 'workflow_dispatch' || needs.detect-changes.outputs.run_dummy_tests == 'true')
128131
strategy:
129132
fail-fast: false
130133
matrix:
@@ -133,10 +136,13 @@ jobs:
133136
- ruby-version: '3.4'
134137
node-version: '22'
135138
dependency-level: 'latest'
136-
# Master only: Minimum supported versions (full coverage)
139+
# Master and workflow_dispatch: Minimum supported versions (full coverage)
137140
- ruby-version: '3.2'
138141
node-version: '20'
139142
dependency-level: 'minimum'
143+
# On PRs (not workflow_dispatch), only run latest versions
144+
exclude:
145+
- ${{ github.event_name != 'workflow_dispatch' && github.ref != 'refs/heads/master' && matrix.dependency-level == 'minimum' || null }}
140146
runs-on: ubuntu-22.04
141147
steps:
142148
- uses: actions/checkout@v4

.github/workflows/pr-welcome-comment.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ on:
66

77
jobs:
88
welcome:
9+
# Skip for bots (dependabot, renovate, etc.)
10+
if: github.event.pull_request.user.type != 'Bot'
911
runs-on: ubuntu-22.04
1012
permissions:
1113
pull-requests: write

.github/workflows/pro-integration-tests.yml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ jobs:
3434
# Build webpack test bundles for dummy app
3535
build-dummy-app-webpack-test-bundles:
3636
needs: detect-changes
37-
if: github.ref == 'refs/heads/master' || needs.detect-changes.outputs.run_pro_tests == 'true'
37+
if: github.ref == 'refs/heads/master' || github.event_name == 'workflow_dispatch' || needs.detect-changes.outputs.run_pro_tests == 'true'
3838
runs-on: ubuntu-22.04
3939
env:
4040
REACT_ON_RAILS_PRO_LICENSE: ${{ secrets.REACT_ON_RAILS_PRO_LICENSE }}
@@ -124,7 +124,7 @@ jobs:
124124
needs:
125125
- detect-changes
126126
- build-dummy-app-webpack-test-bundles
127-
if: github.ref == 'refs/heads/master' || needs.detect-changes.outputs.run_pro_tests == 'true'
127+
if: github.ref == 'refs/heads/master' || github.event_name == 'workflow_dispatch' || needs.detect-changes.outputs.run_pro_tests == 'true'
128128
runs-on: ubuntu-22.04
129129
env:
130130
REACT_ON_RAILS_PRO_LICENSE: ${{ secrets.REACT_ON_RAILS_PRO_LICENSE }}
@@ -304,7 +304,7 @@ jobs:
304304
needs:
305305
- detect-changes
306306
- build-dummy-app-webpack-test-bundles
307-
if: github.ref == 'refs/heads/master' || needs.detect-changes.outputs.run_pro_tests == 'true'
307+
if: github.ref == 'refs/heads/master' || github.event_name == 'workflow_dispatch' || needs.detect-changes.outputs.run_pro_tests == 'true'
308308
runs-on: ubuntu-22.04
309309
env:
310310
REACT_ON_RAILS_PRO_LICENSE: ${{ secrets.REACT_ON_RAILS_PRO_LICENSE }}

.github/workflows/pro-package-tests.yml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ jobs:
3434
# Build webpack test bundles for dummy app
3535
build-dummy-app-webpack-test-bundles:
3636
needs: detect-changes
37-
if: github.ref == 'refs/heads/master' || needs.detect-changes.outputs.run_pro_tests == 'true'
37+
if: github.ref == 'refs/heads/master' || github.event_name == 'workflow_dispatch' || needs.detect-changes.outputs.run_pro_tests == 'true'
3838
runs-on: ubuntu-22.04
3939
env:
4040
REACT_ON_RAILS_PRO_LICENSE: ${{ secrets.REACT_ON_RAILS_PRO_LICENSE }}
@@ -124,7 +124,7 @@ jobs:
124124
needs:
125125
- detect-changes
126126
- build-dummy-app-webpack-test-bundles
127-
if: github.ref == 'refs/heads/master' || needs.detect-changes.outputs.run_pro_tests == 'true'
127+
if: github.ref == 'refs/heads/master' || github.event_name == 'workflow_dispatch' || needs.detect-changes.outputs.run_pro_tests == 'true'
128128
runs-on: ubuntu-22.04
129129
# Redis service container
130130
services:
@@ -205,7 +205,7 @@ jobs:
205205
# RSpec tests for Pro package
206206
rspec-package-specs:
207207
needs: detect-changes
208-
if: github.ref == 'refs/heads/master' || needs.detect-changes.outputs.run_pro_tests == 'true'
208+
if: github.ref == 'refs/heads/master' || github.event_name == 'workflow_dispatch' || needs.detect-changes.outputs.run_pro_tests == 'true'
209209
strategy:
210210
matrix:
211211
ruby-version: ['3.3.7']

.github/workflows/run-skipped-ci.yml

Lines changed: 92 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -4,18 +4,62 @@ on:
44
issue_comment:
55
types: [created]
66

7+
# Prevent concurrent runs per PR
8+
concurrency:
9+
group: full-ci-${{ github.event.issue.number }}
10+
cancel-in-progress: true
11+
712
jobs:
813
trigger-full-ci:
914
# Only run on PR comments that match the command
1015
if: |
1116
github.event.issue.pull_request &&
12-
contains(github.event.comment.body, '/run-full-ci')
17+
(
18+
startsWith(github.event.comment.body, '/run-full-ci') ||
19+
contains(github.event.comment.body, '\n/run-full-ci')
20+
)
1321
runs-on: ubuntu-22.04
1422
permissions:
1523
contents: read
1624
pull-requests: write
1725
actions: write
1826
steps:
27+
- name: Check if user has write access
28+
id: check_access
29+
uses: actions/github-script@v7
30+
with:
31+
script: |
32+
try {
33+
const { data: permission } = await github.rest.repos.getCollaboratorPermissionLevel({
34+
owner: context.repo.owner,
35+
repo: context.repo.repo,
36+
username: context.actor
37+
});
38+
39+
const hasAccess = ['admin', 'write'].includes(permission.permission);
40+
console.log(`User ${context.actor} has permission: ${permission.permission}`);
41+
42+
if (!hasAccess) {
43+
await github.rest.issues.createComment({
44+
owner: context.repo.owner,
45+
repo: context.repo.repo,
46+
issue_number: context.issue.number,
47+
body: `@${context.actor} Sorry, only repository collaborators with write access can trigger full CI runs. 🔒`
48+
});
49+
}
50+
51+
return hasAccess;
52+
} catch (error) {
53+
console.error('Error checking permissions:', error);
54+
return false;
55+
}
56+
57+
- name: Exit if no access
58+
if: steps.check_access.outputs.result == 'false'
59+
run: |
60+
echo "User does not have permission to trigger full CI"
61+
exit 1
62+
1963
- name: Add reaction to comment
2064
uses: peter-evans/create-or-update-comment@v4
2165
with:
@@ -52,70 +96,59 @@ jobs:
5296
5397
View progress in the [Actions tab](${{ github.server_url }}/${{ github.repository }}/actions).
5498
55-
- name: Trigger main workflow
99+
- name: Trigger all workflows and collect results
100+
id: trigger_workflows
56101
uses: actions/github-script@v7
57102
with:
58103
script: |
59104
const prData = ${{ steps.pr.outputs.result }};
60-
try {
61-
await github.rest.actions.createWorkflowDispatch({
62-
owner: context.repo.owner,
63-
repo: context.repo.repo,
64-
workflow_id: 'main.yml',
65-
ref: prData.ref
66-
});
67-
console.log('✅ Triggered main.yml');
68-
} catch (error) {
69-
console.error('❌ Failed to trigger main.yml:', error.message);
70-
}
105+
const workflows = [
106+
'main.yml',
107+
'examples.yml',
108+
'pro-integration-tests.yml',
109+
'pro-package-tests.yml'
110+
];
71111
72-
- name: Trigger examples workflow
73-
uses: actions/github-script@v7
74-
with:
75-
script: |
76-
const prData = ${{ steps.pr.outputs.result }};
77-
try {
78-
await github.rest.actions.createWorkflowDispatch({
79-
owner: context.repo.owner,
80-
repo: context.repo.repo,
81-
workflow_id: 'examples.yml',
82-
ref: prData.ref
83-
});
84-
console.log('✅ Triggered examples.yml');
85-
} catch (error) {
86-
console.error('❌ Failed to trigger examples.yml:', error.message);
87-
}
112+
const succeeded = [];
113+
const failed = [];
88114
89-
- name: Trigger Pro integration tests
90-
uses: actions/github-script@v7
91-
with:
92-
script: |
93-
const prData = ${{ steps.pr.outputs.result }};
94-
try {
95-
await github.rest.actions.createWorkflowDispatch({
96-
owner: context.repo.owner,
97-
repo: context.repo.repo,
98-
workflow_id: 'pro-integration-tests.yml',
99-
ref: prData.ref
100-
});
101-
console.log('✅ Triggered pro-integration-tests.yml');
102-
} catch (error) {
103-
console.error('❌ Failed to trigger pro-integration-tests.yml:', error.message);
115+
for (const workflowId of workflows) {
116+
try {
117+
await github.rest.actions.createWorkflowDispatch({
118+
owner: context.repo.owner,
119+
repo: context.repo.repo,
120+
workflow_id: workflowId,
121+
ref: prData.ref
122+
});
123+
console.log(`✅ Triggered ${workflowId}`);
124+
succeeded.push(workflowId);
125+
} catch (error) {
126+
console.error(`❌ Failed to trigger ${workflowId}:`, error.message);
127+
failed.push(workflowId);
128+
}
104129
}
105130
106-
- name: Trigger Pro package tests
107-
uses: actions/github-script@v7
108-
with:
109-
script: |
110-
const prData = ${{ steps.pr.outputs.result }};
111-
try {
112-
await github.rest.actions.createWorkflowDispatch({
113-
owner: context.repo.owner,
114-
repo: context.repo.repo,
115-
workflow_id: 'pro-package-tests.yml',
116-
ref: prData.ref
117-
});
118-
console.log('✅ Triggered pro-package-tests.yml');
119-
} catch (error) {
120-
console.error('❌ Failed to trigger pro-package-tests.yml:', error.message);
131+
// Build the comment body
132+
const status = failed.length === 0 ? '✅ **Successfully triggered all workflows**' : '⚠️ **Some workflows failed to trigger**';
133+
const succeededList = succeeded.length > 0 ? succeeded.map(w => `- ✅ ${w}`).join('\n') : '- None';
134+
const failedList = failed.length > 0 ? `\n\n**Failed workflows:**\n${failed.map(w => `- ❌ ${w}`).join('\n')}` : '';
135+
136+
const body = `${status}
137+
138+
**Triggered workflows:**
139+
${succeededList}${failedList}
140+
141+
View progress in the [Actions tab](${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions).`;
142+
143+
// Post the comment
144+
await github.rest.issues.createComment({
145+
owner: context.repo.owner,
146+
repo: context.repo.repo,
147+
issue_number: context.issue.number,
148+
body: body
149+
});
150+
151+
// Fail the job if any workflows failed to trigger
152+
if (failed.length > 0) {
153+
core.setFailed(`Failed to trigger ${failed.length} workflow(s): ${failed.join(', ')}`);
121154
}

0 commit comments

Comments
 (0)