fix(api-keys): prevent same-day expiry rejection by sending end-of-day UTC#4447
Open
Shashwat-Darshan wants to merge 5 commits intoOWASP:mainfrom
Open
fix(api-keys): prevent same-day expiry rejection by sending end-of-day UTC#4447Shashwat-Darshan wants to merge 5 commits intoOWASP:mainfrom
Shashwat-Darshan wants to merge 5 commits intoOWASP:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFrontend now computes selected expiry dates as the end-of-day UTC timestamp ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Shashwat-Darshan
force-pushed
the
feature/api-key-expiry-eod
branch
from
6e02d07 to
7a45dcf
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
7 issues found across 7 files
Confidence score: 2/5
- High-confidence test-environment regressions are likely:
backend/settings/test.pydrops core apps fromINSTALLED_APPSwhile being used for full pytest runs, which can break app/model loading and invalidate broad test coverage. backend/settings/test.pyalso diverges materially from production (SQLitein-memory andAUTH_USER_MODEL = auth.User), creating concrete risk that tests pass under non-production behavior and miss real user/model issues.- In
backend/tests/apps/owasp/models/test_event.py, tests target a local mockEventand a mismatched date/datetime contract, so regressions inapps.owasp.models.event.Eventmay go undetected; additionaldatetime.now()usage adds CI flakiness around date boundaries. - Pay close attention to
backend/settings/test.py,backend/tests/apps/owasp/models/test_event.py, andfrontend/src/app/settings/api-keys/page.tsx- test fidelity appears reduced and timezone handling may still reject valid same-day selections for users behind UTC.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/app/settings/api-keys/page.tsx">
<violation number="1" location="frontend/src/app/settings/api-keys/page.tsx:24">
P2: Expiry normalization uses end-of-day UTC for the selected date label, which can still reject same-day selections for users behind UTC.</violation>
</file>
<file name="backend/settings/test.py">
<violation number="1" location="backend/settings/test.py:26">
P1: Test settings switch the global database engine to SQLite in-memory despite included apps using PostgreSQL-specific model features, reducing production parity and risking misleading test results.</violation>
<violation number="2" location="backend/settings/test.py:48">
P1: `INSTALLED_APPS` now excludes core project apps (e.g., `apps.api`) while `settings.test` is used for the full pytest run, which will break app/model loading for many tests.</violation>
<violation number="3" location="backend/settings/test.py:60">
P1: Test settings switch `AUTH_USER_MODEL` to `auth.User`, causing tests to run against a different user model than production (`nest.User`) and reducing fidelity for user-related behavior.</violation>
</file>
<file name="backend/tests/apps/owasp/models/test_event.py">
<violation number="1" location="backend/tests/apps/owasp/models/test_event.py:7">
P1: This test file validates a local mock `Event` implementation instead of the real `apps.owasp.models.event.Event`, so it can’t catch regressions in production behavior.</violation>
<violation number="2" location="backend/tests/apps/owasp/models/test_event.py:15">
P2: Tests use multiple independent `datetime.now()` calls, making outcomes date-boundary dependent and potentially flaky in CI.</violation>
<violation number="3" location="backend/tests/apps/owasp/models/test_event.py:16">
P2: New tests use a mock `date`/`datetime` contract (`e.date.date()`) that does not match the real `Event.start_date` DateField behavior, potentially masking real regressions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/apps/owasp/models/test_event.py`:
- Around line 7-49: The test defines a local shadow Event class and tests
Event.upcoming_events instead of the real production model; remove the local
class definition (the Event class at the top of this file) and update
TestEventUpcomingEvents to import the real Event model and call
Event.upcoming_events from the production module (or delete this shadow test
file entirely and keep the assertions in the existing backend tests such as
event_test.py); ensure TestEventUpcomingEvents still constructs datetime-based
fixtures the same way and uses the imported Event.upcoming_events so assertions
exercise the real backend/apps/owasp/models/event.py implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e22bc753-d386-474b-883c-a4e5a5f4be5d
📒 Files selected for processing (7)
backend/apps/owasp/models/event.pybackend/settings/test.pybackend/tests/apps/api/internal/mutations/api_key_test.pybackend/tests/apps/owasp/models/event_test.pybackend/tests/apps/owasp/models/test_event.pyfrontend/__tests__/unit/pages/ApiKeysPage.test.tsxfrontend/src/app/settings/api-keys/page.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/apps/api/internal/mutations/api_key_test.py (1)
60-71: Patchtimezone.nowdirectly instead of patching the fulltimezonemodule.The mutation code uses only
timezone.now()(line 52 of api_key.py), nottimezone.utc. Narrowing the patch scope improves test clarity and avoids over-mocking. Line 70 (mock_timezone.utc = timezone.utc) is unnecessary and can be removed.Proposed diff
`@patch`("apps.api.internal.mutations.api_key.ApiKey.create") -@patch("apps.api.internal.mutations.api_key.timezone") +@patch("apps.api.internal.mutations.api_key.timezone.now") def test_create_api_key_end_of_day_is_valid( - self, mock_timezone, mock_api_key_create, api_key_mutations + self, mock_now, mock_api_key_create, api_key_mutations ): """Ensure an end-of-day expiry on the current date is accepted.""" info = mock_info() user = info.context.request.user fixed_now = datetime(2026, 1, 1, 12, 0, tzinfo=timezone.utc) - mock_timezone.now.return_value = fixed_now - mock_timezone.utc = timezone.utc + mock_now.return_value = fixed_now🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/api/internal/mutations/api_key_test.py` around lines 60 - 71, Patch only timezone.now instead of the entire timezone module in test_create_api_key_end_of_day_is_valid: replace the `@patch`("apps.api.internal.mutations.api_key.timezone") with a patch targeting timezone.now (e.g., `@patch`("apps.api.internal.mutations.api_key.timezone.now")) so the test sets mock_timezone.now.return_value = fixed_now and remove the unnecessary mock_timezone.utc = timezone.utc line; keep references to the test function name test_create_api_key_end_of_day_is_valid and the mocked behavior around timezone.now to ensure the mutation logic in api_key.py uses the fixed time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/tests/apps/api/internal/mutations/api_key_test.py`:
- Around line 60-71: Patch only timezone.now instead of the entire timezone
module in test_create_api_key_end_of_day_is_valid: replace the
`@patch`("apps.api.internal.mutations.api_key.timezone") with a patch targeting
timezone.now (e.g., `@patch`("apps.api.internal.mutations.api_key.timezone.now"))
so the test sets mock_timezone.now.return_value = fixed_now and remove the
unnecessary mock_timezone.utc = timezone.utc line; keep references to the test
function name test_create_api_key_end_of_day_is_valid and the mocked behavior
around timezone.now to ensure the mutation logic in api_key.py uses the fixed
time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83ef7af6-a4c4-459a-bb6c-a7683f061bed
📒 Files selected for processing (2)
backend/tests/apps/api/internal/mutations/api_key_test.pyfrontend/__tests__/unit/pages/ApiKeysPage.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/pages/ApiKeysPage.test.tsx
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Fixes API key expiry handling so selecting “today” doesn’t get rejected due to date-only → UTC timestamp conversion artifacts, aligning the frontend payload with backend validation (expires_at > now).
Changes:
- Frontend: convert
yyyy-MM-dddate input into an end-of-day ISO timestamp for the create mutation payload. - Frontend tests: update expected
expiresAtpayloads and add a regression test for “today” expiry. - Backend tests: add a fixed-
nowvalidation test confirming same-day end-of-day expiry is accepted.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/app/settings/api-keys/page.tsx | Adds helper to convert selected expiry date to an end-of-day ISO timestamp before sending mutation. |
| frontend/tests/unit/pages/ApiKeysPage.test.tsx | Updates payload expectations to end-of-day and adds a “today” regression test. |
| backend/tests/apps/api/internal/mutations/api_key_test.py | Adds a test ensuring same-day end-of-day expiry passes backend validation with mocked timezone.now(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/apps/api/internal/mutations/api_key_test.py">
<violation number="1" location="backend/tests/apps/api/internal/mutations/api_key_test.py:63">
P1: The added `):` line and de-indented `mock_now.return_value` break the function definition/indentation, causing a syntax error in this test file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/__tests__/unit/pages/ApiKeysPage.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/ApiKeysPage.test.tsx:118">
P2: The test helper computes local end-of-day, not end-of-day UTC, so these assertions no longer validate the intended UTC expiry behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/app/settings/api-keys/page.tsx (2)
33-34: Brief doc comment would clarify intent for future maintainers.The function name
toEndOfDayUtcIsois reasonable, but a short comment explaining why end-of-day is used (to prevent same-day rejection due to timezone conversion) would aid maintainability. This is optional.📝 Suggested inline comment
+/** + * Converts a YYYY-MM-DD date string to an ISO timestamp at end-of-day (23:59:59.999) + * in the user's local timezone, then serialized to UTC. This ensures selecting "today" + * is not rejected by backend validation that requires expires_at > now. + */ const toEndOfDayUtcIso = (date: string): string => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/settings/api-keys/page.tsx` around lines 33 - 34, Add a short doc comment above the toEndOfDayUtcIso function explaining intent: state that the function returns the ISO timestamp for the local end of day (23:59:59.999) and that this is done to avoid same-day rejection when converting local dates to UTC (i.e., pushing the cutoff to end-of-day so timezone shifts don’t move the date to the previous/next day). Keep it concise—one or two sentences—mentioning the timezone conversion rationale for future maintainers.
23-35: Fallback preserves the original problematic behavior.If parsing fails (lines 29-31), the fallback
new Date(date).toISOString()produces a UTC-midnight timestamp—the exact issue this PR fixes. While the HTML<input type="date">always returnsYYYY-MM-DD, making this path unlikely, consider applying end-of-day logic in the fallback as well for defensive consistency.♻️ Suggested fix for defensive fallback
if (!Number.isFinite(year) || !Number.isFinite(month) || !Number.isFinite(day)) { - return new Date(date).toISOString() + const fallbackDate = new Date(date) + fallbackDate.setHours(23, 59, 59, 999) + return fallbackDate.toISOString() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/settings/api-keys/page.tsx` around lines 23 - 35, The fallback in toEndOfDayUtcIso still returns new Date(date).toISOString(), preserving the original midnight-UTC bug; change the fallback to compute and return the end-of-day UTC timestamp instead: when parsing fails, create a Date from the input (const d = new Date(date)), check it's valid, extract UTC components (d.getUTCFullYear(), d.getUTCMonth(), d.getUTCDate()) and return new Date(Date.UTC(year, month, day, 23, 59, 59, 999)). If the created Date is invalid (isNaN), then fall back to d.toISOString() to avoid throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/app/settings/api-keys/page.tsx`:
- Around line 33-34: Add a short doc comment above the toEndOfDayUtcIso function
explaining intent: state that the function returns the ISO timestamp for the
local end of day (23:59:59.999) and that this is done to avoid same-day
rejection when converting local dates to UTC (i.e., pushing the cutoff to
end-of-day so timezone shifts don’t move the date to the previous/next day).
Keep it concise—one or two sentences—mentioning the timezone conversion
rationale for future maintainers.
- Around line 23-35: The fallback in toEndOfDayUtcIso still returns new
Date(date).toISOString(), preserving the original midnight-UTC bug; change the
fallback to compute and return the end-of-day UTC timestamp instead: when
parsing fails, create a Date from the input (const d = new Date(date)), check
it's valid, extract UTC components (d.getUTCFullYear(), d.getUTCMonth(),
d.getUTCDate()) and return new Date(Date.UTC(year, month, day, 23, 59, 59,
999)). If the created Date is invalid (isNaN), then fall back to d.toISOString()
to avoid throwing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e42ca86-dbca-48c5-96b8-be77c64b3210
📒 Files selected for processing (3)
backend/tests/apps/api/internal/mutations/api_key_test.pyfrontend/__tests__/unit/pages/ApiKeysPage.test.tsxfrontend/src/app/settings/api-keys/page.tsx
✅ Files skipped from review due to trivial changes (1)
- backend/tests/apps/api/internal/mutations/api_key_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/pages/ApiKeysPage.test.tsx
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
Resolves #4426
Fix API key expiry date handling so selecting today does not get rejected due to midnight UTC conversion.
Root cause
Date input value was converted with Date(...).toISOString(), producing midnight UTC. Backend correctly rejects expires_at <= now, which made same-day selections intermittently fail by timezone/time-of-day.
What changed
Why this approach
Minimal, deterministic fix that aligns UI date-only intent with existing backend rule without changing backend business logic.
Verification
Checklist