fix: eliminate race condition in API key expiration validation#1456
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
…in API key expiration Co-authored-by: riderx <4084527+riderx@users.noreply.github.com>
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27beec8688
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (expirationDate < new Date()) { | ||
| throw simpleError('invalid_expiration_date', 'Expiration date must be in the future') |
There was a problem hiding this comment.
Allowing expires_at equal to the current time now passes validation, but subkey lookups still require expires_at > now() in SQL (see checkKeyById in supabase.ts and checkKeyByIdPg in hono_middleware.ts). This means a key created with an expiration timestamp equal to “now” is immediately treated as expired when used via x-limited-key-id, producing a 401 despite successful creation. This only occurs when clients send an expiration equal to the current moment, but it is a regression introduced by this change; either keep the validation strict (<=) or align the lookup logic to accept equality.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition in API key expiration validation that caused intermittent test failures. The issue occurred when validation happened at the exact millisecond a date was created.
Changes:
- Changed comparison operator from
<=to<invalidateExpirationDatefunction to allow dates equal to the current moment to pass validation while still rejecting past dates
* feat: add batch stats endpoint support
Enable stats endpoint to accept both single events and arrays of events, allowing clients to send multiple events in a single request. Single events maintain backward compatibility with simple { status: 'ok' } responses, while batch requests return detailed results per event including status and error details.
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
* fix: address PR review comments
- Validate all events in batch have same app_id to prevent rate limit bypass
- Update batch tests to use it.concurrent for parallel execution per AGENTS.md
- Add test for mixed app_id rejection
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
* chore: remove auto-generated deno.lock
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
* fix: address CodeRabbit PR review comments
- Validate first event's app_id early before batch/rate-limit checks
- Fix single-event error handling to propagate proper 400 status codes
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
* chore: remove auto-generated deno.lock
* fix: eliminate race condition in API key expiration validation (#1456)
* fix: address PR review comments for batch stats endpoint
- Guard device_id normalization to only call toLowerCase on strings
- Handle isOnprem flag properly and return 429 for single requests,
error status in batch results for on-premise apps
- Add regex validation for all batch event app_ids, not just the first
- Add test coverage for onprem apps in batch requests
- Revert unrelated supabase.ts expiration date validation change
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* fix: preserve debugging fields in error responses
Add moreInfo field to PostResult and BatchStatsResult interfaces
to pass debugging information (app_id, version_name) through to
error responses, maintaining backward compatibility.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* fix: guard against non-object items in batch validation
- Add object type checks before accessing device_id in parseBodyRaw
- Use optional chaining for app_id access to handle null/primitive items
- Allows per-item validation errors instead of throwing on malformed input
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Summary
The
validateExpirationDatefunction used<=to compare dates, causing intermittent test failures when validation occurred at the exact millisecond a date was created. Changed to<to eliminate the race condition.Before:
After:
This allows dates equal to the current moment to pass validation while still rejecting past dates, eliminating timing-dependent failures.
Test plan
Tests should pass consistently in CI. The change affects
tests/apikeys-expiration.test.tswhich was failing intermittently due to millisecond-level timing between date creation and validation.Screenshots
N/A - backend change only
Checklist
bun run lint:backend && bun run lint.accordingly.
my tests
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
npm.jsr.io/home/REDACTED/.bun/bin/bun bun install(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Note
Fix date comparison to remove race condition
validateExpirationDate(supabase/functions/_backend/utils/supabase.ts), change<=to<when comparingexpiresAttonew Date()Written by Cursor Bugbot for commit 27beec8. This will update automatically on new commits. Configure here.