Merge pull request #3141 from cdr/jsjoeio/fix-login-rate-limiter

fix(login): rate limiter shouldn't count successful logins

src/node/routes/login.ts

@@ -12,16 +12,20 @@ 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")
 
-  public try(): boolean {
-    if (this.minuteLimiter.tryRemoveTokens(1)) {
-      return true
-    }
-    return this.hourLimiter.tryRemoveTokens(1)
+  public canTry(): boolean {
+    // 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 {
+    return this.minuteLimiter.tryRemoveTokens(1) || this.hourLimiter.tryRemoveTokens(1)
   }
 }
 
@@ -59,7 +63,8 @@ router.get("/", async (req, res) => {
 
 router.post("/", async (req, res) => {
   try {
-    if (!limiter.try()) {
+    // Check to see if they exceeded their login attempts
+    if (!limiter.canTry()) {
       throw new Error("Login rate limited!")
     }
 
@@ -84,6 +89,10 @@ 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
+    limiter.removeToken()
+
     console.error(
       "Failed login attempt",
       JSON.stringify({

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: "on",
 }
 
 // Run tests in three browsers.

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
@@ -20,4 +26,46 @@ 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"))
+  })
+
+  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")
+      // 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 for a different reason:
+    // login rate
+    await page.click(".submit")
+    await page.waitForLoadState("networkidle")
+    expect(await page.isVisible("text=Login rate limited!"))
+  })
 })

test/e2e/loginPage.test.ts

@@ -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")
-  })
-})

test/unit/routes/login.test.ts

@@ -0,0 +1,37 @@
+import { RateLimiter } from "../../../src/node/routes/login"
+
+describe("login", () => {
+  describe("RateLimiter", () => {
+    it("should allow one try ", () => {
+      const limiter = new RateLimiter()
+      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.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.removeToken()).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.removeToken()).toBe(true)
+      }
+
+      expect(limiter.canTry()).toBe(false)
+      expect(limiter.removeToken()).toBe(false)
+    })
+  })
+})