mirror of
https://github.com/solidtime-io/solidtime.git
synced 2026-08-08 00:02:15 +01:00
add validationa for project names in toggl importer
This commit is contained in:
@@ -123,6 +123,7 @@ class TogglDataImporter extends DefaultImporter
|
|||||||
}
|
}
|
||||||
|
|
||||||
foreach ($projects as $project) {
|
foreach ($projects as $project) {
|
||||||
|
$projectExternalId = $this->guardExternalIdentifier($project->id);
|
||||||
$clientId = null;
|
$clientId = null;
|
||||||
if ($project->client_id !== null) {
|
if ($project->client_id !== null) {
|
||||||
$clientId = $this->clientImportHelper->getKeyByExternalIdentifier((string) $project->client_id);
|
$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,
|
'billable_rate' => $project->rate !== null ? (int) ($project->rate * 100) : null,
|
||||||
], (string) $project->id);
|
], (string) $project->id);
|
||||||
|
|
||||||
if (! file_exists($temporaryDirectory->path('projects_users/'.$project->id.'.json'))) {
|
if (! file_exists($temporaryDirectory->path('projects_users/'.$projectExternalId.'.json'))) {
|
||||||
throw new ImportException('File "projects_users/'.$project->id.'.json" missing in ZIP');
|
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) {
|
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);
|
$projectMembers = json_decode($projectMembersFileContent);
|
||||||
if ($projectMembers === null) {
|
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) {
|
foreach ($projectMembers as $projectMember) {
|
||||||
$userId = $this->userImportHelper->getKeyByExternalIdentifier((string) $projectMember->user_id);
|
$userId = $this->userImportHelper->getKeyByExternalIdentifier((string) $projectMember->user_id);
|
||||||
@@ -170,6 +171,7 @@ class TogglDataImporter extends DefaultImporter
|
|||||||
}
|
}
|
||||||
$projectIds = $this->projectImportHelper->getExternalIds();
|
$projectIds = $this->projectImportHelper->getExternalIds();
|
||||||
foreach ($projectIds as $projectIdExternal) {
|
foreach ($projectIds as $projectIdExternal) {
|
||||||
|
$projectIdExternal = $this->guardExternalIdentifier($projectIdExternal);
|
||||||
if (! file_exists($temporaryDirectory->path('tasks/'.$projectIdExternal.'.json'))) {
|
if (! file_exists($temporaryDirectory->path('tasks/'.$projectIdExternal.'.json'))) {
|
||||||
continue;
|
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]
|
#[Override]
|
||||||
public function getName(): string
|
public function getName(): string
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -11,6 +11,8 @@ use App\Service\Import\Importers\ImportException;
|
|||||||
use App\Service\Import\Importers\TogglDataImporter;
|
use App\Service\Import\Importers\TogglDataImporter;
|
||||||
use Exception;
|
use Exception;
|
||||||
use PHPUnit\Framework\Attributes\CoversClass;
|
use PHPUnit\Framework\Attributes\CoversClass;
|
||||||
|
use Spatie\TemporaryDirectory\TemporaryDirectory;
|
||||||
|
use ZipArchive;
|
||||||
|
|
||||||
#[CoversClass(TogglDataImporter::class)]
|
#[CoversClass(TogglDataImporter::class)]
|
||||||
#[CoversClass(ImportException::class)]
|
#[CoversClass(ImportException::class)]
|
||||||
@@ -88,6 +90,82 @@ class TogglDataImporterTest extends ImporterTestAbstract
|
|||||||
$this->assertSame(0, $report->clientsCreated);
|
$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
|
public function test_import_of_user_with_unknown_timezone_will_be_mapped_to_utc(): void
|
||||||
{
|
{
|
||||||
// Arrange
|
// Arrange
|
||||||
|
|||||||
Reference in New Issue
Block a user