Skip to content

Commit 653f822

Browse files
jbagsikcursoragent
andcommitted
Address review findings in verapdf test helpers
Fix jar tracker by-reference capture, throw from buildZipArchive instead of Pest expect, add buildMaliciousZip, and dataset install zip-slip cases. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 8184096 commit 653f822

2 files changed

Lines changed: 37 additions & 33 deletions

File tree

packages/verapdf/tests/Helpers.php

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,10 @@ function verapdfTempDir(string $prefix): string
2727
function buildZipArchive(string $zipPath, array $entries): void
2828
{
2929
$zip = new ZipArchive;
30-
expect($zip->open($zipPath, ZipArchive::CREATE))->toBeTrue();
30+
31+
if ($zip->open($zipPath, ZipArchive::CREATE) !== true) {
32+
throw new RuntimeException('Cannot create ZIP archive: '.$zipPath);
33+
}
3134

3235
foreach ($entries as $entry) {
3336
$zip->addFromString($entry['name'], $entry['content']);
@@ -55,6 +58,17 @@ function buildZipAt(string $prefix, array $entries): string
5558
return $zipPath;
5659
}
5760

61+
function buildMaliciousZip(
62+
string $prefix,
63+
string $entryName,
64+
string $content = 'pwned',
65+
bool $symlink = false,
66+
): string {
67+
return buildZipAt($prefix, [
68+
['name' => $entryName, 'content' => $content, 'symlink' => $symlink],
69+
]);
70+
}
71+
5872
/**
5973
* @return array{path: string, bytes: string, sha256: string}
6074
*/
@@ -135,7 +149,7 @@ function fakeJavaProcessTrackingJar(): array
135149
{
136150
$state = ['installerJarRan' => false];
137151

138-
Process::fake(function (PendingProcess $process) use ($state) {
152+
Process::fake(function (PendingProcess $process) use (&$state) {
139153
$command = $process->command;
140154
if (is_array($command) && in_array('-jar', $command, true)) {
141155
$state['installerJarRan'] = true;

packages/verapdf/tests/Unit/InstallVeraPdfCommandTest.php

Lines changed: 21 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -57,46 +57,36 @@
5757
->and($jarTracker['installerJarRan'])->toBeFalse();
5858
});
5959

60-
test('install aborts on zip-slip archive before running the installer', function (): void {
61-
$installer = buildInstallerZipWithChecksum('cmd-slip', [
62-
['name' => '../evil.txt', 'content' => 'pwned'],
63-
]);
64-
65-
fakeInstallerDownload($installer['bytes'], $installer['sha256']);
66-
67-
$jarTracker = fakeJavaProcessTrackingJar();
60+
test('install aborts on zip-slip archive', function (bool $force, bool $seedExisting): void {
61+
$base = $seedExisting ? seedCliInstallLayout() : (string) config('verapdf.base_path');
6862

69-
$this->artisan('verapdf:install')
70-
->expectsOutputToContain('unsafe ZIP entry')
71-
->assertFailed();
63+
if ($seedExisting) {
64+
file_put_contents($base.'/bin/cli-1.30.1.jar', 'existing-cli');
65+
}
7266

73-
$base = (string) config('verapdf.base_path');
74-
expect(is_file($base.'/verapdf'))->toBeFalse()
75-
->and($jarTracker['installerJarRan'])->toBeFalse();
76-
77-
File::delete($installer['path']);
78-
});
79-
80-
test('install aborts on zip-slip with --force without wiping an existing install', function (): void {
81-
$base = seedCliInstallLayout();
82-
file_put_contents($base.'/bin/cli-1.30.1.jar', 'existing-cli');
83-
84-
$installer = buildInstallerZipWithChecksum('cmd-force-slip', [
67+
$installer = buildInstallerZipWithChecksum('cmd-slip', [
8568
['name' => '../evil.txt', 'content' => 'pwned'],
8669
]);
8770

8871
fakeInstallerDownload($installer['bytes'], $installer['sha256']);
8972

9073
$jarTracker = fakeJavaProcessTrackingJar();
9174

92-
$this->artisan('verapdf:install', ['--force' => true])
93-
->expectsOutputToContain('unsafe ZIP entry')
94-
->assertFailed();
75+
$command = $this->artisan('verapdf:install', $force ? ['--force' => true] : []);
76+
$command->expectsOutputToContain('unsafe ZIP entry')->assertFailed();
9577

96-
expect(is_file($base.'/verapdf'))->toBeTrue()
97-
->and(is_file($base.'/bin/cli-1.30.1.jar'))->toBeTrue()
98-
->and((string) file_get_contents($base.'/bin/cli-1.30.1.jar'))->toBe('existing-cli')
99-
->and($jarTracker['installerJarRan'])->toBeFalse();
78+
if ($seedExisting) {
79+
expect(is_file($base.'/verapdf'))->toBeTrue()
80+
->and(is_file($base.'/bin/cli-1.30.1.jar'))->toBeTrue()
81+
->and((string) file_get_contents($base.'/bin/cli-1.30.1.jar'))->toBe('existing-cli');
82+
} else {
83+
expect(is_file($base.'/verapdf'))->toBeFalse();
84+
}
85+
86+
expect($jarTracker['installerJarRan'])->toBeFalse();
10087

10188
File::delete($installer['path']);
102-
});
89+
})->with([
90+
'before running the installer' => [false, false],
91+
'with --force without wiping an existing install' => [true, true],
92+
]);

0 commit comments

Comments
 (0)