1
0
Fork 0

update test case and handling

pull/1666/head
bethanyj28 2024-02-23 13:41:40 -05:00
parent 76489f433b
commit 4256ea99c5
3 changed files with 20 additions and 15 deletions

View File

@ -180,7 +180,7 @@ describe('download-artifact', () => {
expect(response.downloadPath).toBe(fixtures.workspaceDir) expect(response.downloadPath).toBe(fixtures.workspaceDir)
}) })
it.only('should not allow path traversal from malicious artifacts', async () => { it('should not allow path traversal from malicious artifacts', async () => {
const downloadArtifactMock = github.getOctokit(fixtures.token).rest const downloadArtifactMock = github.getOctokit(fixtures.token).rest
.actions.downloadArtifact as MockedDownloadArtifact .actions.downloadArtifact as MockedDownloadArtifact
downloadArtifactMock.mockResolvedValueOnce({ downloadArtifactMock.mockResolvedValueOnce({
@ -200,12 +200,12 @@ describe('download-artifact', () => {
} }
) )
const response = await downloadArtifactPublic( await expect(downloadArtifactPublic(
fixtures.artifactID, fixtures.artifactID,
fixtures.repositoryOwner, fixtures.repositoryOwner,
fixtures.repositoryName, fixtures.repositoryName,
fixtures.token fixtures.token
) )).rejects.toBeInstanceOf(Error)
expect(downloadArtifactMock).toHaveBeenCalledWith({ expect(downloadArtifactMock).toHaveBeenCalledWith({
owner: fixtures.repositoryOwner, owner: fixtures.repositoryOwner,
@ -221,10 +221,6 @@ describe('download-artifact', () => {
expect(mockGetArtifactMalicious).toHaveBeenCalledWith( expect(mockGetArtifactMalicious).toHaveBeenCalledWith(
fixtures.blobStorageUrl fixtures.blobStorageUrl
) )
expect(
fs.readFileSync(path.join(fixtures.workspaceDir, 'etc/hosts'), 'utf8')
).toEqual('foo')
expect(response.downloadPath).toBe(fixtures.workspaceDir)
}) })
it('should successfully download an artifact to user defined path', async () => { it('should successfully download an artifact to user defined path', async () => {

View File

@ -46,6 +46,9 @@ async function streamExtract(url: string, directory: string): Promise<void> {
await streamExtractExternal(url, directory) await streamExtractExternal(url, directory)
return return
} catch (error) { } catch (error) {
if (error.message.includes('Malformed extraction path')) {
throw new Error(`Artifact download failed with unretryable error: ${error.message}`)
}
retryCount++ retryCount++
core.debug( core.debug(
`Failed to download artifact after ${retryCount} retries due to ${error.message}. Retrying in 5 seconds...` `Failed to download artifact after ${retryCount} retries due to ${error.message}. Retrying in 5 seconds...`
@ -81,6 +84,7 @@ export async function streamExtractExternal(
const timer = setTimeout(timerFn, timeout) const timer = setTimeout(timerFn, timeout)
const promises: Promise<void>[] = [] const promises: Promise<void>[] = []
const createdDirectories = new Set<string>()
response.message response.message
.on('data', () => { .on('data', () => {
timer.refresh() timer.refresh()
@ -94,20 +98,25 @@ export async function streamExtractExternal(
}) })
.pipe(unzip.Parse()) .pipe(unzip.Parse())
.on('entry', (entry: unzip.Entry) => { .on('entry', (entry: unzip.Entry) => {
console.log(`entryPath: ${entry.path}`)
const fullPath = path.normalize(path.join(directory, entry.path)) const fullPath = path.normalize(path.join(directory, entry.path))
console.log(`fullPath: ${fullPath}`) if (fullPath.indexOf(directory) !== 0) {
if (fullPath.indexOf(directory) != 0) { reject(new Error(`Malformed extraction path: ${fullPath}`))
reject(new Error(`Invalid file path: ${fullPath}`))
} }
core.debug(`Extracting artifact entry: ${fullPath}`) core.debug(`Extracting artifact entry: ${fullPath}`)
if (entry.type === 'Directory') { if (entry.type === 'Directory') {
if (!createdDirectories.has(fullPath)) {
promises.push(resolveOrCreateDirectory(fullPath).then(() => {})) promises.push(resolveOrCreateDirectory(fullPath).then(() => {}))
createdDirectories.add(fullPath)
}
entry.autodrain() entry.autodrain()
} else { } else {
if (!createdDirectories.has(path.dirname(fullPath))) {
promises.push( promises.push(
resolveOrCreateDirectory(path.dirname(fullPath)).then(() => {}) resolveOrCreateDirectory(path.dirname(fullPath)).then(() => {})
) )
createdDirectories.add(path.dirname(fullPath))
}
const writeStream = createWriteStream(fullPath) const writeStream = createWriteStream(fullPath)
promises.push( promises.push(
new Promise((resolve, reject) => { new Promise((resolve, reject) => {