Skip to content

Security: Database credentials stored in plaintext#2056

Open
tuanaiseo wants to merge 2 commits intohyperdxio:mainfrom
tuanaiseo:contribai/fix/security/database-credentials-stored-in-plaintext
Open

Security: Database credentials stored in plaintext#2056
tuanaiseo wants to merge 2 commits intohyperdxio:mainfrom
tuanaiseo:contribai/fix/security/database-credentials-stored-in-plaintext

Conversation

@tuanaiseo
Copy link
Copy Markdown

Problem

The Connection model stores password as a plain string. select: false only hides it from default queries, but does not protect data at rest. A database leak or privileged read would expose raw credentials.

Severity: high
File: packages/api/src/models/connection.ts

Solution

Encrypt password before persistence using an authenticated encryption scheme (e.g., AES-GCM via KMS-managed key), decrypt only when needed, and rotate existing stored secrets.

Changes

  • packages/api/src/models/connection.ts (modified)
  • packages/api/src/controllers/connection.ts (modified)

Testing

  • Existing tests pass
  • Manual review completed
  • No new warnings/errors introduced

The `Connection` model stores `password` as a plain string. `select: false` only hides it from default queries, but does not protect data at rest. A database leak or privileged read would expose raw credentials.

Affected files: connection.ts, connection.ts

Signed-off-by: tuanaiseo <221258316+tuanaiseo@users.noreply.github.com>
The `Connection` model stores `password` as a plain string. `select: false` only hides it from default queries, but does not protect data at rest. A database leak or privileged read would expose raw credentials.

Affected files: connection.ts, connection.ts

Signed-off-by: tuanaiseo <221258316+tuanaiseo@users.noreply.github.com>
@vercel
Copy link
Copy Markdown

vercel bot commented Apr 3, 2026

@tuanaiseo is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 3, 2026

⚠️ No Changeset found

Latest commit: 76765f8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

PR Review

  • CONNECTION_PASSWORD_ENCRYPTION_KEY missing from all .env.* files → This is a required env var that throws on any connection operation if unset; will break existing deployments on upgrade. Add it to .env.development, .env.test, and .env.e2e with a default dev/test value.

  • No migration for existing plaintext passwords → Existing stored passwords remain unencrypted at rest (the decryptConnectionPassword fallback silently returns plaintext). A one-time migration script or startup migration is needed to actually secure existing data.

  • ⚠️ Dual encryption path is confusing and fragilecreateConnection explicitly encrypts before calling Connection.create(), which also fires the pre-save hook that calls encryptConnectionPassword() again. This only works correctly due to the idempotency prefix check. Meanwhile updateConnection uses findOneAndUpdate (bypasses hooks), so the hook adds no value for updates. The pre-save hook should be removed and encryption kept solely in the controller, or vice versa — not both.

  • ⚠️ No tests for crypto functionsencryptConnectionPassword/decryptConnectionPassword have no unit tests. At minimum, roundtrip and idempotency tests should be added given this is security-critical code.

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.

1 participant