From 80eebe38f0952528e8d30df067d86b6cac71c512 Mon Sep 17 00:00:00 2001 From: Grant Date: Tue, 18 Jun 2024 15:28:58 -0600 Subject: [PATCH] add ratelimiting (fixes #40) & fix redis race-condition --- package-lock.json | 27 +++++++++++++++++ packages/server/package.json | 2 ++ packages/server/src/api/admin.ts | 3 ++ packages/server/src/api/client.ts | 30 ++++++++++++++----- packages/server/src/index.ts | 8 +++++- packages/server/src/lib/RateLimiter.ts | 40 ++++++++++++++++++++++++++ packages/server/src/lib/redis.ts | 21 +++++++++++++- packages/server/src/types.ts | 1 + 8 files changed, 123 insertions(+), 9 deletions(-) create mode 100644 packages/server/src/lib/RateLimiter.ts diff --git a/package-lock.json b/package-lock.json index 16f3b6d..599325c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9379,6 +9379,20 @@ "node": ">= 0.10.0" } }, + "node_modules/express-rate-limit": { + "version": "7.3.1", + "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-7.3.1.tgz", + "integrity": "sha512-BbaryvkY4wEgDqLgD18/NSy2lDO2jTuT9Y8c1Mpx0X63Yz0sYd5zN6KPe7UvpuSVvV33T6RaE1o1IVZQjHMYgw==", + "engines": { + "node": ">= 16" + }, + "funding": { + "url": "https://github.com/sponsors/express-rate-limit" + }, + "peerDependencies": { + "express": "4 || 5 || ^5.0.0-beta.1" + } + }, "node_modules/express-session": { "version": "1.17.3", "resolved": "https://registry.npmjs.org/express-session/-/express-session-1.17.3.tgz", @@ -12519,6 +12533,17 @@ "node": ">= 0.6" } }, + "node_modules/rate-limit-redis": { + "version": "4.2.0", + "resolved": "https://registry.npmjs.org/rate-limit-redis/-/rate-limit-redis-4.2.0.tgz", + "integrity": "sha512-wV450NQyKC24NmPosJb2131RoczLdfIJdKCReNwtVpm5998U8SgKrAZrIHaN/NfQgqOHaan8Uq++B4sa5REwjA==", + "engines": { + "node": ">= 16" + }, + "peerDependencies": { + "express-rate-limit": ">= 6" + } + }, "node_modules/raw-body": { "version": "2.5.1", "resolved": "https://registry.npmjs.org/raw-body/-/raw-body-2.5.1.tgz", @@ -16269,9 +16294,11 @@ "connect-redis": "^7.1.1", "cors": "^2.8.5", "express": "^4.18.2", + "express-rate-limit": "^7.3.1", "express-session": "^1.17.3", "openid-client": "^5.6.5", "prisma-dbml-generator": "^0.12.0", + "rate-limit-redis": "^4.2.0", "redis": "^4.6.12", "socket.io": "^4.7.2", "winston": "^3.11.0" diff --git a/packages/server/package.json b/packages/server/package.json index 420a9e6..d6e8e03 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -35,9 +35,11 @@ "connect-redis": "^7.1.1", "cors": "^2.8.5", "express": "^4.18.2", + "express-rate-limit": "^7.3.1", "express-session": "^1.17.3", "openid-client": "^5.6.5", "prisma-dbml-generator": "^0.12.0", + "rate-limit-redis": "^4.2.0", "redis": "^4.6.12", "socket.io": "^4.7.2", "winston": "^3.11.0" diff --git a/packages/server/src/api/admin.ts b/packages/server/src/api/admin.ts index c0ea7d3..9e3a8d8 100644 --- a/packages/server/src/api/admin.ts +++ b/packages/server/src/api/admin.ts @@ -2,9 +2,12 @@ import { Router } from "express"; import { User } from "../models/User"; import Canvas from "../lib/Canvas"; import { Logger } from "../lib/Logger"; +import { RateLimiter } from "../lib/RateLimiter"; const app = Router(); +app.use(RateLimiter.ADMIN); + app.use(async (req, res, next) => { if (!req.session.user) { res.status(401).json({ diff --git a/packages/server/src/api/client.ts b/packages/server/src/api/client.ts index 1495c1f..45bbd8e 100644 --- a/packages/server/src/api/client.ts +++ b/packages/server/src/api/client.ts @@ -4,6 +4,7 @@ import { OpenID } from "../lib/oidc"; import { TokenSet, errors as OIDC_Errors } from "openid-client"; import { Logger } from "../lib/Logger"; import Canvas from "../lib/Canvas"; +import { RateLimiter } from "../lib/RateLimiter"; const ClientParams = { TYPE: "auth_type", @@ -23,6 +24,9 @@ const buildQuery = (obj: { [k in keyof typeof ClientParams]?: string }) => { const app = Router(); +/** + * Redirect to actual authorization page + */ app.get("/login", (req, res) => { res.redirect( OpenID.client.authorizationUrl({ @@ -34,10 +38,12 @@ app.get("/login", (req, res) => { // TODO: logout endpoint -app.get("/callback", async (req, res) => { - // TODO: return proper UIs for errors intead of raw JSON (#35) - // const { code } = req.query; - +/** + * Process token exchange from openid server + * + * This executes multiple database queries and should be ratelimited + */ +app.get("/callback", RateLimiter.HIGH, async (req, res) => { let exchange: TokenSet; try { @@ -190,8 +196,7 @@ app.get("/callback", async (req, res) => { res.redirect("/"); }); -// TODO: Ratelimiting #40 -app.get("/canvas/pixel/:x/:y", async (req, res) => { +app.get("/canvas/pixel/:x/:y", RateLimiter.HIGH, async (req, res) => { const x = parseInt(req.params.x); const y = parseInt(req.params.y); @@ -234,6 +239,12 @@ app.get("/canvas/pixel/:x/:y", async (req, res) => { }); }); +/** + * Get the heatmap + * + * This is cached, so no need to ratelimit this + * Even if the heatmap isn't ready, this doesn't cause the heatmap to get generated + */ app.get("/heatmap", async (req, res) => { const heatmap = await Canvas.getCachedHeatmap(); @@ -244,7 +255,12 @@ app.get("/heatmap", async (req, res) => { res.json({ success: true, heatmap }); }); -app.get("/user/:sub", async (req, res) => { +/** + * Get user information from the sub (grant@toast.ooo) + * + * This causes a database query, so ratelimit it + */ +app.get("/user/:sub", RateLimiter.HIGH, async (req, res) => { const user = await prisma.user.findFirst({ where: { sub: req.params.sub } }); if (!user) { return res.status(404).json({ success: false, error: "unknown_user" }); diff --git a/packages/server/src/index.ts b/packages/server/src/index.ts index 5bdc510..ae84901 100644 --- a/packages/server/src/index.ts +++ b/packages/server/src/index.ts @@ -39,6 +39,12 @@ if (!process.env.REDIS_SESSION_PREFIX) { ); } +if (!process.env.REDIS_RATELIMIT_PREFIX) { + Logger.info( + "REDIS_RATELIMIT_PREFIX was not defined, defaulting to canvas_ratelimit:" + ); +} + if (!process.env.AUTH_ENDPOINT) { Logger.error("AUTH_ENDPOINT is not defined"); process.exit(1); @@ -61,7 +67,7 @@ if (!process.env.OIDC_CALLBACK_HOST) { // run startup tasks, all of these need to be completed to serve Promise.all([ - Redis.connect(), + Redis.getClient(), OpenID.setup().then(() => { Logger.info("Setup OpenID"); }), diff --git a/packages/server/src/lib/RateLimiter.ts b/packages/server/src/lib/RateLimiter.ts new file mode 100644 index 0000000..8f0cc71 --- /dev/null +++ b/packages/server/src/lib/RateLimiter.ts @@ -0,0 +1,40 @@ +import rateLimit from "express-rate-limit"; +import RedisStore from "rate-limit-redis"; +import { Redis } from "./redis"; + +const REDIS_PREFIX = process.env.REDIS_RATELIMIT_PREFIX || "canavs_ratelimit:"; + +export const RateLimiter = { + ADMIN: rateLimit({ + windowMs: 15 * 60 * 1000, + max: 15, + standardHeaders: true, + legacyHeaders: false, + + skipSuccessfulRequests: true, + + store: new RedisStore({ + prefix: REDIS_PREFIX + "admin:", + sendCommand: async (...args: string[]) => { + const client = await Redis.getClient(); + + return await client.sendCommand(args); + }, + }), + }), + HIGH: rateLimit({ + windowMs: 15 * 60 * 1000, + max: 50, + standardHeaders: true, + legacyHeaders: false, + + store: new RedisStore({ + prefix: REDIS_PREFIX + "high:", + sendCommand: async (...args: string[]) => { + const client = await Redis.getClient(); + + return await client.sendCommand(args); + }, + }), + }), +}; diff --git a/packages/server/src/lib/redis.ts b/packages/server/src/lib/redis.ts index d7d1a80..12223d8 100644 --- a/packages/server/src/lib/redis.ts +++ b/packages/server/src/lib/redis.ts @@ -26,9 +26,12 @@ const RedisKeys: IRedisKeys = { }; class _Redis { + isConnecting = false; isConnected = false; client: RedisClientType; + waitingForConnect: ((...args: any) => any)[] = []; + keys: IRedisKeys; /** @@ -48,9 +51,17 @@ class _Redis { if (this.isConnected) throw new Error("Attempted to run Redis#connect when already connected"); + this.isConnecting = true; await this.client.connect(); - Logger.info("Connected to Redis"); + Logger.info( + `Connected to Redis, there's ${this.waitingForConnect.length} function(s) waiting for Redis` + ); + this.isConnecting = false; this.isConnected = true; + + for (const func of this.waitingForConnect) { + func(); + } } async disconnect() { @@ -65,6 +76,14 @@ class _Redis { } async getClient() { + if (this.isConnecting) { + await (() => + new Promise((res) => { + Logger.warn("getClient() called and is now pending in queue"); + this.waitingForConnect.push(res); + }))(); + } + if (!this.isConnected) { await this.connect(); this.isConnected = true; diff --git a/packages/server/src/types.ts b/packages/server/src/types.ts index 48570b5..296854b 100644 --- a/packages/server/src/types.ts +++ b/packages/server/src/types.ts @@ -24,6 +24,7 @@ declare global { SESSION_SECRET: string; REDIS_HOST: string; REDIS_SESSION_PREFIX: string; + REDIS_RATELIMIT_PREFIX: string; /** * hostname that is used in the callback