Implement Accounts creation rate limit #1800

hectorgomezv · 2024-08-05T11:00:44Z

Summary

This PR introduces a rate limitation to creating User Accounts using the related AccountsController endpoint.

Changes

Adds a rate limitation controlled by a cache key before proceeding with account creation.

hectorgomezv · 2024-08-05T11:02:38Z

src/config/entities/configuration.ts

+    creationRateLimitPeriodSeconds: parseInt(
+      process.env.ACCOUNT_CREATION_RATE_LIMIT_PERIOD_SECONDS ?? `${60}`,
+    ),
+    creationRateLimitCalls: parseInt(
+      process.env.ACCOUNT_CREATION_RATE_LIMIT_CALLS_BY_PERIOD ?? `${1}`,
+    ),
+  },


These defaults allow each IP to create an account per minute. As the account is cross-chain, I thought that one account should be sufficient for most users.

hectorgomezv · 2024-08-05T11:03:51Z

src/datasources/accounts/accounts.datasource.ts

+    address: `0x${string}`;
+    clientIp: string;


As this function's arity increased to 2, I've wrapped the arguments on a type, which caused some additional changes in the PR.

hectorgomezv · 2024-08-05T11:05:43Z

src/datasources/accounts/accounts.datasource.ts

+      this.loggingService.warn(
+        `Invalid client IP while creating account: ${clientIp}`,
+      );


If the IP is not defined or it's not well formatted, the account creation is allowed. This is mostly done to avoid blockers, but I think we need to keep an eye on logs if we start seeing too many warnings here.

In my understanding, it cannot happen.

you get this IP address from web server, who gets it from http(s) which is over tpc, where server always has to know the address on the other side.

even with http3, the udp packets would still contain originators ip. To become empty or malformed, it would need to be specially crafted.

So for this case I would propose to throw an error as well

Makes sense to me, I've added a ticket to change it: #1838

Thanks!

hectorgomezv · 2024-08-05T11:06:48Z

src/datasources/accounts/accounts.datasource.ts

+        this.loggingService.warn(
+          `Limit of ${this.accountCreationRateLimitCalls} reached for IP ${clientIp}`,
+        );


As before, I added a log line here also, as getting too many rate limit conditions shouldn't be normal.

coveralls · 2024-08-05T11:07:15Z

Pull Request Test Coverage Report for Build 10270314343

Details

5 of 17 (29.41%) changed or added relevant lines in 3 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.03%) to 48.03%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/domain/accounts/accounts.repository.ts	0	1	0.0%
src/datasources/accounts/accounts.datasource.ts	3	14	21.43%

Files with Coverage Reduction	New Missed Lines	%
src/domain/accounts/accounts.repository.ts	1	10.53%
src/datasources/accounts/accounts.datasource.ts	1	17.07%

Totals
Change from base Build 10247478509:	-0.03%
Covered Lines:	4427
Relevant Lines:	7440

💛 - Coveralls

iamacook

I'm not sure if this should be happening in the datasource. Rate limiting is more guard behaviour to me. What do you think?

iamacook · 2024-08-06T10:17:24Z

src/datasources/accounts/accounts.datasource.spec.ts

@@ -88,20 +91,126 @@ describe('AccountsDatasource tests', () => {
      );
    });

