🔐 fix: Redirect OAuth Browser Navigations Instead of Rendering JSON (#15440)

* 🔐 fix: Redirect OAuth Browser Navigations Instead of Rendering JSON

The /oauth/* routes are reached by top-level browser navigation, so a
middleware that rejects one with a JSON body makes the browser render
that body as the page. Hitting the login rate limit during a social
sign-in dropped the user on a blank page showing:

  {"message": "Too many login attempts, please try again after 5 minutes."}

Mark those requests with markOAuthNavigation and have the two middlewares
that can reject them answer with a redirect to the login page carrying an
error code the client localizes. The login limiter stays a single instance
so the attempt budget is still shared with POST /api/auth/login, and that
route keeps its JSON 429 for the SPA.

* 🧪 fix: Address PR Review Bot Findings

chatgpt-codex-connector:
- Split the inline `type TranslationKeys` specifier out of the value
  import, per the standalone `import type` convention in CLAUDE.md. This
  also clears the Static checks import-sort failure.
- Cover the new OAuth error branches in Login.spec.tsx: each mapped
  `?error=` code raises its localized toast and the query is cleaned,
  and unmapped codes including prototype members raise nothing.

api/server/middleware/checkBan.js

@@ -1,9 +1,10 @@
 const { Keyv } = require('keyv');
 const uap = require('ua-parser-js');
 const { logger } = require('@librechat/data-schemas');
-const { ViolationTypes } = require('librechat-data-provider');
+const { ErrorTypes, ViolationTypes } = require('librechat-data-provider');
 const { isEnabled, keyvMongo, removePorts } = require('@librechat/api');
 const { getLogStores } = require('~/cache');
+const { isOAuthNavigation, redirectOAuthFailure } = require('./oauthNavigation');
 const denyRequest = require('./denyRequest');
 const { findUser } = require('~/models');
 
@@ -50,9 +51,13 @@ const isInteractiveAgentChatRequest = (req) => {
  * @param {Object} req - Express Request object.
  * @param {Object} res - Express Response object.
  *
- * @returns {Promise<Object>} - Returns a Promise which sends a JSON 403 unless this is an interactive browser agent chat request, in which case it calls `denyRequest()`.
+ * @returns {Promise<Object>} - Returns a Promise which sends a JSON 403, unless this is an interactive browser agent chat request (`denyRequest()`) or an OAuth browser navigation (redirect to the login page).
  */
 const banResponse = async (req, res) => {
+  if (isOAuthNavigation(req)) {
+    return redirectOAuthFailure(res, ErrorTypes.AUTH_BANNED);
+  }
+
   const ua = uap(req.headers['user-agent']);
   if (!ua.browser.name) {
     return res.status(403).json({ message });

api/server/middleware/index.js

@@ -10,6 +10,7 @@ const {
   sendValidationResponse,
 } = require('./messageValidation');
 const checkDomainAllowed = require('./checkDomainAllowed');
+const { markOAuthNavigation } = require('./oauthNavigation');
 const requireLocalAuth = require('./requireLocalAuth');
 const canDeleteAccount = require('./canDeleteAccount');
 const accessResources = require('./accessResources');
@@ -41,6 +42,7 @@ module.exports = {
   uaParser,
   setHeaders,
   logHeaders,
+  markOAuthNavigation,
   moderateText,
   validateModel,
   requireJwtAuth,

api/server/middleware/limiters/loginLimiter.js

@@ -1,6 +1,7 @@
 const rateLimit = require('express-rate-limit');
-const { ViolationTypes } = require('librechat-data-provider');
+const { ErrorTypes, ViolationTypes } = require('librechat-data-provider');
 const { limiterCache, removePorts } = require('@librechat/api');
+const { isOAuthNavigation, redirectOAuthFailure } = require('~/server/middleware/oauthNavigation');
 const { logViolation } = require('~/cache');
 
 const { LOGIN_WINDOW = 5, LOGIN_MAX = 7, LOGIN_VIOLATION_SCORE: score } = process.env;
@@ -18,6 +19,12 @@ const handler = async (req, res) => {
   };
 
   await logViolation(req, res, type, errorMessage, score);
+
+  /** A browser navigation would render the JSON body as the page. */
+  if (isOAuthNavigation(req)) {
+    return redirectOAuthFailure(res, ErrorTypes.AUTH_RATE_LIMITED);
+  }
+
   return res.status(429).json({ message });
 };
 

api/server/middleware/limiters/loginLimiter.spec.js

@@ -0,0 +1,92 @@
+const express = require('express');
+const request = require('supertest');
+
+const originalEnv = process.env;
+
+const createApp = () => {
+  jest.resetModules();
+  process.env = {
+    ...originalEnv,
+    DOMAIN_CLIENT: 'http://client.test',
+    LOGIN_MAX: '1',
+    LOGIN_WINDOW: '5',
+  };
+
+  jest.doMock('@librechat/api', () => ({
+    ...jest.requireActual('@librechat/api'),
+    limiterCache: jest.fn(() => undefined),
+    removePorts: (req) => req?.['ip'],
+  }));
+  jest.doMock('~/cache', () => ({
+    logViolation: jest.fn().mockResolvedValue(undefined),
+  }));
+
+  const { markOAuthNavigation } = require('../oauthNavigation');
+  const loginLimiter = require('./loginLimiter');
+  const { logViolation } = require('~/cache');
+
+  const app = express();
+  app.set('trust proxy', 1);
+  app.use(express.json());
+  app.post('/api/auth/login', loginLimiter, (_req, res) => res.status(204).end());
+  app.use('/oauth', markOAuthNavigation, loginLimiter);
+  app.get('/oauth/openid', (_req, res) => res.status(204).end());
+
+  return { app, logViolation };
+};
+
+describe('loginLimiter', () => {
+  afterEach(() => {
+    jest.dontMock('@librechat/api');
+    jest.dontMock('~/cache');
+    process.env = originalEnv;
+  });
+
+  it('answers API login requests with JSON', async () => {
+    const { app, logViolation } = createApp();
+
+    await request(app).post('/api/auth/login').set('x-forwarded-for', '198.51.100.4').expect(204);
+
+    const response = await request(app)
+      .post('/api/auth/login')
+      .set('x-forwarded-for', '198.51.100.4')
+      .expect(429);
+
+    expect(response.body).toEqual({
+      message: 'Too many login attempts, please try again after 5 minutes.',
+    });
+    expect(logViolation).toHaveBeenCalledTimes(1);
+  });
+
+  it('redirects a rate limited OAuth navigation to the login page', async () => {
+    const { app, logViolation } = createApp();
+
+    await request(app).get('/oauth/openid').set('x-forwarded-for', '203.0.113.7').expect(204);
+
+    const response = await request(app)
+      .get('/oauth/openid')
+      .set('x-forwarded-for', '203.0.113.7')
+      .expect(302);
+
+    expect(response.headers.location).toBe(
+      'http://client.test/login?redirect=false&error=auth_rate_limited',
+    );
+    expect(response.text).not.toContain('Too many login attempts');
+    expect(logViolation).toHaveBeenCalledTimes(1);
+  });
+
+  it('shares one attempt budget between the API login and OAuth routes', async () => {
+    const { app } = createApp();
+
+    await request(app).post('/api/auth/login').set('x-forwarded-for', '203.0.113.9').expect(204);
+
+    const response = await request(app)
+      .get('/oauth/openid')
+      .set('x-forwarded-for', '203.0.113.9')
+      .expect(302);
+
+    expect(response.headers.location).toBe(
+      'http://client.test/login?redirect=false&error=auth_rate_limited',
+    );
+  });
+});

api/server/middleware/oauthNavigation.js

@@ -0,0 +1,36 @@
+const { redirectToAuthFailure } = require('@librechat/api');
+
+/**
+ * Flags the request as a top-level browser navigation belonging to the OAuth flow.
+ *
+ * These routes are reached by the address bar (social button click, IdP redirect, SAML/Apple
+ * form post), never by fetch, so any middleware that rejects them with a JSON body renders that
+ * body as the page. Rejections must redirect back to the login page instead.
+ *
+ * @type {import('express').RequestHandler}
+ */
+const markOAuthNavigation = (req, _res, next) => {
+  req.isOAuthNavigation = true;
+  next();
+};
+
+/**
+ * @param {import('express').Request} req
+ * @returns {boolean} Whether the request is a top-level browser navigation in the OAuth flow.
+ */
+const isOAuthNavigation = (req) => req?.isOAuthNavigation === true;
+
+/**
+ * Sends the browser back to the login page carrying an error code the client localizes.
+ *
+ * @param {import('express').Response} res
+ * @param {string} authFailedError - An `ErrorTypes` value.
+ */
+const redirectOAuthFailure = (res, authFailedError) =>
+  redirectToAuthFailure(res, { clientDomain: process.env.DOMAIN_CLIENT, authFailedError });
+
+module.exports = {
+  markOAuthNavigation,
+  isOAuthNavigation,
+  redirectOAuthFailure,
+};

api/server/routes/oauth.js

@@ -11,7 +11,12 @@ const {
   getOAuthFailureMessage,
   redirectToAuthFailure,
 } = require('@librechat/api');
-const { checkDomainAllowed, loginLimiter, logHeaders } = require('~/server/middleware');
+const {
+  checkDomainAllowed,
+  loginLimiter,
+  logHeaders,
+  markOAuthNavigation,
+} = require('~/server/middleware');
 const { createOAuthHandler } = require('~/server/controllers/auth/oauth');
 const { findBalanceByUser, upsertBalanceFields } = require('~/models');
 const { getAppConfig } = require('~/server/services/Config');
@@ -35,6 +40,7 @@ const authFailureRedirectOptions = {
 };
 
 router.use(logHeaders);
+router.use(markOAuthNavigation);
 router.use(loginLimiter);
 
 const oauthHandler = createOAuthHandler();

api/server/routes/oauth.test.js

@@ -77,10 +77,16 @@ jest.mock('@librechat/api', () => ({
   redirectToAuthFailure: (...args) => mockRedirectToAuthFailure(...args),
 }));
 
+const mockMarkOAuthNavigation = jest.fn((req, _res, next) => {
+  req.isOAuthNavigation = true;
+  next();
+});
+
 jest.mock('~/server/middleware', () => ({
   checkDomainAllowed: jest.fn((_req, _res, next) => next()),
   loginLimiter: jest.fn((_req, _res, next) => next()),
   logHeaders: jest.fn((_req, _res, next) => next()),
+  markOAuthNavigation: (...args) => mockMarkOAuthNavigation(...args),
 }));
 
 jest.mock('~/server/controllers/auth/oauth', () => ({
@@ -136,11 +142,21 @@ describe('OAuth route failure logging', () => {
     mockGetOAuthFailureMessage.mockClear();
     mockRedirectToAuthFailure.mockClear();
     mockPassportAuthenticate.mockClear();
+    mockMarkOAuthNavigation.mockClear();
     mockOpenIDCallbackAuthenticatorOptions = undefined;
     mockPassportAuthenticate.mockImplementation(() => (_req, _res, next) => next());
     mockOpenIDCallbackMiddleware.mockImplementation((_req, _res, next) => next());
   });
 
+  it('marks OAuth requests as browser navigations so rejections can redirect', async () => {
+    const app = createApp();
+
+    await request(app).get('/oauth/openid/callback?code=secret-code').expect(204);
+
+    expect(mockMarkOAuthNavigation).toHaveBeenCalled();
+    expect(mockOAuthHandler.mock.calls[0][0].isOAuthNavigation).toBe(true);
+  });
+
   it('wires the package OpenID callback middleware into the route', async () => {
     const app = createApp();
 

api/test/server/middleware/checkBan.test.js

@@ -41,6 +41,8 @@ jest.mock('@librechat/api', () => ({
   },
   keyvMongo: {},
   removePorts: jest.fn((req) => req.ip),
+  redirectToAuthFailure: (res, { clientDomain, authFailedError }) =>
+    res.redirect(`${clientDomain}/login?redirect=false&error=${authFailedError}`),
 }));
 
 jest.mock('~/models', () => ({
@@ -72,6 +74,7 @@ const createReq = (overrides = {}) => ({
 const createRes = () => ({
   status: jest.fn().mockReturnThis(),
   json: jest.fn().mockReturnThis(),
+  redirect: jest.fn().mockReturnThis(),
 });
 
 describe('checkBan middleware', () => {
@@ -152,6 +155,26 @@ describe('checkBan middleware', () => {
       expect(res.status).toHaveBeenCalledWith(403);
     });
 
+    it('redirects instead of sending JSON when the ban hits an OAuth navigation', async () => {
+      process.env.DOMAIN_CLIENT = 'http://client.test';
+      mockBanCacheGet.mockResolvedValueOnce({ expiresAt: Date.now() + 60000 });
+      const next = jest.fn();
+      const req = createReq({
+        isOAuthNavigation: true,
+        baseUrl: '/oauth',
+        originalUrl: '/oauth/openid/callback',
+      });
+      const res = createRes();
+
+      await checkBan(req, res, next);
+
+      expect(req.banned).toBe(true);
+      expect(res.json).not.toHaveBeenCalled();
+      expect(res.redirect).toHaveBeenCalledWith(
+        'http://client.test/login?redirect=false&error=auth_banned',
+      );
+    });
+
     it('returns 403 when user ban is cached (IP miss)', async () => {
       mockBanCacheGet
         .mockResolvedValueOnce(undefined)

client/src/components/Auth/Login.tsx

@@ -3,6 +3,7 @@ import { OpenIDIcon, useToastContext } from '@librechat/client';
 import { ErrorTypes, registerPage } from 'librechat-data-provider';
 import { useOutletContext, useSearchParams, useLocation } from 'react-router-dom';
 import type { TLoginLayoutContext } from '~/common';
+import type { TranslationKeys } from '~/hooks';
 import { getLoginError, persistRedirectToSession } from '~/utils';
 import { ErrorMessage } from '~/components/Auth/ErrorMessage';
 import SocialButton from '~/components/Auth/SocialButton';
@@ -14,6 +15,13 @@ interface LoginLocationState {
   redirect_to?: string;
 }
 
+/** Error codes the server appends to the login redirect when an OAuth navigation is rejected. */
+const oauthErrorKeys: Record<string, TranslationKeys> = {
+  [ErrorTypes.AUTH_FAILED]: 'com_auth_error_oauth_failed',
+  [ErrorTypes.AUTH_RATE_LIMITED]: 'com_auth_error_login_rl',
+  [ErrorTypes.AUTH_BANNED]: 'com_auth_error_login_ban',
+};
+
 function Login() {
   const localize = useLocalize();
   const { showToast } = useToastContext();
@@ -38,9 +46,14 @@ function Login() {
     }
 
     const oauthError = searchParams?.get('error');
-    if (oauthError && oauthError === ErrorTypes.AUTH_FAILED) {
+    /** `hasOwn` keeps an attacker-supplied `?error=toString` off the prototype chain. */
+    const oauthErrorKey =
+      oauthError != null && Object.hasOwn(oauthErrorKeys, oauthError)
+        ? oauthErrorKeys[oauthError]
+        : undefined;
+    if (oauthErrorKey != null) {
       showToast({
-        message: localize('com_auth_error_oauth_failed'),
+        message: localize(oauthErrorKey),
         status: 'error',
       });
       const newParams = new URLSearchParams(searchParams);

client/src/components/Auth/__tests__/Login.spec.tsx

@@ -11,6 +11,13 @@ import Login from '~/components/Auth/Login';
 
 jest.mock('librechat-data-provider/react-query');
 
+const mockShowToast = jest.fn();
+
+jest.mock('@librechat/client', () => ({
+  ...jest.requireActual('@librechat/client'),
+  useToastContext: () => ({ showToast: mockShowToast }),
+}));
+
 const mockStartupConfig = {
   isFetching: false,
   isLoading: false,
@@ -205,3 +212,43 @@ test('Navigates to / on successful login', async () => {
 
   waitFor(() => expect(window.location.pathname).toBe('/'));
 });
+
+describe('OAuth rejection redirects', () => {
+  const enterAt = (search: string) => {
+    window.history.pushState({}, '', `/login${search}`);
+  };
+
+  afterEach(() => {
+    mockShowToast.mockClear();
+    window.history.pushState({}, '', '/login');
+  });
+
+  test.each([
+    [
+      'auth_rate_limited',
+      'Too many login attempts in a short amount of time. Please try again later.',
+    ],
+    ['auth_banned', 'Your account has been temporarily banned due to violations of our service.'],
+    ['auth_failed', 'Authentication failed. Please check your login method and try again.'],
+  ])('surfaces a localized toast for %s and clears the query', async (code, message) => {
+    enterAt(`?redirect=false&error=${code}`);
+
+    setup();
+
+    await waitFor(() => expect(mockShowToast).toHaveBeenCalledWith({ message, status: 'error' }));
+    await waitFor(() => expect(window.location.search).not.toContain('error='));
+    expect(window.location.search).not.toContain('redirect=');
+  });
+
+  test.each(['toString', 'constructor', 'unmapped_code'])(
+    'ignores the unmapped error code %s',
+    async (code) => {
+      enterAt(`?error=${code}`);
+
+      setup();
+
+      await waitFor(() => expect(getByTestId(document.body, 'login-button')).toBeInTheDocument());
+      expect(mockShowToast).not.toHaveBeenCalled();
+    },
+  );
+});

packages/data-provider/src/config.ts

@@ -3082,6 +3082,14 @@ export enum ErrorTypes {
    * Generic Authentication failure
    */
   AUTH_FAILED = 'auth_failed',
+  /**
+   * Authentication rejected by a rate limiter
+   */
+  AUTH_RATE_LIMITED = 'auth_rate_limited',
+  /**
+   * Authentication rejected because the account or IP is banned
+   */
+  AUTH_BANNED = 'auth_banned',
   /**
    * Model refused to respond (content policy violation)
    */