From 13a05f6d0d5ce94b0d95fff5f815feb5f09f9374 Mon Sep 17 00:00:00 2001 From: bisco Date: Wed, 29 Apr 2026 21:49:21 +0200 Subject: [PATCH] fix(security): separate booking and check-in tokens --- backend/bookings/services.py | 25 +++++----- backend/bookings/test_admin.py | 25 +++++----- backend/bookings/test_api.py | 66 +++++++++++++++++++++------ backend/bookings/test_services.py | 76 ++++++++++++++++++++++++++----- backend/checkins/services.py | 9 +--- backend/checkins/test_api.py | 37 +++++++++++++++ backend/checkins/test_services.py | 22 +++++++++ docs/api-contract.md | 5 +- docs/booking-flow.md | 8 ++-- docs/security-notes.md | 5 +- 10 files changed, 214 insertions(+), 64 deletions(-) diff --git a/backend/bookings/services.py b/backend/bookings/services.py index 389c9d7..9d963f3 100644 --- a/backend/bookings/services.py +++ b/backend/bookings/services.py @@ -113,16 +113,19 @@ def create_pending_reservation( expires_at=confirmation_expires_at, ) + transaction.on_commit( + lambda reservation=reservation, raw_confirmation_token=raw_confirmation_token: send_confirmation_email( + reservation=reservation, + raw_confirmation_token=raw_confirmation_token, + ) + ) + result = PendingReservationResult( reservation=reservation, confirmation_token=confirmation_token, raw_confirmation_token=raw_confirmation_token, available_seats=available_seats, ) - send_confirmation_email( - reservation=result.reservation, - raw_confirmation_token=result.raw_confirmation_token, - ) return result @@ -195,22 +198,22 @@ def confirm_reservation_from_token(raw_token): available_seats=available_seats - reservation.party_size, qr_code_image=generate_check_in_qr_base64( reservation=reservation, - raw_check_in_token=raw_token, + raw_check_in_token=raw_check_in_token, ), - qr_code_url=build_check_in_preview_url(raw_token), + qr_code_url=build_check_in_preview_url(raw_check_in_token), ) def retrieve_reservation_qr_from_token(raw_token): try: - confirmation_token = ReservationToken.objects.select_related("reservation").get( - token_hash=ReservationToken.hash_token(raw_token), - purpose=ReservationToken.Purpose.CONFIRMATION, + check_in_token = ReservationToken.objects.select_related("reservation").get_valid_token( + raw_token, + ReservationToken.Purpose.CHECK_IN, ) except ReservationToken.DoesNotExist as exc: - raise InvalidToken("Confirmation token is invalid.") from exc + raise InvalidToken("Check-in token is invalid.") from exc - reservation = confirmation_token.reservation + reservation = check_in_token.reservation if reservation.status != Reservation.Status.CONFIRMED: raise ReservationNotConfirmed("Reservation must be confirmed before QR retrieval.") diff --git a/backend/bookings/test_admin.py b/backend/bookings/test_admin.py index 7f4eb95..37367f3 100644 --- a/backend/bookings/test_admin.py +++ b/backend/bookings/test_admin.py @@ -53,18 +53,19 @@ class ReservationAdminTests(TestCase): self.assertContains(response, "The reservation stays pending") def test_admin_can_create_manual_reservation_with_standard_email_flow(self): - response = self.client.post( - reverse("admin:bookings_reservation_add"), - { - "performance": self.performance.id, - "name": "Maria Rossi", - "email": "maria@example.com", - "phone": "+390600000000", - "party_size": 2, - "notes": "Entered by staff at the venue desk.", - "_save": "Save", - }, - ) + with self.captureOnCommitCallbacks(execute=True): + response = self.client.post( + reverse("admin:bookings_reservation_add"), + { + "performance": self.performance.id, + "name": "Maria Rossi", + "email": "maria@example.com", + "phone": "+390600000000", + "party_size": 2, + "notes": "Entered by staff at the venue desk.", + "_save": "Save", + }, + ) reservation = Reservation.objects.get() self.assertEqual(response.status_code, 302) diff --git a/backend/bookings/test_api.py b/backend/bookings/test_api.py index b5ddab4..6e1a6ac 100644 --- a/backend/bookings/test_api.py +++ b/backend/bookings/test_api.py @@ -63,17 +63,18 @@ class BookingApiTests(APITestCase): @override_settings(SITE_BASE_URL="https://tickets.azionelab.example") def test_reservation_creation_success(self): - response = self.client.post( - reverse("api-reservation-create", kwargs={"performance_id": self.performance.id}), - { - "name": "Maria Rossi", - "email": "maria@example.com", - "phone": "+390600000000", - "party_size": 2, - "notes": "Front row if possible.", - }, - format="json", - ) + with self.captureOnCommitCallbacks(execute=True): + response = self.client.post( + reverse("api-reservation-create", kwargs={"performance_id": self.performance.id}), + { + "name": "Maria Rossi", + "email": "maria@example.com", + "phone": "+390600000000", + "party_size": 2, + "notes": "Front row if possible.", + }, + format="json", + ) self.assertEqual(response.status_code, status.HTTP_201_CREATED) self.assertEqual(response.data["status"], Reservation.Status.PENDING) @@ -87,6 +88,22 @@ class BookingApiTests(APITestCase): mail.outbox[0].body, ) + def test_reservation_creation_schedules_email_after_commit(self): + with self.captureOnCommitCallbacks(execute=False) as callbacks: + response = self.client.post( + reverse("api-reservation-create", kwargs={"performance_id": self.performance.id}), + { + "name": "Maria Rossi", + "email": "maria@example.com", + "party_size": 2, + }, + format="json", + ) + + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertEqual(len(callbacks), 1) + self.assertEqual(len(mail.outbox), 0) + def test_reservation_creation_with_insufficient_seats(self): response = self.client.post( reverse("api-reservation-create", kwargs={"performance_id": self.performance.id}), @@ -123,6 +140,7 @@ class BookingApiTests(APITestCase): "https://tickets.azionelab.example/api/check-ins/preview/?token=" ) ) + self.assertNotIn(raw_token, response.data["qr_code_url"]) self.assertNotIn("token", response.data) self.assertTrue(response.data["qr_code_image"].startswith("data:image/png;base64,")) self.assertEqual(reservation.status, Reservation.Status.CONFIRMED) @@ -174,15 +192,16 @@ class BookingApiTests(APITestCase): def test_qr_retrieval_success_for_confirmed_reservation(self): reservation = self.create_reservation() _, raw_token = generate_confirmation_token(reservation) - self.client.post( + confirmation_response = self.client.post( reverse("api-reservation-confirm"), {"token": raw_token}, format="json", ) + check_in_token = confirmation_response.data["qr_code_url"].split("token=", 1)[1] response = self.client.get( reverse("api-reservation-qr"), - {"token": raw_token}, + {"token": check_in_token}, ) self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -196,6 +215,23 @@ class BookingApiTests(APITestCase): self.assertNotIn("email", response.data) self.assertNotIn("name", response.data) + def test_qr_retrieval_rejects_confirmation_token(self): + reservation = self.create_reservation() + _, raw_token = generate_confirmation_token(reservation) + self.client.post( + reverse("api-reservation-confirm"), + {"token": raw_token}, + format="json", + ) + + response = self.client.get( + reverse("api-reservation-qr"), + {"token": raw_token}, + ) + + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertEqual(response.data["status"], "invalid_token") + def test_qr_retrieval_fails_for_invalid_token(self): response = self.client.get( reverse("api-reservation-qr"), @@ -214,8 +250,8 @@ class BookingApiTests(APITestCase): {"token": raw_token}, ) - self.assertEqual(response.status_code, status.HTTP_409_CONFLICT) - self.assertEqual(response.data["status"], "reservation_not_confirmed") + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertEqual(response.data["status"], "invalid_token") self.assertEqual(reservation.status, Reservation.Status.PENDING) def create_reservation(self, **overrides): diff --git a/backend/bookings/test_services.py b/backend/bookings/test_services.py index 7480e5c..99cccc7 100644 --- a/backend/bookings/test_services.py +++ b/backend/bookings/test_services.py @@ -19,6 +19,7 @@ from bookings.services import ( confirm_reservation_from_token, create_pending_reservation, generate_confirmation_token, + retrieve_reservation_qr_from_token, ) from shows.models import Performance, Show, Venue @@ -64,14 +65,16 @@ class BookingServiceTests(TestCase): EMAIL_BACKEND="django.core.mail.backends.locmem.EmailBackend", SITE_BASE_URL="https://tickets.azionelab.example", ) - def test_create_pending_reservation_sends_confirmation_email(self): - result = create_pending_reservation( - performance_id=self.performance.id, - name="Maria Rossi", - email="maria@example.com", - party_size=1, - ) + def test_create_pending_reservation_sends_confirmation_email_after_commit(self): + with self.captureOnCommitCallbacks(execute=True) as callbacks: + result = create_pending_reservation( + performance_id=self.performance.id, + name="Maria Rossi", + email="maria@example.com", + party_size=1, + ) + self.assertEqual(len(callbacks), 1) self.assertEqual(len(mail.outbox), 1) self.assertEqual(mail.outbox[0].to, ["maria@example.com"]) self.assertIn(result.raw_confirmation_token, mail.outbox[0].body) @@ -80,16 +83,30 @@ class BookingServiceTests(TestCase): mail.outbox[0].body, ) - @patch("bookings.emailing.send_mail", side_effect=RuntimeError("SMTP down")) - def test_create_pending_reservation_logs_email_failure_without_crashing(self, mocked_send_mail): - with self.assertLogs("bookings.emailing", level="ERROR") as captured_logs: - result = create_pending_reservation( + @override_settings(EMAIL_BACKEND="django.core.mail.backends.locmem.EmailBackend") + def test_create_pending_reservation_defers_email_until_commit(self): + with self.captureOnCommitCallbacks(execute=False) as callbacks: + create_pending_reservation( performance_id=self.performance.id, name="Maria Rossi", email="maria@example.com", party_size=1, ) + self.assertEqual(len(callbacks), 1) + self.assertEqual(len(mail.outbox), 0) + + @patch("bookings.emailing.send_mail", side_effect=RuntimeError("SMTP down")) + def test_create_pending_reservation_logs_email_failure_without_crashing(self, mocked_send_mail): + with self.assertLogs("bookings.emailing", level="ERROR") as captured_logs: + with self.captureOnCommitCallbacks(execute=True): + result = create_pending_reservation( + performance_id=self.performance.id, + name="Maria Rossi", + email="maria@example.com", + party_size=1, + ) + self.assertEqual(result.reservation.status, Reservation.Status.PENDING) self.assertEqual(Reservation.objects.count(), 1) mocked_send_mail.assert_called_once() @@ -130,7 +147,7 @@ class BookingServiceTests(TestCase): self.assertEqual(result.available_seats, 1) self.assertEqual( result.qr_code_url, - build_check_in_preview_url(raw_token), + build_check_in_preview_url(result.raw_check_in_token), ) self.assertTrue( result.qr_code_url.startswith( @@ -139,6 +156,19 @@ class BookingServiceTests(TestCase): ) self.assertTrue(result.qr_code_image.startswith("data:image/png;base64,")) + @override_settings(SITE_BASE_URL="https://tickets.azionelab.example") + def test_confirmation_token_cannot_be_reused_as_qr_or_check_in_token(self): + reservation = self.create_reservation() + _, raw_token = generate_confirmation_token(reservation) + + result = confirm_reservation_from_token(raw_token) + + self.assertNotEqual(raw_token, result.raw_check_in_token) + self.assertNotEqual( + build_check_in_preview_url(raw_token), + result.qr_code_url, + ) + @override_settings(SITE_BASE_URL="https://tickets.azionelab.example") def test_qr_code_is_generated_for_confirmed_reservation(self): reservation = self.create_reservation( @@ -171,6 +201,28 @@ class BookingServiceTests(TestCase): raw_check_in_token="opaque-check-in-token", ) + def test_qr_retrieval_rejects_confirmation_token(self): + reservation = self.create_reservation() + _, raw_confirmation_token = generate_confirmation_token(reservation) + confirm_reservation_from_token(raw_confirmation_token) + + with self.assertRaises(InvalidToken): + retrieve_reservation_qr_from_token(raw_confirmation_token) + + def test_qr_retrieval_accepts_check_in_token(self): + reservation = self.create_reservation() + _, raw_confirmation_token = generate_confirmation_token(reservation) + result = confirm_reservation_from_token(raw_confirmation_token) + + qr_result = retrieve_reservation_qr_from_token(result.raw_check_in_token) + + self.assertEqual(qr_result.reservation, reservation) + self.assertEqual( + qr_result.qr_code_url, + build_check_in_preview_url(result.raw_check_in_token), + ) + self.assertTrue(qr_result.qr_code_image.startswith("data:image/png;base64,")) + def test_confirmation_fails_when_capacity_is_exhausted(self): Reservation.objects.create( performance=self.performance, diff --git a/backend/checkins/services.py b/backend/checkins/services.py index 40375f0..cda31d7 100644 --- a/backend/checkins/services.py +++ b/backend/checkins/services.py @@ -98,17 +98,12 @@ def _get_reservation_for_check_in_token(raw_token, *, lock_token=False): try: token = queryset.get( token_hash=ReservationToken.hash_token(raw_token), + purpose=ReservationToken.Purpose.CHECK_IN, ) except ReservationToken.DoesNotExist as exc: raise InvalidToken("Check-in token is invalid.") from exc - if token.purpose == ReservationToken.Purpose.CHECK_IN: - if token.used_at is not None or token.is_expired: - raise InvalidToken("Check-in token is invalid.") - elif token.purpose == ReservationToken.Purpose.CONFIRMATION: - if token.reservation.status != Reservation.Status.CONFIRMED: - raise InvalidToken("Check-in token is invalid.") - else: + if token.used_at is not None or token.is_expired: raise InvalidToken("Check-in token is invalid.") return token.reservation diff --git a/backend/checkins/test_api.py b/backend/checkins/test_api.py index 344014d..b066c78 100644 --- a/backend/checkins/test_api.py +++ b/backend/checkins/test_api.py @@ -87,6 +87,24 @@ class CheckInApiTests(APITestCase): self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) self.assertEqual(response.data["status"], "invalid_token") + def test_preview_rejects_confirmation_token(self): + reservation = self.create_reservation() + _, raw_token = ReservationToken.create_token( + reservation=reservation, + purpose=ReservationToken.Purpose.CONFIRMATION, + expires_at=timezone.now() + timedelta(hours=2), + ) + self.client.force_authenticate(user=self.staff_user) + + response = self.client.post( + reverse("api-check-in-preview"), + {"token": raw_token}, + format="json", + ) + + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertEqual(response.data["status"], "invalid_token") + def test_check_in_success_as_staff_user(self): reservation = self.create_reservation() _, raw_token = self.create_check_in_token(reservation) @@ -171,6 +189,25 @@ class CheckInApiTests(APITestCase): self.assertEqual(second_response.data["status"], "already_checked_in") self.assertEqual(CheckIn.objects.filter(reservation=reservation).count(), 1) + def test_check_in_rejects_confirmation_token(self): + reservation = self.create_reservation() + _, raw_token = ReservationToken.create_token( + reservation=reservation, + purpose=ReservationToken.Purpose.CONFIRMATION, + expires_at=timezone.now() + timedelta(hours=2), + ) + self.client.force_authenticate(user=self.staff_user) + + response = self.client.post( + reverse("api-check-in-confirm"), + {"token": raw_token}, + format="json", + ) + + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertEqual(response.data["status"], "invalid_token") + self.assertFalse(CheckIn.objects.filter(reservation=reservation).exists()) + def create_reservation(self, **overrides): data = { "performance": self.performance, diff --git a/backend/checkins/test_services.py b/backend/checkins/test_services.py index 7f46277..36eb789 100644 --- a/backend/checkins/test_services.py +++ b/backend/checkins/test_services.py @@ -62,6 +62,17 @@ class CheckInServiceTests(TestCase): with self.assertRaises(InvalidToken): preview_check_in_token("invalid-token", staff_user=self.staff_user) + def test_preview_rejects_confirmation_token_even_for_confirmed_reservation(self): + reservation = self.create_reservation() + _, raw_token = ReservationToken.create_token( + reservation=reservation, + purpose=ReservationToken.Purpose.CONFIRMATION, + expires_at=timezone.now() + timedelta(hours=2), + ) + + with self.assertRaises(InvalidToken): + preview_check_in_token(raw_token, staff_user=self.staff_user) + def test_check_in_succeeds_for_confirmed_reservation(self): reservation = self.create_reservation() _, raw_token = self.create_check_in_token(reservation) @@ -114,6 +125,17 @@ class CheckInServiceTests(TestCase): with self.assertRaises(MissingStaffUser): confirm_check_in_from_token(raw_token, staff_user=None) + def test_check_in_rejects_confirmation_token_even_for_confirmed_reservation(self): + reservation = self.create_reservation() + _, raw_token = ReservationToken.create_token( + reservation=reservation, + purpose=ReservationToken.Purpose.CONFIRMATION, + expires_at=timezone.now() + timedelta(hours=2), + ) + + with self.assertRaises(InvalidToken): + confirm_check_in_from_token(raw_token, staff_user=self.staff_user) + def create_reservation(self, **overrides): data = { "performance": self.performance, diff --git a/docs/api-contract.md b/docs/api-contract.md index b0a3f0d..cfdabf5 100644 --- a/docs/api-contract.md +++ b/docs/api-contract.md @@ -204,7 +204,7 @@ Response `200 OK`: "reservation_id": 123, "status": "confirmed", "party_size": 2, - "qr_code_url": "https://example.org/api/reservations/123/qr-code/" + "qr_code_url": "https://example.org/api/check-ins/preview/?token=opaque-check-in-token" } ``` @@ -222,13 +222,14 @@ Status codes: GET /api/reservations/{id}/qr-code/ ``` -Returns the generated QR code for a confirmed reservation. Access must be protected by a valid QR token, signed URL, or equivalent control so that reservation IDs are not enough to retrieve QR codes. +Returns the generated QR code for a confirmed reservation. Access must be protected by a valid opaque `check_in` token, signed URL, or equivalent control so that reservation IDs are not enough to retrieve QR codes. Response `200 OK`: ```json { "reservation_id": 123, + "qr_code_url": "https://example.org/api/check-ins/preview/?token=opaque-check-in-token", "qr_code_image": "data:image/png;base64,...", "printable": true } diff --git a/docs/booking-flow.md b/docs/booking-flow.md index d4335a5..46b8667 100644 --- a/docs/booking-flow.md +++ b/docs/booking-flow.md @@ -25,7 +25,7 @@ Availability shown to visitors is informational. The backend recalculates availa - requested seats do not exceed currently available seats. 6. The backend creates a `pending` reservation. 7. The backend creates a random opaque confirmation token. -8. The backend sends an email with a confirmation link. +8. After the transaction commits successfully, the backend sends an email with a confirmation link. 9. The frontend tells the visitor to check their email. The reservation is not confirmed at this stage. @@ -45,8 +45,8 @@ The reservation is not confirmed at this stage. 6. The backend recalculates confirmed reservations for the performance. 7. The backend confirms the reservation only if enough seats remain. 8. The backend marks the confirmation token as used. -9. The backend creates a QR verification token. -10. The backend generates a QR code containing the opaque QR token or a verification URL. +9. The backend creates a separate `check_in` token for QR verification. +10. The backend generates a QR code containing only the opaque check-in token or a verification URL built from that token. 11. The backend returns or sends the QR code to the visitor. If there is no longer enough capacity, the backend must not confirm the reservation. @@ -111,7 +111,7 @@ This operational flow should still follow the same backend rules as the public b 2. the backend validates booking availability and capacity; 3. the backend creates a `pending` reservation; 4. the backend creates the normal confirmation token; -5. the backend sends the standard confirmation email; +5. after the reservation transaction commits, the backend sends the standard confirmation email; 6. the guest still confirms through the email link before the reservation becomes confirmed and usable for check-in. ## Duplicate Check-In diff --git a/docs/security-notes.md b/docs/security-notes.md index d9a66c8..1c10609 100644 --- a/docs/security-notes.md +++ b/docs/security-notes.md @@ -36,6 +36,9 @@ Rules: - do not encode personal data in tokens; - store token hashes when practical; - treat raw tokens as secrets; +- keep confirmation tokens and check-in tokens separate by purpose; +- allow confirmation tokens only for reservation confirmation; +- allow check-in tokens only for QR retrieval and check-in validation; - mark one-time confirmation tokens as used after successful confirmation; - expire confirmation tokens after a reasonable period; - keep QR tokens valid only for the intended performance and check-in period where practical. @@ -165,7 +168,7 @@ Do not log: Initial residual risks: -- synchronous email can make booking responses depend on SMTP availability; +- synchronous email after commit can still add latency to booking requests even though it no longer runs inside the reservation transaction; - QR codes can be copied, so duplicate check-in prevention must be reliable; - staff account compromise would expose admin and check-in functionality; - retention and deletion rules for personal data still need a project policy.