-
Notifications
You must be signed in to change notification settings - Fork 11.6k
fix: Limit getSchedule endpoint date range to 1 year #25038
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add date clamping in tRPC schema to limit endTime to max 1 year from today - Add date clamping in API v2 DTOs using Transform decorator - Update API documentation to mention the 1-year limit - Prevents DoS attacks from requesting extremely long date ranges Co-Authored-By: [email protected] <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
…ssues Co-Authored-By: [email protected] <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Add unit tests for tRPC schema date clamping logic - Add E2E tests for slots API date range validation - Tests verify that endTime is clamped to max 1 year from today Co-Authored-By: [email protected] <[email protected]>
E2E results are ready! |
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
- Wait for incrementMonth button to be ready before clicking - Use getByTestId().click() pattern for auto-waiting - Replace fixed 200ms timeout with proper waitFor() on enabled days - Follow same pattern as selectFirstAvailableTimeSlotNextMonth helper Co-Authored-By: [email protected] <[email protected]>
…te range limit The tests were using 2049-09-05 as the mocked 'now' date while trying to reserve slots for 2050-09-05, which is more than 1 year in the future. Updated the mocked date to 2050-09-05 so the slot dates are within the 1-year limit. Co-Authored-By: [email protected] <[email protected]>
…te range limit The tests were using 2049-09-05 as the mocked 'now' date while trying to reserve slots for 2050-09-05, which is more than 1 year in the future. Updated the mocked date to 2050-09-05 so the slot dates are within the 1-year limit. Co-Authored-By: [email protected] <[email protected]>
jest-date-mock only mocks new Date(), not Luxon's DateTime.utc(). The slots repository uses DateTime.utc() to check slot reservation expiry, causing slot reservations from previous tests to persist and block subsequent tests with 422 errors. This fix adds Settings.now mocking in addition to advanceTo() to ensure both new Date() and DateTime.utc() return the mocked date. Co-Authored-By: [email protected] <[email protected]>
- Add deleteByEventTypeId method to SelectedSlotRepositoryFixture - Add afterEach cleanup to user-event-type-slots, team-event-type-slots, and org-team-event-type-slots tests - This ensures slot reservations are cleaned up even if a test fails mid-execution - Prevents test interference where stale slot reservations block subsequent tests with 422 errors Co-Authored-By: [email protected] <[email protected]>
The tests intentionally share state (reservedSlot variable) across tests, so the afterEach cleanup was deleting slot reservations before subsequent tests could verify them. The Luxon Settings.now mocking is still in place to fix the date-related issues. Co-Authored-By: [email protected] <[email protected]>
…nstraint violations Co-Authored-By: [email protected] <[email protected]>
The slot reservation tests were failing with 422 errors because they were trying to reserve slots in the past. The mocked 'now' time was set to 12:00 while the slotStartTime was 10:00, meaning the tests were attempting to reserve slots 2 hours in the past. Fixed by changing the mocked 'now' time to 08:00 so that the slot start time (10:00) is 2 hours in the future, allowing the reservation to succeed. Co-Authored-By: [email protected] <[email protected]>
…E tests The afterEach hook already handles date mocking cleanup, so individual tests don't need to call clear() at the end. This ensures consistent date mocking state across all tests. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Changed all test dates from 2050-09-XX to 2026-09-XX so they are within the 1-year date range limit. Removed the global date mocking workarounds that were added to make 2050 dates work with the 1-year limit. Individual test date mocking for slot reservation tests is preserved since those tests need to mock 'now' to be before the slot start time. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
What does this PR do?
Limits the date range that can be queried in the
getScheduleendpoint to a maximum of 1 year from today. Previously, clients could request availability for extremely long date ranges (e.g., 100 years), causing expensive database queries, CPU-intensive slot calculations, and excessive external calendar API calls.Changes:
packages/trpc/server/routers/viewer/slots/types.ts) to automatically limitendTimeto max 1 year from todaypackages/platform/types/slots/slots-2024-09-04/inputs/get-slots.input.ts) using plain Date (avoiding dayjs import to prevent build issues)Behavior:
endTimemore than 1 year in the future, it will be silently clamped to exactly 1 year from today (UTC)Updates since last revision
Latest fix (date mocking cleanup):
clear()calls from individual tests inuser-event-type-slots.controller.e2e-spec.tsafterEachhook already handles date mocking cleanup, so individual tests don't need to callclear()at the endPrevious fixes:
2050-09-05T12:00:00.000Zto2050-09-05T08:00:00.000Zso that slot start time (10:00) is 2 hours in the futurerandomString()to prevent unique constraint violationsdeleteByEventTypeIdmethod toSelectedSlotRepositoryFixturefor bulk cleanupafterEachcleanup hooks in test files to delete slot reservations after each testSettings.nowmocking in addition tojest-date-mock'sadvanceTo()- required because the slots repository usesDateTime.utc()from LuxonMandatory Tasks (DO NOT REMOVE)
How should this be tested?
Automated Tests:
yarn test packages/trpc/server/routers/viewer/slots/types.test.tsyarn e2e apps/web/playwright/slots-date-limit.e2e.ts(requiresready-for-e2elabel)apps/api/v2/src/modules/slots/slots-2024-09-04/controllers/e2e/Manual Testing:
Test tRPC endpoint (internal):
Test API v2 endpoint (public):
Checklist
I haven't read the contributing guideMy code doesn't follow the style guidelines of this projectI haven't commented my code, particularly in hard-to-understand areasI haven't checked if my changes generate no new warningsImportant Review Points
Date mocking cleanup pattern: The latest fix removes
clear()calls from individual tests, relying on theafterEachhook for cleanup. Verify this doesn't cause state leakage between tests.Slot reservation date mocking: The mocked "now" (08:00) is BEFORE
slotStartTime(10:00). Verify this pattern is consistent across all reservation tests.Unique booking UIDs: Tests use
randomString()for booking UIDs. Verify this doesn't break any assertions that depend on specific UID values.Luxon Settings.now mocking: The fix mocks both
jest-date-mock(fornew Date()) and Luxon'sSettings.now(forDateTime.utc()). Verify this properly affects the slot reservation expiry check inslots.repository.ts.afterEach cleanup pattern: The
deleteByEventTypeIdcleanup runs after each test. Verify this doesn't cause issues if the eventTypeId is undefined (e.g., if beforeAll fails).Silent clamping vs error response: This PR implements silent clamping (modifies the request without error). Is this the desired behavior, or should we return a 400 error when the date range exceeds 1 year?
Devin Session: https://app.devin.ai/sessions/b777d0818d2c4aaeb1bc8ab02b218c52
Requested by: [email protected] (@keithwillcode)