diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts index b89470aea..017b88307 100644 --- a/src/node/routes/login.ts +++ b/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({ diff --git a/test/config.ts b/test/config.ts index 2d09e0ec9..6df0324e9 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: "on", } // Run tests in three browsers. diff --git a/test/e2e/login.test.ts b/test/e2e/login.test.ts index 21fcbffc2..4277e2cd3 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 @@ -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!")) + }) }) diff --git a/test/e2e/loginPage.test.ts b/test/e2e/loginPage.test.ts deleted file mode 100644 index 3cf1d9cf5..000000000 --- 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") - }) -}) diff --git a/test/unit/routes/login.test.ts b/test/unit/routes/login.test.ts new file mode 100644 index 000000000..2a2e20466 --- /dev/null +++ b/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) + }) + }) +})