fix: replace permanent blacklisting with clock skew tolerance window for WebSocket telemetry (#596)#617
Open
ionfwsrijan wants to merge 3 commits into
Conversation
Contributor
|
🎉 Thank you for your contribution! Your pull request has been received and will be reviewed shortly. If you enjoy the project, please consider giving the repository a ⭐. You can also follow my GitHub profile to stay updated on future open-source projects. Thanks for being part of the community! 🚀 |
📝 WalkthroughWalkthroughAdds a configurable ChangesClock Skew Filtering for WebSocket Telemetry
Bid Acceptance Test Setup for Wallet Mocking
Flutter Mobile App Refactoring and Cleanup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~13 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
backend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/api/src/sockets/tracker.js (1)
342-354: ⚡ Quick winAdd explicit regression assertions for skew-drop behavior and sequence-key immutability.
The new gate is critical to
#596behavior. Please add/extend unit coverage to assert:
- out-of-window skew returns early and does not call Redis
set,- in-window skew still reaches normal sequence logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/src/sockets/tracker.js` around lines 342 - 354, Add explicit unit test coverage for the clock skew validation logic (the CLOCK_SKEW_VALIDATION gate section) to ensure regression prevention. Create test cases that assert: when the calculated skewMs exceeds CLOCK_SKEW_TOLERANCE_MS, the function returns early without calling any Redis set operations, and when skewMs is within the tolerance threshold, the function continues execution to the normal sequence key logic. This validates both the reject path (out-of-window skew) and the accept path (in-window skew) work correctly together.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/api/src/sockets/tracker.js`:
- Line 11: The CLOCK_SKEW_TOLERANCE_MS constant uses the || operator which
treats the parsed value of "0" as falsy and would incorrectly fall back to the
default, and it does not validate that the parsed value is non-negative. Replace
the || logic with explicit validation to ensure the parsed integer from
process.env.CLOCK_SKEW_TOLERANCE_MS is finite and greater than or equal to zero,
falling back to the default 300000 only when validation fails or the environment
variable is unset.
---
Nitpick comments:
In `@backend/api/src/sockets/tracker.js`:
- Around line 342-354: Add explicit unit test coverage for the clock skew
validation logic (the CLOCK_SKEW_VALIDATION gate section) to ensure regression
prevention. Create test cases that assert: when the calculated skewMs exceeds
CLOCK_SKEW_TOLERANCE_MS, the function returns early without calling any Redis
set operations, and when skewMs is within the tolerance threshold, the function
continues execution to the normal sequence key logic. This validates both the
reject path (out-of-window skew) and the accept path (in-window skew) work
correctly together.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 61a4e604-48ef-4852-8dc5-282bf05dafbf
📒 Files selected for processing (2)
backend/api/.env.examplebackend/api/src/sockets/tracker.js
…for WebSocket telemetry (KanishJebaMathewM#596)
ionfwsrijan
force-pushed
the
fix/issue-596-websocket-clock-skew
branch
from
e06b5cc to
12506e4
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/api/src/sockets/tracker.js (1)
331-333:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix coordinate presence validation to allow valid
0values.At Line 331,
if (!latitude || !longitude)rejects valid coordinates like(0, 0). Validate null/undefined and numeric bounds instead.Suggested fix
- if (!latitude || !longitude) { + const lat = Number(latitude); + const lng = Number(longitude); + if ( + !Number.isFinite(lat) || + !Number.isFinite(lng) || + lat < -90 || lat > 90 || + lng < -180 || lng > 180 + ) { return ws.send(JSON.stringify({ error: 'Missing mandatory tracking parameters (lat, lng).' })); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/src/sockets/tracker.js` around lines 331 - 333, The coordinate validation in the tracking handler currently rejects valid zero values because the condition `if (!latitude || !longitude)` treats 0 as falsy. Replace this validation logic to explicitly check for null and undefined values instead, allowing 0 as a valid coordinate. Additionally, consider adding numeric bounds validation to ensure the coordinates are within acceptable ranges for latitude and longitude values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/api/src/sockets/tracker.js`:
- Around line 331-333: The coordinate validation in the tracking handler
currently rejects valid zero values because the condition `if (!latitude ||
!longitude)` treats 0 as falsy. Replace this validation logic to explicitly
check for null and undefined values instead, allowing 0 as a valid coordinate.
Additionally, consider adding numeric bounds validation to ensure the
coordinates are within acceptable ranges for latitude and longitude values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e3799ca6-4a42-4fd2-9e4e-0a672b13c760
📒 Files selected for processing (2)
backend/api/.env.examplebackend/api/src/sockets/tracker.js
✅ Files skipped from review due to trivial changes (1)
- backend/api/.env.example
…or tests Upstream wallet validation gate returns 422 when wallets are missing, breaking tests that expect 500 from the subsequent RPC error.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/customer/lib/services/profile_service.dart (1)
53-54: 💤 Low valueReplace
Using
print()is not recommended for production Flutter apps. Consider usingdeveloper.log(already imported inApiClient) or a logging framework for consistency with the rest of the codebase.+import 'dart:developer' as developer; + ... } catch (e) { - print('Backend logout failed: $e'); + developer.log('Backend logout failed: $e', name: 'ProfileService'); } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/customer/lib/services/profile_service.dart` around lines 53 - 54, The catch block that handles backend logout failures is using print() for error logging, which is not recommended for production Flutter applications. Replace the print() statement with developer.log(), which is already imported in ApiClient, or use the logging framework consistent with the rest of the codebase. Update the error handling in the catch block to use the appropriate structured logging method instead of the print() call, ensuring the error details are still captured and logged properly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/customer/lib/services/profile_service.dart`:
- Around line 53-54: The catch block that handles backend logout failures is
using print() for error logging, which is not recommended for production Flutter
applications. Replace the print() statement with developer.log(), which is
already imported in ApiClient, or use the logging framework consistent with the
rest of the codebase. Update the error handling in the catch block to use the
appropriate structured logging method instead of the print() call, ensuring the
error details are still captured and logged properly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 362a98c1-8c89-4c0c-80c7-336f736a4d38
📒 Files selected for processing (4)
apps/customer/lib/services/profile_service.dartapps/driver/lib/screens/profile_screen.dartapps/driver/lib/services/marketplace_repository.dartbackend/api/test/unit/tracker.test.js
💤 Files with no reviewable changes (1)
- apps/driver/lib/screens/profile_screen.dart
Contributor
Author
|
@KanishJebaMathewM Please review this and add labels |
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
1 participant
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.
Problem
The WebSocket tracking handler stored device timestamps in Redis with a 24-hour TTL. If a device sent a timestamp ahead of the server (e.g., clock 10 minutes fast), that future value poisoned the sequence cache — all subsequent legitimate pings were silently dropped for 24 hours. Device clock drift from NTP corrections or mobile OS sleep cycles caused permanent false-positive blacklisting.
Solution
CLOCK_SKEW_TOLERANCE_MSconfig (default ±5 minutes), configurable viaprocess.env.CLOCK_SKEW_TOLERANCE_MS|deviceTime - serverTime| > tolerance, the update is ignored with a warning and the Redis sequence key is never updated.env.exampleFiles changed
backend/api/src/sockets/tracker.js— added clock skew validation before seqKey writebackend/api/.env.example— documentedCLOCK_SKEW_TOLERANCE_MSCloses #596
Summary by CodeRabbit
CLOCK_SKEW_TOLERANCE_MSto example environment settings (default:300000ms).