From 4683d8a077b39f05bff0ddd550e4a861bbe77fbb Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 15 Apr 2021 15:07:07 -0700 Subject: [PATCH 01/14] fix: update comment and export rateLimiter --- src/node/routes/login.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts index b89470ae..b1cd34b9 100644 --- a/src/node/routes/login.ts +++ b/src/node/routes/login.ts @@ -12,8 +12,8 @@ export enum Cookie { } // RateLimiter wraps around the limiter library for logins. -// It allows 2 logins every minute and 12 logins every hour. -class RateLimiter { +// It allows 2 logins every minute plus 12 logins every hour. +export class RateLimiter { private readonly minuteLimiter = new Limiter(2, "minute") private readonly hourLimiter = new Limiter(12, "hour") From 58e17c5e50c9ffa4ce6e788b0b305b2f2e4b84b7 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 15 Apr 2021 15:12:07 -0700 Subject: [PATCH 02/14] feat(testing): add tests for RateLimiter --- test/unit/routes/login.test.ts | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 test/unit/routes/login.test.ts diff --git a/test/unit/routes/login.test.ts b/test/unit/routes/login.test.ts new file mode 100644 index 00000000..1ef5b659 --- /dev/null +++ b/test/unit/routes/login.test.ts @@ -0,0 +1,23 @@ +import { RateLimiter } from "../../../src/node/routes/login" + +describe("login", () => { + describe("RateLimiter", () => { + it("should allow one try ", () => { + const limiter = new RateLimiter() + expect(limiter.try()).toBe(true) + }) + + it("should not allow more than 14 tries in less than an hour", () => { + const limiter = new RateLimiter() + + // The limiter allows 2 tries per minute plus 12 per hour + // so if we run it 15 times, 14 should return true and the last + // should return false + for (let i = 1; i <= 14; i++) { + expect(limiter.try()).toBe(true) + } + + expect(limiter.try()).toBe(false) + }) + }) +}) From ebbabc6e048895423b75d19419206ec0deee43a0 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 15 Apr 2021 15:16:50 -0700 Subject: [PATCH 03/14] refactor(testing): combine loginPage with login --- test/e2e/login.test.ts | 6 ++++++ test/e2e/loginPage.test.ts | 18 ------------------ 2 files changed, 6 insertions(+), 18 deletions(-) delete mode 100644 test/e2e/loginPage.test.ts diff --git a/test/e2e/login.test.ts b/test/e2e/login.test.ts index 21fcbffc..e3acbf6b 100644 --- a/test/e2e/login.test.ts +++ b/test/e2e/login.test.ts @@ -10,6 +10,12 @@ test.describe("login", () => { }, } + test("should see the login page", options, async ({ page }) => { + await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" }) + // It should send us to the login page + expect(await page.title()).toBe("code-server login") + }) + test("should be able to login", options, async ({ page }) => { await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" }) // Type in password diff --git a/test/e2e/loginPage.test.ts b/test/e2e/loginPage.test.ts deleted file mode 100644 index 3cf1d9cf..00000000 --- a/test/e2e/loginPage.test.ts +++ /dev/null @@ -1,18 +0,0 @@ -import { test, expect } from "@playwright/test" -import { CODE_SERVER_ADDRESS } from "../utils/constants" - -test.describe("login page", () => { - // Reset the browser so no cookies are persisted - // by emptying the storageState - const options = { - contextOptions: { - storageState: {}, - }, - } - - test("should see the login page", options, async ({ page }) => { - await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" }) - // It should send us to the login page - expect(await page.title()).toBe("code-server login") - }) -}) From faaa0a9e600e12f65a0dc6ecd22c4bb9d7463034 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 15 Apr 2021 15:27:17 -0700 Subject: [PATCH 04/14] feat(testing): add e2e tests for password --- test/e2e/login.test.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/e2e/login.test.ts b/test/e2e/login.test.ts index e3acbf6b..91303ffe 100644 --- a/test/e2e/login.test.ts +++ b/test/e2e/login.test.ts @@ -26,4 +26,23 @@ test.describe("login", () => { // Make sure the editor actually loaded expect(await page.isVisible("div.monaco-workbench")) }) + + test("should see an error message for missing password", options, async ({ page }) => { + await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" }) + // Skip entering password + // Click the submit button and login + await page.click(".submit") + await page.waitForLoadState("networkidle") + expect(await page.isVisible("text=Missing password")) + }) + + test("should see an error message for incorrect password", options, async ({ page }) => { + await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" }) + // Type in password + await page.fill(".password", "password123") + // Click the submit button and login + await page.click(".submit") + await page.waitForLoadState("networkidle") + expect(await page.isVisible("text=Incorrect password")) + }) }) From 83cfbf82cfbd2b483487520bc4b5cc712e154bbd Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 15 Apr 2021 16:34:36 -0700 Subject: [PATCH 05/14] feat: increase timeout for playwright tests --- test/config.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/config.ts b/test/config.ts index 2d09e0ec..95c23a6c 100644 --- a/test/config.ts +++ b/test/config.ts @@ -50,7 +50,7 @@ globalSetup(async () => { const config: Config = { testDir: path.join(__dirname, "e2e"), // Search for tests in this directory. - timeout: 30000, // Each test is given 30 seconds. + timeout: 60000, // Each test is given 60 seconds. retries: 3, // Retry failing tests 2 times } @@ -64,7 +64,7 @@ setConfig(config) const options: PlaywrightOptions = { headless: true, // Run tests in headless browsers. - video: "retain-on-failure", + video: "retry-with-video", } // Run tests in three browsers. From 08521077f054d01c8963244de6dbbb2680c82b01 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 15 Apr 2021 16:34:51 -0700 Subject: [PATCH 06/14] 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 b1cd34b9..3ec339c1 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({ From 1e6f4f2a14861d6166c9abaa6be5528abf94e3fe Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Thu, 15 Apr 2021 16:36:35 -0700 Subject: [PATCH 07/14] feat(testing): add test for rate limiter --- test/e2e/login.test.ts | 47 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/test/e2e/login.test.ts b/test/e2e/login.test.ts index 91303ffe..0621accd 100644 --- a/test/e2e/login.test.ts +++ b/test/e2e/login.test.ts @@ -45,4 +45,51 @@ test.describe("login", () => { await page.waitForLoadState("networkidle") expect(await page.isVisible("text=Incorrect password")) }) + + test("should hit the rate limiter for too many unsuccessful logins", options, async ({ page }) => { + await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" }) + // Type in password + await page.fill(".password", "password123") + // Click the submit button and login + // The current RateLimiter allows 2 logins per minute plus + // 12 logins per hour for a total of 14 + // See: src/node/routes/login.ts + for (let i = 1; i <= 14; i++) { + await page.click(".submit") + await page.waitForLoadState("networkidle") + } + + // The 15th should fail + await page.click(".submit") + await page.waitForLoadState("networkidle") + expect(await page.isVisible("text=Login rate limited!")) + }) + + // This test takes 8mins to run and is probably not worth adding to our e2e suite + // test.only("should not count successful logins against the rate limiter", options, async ({ page }) => { + // for (let i = 1; i <= 14; i++) { + // await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" }) + // await page.fill(".password", PASSWORD) + // await page.click(".submit") + // await page.waitForLoadState("networkidle") + // // Make sure the editor actually loaded + // await page.isVisible("div.monaco-workbench") + + // // Delete cookie + // await page.evaluate(() => { + // document.cookie = "key" + "=; Path=/; Expires=Thu, 01 Jan 1970 00:00:01 GMT;" + // return Promise.resolve() + // }) + + // // Go back to address, which should be the login page + // await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" }) + // } + + // // On the 15th time, we should see the editor + // await page.fill(".password", PASSWORD) + // await page.click(".submit") + // await page.waitForLoadState("networkidle") + // // Make sure the editor actually loaded + // expect(await page.isVisible("div.monaco-workbench")) + // }) }) From a8719e1f793336c5b2c9a56af17715bf0c3a7bf8 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 16 Apr 2021 14:12:10 -0700 Subject: [PATCH 08/14] refactor: change config to save all e2e videos --- test/config.ts | 2 +- test/e2e/login.test.ts | 28 ---------------------------- 2 files changed, 1 insertion(+), 29 deletions(-) diff --git a/test/config.ts b/test/config.ts index 95c23a6c..6df0324e 100644 --- a/test/config.ts +++ b/test/config.ts @@ -64,7 +64,7 @@ setConfig(config) const options: PlaywrightOptions = { headless: true, // Run tests in headless browsers. - video: "retry-with-video", + video: "on", } // Run tests in three browsers. diff --git a/test/e2e/login.test.ts b/test/e2e/login.test.ts index 0621accd..d82b2075 100644 --- a/test/e2e/login.test.ts +++ b/test/e2e/login.test.ts @@ -64,32 +64,4 @@ test.describe("login", () => { await page.waitForLoadState("networkidle") expect(await page.isVisible("text=Login rate limited!")) }) - - // This test takes 8mins to run and is probably not worth adding to our e2e suite - // test.only("should not count successful logins against the rate limiter", options, async ({ page }) => { - // for (let i = 1; i <= 14; i++) { - // await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" }) - // await page.fill(".password", PASSWORD) - // await page.click(".submit") - // await page.waitForLoadState("networkidle") - // // Make sure the editor actually loaded - // await page.isVisible("div.monaco-workbench") - - // // Delete cookie - // await page.evaluate(() => { - // document.cookie = "key" + "=; Path=/; Expires=Thu, 01 Jan 1970 00:00:01 GMT;" - // return Promise.resolve() - // }) - - // // Go back to address, which should be the login page - // await page.goto(CODE_SERVER_ADDRESS, { waitUntil: "networkidle" }) - // } - - // // On the 15th time, we should see the editor - // await page.fill(".password", PASSWORD) - // await page.click(".submit") - // await page.waitForLoadState("networkidle") - // // Make sure the editor actually loaded - // expect(await page.isVisible("div.monaco-workbench")) - // }) }) From d8e45057c71ac00bfd9bcf8282f70eab4e0c2a17 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 16 Apr 2021 14:22:09 -0700 Subject: [PATCH 09/14] refactor: update rateLimiter to check try This changes adds a new method called `.canTry` to the rate limiter to check if there are tokens remaining in the bucket. It also adds suggestions from @oxy to make sure the user can brute force past the rate limiter. --- src/node/routes/login.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts index 3ec339c1..809c31f0 100644 --- a/src/node/routes/login.ts +++ b/src/node/routes/login.ts @@ -17,11 +17,15 @@ export class RateLimiter { private readonly minuteLimiter = new Limiter(2, "minute") private readonly hourLimiter = new Limiter(12, "hour") + public canTry(): boolean { + return this.minuteLimiter.getTokensRemaining() > 0 || this.hourLimiter.getTokensRemaining() > 0 + } + public try(): boolean { - if (this.minuteLimiter.tryRemoveTokens(1)) { - return true + if (this.canTry()) { + return this.minuteLimiter.tryRemoveTokens(1) || this.hourLimiter.tryRemoveTokens(1) } - return this.hourLimiter.tryRemoveTokens(1) + return false } } @@ -59,6 +63,11 @@ router.get("/", async (req, res) => { router.post("/", async (req, res) => { try { + // Check to see if they exceeded their login attempts + if (!limiter.canTry()) { + throw new Error("Login rate limited!") + } + if (!req.body.password) { throw new Error("Missing password") } From 7928dc2bff2d74a4da27ac3c88e3fa891369a80e Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 16 Apr 2021 14:23:46 -0700 Subject: [PATCH 10/14] feat: add test for limiter.canTry() --- test/unit/routes/login.test.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/unit/routes/login.test.ts b/test/unit/routes/login.test.ts index 1ef5b659..fde02b2c 100644 --- a/test/unit/routes/login.test.ts +++ b/test/unit/routes/login.test.ts @@ -7,6 +7,19 @@ describe("login", () => { expect(limiter.try()).toBe(true) }) + it("should pull tokens from both limiters (minute & hour)", () => { + const limiter = new RateLimiter() + + // Try twice, which pulls two from the minute bucket + limiter.try() + limiter.try() + + // Check that we can still try + // which should be true since there are 12 remaining in the hour bucket + expect(limiter.canTry()).toBe(true) + expect(limiter.try()).toBe(true) + }) + it("should not allow more than 14 tries in less than an hour", () => { const limiter = new RateLimiter() From a3f18d61582e84336b7a8543d022ffe3e5ee6588 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Mon, 19 Apr 2021 10:57:50 -0700 Subject: [PATCH 11/14] refactor: change limiter.Try() to .removeToken() --- src/node/routes/login.ts | 9 +++------ test/unit/routes/login.test.ts | 13 +++++++------ 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts index 809c31f0..f02c9c08 100644 --- a/src/node/routes/login.ts +++ b/src/node/routes/login.ts @@ -21,11 +21,8 @@ export class RateLimiter { return this.minuteLimiter.getTokensRemaining() > 0 || this.hourLimiter.getTokensRemaining() > 0 } - public try(): boolean { - if (this.canTry()) { - return this.minuteLimiter.tryRemoveTokens(1) || this.hourLimiter.tryRemoveTokens(1) - } - return false + public removeToken(): boolean { + return this.minuteLimiter.tryRemoveTokens(1) || this.hourLimiter.tryRemoveTokens(1) } } @@ -91,7 +88,7 @@ router.post("/", async (req, res) => { // Note: successful logins should not count against the RateLimiter // which is why this logic must come after the successful login logic - if (!limiter.try()) { + if (!limiter.removeToken()) { throw new Error("Login rate limited!") } diff --git a/test/unit/routes/login.test.ts b/test/unit/routes/login.test.ts index fde02b2c..2a2e2046 100644 --- a/test/unit/routes/login.test.ts +++ b/test/unit/routes/login.test.ts @@ -4,20 +4,20 @@ describe("login", () => { describe("RateLimiter", () => { it("should allow one try ", () => { const limiter = new RateLimiter() - expect(limiter.try()).toBe(true) + expect(limiter.removeToken()).toBe(true) }) it("should pull tokens from both limiters (minute & hour)", () => { const limiter = new RateLimiter() // Try twice, which pulls two from the minute bucket - limiter.try() - limiter.try() + limiter.removeToken() + limiter.removeToken() // Check that we can still try // which should be true since there are 12 remaining in the hour bucket expect(limiter.canTry()).toBe(true) - expect(limiter.try()).toBe(true) + expect(limiter.removeToken()).toBe(true) }) it("should not allow more than 14 tries in less than an hour", () => { @@ -27,10 +27,11 @@ describe("login", () => { // so if we run it 15 times, 14 should return true and the last // should return false for (let i = 1; i <= 14; i++) { - expect(limiter.try()).toBe(true) + expect(limiter.removeToken()).toBe(true) } - expect(limiter.try()).toBe(false) + expect(limiter.canTry()).toBe(false) + expect(limiter.removeToken()).toBe(false) }) }) }) From 958f01262ba92633310810e264b70cfa72d6f557 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Mon, 19 Apr 2021 11:11:52 -0700 Subject: [PATCH 12/14] refactor: check errorMessage in login e2e test --- test/e2e/login.test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/e2e/login.test.ts b/test/e2e/login.test.ts index d82b2075..daefd2e3 100644 --- a/test/e2e/login.test.ts +++ b/test/e2e/login.test.ts @@ -54,12 +54,16 @@ test.describe("login", () => { // The current RateLimiter allows 2 logins per minute plus // 12 logins per hour for a total of 14 // See: src/node/routes/login.ts - for (let i = 1; i <= 14; i++) { + for (let i = 1; i <= 13; i++) { await page.click(".submit") await page.waitForLoadState("networkidle") + // We double-check that the correct error message shows + // which should be for incorrect password + expect(await page.isVisible("text=Incorrect password")) } - // The 15th should fail + // The 15th should fail for a different reason: + // login rate await page.click(".submit") await page.waitForLoadState("networkidle") expect(await page.isVisible("text=Login rate limited!")) From 7a5042176ef1fb1c13dbe94e05ee0ce21d2e127c Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Mon, 19 Apr 2021 11:12:43 -0700 Subject: [PATCH 13/14] fix: update logic for removing token from limiter --- src/node/routes/login.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts index f02c9c08..e0b5ddda 100644 --- a/src/node/routes/login.ts +++ b/src/node/routes/login.ts @@ -88,9 +88,7 @@ router.post("/", async (req, res) => { // Note: successful logins should not count against the RateLimiter // which is why this logic must come after the successful login logic - if (!limiter.removeToken()) { - throw new Error("Login rate limited!") - } + limiter.removeToken() console.error( "Failed login attempt", From f80d5c3764a06ff1a4cffc8835d3afa2dc6d12a8 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Mon, 19 Apr 2021 11:21:38 -0700 Subject: [PATCH 14/14] refactor: rateLimiter.canTry logic to check >= 1 --- src/node/routes/login.ts | 5 ++++- test/e2e/login.test.ts | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts index e0b5ddda..017b8830 100644 --- a/src/node/routes/login.ts +++ b/src/node/routes/login.ts @@ -18,7 +18,10 @@ export class RateLimiter { private readonly hourLimiter = new Limiter(12, "hour") public canTry(): boolean { - return this.minuteLimiter.getTokensRemaining() > 0 || this.hourLimiter.getTokensRemaining() > 0 + // Note: we must check using >= 1 because technically when there are no tokens left + // you get back a number like 0.00013333333333333334 + // which would cause fail if the logic were > 0 + return this.minuteLimiter.getTokensRemaining() >= 1 || this.hourLimiter.getTokensRemaining() >= 1 } public removeToken(): boolean { diff --git a/test/e2e/login.test.ts b/test/e2e/login.test.ts index daefd2e3..4277e2cd 100644 --- a/test/e2e/login.test.ts +++ b/test/e2e/login.test.ts @@ -54,7 +54,7 @@ test.describe("login", () => { // The current RateLimiter allows 2 logins per minute plus // 12 logins per hour for a total of 14 // See: src/node/routes/login.ts - for (let i = 1; i <= 13; i++) { + for (let i = 1; i <= 14; i++) { await page.click(".submit") await page.waitForLoadState("networkidle") // We double-check that the correct error message shows