-    it('throws when an account with the same address already exists', async () => {
+    it('creates an account successfully if the clientIp is not a valid IP', async () => {


Suggested change

it('creates an account successfully if the clientIp is not a valid IP', async () => {

it('creates an account successfully if the clientIp is a valid IP', async () => {

Hmm, this is intended, the current behavior is to allow nonvalid IPs to create an account, and log this fact as a warning. The rationale is to cover the case where for some reason our infra sees a modified IP somehow. The idea is to not block users whose IP is obfuscated in some way. I'm open to restricting the account creation to valid IPs only btw. I guess we need to evaluate this while running the service. Wdyt?

It seems trivial to rate limit based on IP but allow users to bypass it by obfuscating their IP. Do we know if there's a high chance of modified IPs? I am impartial regarding this nonetheless so we can leave it as if we want.

I guess we will see how this goes through time, I don't think users can obfuscate their IPs easily. I think we can start allowing account creation but keep an eye on logging to see if this (a malformed IP reaching the service) happens often. So I'd leave it as it is for now, and maybe reconsider it based on the logs.

src/datasources/accounts/accounts.datasource.spec.ts

src/datasources/accounts/accounts.datasource.ts

hectorgomezv · 2024-08-06T12:57:17Z

I'm not sure if this should be happening in the datasource. Rate limiting is more guard behaviour to me. What do you think?

I don't have a super firm opinion, but I think it makes sense to have the limiter on the datasource for this use case as we have to limit the actual account creations. I guess if we add the rate limiter to the controller layer it wouldn't consider failed attempts for example. For a GET endpoint, I'd be more in favor of limiting the endpoint directly.

Also, even if we put this on a Guard, it would need to have access to the Redis instance, as the rate limiter count needs to be shared among the CGW pods (maybe this could help if we want to do it https://github.com/kkoomen/nestjs-throttler-storage-redis).

iamacook · 2024-08-06T14:24:05Z

I don't have a super firm opinion, but I think it makes sense to have the limiter on the datasource for this use case as we have to limit the actual account creations. I guess if we add the rate limiter to the controller layer it wouldn't consider failed attempts for example. For a GET endpoint, I'd be more in favor of limiting the endpoint directly.

Also, even if we put this on a Guard, it would need to have access to the Redis instance, as the rate limiter count needs to be shared among the CGW pods (maybe this could help if we want to do it https://github.com/kkoomen/nestjs-throttler-storage-redis).

Considering we want to limit creations and this can happen via various methods - I am happy with keeping it as it is. Although a nit, I think we should add a comment explaining the above.

hectorgomezv · 2024-08-06T16:26:05Z

Considering we want to limit creations and this can happen via various methods - I am happy with keeping it as it is. Although a nit, I think we should add a comment explaining the above.

Great, thanks! I've included a note in 031aaca, please let me know if you think it's not clearer enough, or if you would like to rephrase it.

parius · 2024-08-27T16:51:31Z

src/datasources/accounts/accounts.datasource.ts

+        this.loggingService.warn(
+          `Limit of ${this.accountCreationRateLimitCalls} reached for IP ${clientIp}`,
+        );
+        throw new LimitReachedError();


Which http code would a client get?
this Error is not extending HttpException. Is it necessary to implement an ExceptionFilter for this?

With this implementation, the client would get a generic 500 status code error.

I've created a PR to extend HttpException and set the correct status codes for both accounts and Counterfactual Safes rate limit hit errors on creation: #1865

Thank you!

Adds a rate limitation controlled by a cache key before proceeding with account creation

Implement Accounts creation rate limit

4939cfd

hectorgomezv self-assigned this Aug 5, 2024

hectorgomezv requested a review from a team as a code owner August 5, 2024 11:00

hectorgomezv commented Aug 5, 2024

View reviewed changes

iamacook reviewed Aug 6, 2024

View reviewed changes

iamacook mentioned this pull request Aug 6, 2024

Implement Counterfactual Safes creation rate limit #1801

Merged

PR comments

b2e5161

hectorgomezv requested a review from iamacook August 6, 2024 13:16

PR comment

031aaca

iamacook approved these changes Aug 7, 2024

View reviewed changes

hectorgomezv merged commit 234a559 into main Aug 7, 2024
16 checks passed

hectorgomezv deleted the add-accounts-creation-ip-rate-limit branch August 7, 2024 08:26

hectorgomezv mentioned this pull request Aug 21, 2024

Throw error for malformed IPs on user profile creation #1838

Open

parius reviewed Aug 27, 2024

View reviewed changes

DenSmolonski pushed a commit to Zilliqa/safe-client-gateway that referenced this pull request Oct 24, 2024

Implement Accounts creation rate limit (safe-global#1800)

072599a

Adds a rate limitation controlled by a cache key before proceeding with account creation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Accounts creation rate limit #1800

Implement Accounts creation rate limit #1800

hectorgomezv commented Aug 5, 2024

hectorgomezv Aug 5, 2024

hectorgomezv Aug 5, 2024

hectorgomezv Aug 5, 2024

parius Aug 20, 2024

hectorgomezv Aug 21, 2024

hectorgomezv Aug 5, 2024

coveralls commented Aug 5, 2024 •

edited

Loading

iamacook left a comment

iamacook Aug 6, 2024

hectorgomezv Aug 6, 2024 •

edited

Loading

iamacook Aug 6, 2024

hectorgomezv Aug 6, 2024 •

edited

Loading

hectorgomezv commented Aug 6, 2024 •

edited

Loading

iamacook commented Aug 6, 2024

hectorgomezv commented Aug 6, 2024

parius Aug 27, 2024

hectorgomezv Aug 27, 2024

	it('creates an account successfully if the clientIp is not a valid IP', async () => {
	it('creates an account successfully if the clientIp is a valid IP', async () => {

Implement Accounts creation rate limit #1800

Implement Accounts creation rate limit #1800

Conversation

hectorgomezv commented Aug 5, 2024

Summary

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 5, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10270314343

Details

💛 - Coveralls

iamacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorgomezv Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorgomezv Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

hectorgomezv commented Aug 6, 2024 • edited Loading

iamacook commented Aug 6, 2024

hectorgomezv commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 5, 2024 •

edited

Loading

hectorgomezv Aug 6, 2024 •

edited

Loading

hectorgomezv Aug 6, 2024 •

edited

Loading

hectorgomezv commented Aug 6, 2024 •

edited

Loading