Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rate limiting in the server using built in Nest.js capability #3566

Merged
merged 10 commits into from
Feb 7, 2024

Conversation

jss475
Copy link
Contributor

@jss475 jss475 commented Jan 20, 2024

#3545

  • Add rate limiting for graphql and http requests in the backend using built in Nest.js capability

Copy link

github-actions bot commented Jan 20, 2024

Warnings
⚠️ Changes were made to the environment variables, but not to the documentation - Please review your changes and check if a change needs to be documented!

Generated by 🚫 dangerJS against 7a6a99d

Copy link
Member

@magrinj magrinj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution !
Some changes and tests are needed before merging it 🙂

packages/twenty-server/src/guards/gql-throttler.guard.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/throttler-config.service.ts Outdated Show resolved Hide resolved
packages/twenty-server/.env.test Outdated Show resolved Hide resolved
packages/twenty-server/src/guards/gql-throttler.guard.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/app.module.ts Outdated Show resolved Hide resolved
packages/twenty-server/.env.example Outdated Show resolved Hide resolved
packages/twenty-server/.env.test Outdated Show resolved Hide resolved
@magrinj
Copy link
Member

magrinj commented Feb 6, 2024

@jss475 Thanks for your work, I've change the code because our graphQL schema is dynamically constructed, so @nestjs/throttler was not handling request on the dynamic side.
Instead I've created a graphql-yoga plugin.
cc @charlesBochet

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with comments. We should also add the environment variables to the documentation

@@ -52,3 +52,5 @@ SIGN_IN_PREFILLED=true
# EMAIL_SMTP_USER=
# EMAIL_SMTP_PASSWORD=
# PASSWORD_RESET_TOKEN_EXPIRES_IN=5m
# LOGGED_IN_LONG_TTL=
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about this naming, reading the code to understand what it is

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API_RATE_LIMITING_TTL
API_RATE_LIMITING_LIMIT

@charlesBochet charlesBochet merged commit 850eab8 into twentyhq:main Feb 7, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants