Skip to content

Commit f9e715a

Browse files
aiqiaoyaiqiaoy
authored
block checking out fork pr for pull_request_target and workflow_run (#2454)
* block checking out fork pr for some events * address copilot and reviewer feedback * run prettier formatting * build * update urls * update readme * update description and url again * edit url one more time
1 parent df4cb1c commit f9e715a

10 files changed

Lines changed: 509 additions & 2 deletions

‎README.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,15 @@ Please refer to the [release page](https://github.com/actions/checkout/releases/
160160
# running from unless specified. Example URLs are https://github.com or
161161
# https://my-ghes-server.example.com
162162
github-server-url: ''
163+
164+
# Required to check out fork pull request code from a workflow triggered by
165+
# `pull_request_target` or `workflow_run`. These workflows run with the base
166+
# repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner
167+
# access; fetching and executing a fork's code in that trusted context commonly
168+
# leads to "pwn request" vulnerabilities. Set to `true` only after reviewing the
169+
# risks at https://gh.io/securely-using-pull_request_target.
170+
# Default: false
171+
allow-unsafe-pr-checkout: ''
163172
```
164173
<!-- end usage -->
165174

‎__test__/git-auth-helper.test.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1173,7 +1173,8 @@ async function setup(testName: string): Promise<void> {
11731173
sshUser: '',
11741174
workflowOrganizationId: 123456,
11751175
setSafeDirectory: true,
1176-
githubServerUrl: githubServerUrl
1176+
githubServerUrl: githubServerUrl,
1177+
allowUnsafePrCheckout: false
11771178
}
11781179
}
11791180

‎__test__/input-helper.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@ describe('input-helper tests', () => {
9191
expect(settings.repositoryOwner).toBe('some-owner')
9292
expect(settings.repositoryPath).toBe(gitHubWorkspace)
9393
expect(settings.setSafeDirectory).toBe(true)
94+
expect(settings.allowUnsafePrCheckout).toBe(false)
9495
})
9596

9697
it('qualifies ref', async () => {

‎__test__/unsafe-pr-checkout-helper.test.ts‎

Lines changed: 267 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,267 @@
1+
import * as github from '@actions/github'
2+
import {assertSafePrCheckout} from '../lib/unsafe-pr-checkout-helper'
3+
4+
// Shallow clone original @actions/github context
5+
const originalContext = {...github.context}
6+
const originalEventName = github.context.eventName
7+
const originalPayload = github.context.payload
8+
9+
const BASE_REPO_ID = 100
10+
const FORK_REPO_ID = 200
11+
const PR_HEAD_SHA = '1111111111111111111111111111111111111111'
12+
const PR_MERGE_SHA = '2222222222222222222222222222222222222222'
13+
const SAFE_BASE_SHA = '3333333333333333333333333333333333333333'
14+
const WORKFLOW_RUN_HEAD_COMMIT_SHA = '4444444444444444444444444444444444444444'
15+
const BASE_QUALIFIED_REPO = 'some-owner/some-repo'
16+
const FORK_QUALIFIED_REPO = 'another-repo/fork'
17+
18+
function setContext(eventName: string, payload: object): void {
19+
;(github.context as {eventName: string}).eventName = eventName
20+
;(github.context as {payload: object}).payload = payload
21+
}
22+
23+
function forkPullRequestTargetPayload(): object {
24+
return {
25+
repository: {id: BASE_REPO_ID},
26+
pull_request: {
27+
head: {
28+
sha: PR_HEAD_SHA,
29+
repo: {id: FORK_REPO_ID, full_name: FORK_QUALIFIED_REPO}
30+
},
31+
merge_commit_sha: PR_MERGE_SHA
32+
}
33+
}
34+
}
35+
36+
function sameRepoPullRequestTargetPayload(): object {
37+
return {
38+
repository: {id: BASE_REPO_ID},
39+
pull_request: {
40+
head: {
41+
sha: PR_HEAD_SHA,
42+
repo: {id: BASE_REPO_ID, full_name: BASE_QUALIFIED_REPO}
43+
},
44+
merge_commit_sha: PR_MERGE_SHA
45+
}
46+
}
47+
}
48+
49+
function forkWorkflowRunPayload(): object {
50+
return {
51+
repository: {id: BASE_REPO_ID},
52+
workflow_run: {
53+
event: 'pull_request',
54+
head_commit: {id: WORKFLOW_RUN_HEAD_COMMIT_SHA},
55+
head_repository: {id: FORK_REPO_ID, full_name: FORK_QUALIFIED_REPO}
56+
}
57+
}
58+
}
59+
60+
describe('unsafe-pr-checkout-helper', () => {
61+
beforeAll(() => {
62+
jest.spyOn(github.context, 'repo', 'get').mockReturnValue({
63+
owner: 'some-owner',
64+
repo: 'some-repo'
65+
})
66+
})
67+
68+
afterEach(() => {
69+
;(github.context as {eventName: string}).eventName = originalEventName
70+
;(github.context as {payload: object}).payload = originalPayload
71+
})
72+
73+
afterAll(() => {
74+
;(github.context as {eventName: string}).eventName =
75+
originalContext.eventName
76+
;(github.context as {payload: object}).payload = originalContext.payload
77+
jest.restoreAllMocks()
78+
})
79+
80+
it('allows pull_request events untouched', () => {
81+
setContext('pull_request', forkPullRequestTargetPayload())
82+
expect(() =>
83+
assertSafePrCheckout({
84+
qualifiedRepository: 'attacker/fork',
85+
ref: 'refs/pull/1/merge',
86+
commit: '',
87+
allowUnsafePrCheckout: false
88+
})
89+
).not.toThrow()
90+
})
91+
92+
it('allows pull_request_target default checkout (base branch)', () => {
93+
setContext('pull_request_target', forkPullRequestTargetPayload())
94+
expect(() =>
95+
assertSafePrCheckout({
96+
qualifiedRepository: BASE_QUALIFIED_REPO,
97+
ref: 'refs/heads/main',
98+
commit: SAFE_BASE_SHA,
99+
allowUnsafePrCheckout: false
100+
})
101+
).not.toThrow()
102+
})
103+
104+
it('allows same-repo pull_request_target checkout of PR head', () => {
105+
setContext('pull_request_target', sameRepoPullRequestTargetPayload())
106+
expect(() =>
107+
assertSafePrCheckout({
108+
qualifiedRepository: BASE_QUALIFIED_REPO,
109+
ref: '',
110+
commit: PR_HEAD_SHA,
111+
allowUnsafePrCheckout: false
112+
})
113+
).not.toThrow()
114+
})
115+
116+
it('refuses pull_request_target fork PR head SHA checkout', () => {
117+
setContext('pull_request_target', forkPullRequestTargetPayload())
118+
expect(() =>
119+
assertSafePrCheckout({
120+
qualifiedRepository: BASE_QUALIFIED_REPO,
121+
ref: '',
122+
commit: PR_HEAD_SHA,
123+
allowUnsafePrCheckout: false
124+
})
125+
).toThrow(/Refusing to check out fork pull request code/)
126+
})
127+
128+
it('refuses pull_request_target fork PR merge_commit_sha checkout', () => {
129+
setContext('pull_request_target', forkPullRequestTargetPayload())
130+
expect(() =>
131+
assertSafePrCheckout({
132+
qualifiedRepository: BASE_QUALIFIED_REPO,
133+
ref: '',
134+
commit: PR_MERGE_SHA,
135+
allowUnsafePrCheckout: false
136+
})
137+
).toThrow(/allow-unsafe-pr-checkout/)
138+
})
139+
140+
it('refuses pull_request_target fork PR ref pattern (head)', () => {
141+
setContext('pull_request_target', forkPullRequestTargetPayload())
142+
expect(() =>
143+
assertSafePrCheckout({
144+
qualifiedRepository: BASE_QUALIFIED_REPO,
145+
ref: 'refs/pull/42/head',
146+
commit: '',
147+
allowUnsafePrCheckout: false
148+
})
149+
).toThrow()
150+
})
151+
152+
it('refuses pull_request_target fork PR ref pattern (merge)', () => {
153+
setContext('pull_request_target', forkPullRequestTargetPayload())
154+
expect(() =>
155+
assertSafePrCheckout({
156+
qualifiedRepository: BASE_QUALIFIED_REPO,
157+
ref: 'refs/pull/42/merge',
158+
commit: '',
159+
allowUnsafePrCheckout: false
160+
})
161+
).toThrow()
162+
})
163+
164+
it('refuses pull_request_target when repository points at the fork', () => {
165+
setContext('pull_request_target', forkPullRequestTargetPayload())
166+
expect(() =>
167+
assertSafePrCheckout({
168+
qualifiedRepository: FORK_QUALIFIED_REPO,
169+
ref: 'refs/heads/main',
170+
commit: '',
171+
allowUnsafePrCheckout: false
172+
})
173+
).toThrow()
174+
})
175+
176+
it('allows pull_request_target checkout of an unrelated third-party repo', () => {
177+
setContext('pull_request_target', forkPullRequestTargetPayload())
178+
expect(() =>
179+
assertSafePrCheckout({
180+
qualifiedRepository: 'some-other/unrelated',
181+
ref: 'refs/heads/main',
182+
commit: '',
183+
allowUnsafePrCheckout: false
184+
})
185+
).not.toThrow()
186+
})
187+
188+
it('refuses pull_request_target ignoring repository case differences', () => {
189+
setContext('pull_request_target', forkPullRequestTargetPayload())
190+
expect(() =>
191+
assertSafePrCheckout({
192+
qualifiedRepository: FORK_QUALIFIED_REPO.toUpperCase(),
193+
ref: '',
194+
commit: '',
195+
allowUnsafePrCheckout: false
196+
})
197+
).toThrow()
198+
})
199+
200+
it('refuses pull_request_target ignoring commit SHA case differences', () => {
201+
setContext('pull_request_target', forkPullRequestTargetPayload())
202+
expect(() =>
203+
assertSafePrCheckout({
204+
qualifiedRepository: BASE_QUALIFIED_REPO,
205+
ref: '',
206+
commit: PR_HEAD_SHA.toUpperCase(),
207+
allowUnsafePrCheckout: false
208+
})
209+
).toThrow()
210+
})
211+
212+
it('allows pull_request_target fork PR checkout when opted in', () => {
213+
setContext('pull_request_target', forkPullRequestTargetPayload())
214+
expect(() =>
215+
assertSafePrCheckout({
216+
qualifiedRepository: BASE_QUALIFIED_REPO,
217+
ref: 'refs/pull/42/merge',
218+
commit: '',
219+
allowUnsafePrCheckout: true
220+
})
221+
).not.toThrow()
222+
})
223+
224+
it('refuses workflow_run fork PR head_commit.id checkout', () => {
225+
setContext('workflow_run', forkWorkflowRunPayload())
226+
expect(() =>
227+
assertSafePrCheckout({
228+
qualifiedRepository: BASE_QUALIFIED_REPO,
229+
ref: '',
230+
commit: WORKFLOW_RUN_HEAD_COMMIT_SHA,
231+
allowUnsafePrCheckout: false
232+
})
233+
).toThrow()
234+
})
235+
236+
it('refuses workflow_run with pull_request_target underlying event', () => {
237+
const payload = forkWorkflowRunPayload() as {
238+
workflow_run: {event: string}
239+
}
240+
payload.workflow_run.event = 'pull_request_target'
241+
setContext('workflow_run', payload)
242+
expect(() =>
243+
assertSafePrCheckout({
244+
qualifiedRepository: BASE_QUALIFIED_REPO,
245+
ref: '',
246+
commit: WORKFLOW_RUN_HEAD_COMMIT_SHA,
247+
allowUnsafePrCheckout: false
248+
})
249+
).toThrow()
250+
})
251+
252+
it('allows workflow_run same-repo PR (head_repository.id matches base)', () => {
253+
const payload = forkWorkflowRunPayload() as {
254+
workflow_run: {head_repository: {id: number}}
255+
}
256+
payload.workflow_run.head_repository.id = BASE_REPO_ID
257+
setContext('workflow_run', payload)
258+
expect(() =>
259+
assertSafePrCheckout({
260+
qualifiedRepository: BASE_QUALIFIED_REPO,
261+
ref: '',
262+
commit: WORKFLOW_RUN_HEAD_COMMIT_SHA,
263+
allowUnsafePrCheckout: false
264+
})
265+
).not.toThrow()
266+
})
267+
})

‎action.yml‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,15 @@ inputs:
9898
github-server-url:
9999
description: The base URL for the GitHub instance that you are trying to clone from, will use environment defaults to fetch from the same instance that the workflow is running from unless specified. Example URLs are https://github.com or https://my-ghes-server.example.com
100100
required: false
101+
allow-unsafe-pr-checkout:
102+
description: >
103+
Required to check out fork pull request code from a workflow triggered by
104+
`pull_request_target` or `workflow_run`. These workflows run with the
105+
base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and
106+
runner access; fetching and executing a fork's code in that trusted
107+
context commonly leads to "pwn request" vulnerabilities. Set to `true`
108+
only after reviewing the risks at https://gh.io/securely-using-pull_request_target.
109+
default: false
101110
outputs:
102111
ref:
103112
description: 'The branch, tag or SHA that was checked out'

0 commit comments

Comments
 (0)
⚡