[IND-526] Block subscribing to subaccounts from restricted regions for read-only mode. #896
Conversation
…r read-only mode.
|
Warning Rate Limit Exceeded@vincentwschau has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 12 minutes and 57 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe changes reflect an update to a compliance-related feature within an indexer's codebase. Constants regarding geoblocking messages have been explicitly typed, and a new constant for country header keys has been introduced. Testing has been extended to include mock compliance checks and country-related variables. The main service logic now incorporates country checks, with a new Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (7)
- indexer/packages/compliance/src/constants.ts (1 hunks)
- indexer/services/socks/tests/lib/subscriptions.test.ts (13 hunks)
- indexer/services/socks/tests/websocket/index.test.ts (4 hunks)
- indexer/services/socks/src/lib/subscription.ts (5 hunks)
- indexer/services/socks/src/types.ts (1 hunks)
- indexer/services/socks/src/websocket/index.ts (2 hunks)
- indexer/services/socks/src/websocket/restrict-countries.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/packages/compliance/src/constants.ts
Additional comments: 13
indexer/services/socks/__tests__/lib/subscriptions.test.ts (5)
13-18: The addition of a mock for
@dydxprotocol-indexer/complianceand the introduction ofrestrictedCountryandnonRestrictedCountryvariables are consistent with the PR's objectives to handle regional restrictions in websocket subscriptions. The mock implementation ofisRestrictedCountryis set up correctly to simulate a restricted region.61-62: The
restrictedCountryandnonRestrictedCountryvariables have been added to the test file to facilitate testing the new functionality. Ensure that these variables are used consistently throughout the test cases to simulate the correct behavior based on the country's restriction status.86-88: The mock implementation of
isRestrictedCountrycorrectly returnstruefor therestrictedCountry, which aligns with the test's intention to simulate a restricted region. This is a critical part of testing the new functionality.108-109: The
subscribemethod has been updated to accept additional parameters for the country code and a boolean flag. The test cases have been updated accordingly to include these parameters. This change is crucial for testing the new functionality that blocks subscriptions from restricted countries.Also applies to: 152-153, 181-182, 196-197, 219-220, 255-257, 307-308, 344-345, 364-365, 388-389
- 272-284: The test case specifically designed to test the blocking of subscriptions from restricted countries is a good addition and covers the new functionality introduced in the PR. This test case is essential for verifying that the regional restrictions are enforced as intended.
indexer/services/socks/__tests__/websocket/index.test.ts (1)
- 143-151: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [15-263]
The changes in the provided hunks align with the PR objectives and do not introduce any issues. The addition of
COUNTRY_HEADER_KEYand the handling ofcountryCodein tests are consistent with the new functionality to enforce regional restrictions on websocket subscriptions.indexer/services/socks/src/lib/subscription.ts (4)
78-81: The addition of the
countryparameter to thesubscribemethod is consistent with the PR's objectives to enforce regional restrictions.582-588: The update to
getInitialResponsesForChannelsto accept acountryparameter and the subsequent call togetInitialResponseForSubaccountSubscriptionwith this parameter is in line with the PR's intent to enforce regional restrictions.486-499: The implementation of country-based restrictions in
getInitialResponseForSubaccountSubscriptionby throwing aBlockedErrorwhen thecountryis restricted aligns with the PR's compliance requirements. However, ensure that the comment on line 494-496 is addressed if it's part of the technical specification.582-591: The conditional logic in
getInitialResponsesForChannelsto handleChannel.V4_ACCOUNTSdifferently by invokinggetInitialResponseForSubaccountSubscriptionis a necessary change to support the new country-based restrictions.indexer/services/socks/src/types.ts (1)
- 119-122: The addition of the
countryCodeproperty to theConnectioninterface is consistent with the PR's objective to handle regional restrictions for websocket connections. Ensure that all parts of the codebase that instantiate or manipulateConnectionobjects are updated to handle this new property appropriately.Verification successful
The previous scripts contained a few errors. I will correct the
rgcommand to properly search for TypeScript files and fix the regex pattern. Additionally, I will adjust theast-greppattern to search for object literals that match theConnectioninterface, which should include the newcountryCodeproperty.
The
rgcommand has provided us with files that reference theConnectioninterface. To ensure that thecountryCodeproperty is being handled correctly, I will search within these files for object literals that may representConnectionobjects. Additionally, I will correct theast-grepcommand to search for object literals that match theConnectioninterface.
The
ast-grepcommand confirmed the structure of theConnectioninterface, but we still need to verify how instances of this interface are being handled in the codebase. I will correct thergpattern to search for object literals that may representConnectionobjects.
The current output indicates where the
Connectioninterface is referenced, but it does not provide details on how thecountryCodeproperty is being handled. To verify the correct handling of thecountryCodeproperty, I will search for its usage withinConnectionobjects.
The verification process has provided evidence that the
countryCodeproperty is being set when new connections are established and is being accessed when handling subscription messages, which aligns with the PR's objectives. Additionally, the property is considered in the unit tests, indicating that the new functionality is being tested as intended.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for instances where Connection objects are created or manipulated. rg --type typescript --files-with-matches 'Connection' | xargs rg 'new Connection|{.*Connection.*}' # Verify that the countryCode property is being handled correctly where Connection objects are used. ast-grep --lang typescript --pattern $'new Connection({$$$})'Length of output: 319
Script:
#!/bin/bash # Correct the file type for rg and fix the regex pattern to search for Connection objects creation or manipulation rg --type ts --files-with-matches 'Connection' | xargs rg 'new Connection|\{.*Connection.*\}' # Adjust the ast-grep pattern to search for object literals that match the Connection interface ast-grep --lang typescript --pattern $'interface Connection { $$$ }' --exec 'rg "$_"'Length of output: 573
Script:
#!/bin/bash # Search within the identified files for object literals that may represent Connection objects rg --type ts --glob 'index.ts' --glob 'invalid-message.ts' '{.*Connection.*}' # Correct the ast-grep command to search for object literals that match the Connection interface ast-grep --lang typescript --pattern $'interface Connection { $$$ }'Length of output: 688
Script:
#!/bin/bash # Correct the regex pattern to search for object literals that may represent Connection objects rg --type ts --glob 'index.ts' --glob 'invalid-message.ts' '\{.*Connection.*\}'Length of output: 274
Script:
#!/bin/bash # Search for the usage of the countryCode property within Connection objects rg --type ts 'countryCode' --context 5Length of output: 5327
indexer/services/socks/src/websocket/index.ts (2)
109-115: The implementation of the country restriction logic in the
onConnectionmethod correctly terminates connections from restricted countries before they are added to theconnectionsobject. This preemptive check ensures that no further processing is done for connections that should not be allowed, which is a good security practice.288-294: The use of
countryCodein thesubscribemethod call is consistent with the PR's objective to enforce regional restrictions on subscriptions. The surrounding try-catch block is a good practice to handle any potential errors that may occur during the subscription process.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/packages/compliance/src/constants.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/packages/compliance/src/constants.ts
Changelist
Update
socksto return a blocked error message when subscribing to subaccounts channel from a restricted region. This is done by storing additional metadata per websocket connection indicating which country the connection was made from.In the case where no country exists, this blocking logic fails-open (allows subscribing to subaccounts).
Test Plan
Unit tests.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.