From 8d34513bde1df6c41f11c887d68ae3ed58758a37 Mon Sep 17 00:00:00 2001 From: Constantin Graf Date: Fri, 9 Oct 2026 13:23:51 +0200 Subject: [PATCH] Prevent time entries with end before start Partial updates of time entries only validated end against start if both were part of the payload. Start and end are now also validated against the persisted value if only one of them is sent (fixes #1189). The importers now fail if the end of a time entry is before the start and the database consistency check reports such time entries. --- .../SelfHost/SelfHostDatabaseConsistency.php | 8 ++ .../V1/TimeEntry/TimeEntryUpdateRequest.php | 34 +++++++ .../Importers/ClockifyTimeEntriesImporter.php | 3 + .../Importers/GenericTimeEntriesImporter.php | 3 + .../Importers/HarvestTimeEntriesImporter.php | 3 + .../Import/Importers/SolidtimeImporter.php | 3 + .../Importers/TogglTimeEntriesImporter.php | 3 + ...SelfHostDatabaseConsistencyCommandTest.php | 23 +++++ .../Endpoint/Api/V1/TimeEntryEndpointTest.php | 95 +++++++++++++++++++ .../ClockifyTimeEntriesImporterTest.php | 25 +++++ .../GenericTimeEntriesImporterTest.php | 24 +++++ .../HarvestTimeEntriesImporterTest.php | 23 +++++ .../Importers/SolidtimeImporterTest.php | 30 ++++++ .../TogglTimeEntriesImporterTest.php | 23 +++++ 14 files changed, 300 insertions(+) diff --git a/app/Console/Commands/SelfHost/SelfHostDatabaseConsistency.php b/app/Console/Commands/SelfHost/SelfHostDatabaseConsistency.php index 88a11184..5c96eebd 100644 --- a/app/Console/Commands/SelfHost/SelfHostDatabaseConsistency.php +++ b/app/Console/Commands/SelfHost/SelfHostDatabaseConsistency.php @@ -59,6 +59,14 @@ class SelfHostDatabaseConsistency extends Command ->get(); $this->logProblems($problems, 'Time entries have a client but no project', $hadAProblem); + // End of time entries can not be before the start + $problems = DB::table('time_entries') + ->select(['id']) + ->whereNotNull('end') + ->whereColumn('end', '<', 'start') + ->get(); + $this->logProblems($problems, 'Time entries have an end that is before the start', $hadAProblem); + // Every user needs to be a member of at least one organization $problems = DB::table('users') ->select(['users.id as id']) diff --git a/app/Http/Requests/V1/TimeEntry/TimeEntryUpdateRequest.php b/app/Http/Requests/V1/TimeEntry/TimeEntryUpdateRequest.php index 92c84f32..c679d383 100644 --- a/app/Http/Requests/V1/TimeEntry/TimeEntryUpdateRequest.php +++ b/app/Http/Requests/V1/TimeEntry/TimeEntryUpdateRequest.php @@ -13,9 +13,11 @@ use App\Models\Tag; use App\Models\Task; use App\Models\TimeEntry; use App\Service\PermissionStore; +use Carbon\Exceptions\InvalidFormatException; use Closure; use Illuminate\Contracts\Validation\ValidationRule; use Illuminate\Database\Eloquent\Builder; +use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Auth; use Illuminate\Validation\ConditionalRules; use Illuminate\Validation\Rule; @@ -90,12 +92,32 @@ class TimeEntryUpdateRequest extends BaseFormRequest // Start of time entry (Format: "Y-m-d\TH:i:s\Z", UTC timezone, Example: "2000-02-22T14:58:59Z") 'start' => [ 'date_format:Y-m-d\TH:i:s\Z', + function (string $attribute, mixed $value, Closure $fail) use ($timeEntry): void { + // If the payload does not contain an end, the start needs to be validated against the persisted end + if ($this->has('end') || $timeEntry?->end === null) { + return; + } + $start = $this->parseDate($value); + if ($start !== null && $start->gt($timeEntry->end)) { + $fail('The start field must be a date before or equal to end.'); + } + }, ], // End of time entry (Format: "Y-m-d\TH:i:s\Z", UTC timezone, Example: "2000-02-22T14:58:59Z") 'end' => [ 'nullable', 'date_format:Y-m-d\TH:i:s\Z', 'after_or_equal:start', + function (string $attribute, mixed $value, Closure $fail) use ($timeEntry): void { + // If the payload does not contain a start, the end needs to be validated against the persisted start + if ($this->has('start') || $timeEntry === null) { + return; + } + $end = $this->parseDate($value); + if ($end !== null && $end->lt($timeEntry->start)) { + $fail('The end field must be a date after or equal to start.'); + } + }, ], // Whether time entry is billable 'billable' => [ @@ -137,4 +159,16 @@ class TimeEntryUpdateRequest extends BaseFormRequest ], ]; } + + private function parseDate(mixed $value): ?Carbon + { + if (! is_string($value)) { + return null; + } + try { + return Carbon::createFromFormat('Y-m-d\TH:i:s\Z', $value, 'UTC'); + } catch (InvalidFormatException) { + return null; + } + } } diff --git a/app/Service/Import/Importers/ClockifyTimeEntriesImporter.php b/app/Service/Import/Importers/ClockifyTimeEntriesImporter.php index f74919b2..7ec2d2be 100644 --- a/app/Service/Import/Importers/ClockifyTimeEntriesImporter.php +++ b/app/Service/Import/Importers/ClockifyTimeEntriesImporter.php @@ -190,6 +190,9 @@ class ClockifyTimeEntriesImporter extends DefaultImporter if ($end === null) { throw new ImportException('End date ("'.$endDateStr.'") or time ("'.$endTimeStr.'") are invalid'); } + if ($end->lt($start)) { + throw new ImportException('End ("'.$endStr.'") is before start ("'.$startStr.'")'); + } $timeEntry->end = $end->utc(); $timeEntry->billable_rate = $this->billableRateService->getBillableRateForTimeEntryWithGivenRelations( diff --git a/app/Service/Import/Importers/GenericTimeEntriesImporter.php b/app/Service/Import/Importers/GenericTimeEntriesImporter.php index bb19ac0e..c0fe90b7 100644 --- a/app/Service/Import/Importers/GenericTimeEntriesImporter.php +++ b/app/Service/Import/Importers/GenericTimeEntriesImporter.php @@ -155,6 +155,9 @@ class GenericTimeEntriesImporter extends DefaultImporter if ($end === null) { throw new ImportException('Value of end ("'.$record['end'].'") is invalid'); } + if ($end->lt($start)) { + throw new ImportException('Value of end ("'.$record['end'].'") is before start ("'.$record['start'].'")'); + } $timeEntry->end = $end->utc(); $timeEntry->billable_rate = $this->billableRateService->getBillableRateForTimeEntryWithGivenRelations( $timeEntry, diff --git a/app/Service/Import/Importers/HarvestTimeEntriesImporter.php b/app/Service/Import/Importers/HarvestTimeEntriesImporter.php index 57a66f49..77e965ef 100644 --- a/app/Service/Import/Importers/HarvestTimeEntriesImporter.php +++ b/app/Service/Import/Importers/HarvestTimeEntriesImporter.php @@ -136,6 +136,9 @@ class HarvestTimeEntriesImporter extends DefaultImporter throw new ImportException('Hours ("'.$record['Hours'].'") is invalid'); } $hours = (float) $hoursField; + if ($hours < 0) { + throw new ImportException('Hours ("'.$record['Hours'].'") is negative'); + } $timeEntry->start = $date->copy()->startOfDay()->utc(); $timeEntry->end = $date->copy()->startOfDay()->addHours($hours)->utc(); $timeEntry->billable_rate = $this->billableRateService->getBillableRateForTimeEntryWithGivenRelations( diff --git a/app/Service/Import/Importers/SolidtimeImporter.php b/app/Service/Import/Importers/SolidtimeImporter.php index f6eade0e..7089d469 100644 --- a/app/Service/Import/Importers/SolidtimeImporter.php +++ b/app/Service/Import/Importers/SolidtimeImporter.php @@ -280,6 +280,9 @@ class SolidtimeImporter extends DefaultImporter if ($end === null) { throw new ImportException('End date ("'.$timeEntryRow['end'].'") is invalid'); } + if ($end->lt($start)) { + throw new ImportException('End date ("'.$timeEntryRow['end'].'") is before start date ("'.$timeEntryRow['start'].'")'); + } $timeEntry->end = $end->utc(); } else { $timeEntry->end = null; diff --git a/app/Service/Import/Importers/TogglTimeEntriesImporter.php b/app/Service/Import/Importers/TogglTimeEntriesImporter.php index e18cf606..c2f70843 100644 --- a/app/Service/Import/Importers/TogglTimeEntriesImporter.php +++ b/app/Service/Import/Importers/TogglTimeEntriesImporter.php @@ -139,6 +139,9 @@ class TogglTimeEntriesImporter extends DefaultImporter if ($end === null) { throw new ImportException('End date ("'.$record['End date'].'") or time ("'.$record['End time'].'") are invalid'); } + if ($end->lt($start)) { + throw new ImportException('End ("'.$record['End date'].' '.$record['End time'].'") is before start ("'.$record['Start date'].' '.$record['Start time'].'")'); + } $timeEntry->end = $end->utc(); $timeEntry->billable_rate = $this->billableRateService->getBillableRateForTimeEntryWithGivenRelations( $timeEntry, diff --git a/tests/Unit/Console/Commands/SelfHost/SelfHostDatabaseConsistencyCommandTest.php b/tests/Unit/Console/Commands/SelfHost/SelfHostDatabaseConsistencyCommandTest.php index 26491860..b235d38a 100644 --- a/tests/Unit/Console/Commands/SelfHost/SelfHostDatabaseConsistencyCommandTest.php +++ b/tests/Unit/Console/Commands/SelfHost/SelfHostDatabaseConsistencyCommandTest.php @@ -13,6 +13,7 @@ use App\Models\Task; use App\Models\TimeEntry; use App\Models\User; use Illuminate\Console\Command; +use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Artisan; use PHPUnit\Framework\Attributes\CoversClass; use Tests\TestCaseWithDatabase; @@ -158,4 +159,26 @@ class SelfHostDatabaseConsistencyCommandTest extends TestCaseWithDatabase $output = Artisan::output(); $this->assertSame("Consistency problem: Users have a current organization that they are not a member of\n - ".$user1->user->getKey()."\n", $output); } + + public function test_checks_that_end_of_time_entries_is_not_before_start(): void + { + // Arrange + $user = $this->createUserWithRole(Role::Owner); + $timeEntry = TimeEntry::factory()->forMember($user->member)->create([ + 'start' => Carbon::parse('2026-08-01T03:00:00Z'), + 'end' => Carbon::parse('2026-08-01T02:59:55Z'), + ]); + TimeEntry::factory()->forMember($user->member)->create([ + 'start' => Carbon::parse('2026-08-01T04:00:00Z'), + 'end' => Carbon::parse('2026-08-01T04:00:00Z'), + ]); + + // Act + $exitCode = $this->withoutMockingConsoleOutput()->artisan('self-host:database-consistency'); + + // Assert + $this->assertSame(Command::FAILURE, $exitCode); + $output = Artisan::output(); + $this->assertSame("Consistency problem: Time entries have an end that is before the start\n - ".$timeEntry->getKey()."\n", $output); + } } diff --git a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php index 733693cb..fb32eaff 100644 --- a/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/TimeEntryEndpointTest.php @@ -2961,6 +2961,101 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract ]); } + public function test_update_endpoint_validation_fails_if_only_end_is_sent_and_it_is_before_the_persisted_start(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + ]); + $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->create([ + 'start' => Carbon::parse('2026-08-01T03:00:00Z'), + 'end' => Carbon::parse('2026-08-01T03:01:00Z'), + ]); + Passport::actingAs($data->user); + + // Act + $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [ + 'end' => '2026-08-01T02:59:55Z', + ]); + + // Assert + $response->assertStatus(422); + $response->assertJsonValidationErrors([ + 'end' => 'The end field must be a date after or equal to start.', + ]); + $timeEntry->refresh(); + $this->assertSame('2026-08-01T03:01:00Z', $timeEntry->end->toIso8601ZuluString()); + } + + public function test_update_endpoint_validation_fails_if_only_start_is_sent_and_it_is_after_the_persisted_end(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + ]); + $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->create([ + 'start' => Carbon::parse('2026-08-01T03:00:00Z'), + 'end' => Carbon::parse('2026-08-01T03:01:00Z'), + ]); + Passport::actingAs($data->user); + + // Act + $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [ + 'start' => '2026-08-01T03:01:05Z', + ]); + + // Assert + $response->assertStatus(422); + $response->assertJsonValidationErrors([ + 'start' => 'The start field must be a date before or equal to end.', + ]); + $timeEntry->refresh(); + $this->assertSame('2026-08-01T03:00:00Z', $timeEntry->start->toIso8601ZuluString()); + } + + public function test_update_endpoint_allows_updating_only_start_of_running_time_entry(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + ]); + $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->active()->create(); + Passport::actingAs($data->user); + + // Act + $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [ + 'start' => '2026-08-01T03:00:00Z', + ]); + + // Assert + $response->assertStatus(200); + $timeEntry->refresh(); + $this->assertSame('2026-08-01T03:00:00Z', $timeEntry->start->toIso8601ZuluString()); + } + + public function test_update_endpoint_allows_updating_only_end_if_it_is_after_the_persisted_start(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'time-entries:update:own', + ]); + $timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->create([ + 'start' => Carbon::parse('2026-08-01T03:00:00Z'), + 'end' => Carbon::parse('2026-08-01T03:01:00Z'), + ]); + Passport::actingAs($data->user); + + // Act + $response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [ + 'end' => '2026-08-01T03:00:00Z', + ]); + + // Assert + $response->assertStatus(200); + $timeEntry->refresh(); + $this->assertSame('2026-08-01T03:00:00Z', $timeEntry->end->toIso8601ZuluString()); + } + public function test_update_endpoint_validation_fails_if_project_id_is_missing_but_request_has_task_id(): void { // Arrange diff --git a/tests/Unit/Service/Import/Importers/ClockifyTimeEntriesImporterTest.php b/tests/Unit/Service/Import/Importers/ClockifyTimeEntriesImporterTest.php index 22d6d693..15be2f52 100644 --- a/tests/Unit/Service/Import/Importers/ClockifyTimeEntriesImporterTest.php +++ b/tests/Unit/Service/Import/Importers/ClockifyTimeEntriesImporterTest.php @@ -231,4 +231,29 @@ class ClockifyTimeEntriesImporterTest extends ImporterTestAbstract $this->assertSame(0, Tag::query()->count()); $this->assertSame(0, Client::query()->count()); } + + public function test_import_fails_if_end_is_before_start(): void + { + // Arrange + $organization = Organization::factory()->create(); + $timezone = 'Europe/Vienna'; + $importer = new ClockifyTimeEntriesImporter; + $importer->init($organization); + $data = <<<'CSV' + "Project","Client","Description","Task","User","Group","Email","Tags","Billable","Start Date","Start Time","End Date","End Time","Duration (h)","Duration (decimal)","Billable Rate (USD)","Billable Amount (USD)" + "Project","Client","Working hard","","Peter Tester","","peter.test@email.test","","Yes","03/04/2024","10:30:00 AM","03/04/2024","10:00:00 AM","00:30:00","0.50","0.00","0.00" + CSV; + + // Act + try { + $importer->importData($data, $timezone); + } catch (ImportException $e) { + // Assert + $this->assertSame('End ("03/04/2024 10:00:00 AM") is before start ("03/04/2024 10:30:00 AM")', $e->getMessage()); + $this->assertSame(0, TimeEntry::query()->count()); + + return; + } + $this->fail(); + } } diff --git a/tests/Unit/Service/Import/Importers/GenericTimeEntriesImporterTest.php b/tests/Unit/Service/Import/Importers/GenericTimeEntriesImporterTest.php index 69db995c..fb9e76fd 100644 --- a/tests/Unit/Service/Import/Importers/GenericTimeEntriesImporterTest.php +++ b/tests/Unit/Service/Import/Importers/GenericTimeEntriesImporterTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Tests\Unit\Service\Import\Importers; use App\Models\Organization; +use App\Models\TimeEntry; use App\Service\Import\Importers\DefaultImporter; use App\Service\Import\Importers\GenericTimeEntriesImporter; use App\Service\Import\Importers\ImportException; @@ -89,4 +90,27 @@ class GenericTimeEntriesImporterTest extends ImporterTestAbstract } $this->fail(); } + + public function test_import_fails_if_end_is_before_start(): void + { + // Arrange + $organization = Organization::factory()->create(); + $timezone = 'Europe/Vienna'; + $importer = new GenericTimeEntriesImporter; + $importer->init($organization); + $data = "description,billable,client,project,tags,start,end,task,user_name,user_email\n". + '"Working hard","true","Big Company","Project for Big Company","","2024-03-04T10:23:00Z","2024-03-04T09:23:00Z","","Peter Tester","peter.test@email.test"'; + + // Act + try { + $importer->importData($data, $timezone); + } catch (ImportException $e) { + // Assert + $this->assertSame('Value of end ("2024-03-04T09:23:00Z") is before start ("2024-03-04T10:23:00Z")', $e->getMessage()); + $this->assertSame(0, TimeEntry::query()->count()); + + return; + } + $this->fail(); + } } diff --git a/tests/Unit/Service/Import/Importers/HarvestTimeEntriesImporterTest.php b/tests/Unit/Service/Import/Importers/HarvestTimeEntriesImporterTest.php index c94f93c5..81f986b1 100644 --- a/tests/Unit/Service/Import/Importers/HarvestTimeEntriesImporterTest.php +++ b/tests/Unit/Service/Import/Importers/HarvestTimeEntriesImporterTest.php @@ -105,4 +105,27 @@ class HarvestTimeEntriesImporterTest extends ImporterTestAbstract $this->assertSame(2, $report->projectsCreated); $this->assertSame(1, $report->clientsCreated); } + + public function test_import_fails_if_hours_are_negative(): void + { + // Arrange + $organization = Organization::factory()->create(); + $timezone = 'Europe/Vienna'; + $importer = new HarvestTimeEntriesImporter; + $importer->init($organization); + $data = "Date,Client,Project,Project Code,Task,Notes,Hours,Billable?,Invoiced?,Approved?,First Name,Last Name,Roles,Employee?,Billable Rate,Billable Amount,Cost Rate,Cost Amount,Currency,External Reference URL\n". + '2024-03-04,,Project without Client,,,"","-2,0",No,No,No,Peter,Tester,,Yes,"100,0","2.000,0","0,0","0,0",Euro - EUR,'; + + // Act + try { + $importer->importData($data, $timezone); + } catch (ImportException $e) { + // Assert + $this->assertSame('Hours ("-2,0") is negative', $e->getMessage()); + $this->assertSame(0, TimeEntry::query()->count()); + + return; + } + $this->fail(); + } } diff --git a/tests/Unit/Service/Import/Importers/SolidtimeImporterTest.php b/tests/Unit/Service/Import/Importers/SolidtimeImporterTest.php index 3e93d675..dc390dc6 100644 --- a/tests/Unit/Service/Import/Importers/SolidtimeImporterTest.php +++ b/tests/Unit/Service/Import/Importers/SolidtimeImporterTest.php @@ -15,7 +15,9 @@ use App\Service\Import\Importers\SolidtimeImporter; use Exception; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Queue; +use Illuminate\Support\Str; use PHPUnit\Framework\Attributes\CoversClass; +use ZipArchive; #[CoversClass(SolidtimeImporter::class)] #[CoversClass(ImportException::class)] @@ -177,4 +179,32 @@ class SolidtimeImporterTest extends ImporterTestAbstract Queue::assertPushed(RecalculateSpentTimeForProject::class, 1); Queue::assertPushed(RecalculateSpentTimeForTask::class, 1); } + + public function test_import_fails_if_end_of_time_entry_is_before_start(): void + { + // Arrange + $organization = Organization::factory()->create(); + $timezone = 'Europe/Vienna'; + $zipPath = $this->createTestZip('solidtime_import_test_1'); + $zip = new ZipArchive; + $zip->open($zipPath); + $timeEntries = $zip->getFromName('time_entries.csv'); + $timeEntries = Str::replaceFirst(',2024-03-04T09:23:52Z,2024-03-04T09:23:52Z,', ',2024-03-04T09:23:52Z,2024-03-04T08:23:52Z,', $timeEntries); + $zip->addFromString('time_entries.csv', $timeEntries); + $zip->close(); + $importer = new SolidtimeImporter; + $importer->init($organization); + $data = file_get_contents($zipPath); + + // Act + try { + $importer->importData($data, $timezone); + } catch (ImportException $e) { + // Assert + $this->assertSame('End date ("2024-03-04T08:23:52Z") is before start date ("2024-03-04T09:23:52Z")', $e->getMessage()); + + return; + } + $this->fail(); + } } diff --git a/tests/Unit/Service/Import/Importers/TogglTimeEntriesImporterTest.php b/tests/Unit/Service/Import/Importers/TogglTimeEntriesImporterTest.php index 719bea69..61e9f11a 100644 --- a/tests/Unit/Service/Import/Importers/TogglTimeEntriesImporterTest.php +++ b/tests/Unit/Service/Import/Importers/TogglTimeEntriesImporterTest.php @@ -120,4 +120,27 @@ class TogglTimeEntriesImporterTest extends ImporterTestAbstract Queue::assertPushed(RecalculateSpentTimeForProject::class, 2); Queue::assertPushed(RecalculateSpentTimeForTask::class, 1); } + + public function test_import_fails_if_end_is_before_start(): void + { + // Arrange + $organization = Organization::factory()->create(); + $timezone = 'Europe/Vienna'; + $importer = new TogglTimeEntriesImporter; + $importer->init($organization); + $data = "User,Email,Client,Project,Task,Description,Billable,Start date,Start time,End date,End time,Duration,Tags,Amount (EUR)\n". + 'Peter Tester,peter.test@email.test,,Project without Client,,"",No,2024-03-04,10:23:52,2024-03-04,09:23:52,-01:00:00,"",'; + + // Act + try { + $importer->importData($data, $timezone); + } catch (ImportException $e) { + // Assert + $this->assertSame('End ("2024-03-04 09:23:52") is before start ("2024-03-04 10:23:52")', $e->getMessage()); + $this->assertSame(0, TimeEntry::query()->count()); + + return; + } + $this->fail(); + } }