Skip to content

Conversation

@MarceloRGonc
Copy link
Contributor

Part of OPS-3201

Copilot AI review requested due to automatic review settings December 5, 2025 15:54
@linear
Copy link

linear bot commented Dec 5, 2025

@coderabbitai
Copy link

coderabbitai bot commented Dec 5, 2025

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch mg/OPS-3201

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

[AppSystemProp.TELEMETRY_MODE]: 'COLLECTOR',
[AppSystemProp.TELEMETRY_COLLECTOR_URL]: 'https://telemetry.openops.com/save',
[SharedSystemProp.ENABLE_HOST_VALIDATION]: 'true',
[AppSystemProp.OPENOPS_ADMIN_PASSWORD_SALT]: '$2b$10$6zuoB5d8Dz9bzV91gpuynO',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same default value as in the Tables repository

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a fixed salt for admin password hashing to ensure consistent password hashes across deployments. The change introduces a new system property OPENOPS_ADMIN_PASSWORD_SALT and creates dedicated admin user creation/password update methods that use bcrypt with this static salt instead of the standard password hasher.

Key changes:

  • Added OPENOPS_ADMIN_PASSWORD_SALT system property with a default bcrypt salt value
  • Created createAdminUser() and updateAdminPassword() methods that use bcrypt with a fixed salt
  • Removed OpenOps Tables authentication synchronization logic from admin seeding

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
packages/server/shared/src/lib/system/system-prop.ts Added new system property enum for admin password salt
packages/server/shared/src/lib/system/system.ts Defined default bcrypt salt value for admin password hashing
packages/server/api/src/app/user/user-service.ts Implemented dedicated admin user methods using bcrypt with static salt
packages/server/api/src/app/database/seeds/seed-admin.ts Updated admin seeding to use new admin-specific methods and removed external authentication sync

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 5, 2025

Greptile Overview

Greptile Summary

This PR implements a fixed salt for admin password hashing to maintain password consistency across instances, particularly for authentication with OpenOps Tables. The changes introduce dedicated createAdminUser and updateAdminPassword methods that use a static salt from OPENOPS_ADMIN_PASSWORD_SALT, and remove the previous password synchronization logic with OpenOps Tables.

Key Changes:

  • Added OPENOPS_ADMIN_PASSWORD_SALT system property with hardcoded default value $2b$10$6zuoB5d8Dz9bzV91gpuynO
  • Created createAdminUser() and updateAdminPassword() methods using bcrypt with static salt
  • Removed calls to authenticateUserInOpenOpsTables() and resetUserPassword() from seeding flow
  • Refactored create() method to extract common saveUser() logic

Critical Security Concern:
The hardcoded salt in system.ts:103 presents a significant security vulnerability. Using a static salt that's publicly visible in the codebase allows attackers to:

  1. Create rainbow tables specific to this salt
  2. Perform offline brute-force attacks on any leaked admin password hashes
  3. Attack all OpenOps instances simultaneously since they share the same salt

The purpose of salting is defeated when the salt is static and known - bcrypt's strength comes from unique per-password salts.

Confidence Score: 1/5

  • This PR introduces a critical security vulnerability with the hardcoded salt
  • Score reflects the serious security implications of using a hardcoded, publicly visible salt for admin password hashing. While the code changes are technically correct and the refactoring is clean, the security model is fundamentally flawed. This makes all admin passwords across all OpenOps instances vulnerable to pre-computed attacks.
  • Critical attention needed on packages/server/shared/src/lib/system/system.ts (hardcoded salt) and packages/server/api/src/app/user/user-service.ts (static salt usage)

Important Files Changed

File Analysis

Filename Score Overview
packages/server/shared/src/lib/system/system.ts 2/5 Added hardcoded salt value for admin password hashing - security concern with the publicly visible fixed salt
packages/server/shared/src/lib/system/system-prop.ts 5/5 Added new system property OPENOPS_ADMIN_PASSWORD_SALT for configurable salt
packages/server/api/src/app/user/user-service.ts 3/5 Introduced separate createAdminUser and updateAdminPassword methods using fixed salt, refactored user creation logic
packages/server/api/src/app/database/seeds/seed-admin.ts 4/5 Removed OpenOps Tables password synchronization calls, simplified admin user creation flow

Sequence Diagram

sequenceDiagram
    participant Seed as seed-admin
    participant UserService as user-service
    participant DB as Database
    participant System as system config
    participant Bcrypt as bcrypt

    Note over Seed: upsertAdminUser()
    
    Seed->>UserService: getByOrganizationAndEmail(email)
    UserService->>DB: Query user by email
    
    alt User exists
        Seed->>UserService: updateAdminPassword(id, newPassword)
        UserService->>System: getStaticSalt()
        System-->>UserService: $2b$10$6zuoB5d8Dz9bzV91gpuynO
        UserService->>Bcrypt: hash(password, staticSalt)
        Bcrypt-->>UserService: hashedPassword
        UserService->>DB: Update user password
    else User doesn't exist
        Seed->>UserService: createAdminUser(email, password, ...)
        UserService->>System: getStaticSalt()
        System-->>UserService: $2b$10$6zuoB5d8Dz9bzV91gpuynO
        UserService->>Bcrypt: hash(password, staticSalt)
        Bcrypt-->>UserService: hashedPassword
        UserService->>UserService: saveUser(newUser)
        UserService->>DB: Save new admin user
    end
    
    Note over Seed,DB: Password now consistently hashed<br/>with fixed salt for Tables auth
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

@sonarqubecloud
Copy link

@MarceloRGonc MarceloRGonc merged commit f23228e into main Dec 15, 2025
24 checks passed
@MarceloRGonc MarceloRGonc deleted the mg/OPS-3201 branch December 15, 2025 17:21
maor-rozenfeld pushed a commit that referenced this pull request Dec 15, 2025
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.

3 participants