From 08521077f054d01c8963244de6dbbb2680c82b01 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 15 Apr 2021 16:34:51 -0700 Subject: [PATCH] refactor(login): move rate limiter after successful login Before, we weren't checking if a login was successful before counting it against the rate limiter. With this change, we only count unsuccessful logins against the rate limiter. We did this because this was a bug but also because it caused problems with our e2e tests hitting the rate limit. --- src/node/routes/login.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts index b1cd34b9b..3ec339c15 100644 --- a/src/node/routes/login.ts +++ b/src/node/routes/login.ts @@ -59,10 +59,6 @@ router.get("/", async (req, res) => { router.post("/", async (req, res) => { try { - if (!limiter.try()) { - throw new Error("Login rate limited!") - } - if (!req.body.password) { throw new Error("Missing password") } @@ -84,6 +80,12 @@ router.post("/", async (req, res) => { return redirect(req, res, to, { to: undefined }) } + // Note: successful logins should not count against the RateLimiter + // which is why this logic must come after the successful login logic + if (!limiter.try()) { + throw new Error("Login rate limited!") + } + console.error( "Failed login attempt", JSON.stringify({