diff --git a/src/lib/auth-rate-limit.ts b/src/lib/auth-rate-limit.ts index 83d91db22..33008ae0a 100644 --- a/src/lib/auth-rate-limit.ts +++ b/src/lib/auth-rate-limit.ts @@ -32,9 +32,11 @@ export const AUTH_WINDOW_MS = 15 * 60 * 1000; // Maximum requests per IP per window in production. // A full GitHub OAuth sign-in consumes 2 requests (initiation + callback), -// so 5 allows two complete sign-in attempts plus one spare before throttling. -export const AUTH_LIMIT = 5; - +// so 8 allows up to four complete sign-in attempts before throttling — +// enough headroom for a user who interrupts and retries the flow +// (e.g. hitting Back mid-authorization) without hitting the limit +// prematurely, while still guarding against brute-force attempts. +export const AUTH_LIMIT = 8; /** * Path prefixes whose requests count toward the authentication rate limit. * Only the OAuth initiation and callback routes are included; session and diff --git a/src/middleware.ts b/src/middleware.ts index bc6714fb9..f2ecfd0eb 100644 --- a/src/middleware.ts +++ b/src/middleware.ts @@ -308,23 +308,32 @@ export async function middleware(req: NextRequest) { console.warn("auth_rate_limit_hit", { ip, path: pathname }); const headers = buildHeaders({ ...authResult, limit: authLimit }); - const acceptHeader = req.headers.get("accept") ?? ""; + const acceptHeader = req.headers.get("accept") ?? ""; if (acceptHeader.includes("text/html")) { const url = req.nextUrl.clone(); url.pathname = "/auth/signin"; - url.search = "?error=RateLimit"; + url.search = "?error=RateLimitError"; return NextResponse.redirect(url, { status: 307, headers, }); } + // Non-HTML (fetch/XHR) requests come from next-auth's client-side + // signIn({ redirect: false }) call. NextAuth's internal fetcher parses + // `data.url` out of the response — if it's missing, that parsing + // throws and the raw JSON ends up surfacing to the user instead of a + // friendly toast. Always include a valid `url` here, and use the + // "RateLimitError" code so it maps to AUTH_ERROR_MESSAGES on the client. + const signinUrl = req.nextUrl.clone(); + signinUrl.pathname = "/auth/signin"; + signinUrl.search = "?error=RateLimitError"; + return NextResponse.json( - { error: "Too many authentication attempts. Please try again later." }, + { error: "RateLimitError", url: signinUrl.toString() }, { status: 429, headers } ); } - return NextResponse.next(); } diff --git a/test/middleware.test.ts b/test/middleware.test.ts index 9157feaaf..86f4a201d 100644 --- a/test/middleware.test.ts +++ b/test/middleware.test.ts @@ -40,7 +40,7 @@ describe("Middleware - Auth Rate Limiting Redirection", () => { expect(res).toBeDefined(); // Redirect status code is 307 expect(res?.status).toBe(307); - // Redirect location points to the signin page with error=RateLimit + // Redirect location points to the signin page with error=RateLimitError expect(res?.headers.get("Location")).toContain("/auth/signin?error=RateLimit"); // Rate limit headers are present expect(res?.headers.get("X-RateLimit-Limit")).toBe("5"); @@ -48,7 +48,7 @@ describe("Middleware - Auth Rate Limiting Redirection", () => { expect(res?.headers.get("X-RateLimit-Reset")).toBe("1234567890"); }); - it("should return JSON error response for non-html requests when auth rate limit is hit", async () => { + it("should return a valid redirect url in the JSON error response for non-html requests when auth rate limit is hit", async () => { vi.mocked(isAuthSensitivePath).mockReturnValue(true); vi.mocked(checkAuthRateLimit).mockReturnValue({ allowed: false, @@ -69,17 +69,50 @@ describe("Middleware - Auth Rate Limiting Redirection", () => { expect(res).toBeDefined(); // HTTP status code 429 Too Many Requests expect(res?.status).toBe(429); - // Body contains the rate limit error message + const body = await res?.json(); - expect(body).toEqual({ - error: "Too many authentication attempts. Please try again later.", - }); + // Uses a code that maps to AUTH_ERROR_MESSAGES on the client, not a raw sentence + expect(body.error).toBe("RateLimitError"); + // A valid, absolute url must always be present so next-auth's client-side + // signIn({ redirect: false }) fetcher — which parses `data.url` internally + // — never throws and never lets raw JSON reach the screen (issue #2267). + expect(() => new URL(body.url)).not.toThrow(); + expect(body.url).toContain("/auth/signin?error=RateLimitError"); + // Rate limit headers are present expect(res?.headers.get("X-RateLimit-Limit")).toBe("5"); expect(res?.headers.get("X-RateLimit-Remaining")).toBe("0"); expect(res?.headers.get("X-RateLimit-Reset")).toBe("1234567890"); }); + it("should never omit the url field from the rate-limit JSON body, regardless of Accept header (regression #2267)", async () => { + vi.mocked(isAuthSensitivePath).mockReturnValue(true); + vi.mocked(checkAuthRateLimit).mockReturnValue({ + allowed: false, + remaining: 0, + reset: 1234567890, + }); + + // Simulates an interrupted-then-retried OAuth flow: the browser's + // fetch() call (from next-auth's client redirect:false path) typically + // sends Accept: */* rather than text/html. + const req = new NextRequest("http://localhost/api/auth/signin/github", { + headers: { + accept: "*/*", + "x-forwarded-for": "5.6.7.8", + }, + method: "POST", + }); + + const res = await middleware(req); + const body = await res?.json(); + + expect(res?.status).toBe(429); + expect(body).toHaveProperty("url"); + expect(typeof body.url).toBe("string"); + expect(body.url.length).toBeGreaterThan(0); + }); + it("should allow request to proceed (NextResponse.next) if rate limit is not hit", async () => { vi.mocked(isAuthSensitivePath).mockReturnValue(true); vi.mocked(checkAuthRateLimit).mockReturnValue({ @@ -103,4 +136,4 @@ describe("Middleware - Auth Rate Limiting Redirection", () => { expect(res?.status).toBe(200); expect(res?.headers.get("Location")).toBeNull(); }); -}); +}); \ No newline at end of file