🔒 feat: password reset disable option; fix: account email error messa…

…ge (danny-avila#2327) * feat: password reset disable option; fix: account email leak * fix(LoginSpec): typo * test: fixed LoginForm test * fix: disable password reset when undefined * refactor: use a helper function * fix: tests * feat: Remove unused error message in password reset process * chore: Update password reset email message * refactor: only allow password reset if explicitly allowed * feat: Add password reset email service configuration check The code changes in `checks.js` add a new function `checkPasswordReset()` that checks if the email service is configured when password reset is enabled. If the email service is not configured, a warning message is logged. This change ensures secure password reset functionality by prompting the user to configure the email service. Co-authored-by: Berry-13 <root@Berry> Co-authored-by: Danny Avila <[email protected]> Co-authored-by: Danny Avila <[email protected]> * chore: remove import order rules * refactor: simplify password reset logic and align against Observable Response Discrepancy * chore: make password reset warning more prominent * chore(AuthService): better logging for password resets, refactor requestPasswordReset to use req object, fix sendEmail error when email config is not present * refactor: fix styling of password reset email message * chore: add missing type for passwordResetEnabled, TStartupConfig * fix(LoginForm): prevent login form flickering * fix(ci): Update login form to use mocked startupConfig for rendering correctly * refactor: Improve password reset UI, applies DRY * chore: Add logging to password reset validation middleware * chore(CONTRIBUTING): Update import order conventions --------- Co-authored-by: Danny Avila <[email protected]> Co-authored-by: Berry-13 <root@Berry> Co-authored-by: Danny Avila <[email protected]>
jung0han · Jun 6, 2024 · 5452d4c · 5452d4c
1 parent a7f5b57
commit 5452d4c
Show file tree

Hide file tree

Showing 20 changed files with 288 additions and 137 deletions.
diff --git a/.env.example b/.env.example
@@ -319,6 +319,7 @@ ALLOW_EMAIL_LOGIN=true
 ALLOW_REGISTRATION=true
 ALLOW_SOCIAL_LOGIN=false
 ALLOW_SOCIAL_REGISTRATION=false
+ALLOW_PASSWORD_RESET=false
 # ALLOW_ACCOUNT_DELETION=true # note: enabled by default if omitted/commented out
 
 SESSION_EXPIRY=1000 * 60 * 15

diff --git a/.eslintrc.js b/.eslintrc.js
@@ -67,39 +67,6 @@ module.exports = {
     'react/display-name': ['off'],
     'no-unused-vars': ['error', { varsIgnorePattern: '^_' }],
     quotes: ['error', 'single'],
-    'import/order': [
-      'warn',
-      {
-        groups: [
-          ['builtin'], // Node.js standard libraries, react
-          ['external'], // npm packages
-          ['type'], // Type imports (TypeScript)
-          [
-            'internal', // Internal alias imports eg.(~/Component)
-            'parent', // Parent directory imports eg.(../ParentComponent)
-            'sibling', // Sibling imports eg.(./components/MyComponent)
-            'index',
-            'object',
-          ],
-        ],
-        // 'newlines-between': 'always', // Enforce new lines between groups
-        pathGroups: [
-          {
-            pattern: '{react,react-dom/**}',
-            group: 'builtin',
-            position: 'before',
-          },
-          {
-            pattern: '~/**',
-            group: 'internal',
-            position: 'before',
-          },
-        ],
-        pathGroupsExcludedImportTypes: ['builtin', 'type'], // Exclude these types from the path group rule
-        warnOnUnassignedImports: true, // Warn for unassigned imports
-        // alphabetize: { order: 'asc', caseInsensitive: true }, // Alphabetize imports within each group
-      },
-    ],
   },
   overrides: [
     {

diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md
@@ -126,6 +126,18 @@ Apply the following naming conventions to branches, labels, and other Git-relate
 
    - **Current Stance**: At present, this backend transition is of lower priority and might not be pursued.
 
+## 7. Module Import Conventions
+
+- `npm` packages first, 
+     - from shortest line (top) to longest (bottom)
+
+- Followed by typescript types (pertains to data-provider and client workspaces)
+     - longest line (top) to shortest (bottom)
+     - types from package come first
+
+- Lastly, local imports
+     - longest line (top) to shortest (bottom)
+     - imports with alias `~` treated the same as relative import with respect to line length
 
 ---
 

diff --git a/api/server/controllers/AuthController.js b/api/server/controllers/AuthController.js
@@ -39,7 +39,7 @@ const getUserController = async (req, res) => {
 
 const resetPasswordRequestController = async (req, res) => {
   try {
-    const resetService = await requestPasswordReset(req.body.email);
+    const resetService = await requestPasswordReset(req);
     if (resetService instanceof Error) {
       return res.status(400).json(resetService);
     } else {

diff --git a/api/server/middleware/index.js b/api/server/middleware/index.js
@@ -17,6 +17,7 @@ const validateMessageReq = require('./validateMessageReq');
 const buildEndpointOption = require('./buildEndpointOption');
 const validateRegistration = require('./validateRegistration');
 const validateImageRequest = require('./validateImageRequest');
+const validatePasswordReset = require('./validatePasswordReset');
 const moderateText = require('./moderateText');
 const noIndex = require('./noIndex');
 const importLimiters = require('./importLimiters');
@@ -40,6 +41,7 @@ module.exports = {
   buildEndpointOption,
   validateRegistration,
   validateImageRequest,
+  validatePasswordReset,
   validateModel,
   moderateText,
   noIndex,

diff --git a/api/server/middleware/validatePasswordReset.js b/api/server/middleware/validatePasswordReset.js
@@ -0,0 +1,13 @@
+const { isEnabled } = require('~/server/utils');
+const { logger } = require('~/config');
+
+function validatePasswordReset(req, res, next) {
+  if (isEnabled(process.env.ALLOW_PASSWORD_RESET)) {
+    next();
+  } else {
+    logger.warn(`Password reset attempt while not allowed. IP: ${req.ip}`);
+    res.status(403).send('Password reset is not allowed.');
+  }
+}
+
+module.exports = validatePasswordReset;
diff --git a/api/server/middleware/validateRegistration.js b/api/server/middleware/validateRegistration.js
@@ -1,6 +1,7 @@
+const { isEnabled } = require('~/server/utils');
+
 function validateRegistration(req, res, next) {
-  const setting = process.env.ALLOW_REGISTRATION?.toLowerCase();
-  if (setting === 'true') {
+  if (isEnabled(process.env.ALLOW_REGISTRATION)) {
     next();
   } else {
     res.status(403).send('Registration is not allowed.');

diff --git a/api/server/routes/__tests__/config.spec.js b/api/server/routes/__tests__/config.spec.js
@@ -25,6 +25,7 @@ afterEach(() => {
   delete process.env.DOMAIN_SERVER;
   delete process.env.ALLOW_REGISTRATION;
   delete process.env.ALLOW_SOCIAL_LOGIN;
+  delete process.env.ALLOW_PASSWORD_RESET;
   delete process.env.LDAP_URL;
   delete process.env.LDAP_BIND_DN;
   delete process.env.LDAP_BIND_CREDENTIALS;
@@ -55,6 +56,7 @@ describe.skip('GET /', () => {
     process.env.DOMAIN_SERVER = 'http://test-server.com';
     process.env.ALLOW_REGISTRATION = 'true';
     process.env.ALLOW_SOCIAL_LOGIN = 'true';
+    process.env.ALLOW_PASSWORD_RESET = 'true';
     process.env.LDAP_URL = 'Test LDAP URL';
     process.env.LDAP_BIND_DN = 'Test LDAP Bind DN';
     process.env.LDAP_BIND_CREDENTIALS = 'Test LDAP Bind Credentials';
@@ -78,6 +80,7 @@ describe.skip('GET /', () => {
       serverDomain: 'http://test-server.com',
       emailLoginEnabled: 'true',
       registrationEnabled: 'true',
+      passwordResetEnabled: 'true',
       socialLoginEnabled: 'true',
     });
   });

diff --git a/api/server/routes/auth.js b/api/server/routes/auth.js
@@ -15,6 +15,7 @@ const {
   requireLdapAuth,
   requireLocalAuth,
   validateRegistration,
+  validatePasswordReset,
 } = require('../middleware');
 
 const router = express.Router();
@@ -32,7 +33,12 @@ router.post(
 );
 router.post('/refresh', refreshController);
 router.post('/register', registerLimiter, checkBan, validateRegistration, registrationController);
-router.post('/requestPasswordReset', resetPasswordRequestController);
-router.post('/resetPassword', resetPasswordController);
+router.post(
+  '/requestPasswordReset',
+  checkBan,
+  validatePasswordReset,
+  resetPasswordRequestController,
+);
+router.post('/resetPassword', checkBan, validatePasswordReset, resetPasswordController);
 
 module.exports = router;
diff --git a/api/server/routes/config.js b/api/server/routes/config.js
@@ -6,6 +6,7 @@ const { logger } = require('~/config');
 const router = express.Router();
 const emailLoginEnabled =
   process.env.ALLOW_EMAIL_LOGIN === undefined || isEnabled(process.env.ALLOW_EMAIL_LOGIN);
+const passwordResetEnabled = isEnabled(process.env.ALLOW_PASSWORD_RESET);
 
 router.get('/', async function (req, res) {
   const isBirthday = () => {
@@ -42,6 +43,7 @@ router.get('/', async function (req, res) {
         !!process.env.EMAIL_USERNAME &&
         !!process.env.EMAIL_PASSWORD &&
         !!process.env.EMAIL_FROM,
+      passwordResetEnabled,
       checkBalance: isEnabled(process.env.CHECK_BALANCE),
       showBirthdayIcon:
         isBirthday() ||

diff --git a/api/server/services/AuthService.js b/api/server/services/AuthService.js
@@ -16,6 +16,19 @@ const domains = {
 
 const isProduction = process.env.NODE_ENV === 'production';
 
+/**
+ * Check if email configuration is set
+ * @returns {Boolean}
+ */
+function checkEmailConfig() {
+  return (
+    (!!process.env.EMAIL_SERVICE || !!process.env.EMAIL_HOST) &&
+    !!process.env.EMAIL_USERNAME &&
+    !!process.env.EMAIL_PASSWORD &&
+    !!process.env.EMAIL_FROM
+  );
+}
+
 /**
  * Logout user
  *
@@ -114,14 +127,20 @@ const registerUser = async (user) => {
 
 /**
  * Request password reset
- *
- * @param {String} email
- * @returns
+ * @param {Express.Request} req
  */
-const requestPasswordReset = async (email) => {
+const requestPasswordReset = async (req) => {
+  const { email } = req.body;
   const user = await User.findOne({ email }).lean();
+  const emailEnabled = checkEmailConfig();
+
+  logger.warn(`[requestPasswordReset] [Password reset request initiated] [Email: ${email}]`);
+
   if (!user) {
-    return new Error('Email does not exist');
+    logger.warn(`[requestPasswordReset] [No user found] [Email: ${email}] [IP: ${req.ip}]`);
+    return {
+      message: 'If an account with that email exists, a password reset link has been sent to it.',
+    };
   }
 
   let token = await Token.findOne({ userId: user._id });
@@ -140,12 +159,6 @@ const requestPasswordReset = async (email) => {
 
   const link = `${domains.client}/reset-password?token=${resetToken}&userId=${user._id}`;
 
-  const emailEnabled =
-    (!!process.env.EMAIL_SERVICE || !!process.env.EMAIL_HOST) &&
-    !!process.env.EMAIL_USERNAME &&
-    !!process.env.EMAIL_PASSWORD &&
-    !!process.env.EMAIL_FROM;
-
   if (emailEnabled) {
     sendEmail(
       user.email,
@@ -158,10 +171,19 @@ const requestPasswordReset = async (email) => {
       },
       'requestPasswordReset.handlebars',
     );
-    return { link: '' };
+    logger.info(
+      `[requestPasswordReset] Link emailed. [Email: ${email}] [ID: ${user._id}] [IP: ${req.ip}]`,
+    );
   } else {
+    logger.info(
+      `[requestPasswordReset] Link issued. [Email: ${email}] [ID: ${user._id}] [IP: ${req.ip}]`,
+    );
     return { link };
   }
+
+  return {
+    message: 'If an account with that email exists, a password reset link has been sent to it.',
+  };
 };
 
 /**
@@ -190,20 +212,23 @@ const resetPassword = async (userId, token, password) => {
   await User.updateOne({ _id: userId }, { $set: { password: hash } }, { new: true });
 
   const user = await User.findById({ _id: userId });
+  const emailEnabled = checkEmailConfig();
 
-  sendEmail(
-    user.email,
-    'Password Reset Successfully',
-    {
-      appName: process.env.APP_TITLE || 'LibreChat',
-      name: user.name,
-      year: new Date().getFullYear(),
-    },
-    'passwordReset.handlebars',
-  );
+  if (emailEnabled) {
+    sendEmail(
+      user.email,
+      'Password Reset Successfully',
+      {
+        appName: process.env.APP_TITLE || 'LibreChat',
+        name: user.name,
+        year: new Date().getFullYear(),
+      },
+      'passwordReset.handlebars',
+    );
+  }
 
   await passwordResetToken.deleteOne();
-
+  logger.info(`[resetPassword] Password reset successful. [Email: ${user.email}]`);
   return { message: 'Password reset was successful' };
 };
 

diff --git a/api/server/services/start/checks.js b/api/server/services/start/checks.js
@@ -3,6 +3,7 @@ const {
   deprecatedAzureVariables,
   conflictingAzureVariables,
 } = require('librechat-data-provider');
+const { isEnabled } = require('~/server/utils');
 const { logger } = require('~/config');
 
 const secretDefaults = {
@@ -49,6 +50,8 @@ function checkVariables() {
       Please use the config (\`librechat.yaml\`) file for setting up OpenRouter, and use \`OPENROUTER_KEY\` or another environment variable instead.`,
     );
   }
+
+  checkPasswordReset();
 }
 
 /**
@@ -107,4 +110,30 @@ Latest version: ${Constants.CONFIG_VERSION}
   }
 }
 
+function checkPasswordReset() {
+  const emailEnabled =
+    (!!process.env.EMAIL_SERVICE || !!process.env.EMAIL_HOST) &&
+    !!process.env.EMAIL_USERNAME &&
+    !!process.env.EMAIL_PASSWORD &&
+    !!process.env.EMAIL_FROM;
+
+  const passwordResetAllowed = isEnabled(process.env.ALLOW_PASSWORD_RESET);
+
+  if (!emailEnabled && passwordResetAllowed) {
+    logger.warn(
+      `❗❗❗
+
+      Password reset is enabled with \`ALLOW_PASSWORD_RESET\` but email service is not configured.
+      
+      This setup is insecure as password reset links will be issued with a recognized email.
+      
+      Please configure email service for secure password reset functionality.
+      
+      https://www.librechat.ai/docs/configuration/authentication/password_reset
+
+      ❗❗❗`,
+    );
+  }
+}
+
 module.exports = { checkVariables, checkHealth, checkConfig, checkAzureVariables };
diff --git a/client/src/components/Auth/Login.tsx b/client/src/components/Auth/Login.tsx
@@ -14,7 +14,9 @@ function Login() {
   return (
     <>
       {error && <ErrorMessage>{localize(getLoginError(error))}</ErrorMessage>}
-      {startupConfig?.emailLoginEnabled && <LoginForm onSubmit={login} />}
+      {startupConfig?.emailLoginEnabled && (
+        <LoginForm onSubmit={login} startupConfig={startupConfig} />
+      )}
       {startupConfig?.registrationEnabled && (
         <p className="my-4 text-center text-sm font-light text-gray-700 dark:text-white">
           {' '}

diff --git a/client/src/components/Auth/LoginForm.tsx b/client/src/components/Auth/LoginForm.tsx
@@ -1,19 +1,23 @@
 import React from 'react';
 import { useForm } from 'react-hook-form';
+import type { TLoginUser, TStartupConfig } from 'librechat-data-provider';
 import { useLocalize } from '~/hooks';
-import { TLoginUser } from 'librechat-data-provider';
 
 type TLoginFormProps = {
   onSubmit: (data: TLoginUser) => void;
+  startupConfig: TStartupConfig;
 };
 
-const LoginForm: React.FC<TLoginFormProps> = ({ onSubmit }) => {
+const LoginForm: React.FC<TLoginFormProps> = ({ onSubmit, startupConfig }) => {
   const localize = useLocalize();
   const {
     register,
     handleSubmit,
     formState: { errors },
   } = useForm<TLoginUser>();
+  if (!startupConfig) {
+    return null;
+  }
 
   const renderError = (fieldName: string) => {
     const errorMessage = errors[fieldName]?.message;
@@ -81,9 +85,11 @@ const LoginForm: React.FC<TLoginFormProps> = ({ onSubmit }) => {
         </div>
         {renderError('password')}
       </div>
-      <a href="/forgot-password" className="text-sm text-green-500">
-        {localize('com_auth_password_forgot')}
-      </a>
+      {startupConfig.passwordResetEnabled && (
+        <a href="/forgot-password" className="text-sm text-green-500">
+          {localize('com_auth_password_forgot')}
+        </a>
+      )}
       <div className="mt-6">
         <button
           aria-label="Sign in"