Compare commits

...

6 Commits

Author SHA1 Message Date
Gregor Vostrak
7c3f7e2b67 make sure dropdown/combobox lists stay visible during close animation to
avoid layout shifts
2026-07-09 12:46:11 +02:00
Gregor Vostrak
65fbb43aa6 add stable secondary sorting based on id to the tests to avoid flakyness 2026-07-08 18:24:04 +02:00
Gregor Vostrak
4a5ba9ff28 use pinned selection instead of fix items on dropdown open so async
loaded collections update the dropdown properly
2026-07-08 18:06:58 +02:00
Gregor Vostrak
56c45adc1a add validationa for project names in toggl importer 2026-07-08 17:19:57 +02:00
Gregor Vostrak
fa8d350c4a add secondary name sorting as tie breaker in client and project tables 2026-07-08 15:20:00 +02:00
Gregor Vostrak
27f5d4a200 add unique id tiebreaker to paginated index endpoints to make pagination
stable (#1138)
2026-07-08 14:48:54 +02:00
25 changed files with 313 additions and 44 deletions

View File

@@ -43,7 +43,8 @@ class ClientController extends Controller
$clientsQuery = Client::query()
->whereBelongsTo($organization, 'organization')
->orderBy('created_at', 'desc');
->orderBy('created_at', 'desc')
->orderBy('id');
if (! $canViewAllClients) {
$clientsQuery->visibleByEmployee($user);

View File

@@ -42,6 +42,7 @@ class InvitationController extends Controller
$invitations = $organization->organizationInvitations()
->orderBy('created_at', 'desc')
->orderBy('id')
->paginate(config('app.pagination_per_page_default'));
return InvitationCollection::make($invitations);

View File

@@ -61,6 +61,7 @@ class MemberController extends Controller
->whereBelongsTo($organization, 'organization')
->with(['user'])
->orderBy('created_at', 'desc')
->orderBy('id')
->paginate(config('app.pagination_per_page_default'));
return MemberCollection::make($members);

View File

@@ -62,6 +62,7 @@ class ProjectController extends Controller
$projects = $projectsQuery
->orderBy('created_at', 'desc')
->orderBy('id')
->paginate(config('app.pagination_per_page_default'));
$showBillableRate = $this->member($organization)->role !== Role::Employee->value || $organization->employees_can_see_billable_rates;

View File

@@ -49,6 +49,7 @@ class ProjectMemberController extends Controller
$projectMembers = ProjectMember::query()
->whereBelongsTo($project, 'project')
->orderBy('created_at', 'desc')
->orderBy('id')
->paginate(config('app.pagination_per_page_default'));
return new ProjectMemberCollection($projectMembers);

View File

@@ -47,6 +47,7 @@ class ReportController extends Controller
$reports = Report::query()
->orderBy('created_at', 'desc')
->orderBy('id')
->whereBelongsTo($organization, 'organization')
->paginate(config('app.pagination_per_page_default'));

View File

@@ -42,6 +42,7 @@ class TagController extends Controller
$tags = Tag::query()
->whereBelongsTo($organization, 'organization')
->orderBy('created_at', 'desc')
->orderBy('id')
->paginate(config('app.pagination_per_page_default'));
return new TagCollection($tags);

View File

@@ -84,6 +84,7 @@ class TaskController extends Controller
$tasks = $query
->orderBy('created_at', 'desc')
->orderBy('id')
->paginate(config('app.pagination_per_page_default'));
return new TaskCollection($tasks);

View File

@@ -194,7 +194,8 @@ class TimeEntryController extends Controller
$timeEntriesQuery = TimeEntry::query()
->whereBelongsTo($organization, 'organization')
->select($select)
->orderBy('start', 'desc');
->orderBy('time_entries.start', 'desc')
->orderBy('time_entries.id');
$filter = new TimeEntryFilter($timeEntriesQuery);
$filter->addStartFilter($request->input('start'));

View File

@@ -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
{

View File

@@ -502,8 +502,10 @@ test.describe('Project Task Dropdown', () => {
await projectOption.getByText(/Tasks/).click();
await page.getByText(taskName, { exact: true }).click();
// The trigger reflects the selected task.
await expect(page.getByText(taskName)).toBeVisible();
// Scoped to the trigger button: the closing dropdown also contains the name while animating out.
await expect(
page.getByRole('button', { name: `${projectName} ${taskName}` })
).toBeVisible();
});
test('test that keyboard navigation selects a project', async ({ page, ctx }) => {
@@ -654,7 +656,10 @@ test.describe('Project Task Dropdown', () => {
await search.press('ArrowDown');
await search.press('Enter');
await expect(page.getByText(taskName)).toBeVisible();
// Scoped to the trigger button: the closing dropdown also contains the name while animating out.
await expect(
page.getByRole('button', { name: `${projectName} ${taskName}` })
).toBeVisible();
});
test('test that pressing space selects the highlighted project', async ({ page, ctx }) => {

View File

@@ -44,11 +44,14 @@ const projectCountMap = computed(() => {
return map;
});
// Name is always the secondary sort so rows with equal values render
// alphabetically instead of in API (created_at) order.
const sorting = computed<SortingState>(() => [
{
id: props.sortColumn,
desc: props.sortDirection === 'desc',
},
...(props.sortColumn !== 'name' ? [{ id: 'name', desc: false }] : []),
]);
const columns = computed(() => [

View File

@@ -89,11 +89,17 @@ function selectMember(member: Member) {
</Button>
</template>
<template #content>
<!-- kept open so the list stays visible during the popover close animation -->
<ComboboxRoot
v-model:search-term="searchValue"
v-model:open="open"
:open="true"
class="relative"
:filter-function="(val: string[]) => val">
:filter-function="(val: string[]) => val"
@update:open="
(value: boolean) => {
if (!value) open = false;
}
">
<ComboboxAnchor>
<ComboboxInput
ref="searchInput"

View File

@@ -37,7 +37,16 @@ const emit = defineEmits(['update:modelValue', 'changed']);
const activeClients = computed(() => clients.value.filter((c) => !c.is_archived));
const sortedProjects = ref<Project[]>([]);
// Pinned on open so rows don't re-sort while interacting; the project list itself stays reactive.
const pinnedProjectId = ref<string | null>(null);
const sortedProjects = computed(() => {
return [...projects.value].sort((a, b) => {
const aPinned = pinnedProjectId.value === a.id ? 0 : 1;
const bPinned = pinnedProjectId.value === b.id ? 0 : 1;
return aPinned - bPinned;
});
});
const shownProjects = computed(() => {
return sortedProjects.value.filter((project) => {
@@ -65,9 +74,7 @@ watch(open, (isOpen) => {
searchInput.value?.$el?.focus();
});
sortedProjects.value = [...projects.value].sort((iteratingProject) => {
return model.value === iteratingProject.id ? -1 : 1;
});
pinnedProjectId.value = model.value;
}
});
@@ -103,13 +110,19 @@ function updateValue(project: Project) {
</template>
<template #content>
<div v-if="open">
<!-- kept open so the list stays visible during the popover close animation -->
<div>
<ComboboxRoot
v-model:open="open"
:open="true"
:model-value="currentProject"
class="relative"
:ignore-filter="true"
@update:model-value="updateValue">
@update:model-value="updateValue"
@update:open="
(value: boolean) => {
if (!value) open = false;
}
">
<ComboboxAnchor>
<ComboboxInput
ref="searchInput"

View File

@@ -57,12 +57,15 @@ const clientNameMap = computed(() => {
return map;
});
// Convert sort props to TanStack Table format
// Convert sort props to TanStack Table format.
// Name is always the secondary sort so rows with equal values render
// alphabetically instead of in API (created_at) order.
const sorting = computed<SortingState>(() => [
{
id: props.sortColumn,
desc: props.sortDirection === 'desc',
},
...(props.sortColumn !== 'name' ? [{ id: 'name', desc: false }] : []),
]);
// Define column accessors for sorting.

View File

@@ -85,13 +85,18 @@ function updateValue(client: { id: string | null; name: string }) {
<slot name="trigger"></slot>
</template>
<template #content>
<div v-if="open">
<div>
<ComboboxRoot
v-model:open="open"
:open="true"
:model-value="currentClient"
class="relative"
:ignore-filter="true"
@update:model-value="updateValue">
@update:model-value="updateValue"
@update:open="
(value: boolean) => {
if (!value) open = false;
}
">
<ComboboxAnchor>
<ComboboxInput
ref="searchInput"

View File

@@ -1,6 +1,6 @@
<script setup lang="ts" generic="T">
import Dropdown from '@/packages/ui/src/Input/Dropdown.vue';
import { computed, type Ref, ref, watch } from 'vue';
import { computed, ref, watch } from 'vue';
import Checkbox from '@/packages/ui/src/Input/Checkbox.vue';
import {
ComboboxAnchor,
@@ -33,20 +33,25 @@ const props = defineProps<{
const open = ref(false);
const searchValue = ref('');
const sortedItems = ref<T[]>([]) as Ref<T[]>;
// Pinned on open so rows don't re-sort while toggling; the item list itself stays reactive.
const pinnedSelection = ref<Set<string>>(new Set());
watch(open, (isOpen) => {
if (isOpen) {
searchValue.value = '';
sortedItems.value = [...props.items].sort((a, b) => {
const aSelected = model.value.includes(props.getKeyFromItem(a)) ? 0 : 1;
const bSelected = model.value.includes(props.getKeyFromItem(b)) ? 0 : 1;
if (aSelected !== bSelected) return aSelected - bSelected;
return props.getNameForItem(a).localeCompare(props.getNameForItem(b));
});
pinnedSelection.value = new Set(model.value);
}
});
const sortedItems = computed(() => {
return [...props.items].sort((a, b) => {
const aSelected = pinnedSelection.value.has(props.getKeyFromItem(a)) ? 0 : 1;
const bSelected = pinnedSelection.value.has(props.getKeyFromItem(b)) ? 0 : 1;
if (aSelected !== bSelected) return aSelected - bSelected;
return props.getNameForItem(a).localeCompare(props.getNameForItem(b));
});
});
const filteredItems = computed(() => {
const search = searchValue.value.toLowerCase().trim();
if (!search) return sortedItems.value;
@@ -97,7 +102,16 @@ const emit = defineEmits(['update:modelValue', 'changed', 'submit']);
<slot name="trigger"></slot>
</template>
<template #content>
<ComboboxRoot v-model:open="open" class="p-2" :ignore-filter="true">
<!-- kept open so the list stays visible during the popover close animation -->
<ComboboxRoot
:open="true"
class="p-2"
:ignore-filter="true"
@update:open="
(value: boolean) => {
if (!value) open = false;
}
">
<ComboboxAnchor>
<ComboboxInput
v-model="searchValue"

View File

@@ -37,19 +37,24 @@ const model = defineModel<string[]>({
const open = ref(false);
const searchValue = ref('');
const sortedTags = ref<Tag[]>([]);
// Pinned on open so rows don't re-sort while toggling; the tag list itself stays reactive.
const pinnedSelection = ref<Set<string>>(new Set());
watch(open, (isOpen) => {
if (isOpen) {
searchValue.value = '';
sortedTags.value = [...props.tags].sort((a, b) => {
const aSelected = model.value.includes(a.id) ? 0 : 1;
const bSelected = model.value.includes(b.id) ? 0 : 1;
return aSelected - bSelected;
});
pinnedSelection.value = new Set(model.value);
}
});
const sortedTags = computed(() => {
return [...props.tags].sort((a, b) => {
const aSelected = pinnedSelection.value.has(a.id) ? 0 : 1;
const bSelected = pinnedSelection.value.has(b.id) ? 0 : 1;
return aSelected - bSelected;
});
});
const filteredTags = computed(() => {
const search = searchValue.value.toLowerCase().trim();
if (!search) return sortedTags.value;

View File

@@ -594,7 +594,7 @@ const showCreateProject = ref(false);
</slot>
</template>
<template #content>
<div v-if="open">
<div>
<input
ref="searchInput"
:value="searchValue"

View File

@@ -45,7 +45,7 @@ class ClientEndpointTest extends ApiEndpointTestAbstract
// Assert
$response->assertStatus(200);
$response->assertJsonCount(4, 'data');
$clients = Client::query()->orderBy('created_at', 'desc')->get();
$clients = Client::query()->orderBy('created_at', 'desc')->orderBy('id')->get();
$response->assertJson(fn (AssertableJson $json) => $json
->has('data')
->has('links')
@@ -84,9 +84,12 @@ class ClientEndpointTest extends ApiEndpointTestAbstract
->has('links')
->has('meta')
->count('data', 2)
->where('data.0.id', $clients->get(0)->getKey())
->where('data.1.id', $clients->get(1)->getKey())
);
// Both clients share the same created_at, so their relative order is not defined.
$this->assertEqualsCanonicalizing([
$clients->get(0)->getKey(),
$clients->get(1)->getKey(),
], $response->json('data.*.id'));
}
public function test_index_endpoint_without_filter_archived_returns_only_non_archived_clients(): void

View File

@@ -13,6 +13,8 @@ use App\Models\ProjectMember;
use App\Models\Task;
use App\Models\TimeEntry;
use App\Service\BillableRateService;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Str;
use Illuminate\Testing\Fluent\AssertableJson;
use Laravel\Passport\Passport;
use Mockery\MockInterface;
@@ -81,6 +83,49 @@ class ProjectEndpointTest extends ApiEndpointTestAbstract
$this->assertSame([$projectNewest->getKey(), $projectMiddle->getKey(), $projectOldest->getKey()], $ids);
}
public function test_index_endpoint_pagination_returns_every_project_exactly_once_when_they_share_created_at(): void
{
// Arrange
$data = $this->createUserWithPermission([
'projects:view',
'projects:view:all',
]);
config(['app.pagination_per_page_default' => 15]);
// Bulk import: 300 projects that all share the exact same created_at.
$sharedCreatedAt = now()->subDay()->startOfSecond();
$rows = [];
for ($i = 0; $i < 300; $i++) {
$rows[] = [
'id' => (string) Str::uuid(),
'name' => 'Project '.$i,
'color' => '#000000',
'is_billable' => false,
'is_public' => false,
'organization_id' => $data->organization->getKey(),
'created_at' => $sharedCreatedAt,
'updated_at' => $sharedCreatedAt,
];
}
DB::table('projects')->insert($rows);
Passport::actingAs($data->user);
// Act - walk every page like resources/js/utils/fetchAllPages.ts does.
$orgId = $data->organization->getKey();
$first = $this->getJson(route('api.v1.projects.index', [$orgId]).'?page=1');
$this->assertResponseCode($first, 200);
$lastPage = $first->json('meta.last_page');
$collected = collect($first->json('data.*.id'));
for ($page = 2; $page <= $lastPage; $page++) {
$response = $this->getJson(route('api.v1.projects.index', [$orgId]).'?page='.$page);
$this->assertResponseCode($response, 200);
$collected = $collected->concat($response->json('data.*.id'));
}
// Assert - every project appears exactly once, none duplicated or missing.
$this->assertEqualsCanonicalizing(array_column($rows, 'id'), $collected->all(), 'Some projects were duplicated or missing across pages');
}
public function test_index_endpoint_without_filter_archived_returns_only_non_archived_projects(): void
{
// Arrange

View File

@@ -51,7 +51,7 @@ class ReportEndpointTest extends ApiEndpointTestAbstract
// Assert
$response->assertStatus(200);
$response->assertJsonCount(4, 'data');
$reports = Report::query()->orderBy('created_at', 'desc')->get();
$reports = Report::query()->orderBy('created_at', 'desc')->orderBy('id')->get();
$response->assertJson(fn (AssertableJson $json) => $json
->has('data')
->has('links')

View File

@@ -44,7 +44,7 @@ class TagEndpointTest extends ApiEndpointTestAbstract
// Assert
$response->assertStatus(200);
$response->assertJsonCount(4, 'data');
$tags = Tag::query()->orderBy('created_at', 'desc')->get();
$tags = Tag::query()->orderBy('created_at', 'desc')->orderBy('id')->get();
$response->assertJson(fn (AssertableJson $json) => $json
->has('data')
->has('links')

View File

@@ -23,6 +23,7 @@ use App\Models\User;
use App\Service\TimeEntryFilter;
use Illuminate\Support\Carbon;
use Illuminate\Support\Facades\Config;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Facades\Queue;
use Illuminate\Support\Facades\Storage;
@@ -391,6 +392,59 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract
);
}
public function test_index_endpoint_pagination_returns_every_time_entry_exactly_once_with_rounding(): void
{
// Arrange
$data = $this->createUserWithPermission([
'time-entries:view:own',
]);
// Bulk import: 300 time entries that all share the exact same start.
$sharedStart = Carbon::createFromFormat('Y-m-d H:i:s', '2020-01-01 00:00:07');
$rows = [];
for ($i = 0; $i < 300; $i++) {
$rows[] = [
'id' => (string) Str::uuid(),
'description' => 'Entry '.$i,
'start' => $sharedStart,
'end' => $sharedStart,
'billable' => false,
'is_imported' => true,
'user_id' => $data->member->user_id,
'member_id' => $data->member->getKey(),
'organization_id' => $data->organization->getKey(),
'created_at' => $sharedStart,
'updated_at' => $sharedStart,
];
}
DB::table('time_entries')->insert($rows);
$this->actAsOrganizationWithSubscription();
Passport::actingAs($data->user);
// Act - walk every page like the client does (limit/offset), with rounding enabled.
$orgId = $data->organization->getKey();
$limit = 15;
$collected = collect();
$offset = 0;
do {
$response = $this->getJson(route('api.v1.time-entries.index', [
$orgId,
'member_id' => $data->member->getKey(),
'rounding_type' => TimeEntryRoundingType::Nearest,
'rounding_minutes' => 6,
'limit' => $limit,
'offset' => $offset,
]));
$this->assertResponseCode($response, 200);
$ids = $response->json('data.*.id');
$collected = $collected->concat($ids);
$offset += $limit;
} while (count($ids) === $limit);
// Assert - every time entry appears exactly once, none duplicated or missing.
$this->assertEqualsCanonicalizing(array_column($rows, 'id'), $collected->all(), 'Some time entries were duplicated or missing across pages');
}
public function test_index_endpoint_can_round_up(): void
{
// Arrange

View File

@@ -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