diff --git a/lib/Service/Appointments/BookingService.php b/lib/Service/Appointments/BookingService.php index c69ffdfe38..09674edf5b 100644 --- a/lib/Service/Appointments/BookingService.php +++ b/lib/Service/Appointments/BookingService.php @@ -87,9 +87,14 @@ public function __construct(AvailabilityGenerator $availabilityGenerator, * @throws NoSlotFoundException|ClientException|DbException */ public function confirmBooking(Booking $booking, AppointmentConfig $config): Booking { - $bookingSlot = current($this->getAvailableSlots($config, $booking->getStart(), $booking->getEnd())); + $availableSlots = $this->getAvailableSlots($config, $booking->getStart(), $booking->getEnd()); + $selectedSlot = $this->findMatchingSlot( + $availableSlots, + $booking->getStart(), + $booking->getEnd(), + ); - if (!$bookingSlot) { + if ($selectedSlot === null) { throw new NoSlotFoundException('Slot for booking is not available any more'); } @@ -131,9 +136,14 @@ public function confirmBooking(Booking $booking, AppointmentConfig $config): Boo * @throws ServiceException|DbException|NoSlotFoundException|InvalidArgumentException */ public function book(AppointmentConfig $config, int $start, int $end, string $timeZone, string $displayName, string $email, ?string $description = null): Booking { - $bookingSlot = current($this->getAvailableSlots($config, $start, $end)); + $availableSlots = $this->getAvailableSlots($config, $start, $end); + $selectedSlot = $this->findMatchingSlot( + $availableSlots, + $start, + $end, + ); - if (!$bookingSlot) { + if ($selectedSlot === null) { throw new NoSlotFoundException('Could not find slot for booking'); } @@ -150,8 +160,8 @@ public function book(AppointmentConfig $config, int $start, int $end, string $ti $booking->setDisplayName($displayName); $booking->setDescription($description); $booking->setEmail($email); - $booking->setStart($start); - $booking->setEnd($end); + $booking->setStart($selectedSlot->getStart()); + $booking->setEnd($selectedSlot->getEnd()); $booking->setTimezone($tz->getName()); try { $this->bookingMapper->insert($booking); @@ -207,6 +217,21 @@ public function getAvailableSlots(AppointmentConfig $config, int $startTime, int return $available; } + /** + * Find the slot within $slots whose bounds exactly match $start and $end. + * + * @param Interval[] $slots + */ + private function findMatchingSlot(array $slots, int $start, int $end): ?Interval { + foreach ($slots as $slot) { + if ($slot->getStart() === $start && $slot->getEnd() === $end) { + return $slot; + } + } + + return null; + } + /** * @throws ClientException */ diff --git a/tests/php/unit/Service/Appointments/BookingServiceTest.php b/tests/php/unit/Service/Appointments/BookingServiceTest.php index b6280bbb44..e5cf97d9e8 100644 --- a/tests/php/unit/Service/Appointments/BookingServiceTest.php +++ b/tests/php/unit/Service/Appointments/BookingServiceTest.php @@ -15,6 +15,7 @@ use OCA\Calendar\Db\Booking; use OCA\Calendar\Db\BookingMapper; use OCA\Calendar\Exception\ClientException; +use OCA\Calendar\Exception\NoSlotFoundException; use OCA\Calendar\Service\Appointments\AvailabilityGenerator; use OCA\Calendar\Service\Appointments\BookingCalendarWriter; use OCA\Calendar\Service\Appointments\BookingService; @@ -161,7 +162,7 @@ public function testBookInvalidTimezone(): void { ->method('generate'); $this->expectExceptionObject(new InvalidArgumentException('Could not make sense of the timezone')); - $this->service->book(new AppointmentConfig(), 4054546654, 44545454, 'Nighttime/DAYTIME!', 'Test', 'test@test.com', 'Test'); + $this->service->book(new AppointmentConfig(), 1891378800, 1891382400, 'Nighttime/DAYTIME!', 'Test', 'test@test.com', 'Test'); } public function testBook(): void { @@ -197,6 +198,33 @@ public function testBook(): void { $this->service->book(new AppointmentConfig(), $start->getTimestamp(), $end->getTimestamp(), 'Europe/Berlin', 'Test', 'test@test.com'); } + public function testBookSlotMismatch(): void { + // Attacker-supplied range that only overlaps a real slot instead of matching one exactly. + $start = 1891378800; + $end = 1891389600; + $intervals = [ + new Interval(1891382400, 1891386000), + ]; + + $this->availabilityGenerator->expects(self::once()) + ->method('generate') + ->willReturn($intervals); + $this->extrapolator->expects(self::once()) + ->method('extrapolate') + ->willReturnArgument(1); + $this->dailyLimitFilter->expects(self::once()) + ->method('filter') + ->willReturnArgument(1); + $this->eventConflictFilter->expects(self::once()) + ->method('filter') + ->willReturnArgument(1); + $this->bookingMapper->expects(self::never()) + ->method('insert'); + + $this->expectException(NoSlotFoundException::class); + $this->service->book(new AppointmentConfig(), $start, $end, 'Europe/Berlin', 'Test', 'test@test.com'); + } + public function testConfirmBooking(): void { $booking = new Booking(); $booking->setStart(1891378800); @@ -264,6 +292,38 @@ public function testConfirmBookingNoSlot(): void { $this->service->confirmBooking($booking, $config); } + public function testConfirmBookingSlotMismatch(): void { + // The persisted booking's start/end no longer match any currently available slot, + // even though the availability check itself returns a non-empty list. + $booking = new Booking(); + $booking->setStart(1891378800); + $booking->setEnd(1891382400); + $config = new AppointmentConfig(); + $interval = [ + new Interval(1891382400, 1891386000), + ]; + + $this->availabilityGenerator->expects(self::once()) + ->method('generate') + ->willReturn($interval); + $this->extrapolator->expects(self::once()) + ->method('extrapolate') + ->willReturnArgument(1); + $this->dailyLimitFilter->expects(self::once()) + ->method('filter') + ->willReturnArgument(1); + $this->eventConflictFilter->expects(self::once()) + ->method('filter') + ->willReturnArgument(1); + $this->bookingCalendarWriter->expects(self::never()) + ->method('write'); + $this->bookingMapper->expects(self::never()) + ->method('update'); + + $this->expectException(NoSlotFoundException::class); + $this->service->confirmBooking($booking, $config); + } + public function testFindByToken() { $token = 'test';