Skip to content

Conversation

@sergeyteleshev
Copy link
Contributor

return hash.toUpperCase();
}

return md5(value).toUpperCase();

Check failure

Code scanning / CodeQL

Use of password hash with insufficient computational effort High

Password from
an access to oldPassword
is hashed insecurely.
Password from
an access to newPassword
is hashed insecurely.
Password from
an access to adminPassword
is hashed insecurely.

Copilot Autofix

AI about 2 months ago

In general, to fix the use of an insufficiently strong hash (like SHA-256 or MD5) for password hashing, we must replace them with a secure, computationally expensive, and salted password-hashing algorithm. In the JavaScript/TypeScript ecosystem, the most common and well-supported options are bcrypt, scrypt, or PBKDF2—bcrypt is especially simple and robust.

For this project, the change should be limited to only the relevant password-flows, so existing generic hashing functionality (e.g., for checksums, not password hashing) is preserved. You should create a new utility function—such as hashPassword—that uses bcrypt, and replace the problematic use of createHash with a call to the new function where passwords are being hashed (i.e., in UserInfoResource.ts, method updateLocalPassword). Update imports accordingly. Since bcrypt is an external dependency, add the import at the top of the relevant file. You do not need to change the general createHash function, unless you know it is used exclusively for password hashing (which, based on its name and structure, it is not).

Specifically:

  • In UserInfoResource.ts, define and use a secure password hashing function (bcrypt.hashSync or the async version).
  • Add required bcrypt import.
  • Use this new function to hash oldPassword and newPassword in updateLocalPassword, instead of using createHash.
  • You may need to set a salt rounds constant (recommended: 12).
Suggested changeset 2
webapp/packages/core-authentication/src/UserInfoResource.ts
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/webapp/packages/core-authentication/src/UserInfoResource.ts b/webapp/packages/core-authentication/src/UserInfoResource.ts
--- a/webapp/packages/core-authentication/src/UserInfoResource.ts
+++ b/webapp/packages/core-authentication/src/UserInfoResource.ts
@@ -6,6 +6,7 @@
  * you may not use this file except in compliance with the License.
  */
 import { computed, makeObservable, runInAction } from 'mobx';
+import bcrypt from 'bcryptjs';
 
 import { injectable } from '@cloudbeaver/core-di';
 import { AutoRunningTask, type ISyncExecutor, type ITask, SyncExecutor, whileTask } from '@cloudbeaver/core-executor';
@@ -19,6 +20,13 @@
 import type { IAuthCredentials } from './IAuthCredentials.js';
 import { createHash } from '@cloudbeaver/core-utils';
 
