From 56c45adc1ade154b5e4a29004fb995fa7bc2cf9c Mon Sep 17 00:00:00 2001 From: Gregor Vostrak Date: Wed, 8 Jul 2026 17:19:57 +0200 Subject: [PATCH] add validationa for project names in toggl importer --- .../Import/Importers/TogglDataImporter.php | 36 +++++++-- .../Importers/TogglDataImporterTest.php | 78 +++++++++++++++++++ 2 files changed, 109 insertions(+), 5 deletions(-) diff --git a/app/Service/Import/Importers/TogglDataImporter.php b/app/Service/Import/Importers/TogglDataImporter.php index 06f43dca..0785cf33 100644 --- a/app/Service/Import/Importers/TogglDataImporter.php +++ b/app/Service/Import/Importers/TogglDataImporter.php @@ -123,6 +123,7 @@ class TogglDataImporter extends DefaultImporter } foreach ($projects as $project) { + $projectExternalId = $this->guardExternalIdentifier($project->id); $clientId = null; if ($project->client_id !== null) { $clientId = $this->clientImportHelper->getKeyByExternalIdentifier((string) $project->client_id); @@ -146,16 +147,16 @@ class TogglDataImporter extends DefaultImporter 'billable_rate' => $project->rate !== null ? (int) ($project->rate * 100) : null, ], (string) $project->id); - if (! file_exists($temporaryDirectory->path('projects_users/'.$project->id.'.json'))) { - throw new ImportException('File "projects_users/'.$project->id.'.json" missing in ZIP'); + if (! file_exists($temporaryDirectory->path('projects_users/'.$projectExternalId.'.json'))) { + throw new ImportException('File "projects_users/'.$projectExternalId.'.json" missing in ZIP'); } - $projectMembersFileContent = file_get_contents($temporaryDirectory->path('projects_users/'.$project->id.'.json')); + $projectMembersFileContent = file_get_contents($temporaryDirectory->path('projects_users/'.$projectExternalId.'.json')); if ($projectMembersFileContent === false) { - throw new ImportException('File "projects_users/'.$project->id.'.json" can not be opened'); + throw new ImportException('File "projects_users/'.$projectExternalId.'.json" can not be opened'); } $projectMembers = json_decode($projectMembersFileContent); if ($projectMembers === null) { - throw new ImportException('File "projects_users/'.$project->id.'.json" is empty'); + throw new ImportException('File "projects_users/'.$projectExternalId.'.json" is empty'); } foreach ($projectMembers as $projectMember) { $userId = $this->userImportHelper->getKeyByExternalIdentifier((string) $projectMember->user_id); @@ -170,6 +171,7 @@ class TogglDataImporter extends DefaultImporter } $projectIds = $this->projectImportHelper->getExternalIds(); foreach ($projectIds as $projectIdExternal) { + $projectIdExternal = $this->guardExternalIdentifier($projectIdExternal); if (! file_exists($temporaryDirectory->path('tasks/'.$projectIdExternal.'.json'))) { continue; } @@ -209,6 +211,30 @@ class TogglDataImporter extends DefaultImporter } } + /** + * Ensure an externally-sourced identifier can be safely used inside a + * filesystem path. The identifiers originate from the untrusted uploaded + * ZIP, and Spatie's TemporaryDirectory::path() auto-creates any missing + * parent directory of the resolved path, so an unfiltered "../" sequence + * would escape the import sandbox and create/probe arbitrary paths on the + * host (CWE-22). Toggl identifiers are numeric, so restricting them to a + * conservative allow-list rejects traversal without affecting real data. + * + * @throws ImportException + */ + private function guardExternalIdentifier(mixed $id): string + { + if (! is_string($id) && ! is_int($id)) { + throw new ImportException('Invalid identifier in import data'); + } + $id = (string) $id; + if (preg_match('/^[A-Za-z0-9_-]+$/', $id) !== 1) { + throw new ImportException('Invalid identifier in import data'); + } + + return $id; + } + #[Override] public function getName(): string { diff --git a/tests/Unit/Service/Import/Importers/TogglDataImporterTest.php b/tests/Unit/Service/Import/Importers/TogglDataImporterTest.php index c1785344..35723774 100644 --- a/tests/Unit/Service/Import/Importers/TogglDataImporterTest.php +++ b/tests/Unit/Service/Import/Importers/TogglDataImporterTest.php @@ -11,6 +11,8 @@ use App\Service\Import\Importers\ImportException; use App\Service\Import\Importers\TogglDataImporter; use Exception; use PHPUnit\Framework\Attributes\CoversClass; +use Spatie\TemporaryDirectory\TemporaryDirectory; +use ZipArchive; #[CoversClass(TogglDataImporter::class)] #[CoversClass(ImportException::class)] @@ -88,6 +90,82 @@ class TogglDataImporterTest extends ImporterTestAbstract $this->assertSame(0, $report->clientsCreated); } + public function test_import_with_path_traversal_in_project_id_is_rejected_without_touching_the_filesystem(): void + { + // Arrange + $organization = Organization::factory()->create(); + $importer = new TogglDataImporter; + $importer->init($organization); + + $markerDir = sys_get_temp_dir().'/solidtime_path_traversal_'.uniqid(); + $this->assertDirectoryDoesNotExist($markerDir); + // Enough "../" to reach the filesystem root from any temp location, then + // back down into the attacker-chosen marker directory. The importer + // appends ".json", so the parent directory Spatie's TemporaryDirectory + // would auto-create for the resolved path is exactly $markerDir. + $traversalId = str_repeat('../', 40).ltrim($markerDir, '/').'/probe'; + $data = file_get_contents($this->buildTogglZipWithProjectId($traversalId)); + + // Act + try { + $importer->importData($data, 'Europe/Vienna'); + $this->fail('Expected ImportException was not thrown'); + } catch (ImportException $e) { + // Rejected by the identifier guard, not by a downstream + // "missing in ZIP" error (which would mean the sink was reached + // and the directory had already been created). + $this->assertSame('Invalid identifier in import data', $e->getMessage()); + } + + // Assert: no directory was created outside the import sandbox. + $this->assertDirectoryDoesNotExist($markerDir); + } + + public function test_import_with_valid_numeric_project_id_is_accepted(): void + { + // Arrange + $organization = Organization::factory()->create(); + $importer = new TogglDataImporter; + $importer->init($organization); + // A legitimate Toggl numeric id must still pass the guard. The + // projects_users file is intentionally absent, so the importer fails + // with the ordinary "missing in ZIP" error rather than the guard error. + $data = file_get_contents($this->buildTogglZipWithProjectId(402)); + + // Act + try { + $importer->importData($data, 'Europe/Vienna'); + $this->fail('Expected ImportException was not thrown'); + } catch (ImportException $e) { + // Assert: the numeric id passed the guard and reached the ZIP + // content check (proving valid data is not rejected). + $this->assertSame('File "projects_users/402.json" missing in ZIP', $e->getMessage()); + } + } + + private function buildTogglZipWithProjectId(mixed $projectId): string + { + $tempDir = TemporaryDirectory::make(); + $zipPath = $tempDir->path('traversal.zip'); + $zip = new ZipArchive; + $zip->open($zipPath, ZipArchive::CREATE); + $zip->addFromString('clients.json', '[]'); + $zip->addFromString('tags.json', '[]'); + $zip->addFromString('workspace_users.json', '[]'); + $zip->addFromString('projects.json', (string) json_encode([[ + 'id' => $projectId, + 'client_id' => null, + 'color' => '#ff0000', + 'billable' => false, + 'is_private' => false, + 'rate' => null, + 'name' => 'Traversal', + ]])); + $zip->close(); + + return $zipPath; + } + public function test_import_of_user_with_unknown_timezone_will_be_mapped_to_utc(): void { // Arrange