From b975f42228d990dc3630ffba911bff53807fa1ee Mon Sep 17 00:00:00 2001 From: Maximilian Kaske Date: Thu, 1 Oct 2026 21:25:36 +0200 Subject: [PATCH] wip: magic link review fixes and tests --- apps/dashboard/package.json | 5 +- apps/dashboard/src/lib/auth/providers.test.ts | 69 +++++++++++++++++++ apps/dashboard/src/lib/auth/providers.ts | 3 +- .../src/lib/rate-limit/magic-link.test.ts | 57 +++++++++++++++ .../src/lib/rate-limit/magic-link.ts | 3 +- .../src/lib/rate-limit/sso-lookup.ts | 3 +- apps/dashboard/src/test-preload.ts | 5 ++ apps/dashboard/src/test/upstash.mock.ts | 11 +++ apps/dashboard/test.importmap.json | 6 ++ deno.lock | 16 +++++ .../emails/emails/dashboard-magic-link.tsx | 8 +++ packages/emails/src/templates.test.tsx | 2 + packages/upstash/package.json | 5 +- packages/upstash/src/index.ts | 1 + .../upstash/src/redis/incr-with-ttl.test.ts | 47 +++++++++++++ .../upstash/src/redis}/incr-with-ttl.ts | 10 ++- pnpm-lock.yaml | 12 ++++ 17 files changed, 255 insertions(+), 8 deletions(-) create mode 100644 apps/dashboard/src/lib/auth/providers.test.ts create mode 100644 apps/dashboard/src/lib/rate-limit/magic-link.test.ts create mode 100644 apps/dashboard/src/test-preload.ts create mode 100644 apps/dashboard/src/test/upstash.mock.ts create mode 100644 apps/dashboard/test.importmap.json create mode 100644 packages/upstash/src/redis/incr-with-ttl.test.ts rename {apps/dashboard/src/lib/rate-limit => packages/upstash/src/redis}/incr-with-ttl.ts (65%) diff --git a/apps/dashboard/package.json b/apps/dashboard/package.json index c775eb4f..a3c5a88e 100644 --- a/apps/dashboard/package.json +++ b/apps/dashboard/package.json @@ -8,7 +8,8 @@ "start": "next start", "lint": "next lint", "check": "tsc --noEmit", - "tsc": "tsc --noEmit" + "tsc": "tsc --noEmit", + "test": "deno test --parallel -A --no-check --sloppy-imports --import-map=test.importmap.json src" }, "dependencies": { "@ai-sdk/react": "catalog:", @@ -106,6 +107,8 @@ "zod": "catalog:" }, "devDependencies": { + "@std/expect": "jsr:^1.0.19", + "@std/testing": "jsr:^1.0.19", "@tailwindcss/postcss": "catalog:", "@tailwindcss/typography": "catalog:", "@types/dom-speech-recognition": "catalog:", diff --git a/apps/dashboard/src/lib/auth/providers.test.ts b/apps/dashboard/src/lib/auth/providers.test.ts new file mode 100644 index 00000000..734f8242 --- /dev/null +++ b/apps/dashboard/src/lib/auth/providers.test.ts @@ -0,0 +1,69 @@ +import "@/test-preload"; +import { AuthError } from "@auth/core/errors"; +import { redis } from "@openstatus/upstash"; +import { expect } from "@std/expect"; +import { afterEach, describe, test } from "@std/testing/bdd"; +import { type Stub, stub } from "@std/testing/mock"; + +import { ResendProvider } from "./providers"; + +let evalStub: Stub | undefined; +let logStub: Stub | undefined; + +afterEach(() => { + evalStub?.restore(); + logStub?.restore(); +}); + +function sendTo(identifier: string, ip = "1.2.3.4") { + // `Resend()` keeps overrides under `options`; Auth.js merges them at init. + const send = ResendProvider.options?.sendVerificationRequest; + if (!send) throw new Error("sendVerificationRequest override missing"); + return send({ + identifier, + url: "https://app.openstatus.dev/api/auth/callback/resend?token=t", + token: "t", + expires: new Date(Date.now() + 60_000), + provider: ResendProvider, + request: new Request("https://app.openstatus.dev/api/auth/signin/resend", { + headers: { "x-forwarded-for": ip }, + }), + theme: {}, + }); +} + +describe("ResendProvider.sendVerificationRequest", () => { + test("refuses disposable domains before touching the rate limit", async () => { + evalStub = stub(redis, "eval"); + + await expect(sendTo("a@mailinator.com")).rejects.toThrow(AuthError); + await expect(sendTo("a@mailinator.com")).rejects.toThrow( + "disposable domain", + ); + expect(evalStub.calls).toHaveLength(0); + }); + + test("refuses throttled addresses", async () => { + evalStub = stub(redis, "eval", () => Promise.resolve([1, 4])); + + await expect(sendTo("gilfoyle@piedpiper.dev")).rejects.toThrow( + "rate limited", + ); + }); + + test("sends when screening passes, keyed by client ip and address", async () => { + evalStub = stub(redis, "eval", () => Promise.resolve([1, 1])); + logStub = stub(console, "log"); + + await sendTo("gilfoyle@piedpiper.dev", "9.9.9.9"); + + expect(evalStub.calls[0]?.args[1]).toEqual([ + "ratelimit:magic-link:ip:9.9.9.9", + "ratelimit:magic-link:email:gilfoyle@piedpiper.dev", + ]); + const printed = logStub.calls.map((c) => String(c.args[0])).join("\n"); + expect(printed).toContain( + "https://app.openstatus.dev/api/auth/callback/resend?token=t", + ); + }); +}); diff --git a/apps/dashboard/src/lib/auth/providers.ts b/apps/dashboard/src/lib/auth/providers.ts index f4f9ef70..a19e02cb 100644 --- a/apps/dashboard/src/lib/auth/providers.ts +++ b/apps/dashboard/src/lib/auth/providers.ts @@ -1,6 +1,7 @@ +import { AuthError } from "@auth/core/errors"; import { EmailClient } from "@openstatus/emails"; import { resolveClientIp } from "@openstatus/services/page-access"; -import { AuthError, type Profile } from "next-auth"; +import type { Profile } from "next-auth"; import type { OIDCConfig } from "next-auth/providers"; import GitHub from "next-auth/providers/github"; import Google from "next-auth/providers/google"; diff --git a/apps/dashboard/src/lib/rate-limit/magic-link.test.ts b/apps/dashboard/src/lib/rate-limit/magic-link.test.ts new file mode 100644 index 00000000..35e35f54 --- /dev/null +++ b/apps/dashboard/src/lib/rate-limit/magic-link.test.ts @@ -0,0 +1,57 @@ +import "@/test-preload"; +import { redis } from "@openstatus/upstash"; +import { expect } from "@std/expect"; +import { afterEach, describe, test } from "@std/testing/bdd"; +import { type Stub, stub } from "@std/testing/mock"; + +import { magicLinkRateLimit } from "./magic-link"; + +let evalStub: Stub | undefined; +let warnStub: Stub | undefined; + +function stubCounts(counts: number[] | Error) { + evalStub = stub(redis, "eval", () => + counts instanceof Error ? Promise.reject(counts) : Promise.resolve(counts), + ); +} + +afterEach(() => { + evalStub?.restore(); + warnStub?.restore(); +}); + +describe("magicLinkRateLimit", () => { + test("keys the counters by ip and email", async () => { + stubCounts([1, 1]); + + await magicLinkRateLimit({ ip: "1.2.3.4", email: "a@b.c" }); + + expect(evalStub?.calls[0]?.args[1]).toEqual([ + "ratelimit:magic-link:ip:1.2.3.4", + "ratelimit:magic-link:email:a@b.c", + ]); + }); + + test("allows at the limits", async () => { + stubCounts([10, 3]); + expect(await magicLinkRateLimit({ ip: "ip", email: "e" })).toBe(true); + }); + + test("refuses past the ip limit", async () => { + stubCounts([11, 1]); + expect(await magicLinkRateLimit({ ip: "ip", email: "e" })).toBe(false); + }); + + test("refuses past the email limit", async () => { + stubCounts([1, 4]); + expect(await magicLinkRateLimit({ ip: "ip", email: "e" })).toBe(false); + }); + + test("fails open with a warning when redis is down", async () => { + stubCounts(new Error("redis down")); + warnStub = stub(console, "warn"); + + expect(await magicLinkRateLimit({ ip: "ip", email: "e" })).toBe(true); + expect(warnStub.calls).toHaveLength(1); + }); +}); diff --git a/apps/dashboard/src/lib/rate-limit/magic-link.ts b/apps/dashboard/src/lib/rate-limit/magic-link.ts index 8d541443..d69e2245 100644 --- a/apps/dashboard/src/lib/rate-limit/magic-link.ts +++ b/apps/dashboard/src/lib/rate-limit/magic-link.ts @@ -1,4 +1,4 @@ -import { incrWithTtl } from "./incr-with-ttl"; +import { incrWithTtl, redis } from "@openstatus/upstash"; const WINDOW_SECONDS = 60 * 10; const MAX_PER_IP = 10; @@ -14,6 +14,7 @@ export async function magicLinkRateLimit(args: { }): Promise { try { const [byIp, byEmail] = await incrWithTtl( + redis, [ `ratelimit:magic-link:ip:${args.ip}`, `ratelimit:magic-link:email:${args.email}`, diff --git a/apps/dashboard/src/lib/rate-limit/sso-lookup.ts b/apps/dashboard/src/lib/rate-limit/sso-lookup.ts index eab9369f..e63d7d27 100644 --- a/apps/dashboard/src/lib/rate-limit/sso-lookup.ts +++ b/apps/dashboard/src/lib/rate-limit/sso-lookup.ts @@ -1,4 +1,4 @@ -import { incrWithTtl } from "./incr-with-ttl"; +import { incrWithTtl, redis } from "@openstatus/upstash"; const WINDOW_SECONDS = 60 * 10; const MAX_ATTEMPTS = 10; @@ -10,6 +10,7 @@ const MAX_ATTEMPTS = 10; export async function ssoLookupRateLimit(ip: string): Promise { try { const [count] = await incrWithTtl( + redis, [`ratelimit:sso-lookup:${ip}`], WINDOW_SECONDS, ); diff --git a/apps/dashboard/src/test-preload.ts b/apps/dashboard/src/test-preload.ts new file mode 100644 index 00000000..b471ee4a --- /dev/null +++ b/apps/dashboard/src/test-preload.ts @@ -0,0 +1,5 @@ +// Import FIRST in every test file: `@openstatus/emails` validates +// RESEND_API_KEY and snapshots NODE_ENV on import. Development keeps +// EmailClient off the network. +Object.assign(process.env, { NODE_ENV: "development" }); +process.env.RESEND_API_KEY ??= "test-key"; diff --git a/apps/dashboard/src/test/upstash.mock.ts b/apps/dashboard/src/test/upstash.mock.ts new file mode 100644 index 00000000..c20a9af7 --- /dev/null +++ b/apps/dashboard/src/test/upstash.mock.ts @@ -0,0 +1,11 @@ +// Test double for @openstatus/upstash, swapped in via --import-map. The real +// client is a pipelining Proxy that `stub()` cannot intercept. +export { incrWithTtl } from "../../../../packages/upstash/src/redis/incr-with-ttl"; + +export const redis = { + eval: ( + _script: string, + _keys: string[], + _args: unknown[], + ): Promise => Promise.resolve([1, 1]), +}; diff --git a/apps/dashboard/test.importmap.json b/apps/dashboard/test.importmap.json new file mode 100644 index 00000000..b9dd92bc --- /dev/null +++ b/apps/dashboard/test.importmap.json @@ -0,0 +1,6 @@ +{ + "imports": { + "@/": "./src/", + "@openstatus/upstash": "./src/test/upstash.mock.ts" + } +} diff --git a/deno.lock b/deno.lock index af5e05d4..212bdcce 100644 --- a/deno.lock +++ b/deno.lock @@ -60,6 +60,14 @@ ] }, "members": { + "apps/dashboard": { + "packageJson": { + "dependencies": [ + "npm:@jsr/std__expect@^1.0.19", + "npm:@jsr/std__testing@^1.0.19" + ] + } + }, "apps/server": { "packageJson": { "dependencies": [ @@ -292,6 +300,14 @@ ] } }, + "packages/upstash": { + "packageJson": { + "dependencies": [ + "npm:@jsr/std__expect@^1.0.19", + "npm:@jsr/std__testing@^1.0.19" + ] + } + }, "packages/utils": { "packageJson": { "dependencies": [ diff --git a/packages/emails/emails/dashboard-magic-link.tsx b/packages/emails/emails/dashboard-magic-link.tsx index ece4e8ed..e1f824ad 100644 --- a/packages/emails/emails/dashboard-magic-link.tsx +++ b/packages/emails/emails/dashboard-magic-link.tsx @@ -1,9 +1,13 @@ /** @jsxRuntime automatic @jsxImportSource react */ +import { Text } from "react-email"; + import { Actions } from "./_components/actions"; +import { CodeBlock } from "./_components/code-block"; import { Footer } from "./_components/footer"; import { Heading } from "./_components/heading"; import { Layout } from "./_components/layout"; +import { styles } from "./_components/styles"; export interface DashboardMagicLinkProps { link: string; @@ -23,6 +27,10 @@ const DashboardMagicLinkEmail = ({ link }: DashboardMagicLinkProps) => { once. + + If the button doesn’t work, copy this link into your browser: + + {link} ); }; diff --git a/packages/emails/src/templates.test.tsx b/packages/emails/src/templates.test.tsx index 8ad1da96..5bc2d159 100644 --- a/packages/emails/src/templates.test.tsx +++ b/packages/emails/src/templates.test.tsx @@ -668,6 +668,8 @@ describe("account and status page mail", () => { expect(html).toContain("Sign in to openstatus"); expect(html).toContain("24 hours"); expect(html.split(`href="${link}"`).length - 1).toBe(1); + // raw link rendered as copyable text, not a second anchor + expect(html).toContain(`>${link}

`); expect(html).toContain('href="https://www.openstatus.dev"'); }); }); diff --git a/packages/upstash/package.json b/packages/upstash/package.json index 68403d46..fd768f2e 100644 --- a/packages/upstash/package.json +++ b/packages/upstash/package.json @@ -4,7 +4,8 @@ "license": "MIT", "main": "./src/index.ts", "scripts": { - "check": "deno check --sloppy-imports ." + "check": "deno check --sloppy-imports .", + "test": "deno test --parallel -A --no-check --sloppy-imports" }, "dependencies": { "@upstash/qstash": "catalog:", @@ -12,6 +13,8 @@ }, "devDependencies": { "@openstatus/tsconfig": "workspace:*", + "@std/expect": "jsr:^1.0.19", + "@std/testing": "jsr:^1.0.19", "@types/node": "catalog:", "tsup": "catalog:", "typescript": "catalog:" diff --git a/packages/upstash/src/index.ts b/packages/upstash/src/index.ts index 4ecbe4f1..9b7ec401 100644 --- a/packages/upstash/src/index.ts +++ b/packages/upstash/src/index.ts @@ -1,2 +1,3 @@ export * from "./redis/client"; +export * from "./redis/incr-with-ttl"; export * from "@upstash/redis"; diff --git a/packages/upstash/src/redis/incr-with-ttl.test.ts b/packages/upstash/src/redis/incr-with-ttl.test.ts new file mode 100644 index 00000000..03183e50 --- /dev/null +++ b/packages/upstash/src/redis/incr-with-ttl.test.ts @@ -0,0 +1,47 @@ +import { expect } from "@std/expect"; +import { describe, test } from "@std/testing/bdd"; + +import { incrWithTtl } from "./incr-with-ttl"; + +function fakeClient(counts: number[]) { + const calls: { script: string; keys: string[]; args: unknown[] }[] = []; + const client = { + eval: (script: string, keys: string[], args: unknown[]) => { + calls.push({ script, keys, args }); + return Promise.resolve(counts); + }, + }; + return { client: client as never, calls }; +} + +describe("incrWithTtl", () => { + test("sends every key in one eval with the shared window", async () => { + const { client, calls } = fakeClient([3, 1]); + + const counts = await incrWithTtl(client, ["a", "b"], 600); + + expect(counts).toEqual([3, 1]); + expect(calls).toHaveLength(1); + expect(calls[0]?.keys).toEqual(["a", "b"]); + expect(calls[0]?.args).toEqual([600]); + }); + + test("the script sets the TTL only on the key's first hit", () => { + const { client, calls } = fakeClient([1]); + incrWithTtl(client, ["a"], 600); + + const script = calls[0]?.script ?? ""; + expect(script).toContain("for i, key in ipairs(KEYS)"); + expect(script).toContain("redis.call('INCR', key)"); + expect(script).toContain("if count == 1 then"); + expect(script).toContain("redis.call('EXPIRE', key, tonumber(ARGV[1]))"); + }); + + test("propagates client failures", async () => { + const client = { + eval: () => Promise.reject(new Error("redis down")), + } as never; + + await expect(incrWithTtl(client, ["a"], 600)).rejects.toThrow("redis down"); + }); +}); diff --git a/apps/dashboard/src/lib/rate-limit/incr-with-ttl.ts b/packages/upstash/src/redis/incr-with-ttl.ts similarity index 65% rename from apps/dashboard/src/lib/rate-limit/incr-with-ttl.ts rename to packages/upstash/src/redis/incr-with-ttl.ts index caebda9c..221ed63c 100644 --- a/apps/dashboard/src/lib/rate-limit/incr-with-ttl.ts +++ b/packages/upstash/src/redis/incr-with-ttl.ts @@ -1,4 +1,4 @@ -import { redis } from "@openstatus/upstash"; +import type { Redis } from "@upstash/redis"; // INCR + conditional EXPIRE in one round-trip so a crash between the two can't // leave a TTL-less key. Every key shares the window. @@ -15,6 +15,10 @@ const INCR_WITH_TTL = ` `; /** Current count per key after this hit, in `keys` order. */ -export function incrWithTtl(keys: string[], windowSeconds: number) { - return redis.eval<[number], number[]>(INCR_WITH_TTL, keys, [windowSeconds]); +export function incrWithTtl( + client: Pick, + keys: string[], + windowSeconds: number, +) { + return client.eval<[number], number[]>(INCR_WITH_TTL, keys, [windowSeconds]); } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 99315954..f370ecea 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -965,6 +965,12 @@ importers: specifier: 'catalog:' version: 4.6.5 devDependencies: + '@std/expect': + specifier: jsr:^1.0.19 + version: '@jsr/std__expect@1.0.20' + '@std/testing': + specifier: jsr:^1.0.19 + version: '@jsr/std__testing@1.0.20' '@tailwindcss/postcss': specifier: 'catalog:' version: 4.3.3 @@ -3302,6 +3308,12 @@ importers: '@openstatus/tsconfig': specifier: workspace:* version: link:../tsconfig + '@std/expect': + specifier: jsr:^1.0.19 + version: '@jsr/std__expect@1.0.20' + '@std/testing': + specifier: jsr:^1.0.19 + version: '@jsr/std__testing@1.0.20' '@types/node': specifier: 'catalog:' version: 26.6.2 -- 2.51.2