+
+// Secure password hashing function for user passwords
+async function hashPassword(password: string): Promise<string> {
+  const SALT_ROUNDS = 12;
+  return await bcrypt.hash(password, SALT_ROUNDS);
+}
+
 export type UserLogoutInfo = AuthLogoutQuery['result'];
 
 export interface ILoginOptions {
@@ -236,7 +244,10 @@
 
   async updateLocalPassword(oldPassword: string, newPassword: string): Promise<void> {
     await this.performUpdate(undefined, [], async () => {
-      const [oldPasswordHash, newPasswordHash] = await Promise.all([createHash(oldPassword, 'sha256'), createHash(newPassword, 'sha256')]);
+      const [oldPasswordHash, newPasswordHash] = await Promise.all([
+        hashPassword(oldPassword),
+        hashPassword(newPassword),
+      ]);
 
       await this.graphQLService.sdk.authChangeLocalPassword({
         oldPassword: oldPasswordHash,
EOF
@@ -6,6 +6,7 @@
* you may not use this file except in compliance with the License.
*/
import { computed, makeObservable, runInAction } from 'mobx';
import bcrypt from 'bcryptjs';

import { injectable } from '@cloudbeaver/core-di';
import { AutoRunningTask, type ISyncExecutor, type ITask, SyncExecutor, whileTask } from '@cloudbeaver/core-executor';
@@ -19,6 +20,13 @@
import type { IAuthCredentials } from './IAuthCredentials.js';
import { createHash } from '@cloudbeaver/core-utils';


// Secure password hashing function for user passwords
async function hashPassword(password: string): Promise<string> {
const SALT_ROUNDS = 12;
return await bcrypt.hash(password, SALT_ROUNDS);
}

export type UserLogoutInfo = AuthLogoutQuery['result'];

export interface ILoginOptions {
@@ -236,7 +244,10 @@

async updateLocalPassword(oldPassword: string, newPassword: string): Promise<void> {
await this.performUpdate(undefined, [], async () => {
const [oldPasswordHash, newPasswordHash] = await Promise.all([createHash(oldPassword, 'sha256'), createHash(newPassword, 'sha256')]);
const [oldPasswordHash, newPasswordHash] = await Promise.all([
hashPassword(oldPassword),
hashPassword(newPassword),
]);

await this.graphQLService.sdk.authChangeLocalPassword({
oldPassword: oldPasswordHash,
webapp/packages/core-authentication/package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/webapp/packages/core-authentication/package.json b/webapp/packages/core-authentication/package.json
--- a/webapp/packages/core-authentication/package.json
+++ b/webapp/packages/core-authentication/package.json
@@ -37,7 +37,8 @@
     "@cloudbeaver/core-utils": "workspace:*",
     "@dbeaver/js-helpers": "workspace:*",
     "mobx": "^6",
-    "tslib": "^2"
+    "tslib": "^2",
+    "bcryptjs": "^3.0.2"
   },
   "devDependencies": {
     "@cloudbeaver/core-cli": "workspace:*",
EOF
@@ -37,7 +37,8 @@
"@cloudbeaver/core-utils": "workspace:*",
"@dbeaver/js-helpers": "workspace:*",
"mobx": "^6",
"tslib": "^2"
"tslib": "^2",
"bcryptjs": "^3.0.2"
},
"devDependencies": {
"@cloudbeaver/core-cli": "workspace:*",
This fix introduces these dependencies
Package Version Security advisories
bcryptjs (npm) 3.0.2 None
Copilot is powered by AI and may make mistakes. Always verify output.
HocKu7 and others added 4 commits October 6, 2025 14:12
…r-than-md5-hash-to-pass-local-user-passwords' into 4569-cb-3681-use-something-better-than-md5-hash-to-pass-local-user-passwords
@HocKu7 HocKu7 marked this pull request as ready for review October 6, 2025 14:58
private static final Log log = Log.getLog(BruteForceUtils.class);

public static void checkBruteforce(SMControllerConfiguration smConfig, List<UserLoginRecord> latestLoginAttempts)
public static void checkBruteforce(SMControllerConfiguration smConfig, List<UserLoginRecord> latestLoginAttempts,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotations + code style

Map<String, Object> credentials = new LinkedHashMap<>();
credentials.put(LocalAuthProviderConstants.CRED_USER, adminUser.getUserId());
credentials.put(LocalAuthProviderConstants.CRED_PASSWORD, clientPassword);
credentials.put(LocalAuthProviderConstants.CRED_PASSWORD, adminPassword);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove digest?

{
adminName: "test",
adminPassword: "test",
adminPassword: "9F86D081884C7D659A2FEAA0C55AD015A3BF4F1B2B0B822CD15D6C15B0F00A08",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why store digest but password?


private void validatePasswordSha(String passwordSha, Map<String, Object> storedCredentials) throws DBException {
try {
if (!Argon2IdHasher.verify(CommonUtils.toString(storedCredentials.get(LocalAuthProviderConstants.CRED_PASSWORD)), passwordSha)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move all game with argon inside security utils. We mustn't know what algorithms are used on this level of abstraction.

public record UserLoginRecord(SMAuthStatus smAuthStatus, LocalDateTime time) {
public record UserLoginRecord(SMAuthStatus smAuthStatus,
LocalDateTime time,
Map<String, Object> authState
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that it is safe to read auth state. What changed, why do we need it now?

public static final String CRED_PASSWORD = LocalAuthProviderConstants.CRED_PASSWORD;
public static final String AUTH_LOCAL_TYPE = "authLocalType";
public static final String LEGACY_AUTH_LOCAL_TYPE = "legacy";
public static final String NEW_AUTH_LOCAL_TYPE = "new";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's name it sha


public static final String PROVIDER_ID = LocalAuthProviderConstants.PROVIDER_ID;
public static final String CRED_USER = LocalAuthProviderConstants.CRED_USER;
public static final String CRED_PASSWORD_MD_5 = LocalAuthProviderConstants.CRED_PASSWORD_MD5;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MD5

SychevAndrey
SychevAndrey previously approved these changes Oct 7, 2025
Comment on lines 230 to 242
await authInfoService.login(provider.id, {
configurationId: configuration?.id,
credentials: {
...state.credentials,
credentials: {
...state.credentials.credentials,
user: state.credentials.credentials['user']?.trim(),
password: state.credentials.credentials['password']?.trim(),
},
},
forceSessionsLogout: state.forceSessionsLogout,
linkUser,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrap this block with additional try/catch
if it's shouldReloginWithOldHash than run

await authInfoService.login(provider.id, {
              configurationId: configuration?.id,
              credentials: {
                ...state.credentials,
                credentials: {
                  ...state.credentials.credentials,
                  user: state.credentials.credentials['user']?.trim(),
                  password: state.credentials.credentials['password']?.trim(),
                },
              },
              forceSessionsLogout: state.forceSessionsLogout,
              linkUser,
            });

again in catch block, if it throws - higher catch will get exception and handle it properly

Comment on lines 252 to 269
if (shouldReloginWithOldHash) {
try {
await authInfoService.login(provider.id, {
configurationId: configuration?.id,
credentials: {
...state.credentials,
credentials: {
...state.credentials.credentials,
user: state.credentials.credentials['user']?.trim(),
password: state.credentials.credentials['password']?.trim(),
},
},
forceSessionsLogout: state.forceSessionsLogout,
linkUser,
hasOldHash: true,
});
} catch {}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove to upper place without try/catch as described above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants