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

feat(everything that needs rate limit control): rate limit, to contro… #359

Closed
wants to merge 2 commits into from
Closed

Conversation

tiagomol1
Copy link

…l attacks and abusive requests
fix #338

@filipedeschamps, seria assim uma possivel solução para rate limit, controlando as requisições abusivas, etc?
@rodrigoKulb vi na issue que você comentou sobre Captcha também, mas acho que esse controle de rate limit pode fazer sentido em alguns lugares da aplicação.

Obrigado por permitir que a gente contribua nesse projeto que ta ficando delicinha!

@tiagomol1 tiagomol1 added the back Envolve modificações no backend label May 21, 2022
@tiagomol1 tiagomol1 added this to the Milestone 4: TabCoins milestone May 21, 2022
@tiagomol1 tiagomol1 self-assigned this May 21, 2022
@vercel
Copy link

vercel bot commented May 21, 2022

@tiagomol1 is attempting to deploy a commit to the TabNews Team on Vercel.

To accomplish this, @tiagomol1 needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

@tembra
Copy link
Contributor

tembra commented May 21, 2022

@tiagomol1 que implementação delicinha de um middleware utilizando singleton!

Olhei rápido, mas fiz algumas observações no código.

EDIT: Estava pensando que para nós esse tipo de implementação não funciona pois estamos em um ambiente serverless. Isso significa que a cada request uma nova instância completamente crua pode ser inicializada e, portanto, as informações dos IPs seriam perdidas. Dessa forma precisamos realmente ter este controle via banco de dados.

@tiagomol1
Copy link
Author

@tiagomol1 que implementação delicinha de um middleware utilizando singleton!

Olhei rápido, mas fiz algumas observações no código.

EDIT: Estava pensando que para nós esse tipo de implementação não funciona pois estamos em um ambiente serverless. Isso significa que a cada request uma nova instância completamente crua pode ser inicializada e, portanto, as informações dos IPs seriam perdidas. Dessa forma precisamos realmente ter este controle via banco de dados.

Faz sentido, consigo ajustar essa estrutura para controlar via banco de dados. Só temos que ver como ficara a performance das APIs que utilizarem esse middleware.

@tembra
Copy link
Contributor

tembra commented May 23, 2022

Dessa forma precisamos realmente ter este controle via banco de dados.

Talvez exista alguma outra forma mais performática para isso. Foi apenas uma sugestão. Antes de refatorar sugiro aguardarmos uma melhor discussão.

@filipedeschamps
Copy link
Owner

filipedeschamps commented May 23, 2022

Isso significa que a cada request uma nova instância completamente crua pode ser inicializada e, portanto, as informações dos IPs seriam perdidas.

Isso é uma percepção que foi vendida pelos providers de lamba (principalmente AWS), mas que não é integralmente verdade, tive que aprender isso na marra aqui no TabNews por conta do pool do database.js e quase tive um treco de tanto teste que fiz 😂

Mas em resumo, a função lamda é sim pura, mas o closure global não, esse escopo se mantém até a lamba ser encerrada e descartada. Mas ele continua enquanto ela estiver pausada/congelada, e é por isso que você pode declarar coisas fora do closure da lambda e conseguir acessar entre uma chamada e a outra.

Então @tiagomol1 na sua implementação, se você puxar os arrays para fora da lamba, a request seguinte vai conseguir acessar:

De:

export function rateLimit({windowMs, maxRequests, punishMs}){

  const ipsController = []
  const ipsPunish = []

Para:

const ipsController = []
const ipsPunish = []

export function rateLimit({windowMs, maxRequests, punishMs}){

Mas há outros problemas com essa abordagem, dado que cada lambda só consegue processar uma só request por vez (não há concorrência dentro da lambda, e foi surreal testar e ver isso se confirmando). Caso duas requests queiram acessar a mesma lambda, a AWS faz o spawn de uma nova instância para paralelizar a request (e daí o closure global dessa nova instância vai ser um novo, com os arrays zerados). E daí fica a cargo da quantidade de lambdas que a AWS quiser abrir para conseguir consumir todo o tráfego que está querendo entrar.

Então sua solução não é 100% descartável, pois num ataque de DDoS, um mesmo IP pode sim cair numa mesma lambda e é bem possível que isso aconteça. Mas você tem dois vetores desalinhados nessa situação, porque quanto maior o ataque, mais diluídos estarão os arrays entre as instâncias.

Então concordo com o @tembra que a solução deveria ser utilizando o Banco de Dados, até porque os resultados ficam auditáveis. Vai ser muito difícil debuggar o que aconteceu com aqueles arrays na memória.

@tiagomol1
Copy link
Author

tiagomol1 commented May 24, 2022

Perfeito @filipedeschamps, sua explicação foi muito esclarecedora e faz total sentido! (mind blowing moment 😂)

Vou reimplementar essa solução montando uma estrutura massinha no banco de dados.

Obrigado pelo apoio @tembra e @filipedeschamps! 🤝

@filipedeschamps filipedeschamps removed this from the Milestone 4: TabCoins milestone May 25, 2022
@filipedeschamps
Copy link
Owner

@filipedeschamps filipedeschamps deleted the ratelimit branch June 13, 2022 23:44
@tiagomol1
Copy link
Author

@tiagomol1 editei o post comemorativo para destacar esse seu PR https://www.tabnews.com.br/filipedeschamps/novas-melhorias-husky-sistema-de-eventos-firewall-e-melhorias-no-seo

Que massa! 😍 Estou com o tempo meio escasso devido ao trabalho, mas comecei a montar a estrutura pra fazer o controle das requests através do banco de dados. Vai dar boa!

@filipedeschamps
Copy link
Owner

Fala @tiagomol1 sem problema meu caro 😍 E acho que não precisa mais, isso já foi feito no PR #444

Para outras URLs (por exemplo as com GET) vamos usar outra abordagem e deve ser feito fora da camada da aplicação 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back Envolve modificações no backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rate limiting: identificar e deletar comportamentos abusivos
3 participants