Compare commits

...

4 Commits

Author SHA1 Message Date
brian m. carlson f50a2b05b7
Merge 02ade5d400 into 12cd2235ef 2026-07-16 08:24:48 -04:00
Aiqiao Yan 12cd2235ef
trim only ascii whitespace for branch (#2521)
* trim only ascii whitespace for branch

* rebuild
2026-07-15 15:05:42 -04:00
Aiqiao Yan 62661c4e71
skip running unsafe pr check if input is default (#2518) 2026-07-15 10:02:03 -04:00
brian m. carlson 02ade5d400
Don't overwrite annotated tags with commit object
When checking out a repository with full history, a full clone is done
and then the ref is finally updated to point to the commit that caused
the workflow to be run.  Normally, this is a good protection against
someone pushing to the repository twice in short succession, but it
causes problems with annotated tags.

Specifically, because the entry in refs/tags is set to the commit hash,
if an annotated tag was used, the tag is turned merely into a
lightweight one, which breaks `git describe`.  Every other tag in the
repository will continue to remain a valid annotated tag except the one
for which the workflow was invoked, which is not what the user expected.

Let's work around this by not performing a fetch if what we're fetching
is a tag.  Technically, annotated tags can be anywhere in the hierarchy
at any ref, but this should work as a suitable heuristic for now.

Note that the proper solution would be to expose the revision of the
actual object and check against that instead of the commit, but it
doesn't presently appear that that information is exposed.  Also, we
explicitly do not case-fold since Git refs are case sensitive.
2022-02-14 23:18:53 +00:00
4 changed files with 153 additions and 21 deletions

View File

@ -24,9 +24,20 @@ const mockGithubContext: any = {
payload: {}
}
// Replicate @actions/core getInput behavior: it trims whitespace by default
// (String.prototype.trim(), which strips characters such as a leading U+FEFF BOM)
// unless trimWhitespace is explicitly set to false.
const getInputImpl = (name: string, options?: {trimWhitespace?: boolean}) => {
const val = inputs[name] ?? ''
if (options && options.trimWhitespace === false) {
return val
}
return typeof val === 'string' ? val.trim() : val
}
// Mock @actions/core before loading input-helper
jest.unstable_mockModule('@actions/core', () => ({
getInput: jest.fn((name: string) => inputs[name]),
getInput: jest.fn(getInputImpl),
getBooleanInput: jest.fn((name: string) => inputs[name]),
getMultilineInput: jest.fn((name: string) =>
inputs[name] ? String(inputs[name]).split('\n').filter(Boolean) : []
@ -76,9 +87,7 @@ describe('input-helper tests', () => {
inputs = {}
jest.clearAllMocks()
// Re-apply default mocks
;(core.getInput as jest.Mock<any>).mockImplementation(
(name: string) => inputs[name]
)
;(core.getInput as jest.Mock<any>).mockImplementation(getInputImpl as any)
mockDirectoryExistsSync.mockImplementation(
(p: string) => p === gitHubWorkspace
)
@ -176,8 +185,84 @@ describe('input-helper tests', () => {
expect(settings.commit).toBeFalsy()
})
it('does not reclassify a ref as sha when a BOM is prefixed', async () => {
// A fork branch named "<U+FEFF>" + 40 hex chars. core.getInput trims the
// BOM by default, which previously collapsed this into a bare SHA and
// bypassed the unsafe fork PR checkout guard.
inputs.ref = '\uFEFF522d932fae5296da51fdf431934425ecf891c6a2'
const settings: IGitSourceSettings = await inputHelper.getInputs()
expect(settings.commit).toBeFalsy()
expect(settings.ref).toBe('522d932fae5296da51fdf431934425ecf891c6a2')
})
it('does not reclassify a sha-256 ref as sha when a BOM is prefixed', async () => {
inputs.ref =
'\uFEFF1111111111222222222233333333334444444444555555555566666666667777'
const settings: IGitSourceSettings = await inputHelper.getInputs()
expect(settings.commit).toBeFalsy()
expect(settings.ref).toBe(
'1111111111222222222233333333334444444444555555555566666666667777'
)
})
it('treats a sha surrounded by ascii whitespace as a commit', async () => {
// ASCII whitespace can only come from the workflow author's YAML (git ref
// names cannot contain it), so trimming it and treating the value as a
// commit is safe.
inputs.ref = ' 1111111111222222222233333333334444444444 '
const settings: IGitSourceSettings = await inputHelper.getInputs()
expect(settings.ref).toBeFalsy()
expect(settings.commit).toBe('1111111111222222222233333333334444444444')
})
it('sets workflow organization ID', async () => {
const settings: IGitSourceSettings = await inputHelper.getInputs()
expect(settings.workflowOrganizationId).toBe(123456)
})
describe('unsafe PR checkout guard', () => {
const forkPayload = {
repository: {id: 100},
pull_request: {
head: {
sha: '1234567890123456789012345678901234567890',
repo: {id: 200, full_name: 'attacker/fork'}
},
merge_commit_sha: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
}
}
it('allows the default self-checkout on a fork pull_request_target', async () => {
const originalEvent = mockGithubContext.eventName
const originalPayload = mockGithubContext.payload
try {
mockGithubContext.eventName = 'pull_request_target'
mockGithubContext.payload = forkPayload
// Simulate a rebase/fast-forward merge where the base tip (event SHA)
// equals the PR head SHA. The default self-checkout must still succeed.
mockGithubContext.sha = '1234567890123456789012345678901234567890'
const settings: IGitSourceSettings = await inputHelper.getInputs()
expect(settings.commit).toBe('1234567890123456789012345678901234567890')
} finally {
mockGithubContext.eventName = originalEvent
mockGithubContext.payload = originalPayload
}
})
it('refuses an explicit fork repository on pull_request_target', async () => {
const originalEvent = mockGithubContext.eventName
const originalPayload = mockGithubContext.payload
try {
mockGithubContext.eventName = 'pull_request_target'
mockGithubContext.payload = forkPayload
inputs.repository = 'attacker/fork'
await expect(inputHelper.getInputs()).rejects.toThrow(
/Refusing to check out fork pull request code/
)
} finally {
mockGithubContext.eventName = originalEvent
mockGithubContext.payload = originalPayload
}
})
})
})

27
dist/index.js vendored
View File

@ -42100,6 +42100,22 @@ async function getInputs() {
`${github_context.repo.owner}/${github_context.repo.repo}`.toUpperCase();
// Source branch, source version
result.ref = getInput('ref');
// core.getInput()'s default trim strips a range of Unicode characters such as a
// leading BOM (U+FEFF) or NBSP (U+00A0). Those are valid in a git ref name, so
// a fork branch named "<BOM>" + 40 hex chars would trim down to a bare SHA and
// be silently reclassified as a commit, bypassing the unsafe fork PR checkout
// guard.
//
// The trim below strips only the ASCII whitespace characters which are all forbidden
// in a git branch name.
// \t U+0009 horizontal tab - ASCII control, forbidden in ref names
// \n U+000A line feed - ASCII control, forbidden in ref names
// \v U+000B vertical tab - ASCII control, forbidden in ref names
// \f U+000C form feed - ASCII control, forbidden in ref names
// \r U+000D carriage return - ASCII control, forbidden in ref names
// ' ' U+0020 space - forbidden in ref names
const asciiTrimmedRef = getInput('ref', { trimWhitespace: false })
.replace(/^[\t\n\v\f\r ]+|[\t\n\v\f\r ]+$/g, '');
if (!result.ref) {
if (isWorkflowRepository) {
result.ref = github_context.ref;
@ -42112,8 +42128,8 @@ async function getInputs() {
}
}
// SHA?
else if (result.ref.match(/^(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{64})$/)) {
result.commit = result.ref;
else if (asciiTrimmedRef.match(/^(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{64})$/)) {
result.commit = asciiTrimmedRef;
result.ref = '';
}
core_debug(`ref = '${result.ref}'`);
@ -42191,12 +42207,19 @@ async function getInputs() {
(getInput('allow-unsafe-pr-checkout') || 'false').toUpperCase() ===
'TRUE';
core_debug(`allow unsafe PR checkout = ${result.allowUnsafePrCheckout}`);
// The default self-checkout (this repository with no explicit ref) always
// resolves to the trusted ref/commit GitHub set for the triggering event, so
// the fork-checkout guard only needs to run when the caller customized the
// repository or ref.
const isDefaultCheckout = isWorkflowRepository && !getInput('ref');
if (!isDefaultCheckout) {
assertSafePrCheckout({
qualifiedRepository,
ref: result.ref,
commit: result.commit,
allowUnsafePrCheckout: result.allowUnsafePrCheckout
});
}
return result;
}

View File

@ -195,7 +195,7 @@ export async function getSource(settings: IGitSourceSettings): Promise<void> {
// When all history is fetched, the ref we're interested in may have moved to a different
// commit (push or force push). If so, fetch again with a targeted refspec.
if (!(await refHelper.testRef(git, settings.ref, settings.commit))) {
if (!settings.refs.startsWith("refs/tags") && !(await refHelper.testRef(git, settings.ref, settings.commit))) {
refSpec = refHelper.getRefSpec(settings.ref, settings.commit)
await git.fetch(refSpec, fetchOptions)

View File

@ -59,6 +59,23 @@ export async function getInputs(): Promise<IGitSourceSettings> {
// Source branch, source version
result.ref = core.getInput('ref')
// core.getInput()'s default trim strips a range of Unicode characters such as a
// leading BOM (U+FEFF) or NBSP (U+00A0). Those are valid in a git ref name, so
// a fork branch named "<BOM>" + 40 hex chars would trim down to a bare SHA and
// be silently reclassified as a commit, bypassing the unsafe fork PR checkout
// guard.
//
// The trim below strips only the ASCII whitespace characters which are all forbidden
// in a git branch name.
// \t U+0009 horizontal tab - ASCII control, forbidden in ref names
// \n U+000A line feed - ASCII control, forbidden in ref names
// \v U+000B vertical tab - ASCII control, forbidden in ref names
// \f U+000C form feed - ASCII control, forbidden in ref names
// \r U+000D carriage return - ASCII control, forbidden in ref names
// ' ' U+0020 space - forbidden in ref names
const asciiTrimmedRef = core
.getInput('ref', {trimWhitespace: false})
.replace(/^[\t\n\v\f\r ]+|[\t\n\v\f\r ]+$/g, '')
if (!result.ref) {
if (isWorkflowRepository) {
result.ref = github.context.ref
@ -72,8 +89,8 @@ export async function getInputs(): Promise<IGitSourceSettings> {
}
}
// SHA?
else if (result.ref.match(/^(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{64})$/)) {
result.commit = result.ref
else if (asciiTrimmedRef.match(/^(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{64})$/)) {
result.commit = asciiTrimmedRef
result.ref = ''
}
core.debug(`ref = '${result.ref}'`)
@ -168,12 +185,19 @@ export async function getInputs(): Promise<IGitSourceSettings> {
'TRUE'
core.debug(`allow unsafe PR checkout = ${result.allowUnsafePrCheckout}`)
// The default self-checkout (this repository with no explicit ref) always
// resolves to the trusted ref/commit GitHub set for the triggering event, so
// the fork-checkout guard only needs to run when the caller customized the
// repository or ref.
const isDefaultCheckout = isWorkflowRepository && !core.getInput('ref')
if (!isDefaultCheckout) {
unsafePrCheckoutHelper.assertSafePrCheckout({
qualifiedRepository,
ref: result.ref,
commit: result.commit,
allowUnsafePrCheckout: result.allowUnsafePrCheckout
})
}
return result
}