fix: the encryption utility uses hashlib in encryption.py#9210
Open
orbisai0security wants to merge 3 commits into
Open
fix: the encryption utility uses hashlib in encryption.py#9210orbisai0security wants to merge 3 commits into
orbisai0security wants to merge 3 commits into
Conversation
Automated security fix generated by OrbisAI Security
The encryption utility uses hashlib
orbisai0security
requested review from
dheeru0198 and
pablohashescobar
as code owners
Contributor
📝 WalkthroughWalkthroughThe PR implements encryption key rotation while maintaining backwards compatibility. The salt derivation strategy changes from a fixed constant to per-secret hashing, a legacy decryption fallback path is added with status reporting, instance configuration values are transparently re-encrypted during reads if detected as legacy, and comprehensive tests validate correctness, key isolation, non-determinism, and fallback behavior. ChangesEncryption salt rotation with legacy fallback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/license/utils/encryption.py`:
- Around line 16-17: The KDF salt change breaks decryption of existing values;
update the decrypt flow to first attempt using the new salt (current
hashlib.sha256(secret_key.encode()).digest() and dk derived via pbkdf2_hmac),
and on decryption failure attempt a legacy derivation using the old salt
algorithm/constant (create a legacy_salt + legacy dk) to decrypt; if legacy
decryption succeeds, re-encrypt the plaintext immediately with the new derived
key and persist the updated ciphertext (so stored values are migrated), and
ensure failures are logged (not collapsed to empty string) and surfaced; update
the functions/locations that compute salt/dk and perform decrypt/re-encrypt to
include this fallback and migration logic, and add a config flag or
settings.LEGACY_SALT placeholder for the legacy derivation to make the code
explicit.
In `@tests/test_invariant_encryption.py`:
- Around line 47-52: The test currently allows deterministic encryption to pass
by only issuing a warning; replace that behavior with a hard failure or explicit
xfail: when len(ciphertexts) == 1, either call pytest.fail("deterministic
encryption detected") or pytest.xfail("deterministic encryption detected")
instead of pytest.warns, so the branch no longer silently passes; update the
block around ciphertexts, decrypt, secret_key, and plaintext accordingly to
assert non-determinism strictly.
- Around line 8-64: Tests call encrypt/decrypt but the production API is
encrypt_data/decrypt_data which derives keys from settings.SECRET_KEY; update
the tests to import encrypt_data and decrypt_data instead of encrypt/decrypt,
remove passing secret_key arguments (call encrypt_data(plaintext) and
decrypt_data(ciphertext)), and ensure tests set or monkeypatch
settings.SECRET_KEY (or use the existing test settings fixture) so key
derivation uses the intended secret; also update any referenced function names
(e.g., ct1/ct2 generation, ciphertexts set, decrypt calls) to use the new
function names to exercise the real API.
- Line 6: The sys.path.insert call is adding "apps/api" relative to the tests
directory which points to a non-existent test-local path; update the sys.path
entry used in tests/test_invariant_encryption.py (the sys.path.insert and
os.path.join usage) to resolve to the repository-level apps/api by joining up
one directory from the tests folder (e.g., using os.path.dirname(__file__) then
".." to reach repo root) and wrap with os.path.abspath to normalize the path so
imports reliably find the real repository apps/api.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d47e31ca-97cc-4bb8-95ca-94a921606bfe
📒 Files selected for processing (2)
apps/api/plane/license/utils/encryption.pytests/test_invariant_encryption.py
Comment on lines
+16
to
+17
| salt = hashlib.sha256(secret_key.encode()).digest() | ||
| dk = hashlib.pbkdf2_hmac("sha256", secret_key.encode(), salt, 100000) |
Contributor
There was a problem hiding this comment.
Add backward decryption compatibility before switching the KDF salt.
This changes the derived key for the same settings.SECRET_KEY, so previously stored encrypted configuration values can no longer be decrypted. With current error handling, decrypt failures collapse to "", causing silent config breakage at read time. Please add a legacy-decrypt fallback (old salt) plus migration/re-encryption flow before rollout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/plane/license/utils/encryption.py` around lines 16 - 17, The KDF
salt change breaks decryption of existing values; update the decrypt flow to
first attempt using the new salt (current
hashlib.sha256(secret_key.encode()).digest() and dk derived via pbkdf2_hmac),
and on decryption failure attempt a legacy derivation using the old salt
algorithm/constant (create a legacy_salt + legacy dk) to decrypt; if legacy
decryption succeeds, re-encrypt the plaintext immediately with the new derived
key and persist the updated ciphertext (so stored values are migrated), and
ensure failures are logged (not collapsed to empty string) and surfaced; update
the functions/locations that compute salt/dk and perform decrypt/re-encrypt to
include this fallback and migration logic, and add a config flag or
settings.LEGACY_SALT placeholder for the legacy derivation to make the code
explicit.
There was a problem hiding this comment.
@orbisai0security can you address code review comments?
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
+8
to
+64
| from plane.license.utils.encryption import encrypt, decrypt | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("secret_key", [ | ||
| "secret1", | ||
| "secret2", | ||
| "a-]different-key!@#$", | ||
| ]) | ||
| def test_different_keys_produce_different_ciphertexts(secret_key): | ||
| """Invariant: Different secret keys must produce different encrypted outputs | ||
| for the same plaintext, ensuring the key derivation provides uniqueness. | ||
| A hardcoded salt with identical keys would produce identical ciphertexts.""" | ||
| plaintext = "sensitive_data_12345" | ||
| other_key = secret_key + "_different" | ||
|
|
||
| ct1 = encrypt(plaintext, secret_key) | ||
| ct2 = encrypt(plaintext, other_key) | ||
|
|
||
| # Different keys must always produce different ciphertexts | ||
| assert ct1 != ct2, ( | ||
| "Different secret keys produced identical ciphertexts — " | ||
| "key derivation is not properly differentiating keys" | ||
| ) | ||
|
|
||
|
|
||
| def test_same_key_same_plaintext_should_not_always_produce_same_ciphertext(): | ||
| """Invariant: Encrypting the same plaintext with the same key multiple times | ||
| should ideally produce different ciphertexts (due to random IV/nonce). | ||
| If a hardcoded salt AND no random IV are used, ciphertexts will be identical, | ||
| enabling frequency analysis attacks.""" | ||
| secret_key = "my_secret_key" | ||
| plaintext = "repeated_sensitive_value" | ||
|
|
||
| ciphertexts = {encrypt(plaintext, secret_key) for _ in range(5)} | ||
|
|
||
| # If all 5 encryptions produce the same ciphertext, the scheme is deterministic | ||
| # which is a security weakness (no random IV/nonce) | ||
| # Note: This test documents the expected security property even if current | ||
| # implementation fails it | ||
| if len(ciphertexts) == 1: | ||
| pytest.warns(UserWarning, match="deterministic encryption detected") | ||
| # At minimum, verify decrypt still works correctly | ||
| for ct in ciphertexts: | ||
| assert decrypt(ct, secret_key) == plaintext | ||
|
|
||
|
|
||
| def test_decrypt_roundtrip_integrity(): | ||
| """Invariant: Encryption/decryption roundtrip must preserve data integrity.""" | ||
| secret_key = "test_key_for_roundtrip" | ||
| payloads = [ | ||
| "normal_value", | ||
| "", # boundary: empty string | ||
| "a" * 10000, # boundary: large input | ||
| ] | ||
| for plaintext in payloads: | ||
| encrypted = encrypt(plaintext, secret_key) | ||
| decrypted = decrypt(encrypted, secret_key) |
Contributor
There was a problem hiding this comment.
Align tests with the actual encryption utility API contract.
This test imports and calls encrypt/decrypt with a secret_key argument, but the reviewed module exposes encrypt_data/decrypt_data and derives key material from settings.SECRET_KEY. As written, these tests do not exercise the production API and are likely to fail at import/call time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_invariant_encryption.py` around lines 8 - 64, Tests call
encrypt/decrypt but the production API is encrypt_data/decrypt_data which
derives keys from settings.SECRET_KEY; update the tests to import encrypt_data
and decrypt_data instead of encrypt/decrypt, remove passing secret_key arguments
(call encrypt_data(plaintext) and decrypt_data(ciphertext)), and ensure tests
set or monkeypatch settings.SECRET_KEY (or use the existing test settings
fixture) so key derivation uses the intended secret; also update any referenced
function names (e.g., ct1/ct2 generation, ciphertexts set, decrypt calls) to use
the new function names to exercise the real API.
Comment on lines
+47
to
+52
| if len(ciphertexts) == 1: | ||
| pytest.warns(UserWarning, match="deterministic encryption detected") | ||
| # At minimum, verify decrypt still works correctly | ||
| for ct in ciphertexts: | ||
| assert decrypt(ct, secret_key) == plaintext | ||
|
|
Contributor
There was a problem hiding this comment.
Enforce the non-determinism invariant instead of allowing a warning-only pass.
When deterministic output is observed, the test currently still passes. That leaves the security invariant unguarded and can hide regressions. This branch should fail (or be strict xfail) when len(ciphertexts) == 1.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_invariant_encryption.py` around lines 47 - 52, The test currently
allows deterministic encryption to pass by only issuing a warning; replace that
behavior with a hard failure or explicit xfail: when len(ciphertexts) == 1,
either call pytest.fail("deterministic encryption detected") or
pytest.xfail("deterministic encryption detected") instead of pytest.warns, so
the branch no longer silently passes; update the block around ciphertexts,
decrypt, secret_key, and plaintext accordingly to assert non-determinism
strictly.
Member
Code reviewFound 2 issues:
plane/apps/api/plane/license/utils/encryption.py Lines 13 to 18 in 2484c4c
plane/tests/test_invariant_encryption.py Lines 46 to 51 in 2484c4c 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Address PR makeplane#9210 code review comments: - Add legacy key derivation fallback in decrypt_data so previously encrypted values (using old hardcoded salt) remain decryptable - Add lazy re-encryption in instance_value.py to migrate legacy values on read - Relocate tests to proper Django test tree with correct API usage (encrypt_data/decrypt_data with override_settings) - Enforce non-determinism invariant with hard assertion Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/api/plane/license/utils/encryption.py (1)
16-19:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLine 18 still derives the PBKDF2 salt from the same secret being stretched.
sha256(secret_key)is still a deterministic function ofsecret_key, so an attacker can precomputePBKDF2(candidate, sha256(candidate))just as readily asPBKDF2(candidate, b"salt"). That means this change does not actually introduce an independent salt, so the PR still falls short of the stated mitigation.For PBKDF2-HMAC, does deriving the salt from the password/secret itself (for example `salt = SHA-256(secret_key)`) provide the same protection against precomputation or rainbow-table attacks as an independent random salt? Please cite primary cryptography guidance or standards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/license/utils/encryption.py` around lines 16 - 19, derive_key currently computes the PBKDF2 salt as sha256(secret_key) which is still dependent on the secret and does not provide an independent salt; change derive_key to either (A) generate a true random salt (e.g. secrets.token_bytes or os.urandom, 16+ bytes) and return both (salt, dk) so callers can persist the salt with the stored key, or (B) if you need a deterministic deployment-specific salt, derive the salt using an independent deployment secret/pepper (read from config/env) and compute salt = HMAC-SHA256(deployment_secret, b"pbkdf2-salt-context") (not by hashing secret_key) before calling hashlib.pbkdf2_hmac; update references to derive_key to handle the returned salt when storing/validating keys and ensure function name derive_key and locals salt and dk are adjusted accordingly.
🧹 Nitpick comments (1)
apps/api/plane/tests/unit/utils/test_encryption.py (1)
57-81: ⚡ Quick winPlease add one test for the
instance_value.pymigration path itself.These tests stop at
decrypt_data_with_status(); they never assert thatget_configuration_value()re-encrypts a legacyInstanceConfigurationvalue and persists the upgraded ciphertext. That leaves the new write-on-read contract unguarded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/tests/unit/utils/test_encryption.py` around lines 57 - 81, Add a unit test that exercises the instance_value migration path by creating an InstanceConfiguration row containing a legacy Fernet ciphertext (use derive_key_legacy + Fernet to produce legacy_ct) and then calling get_configuration_value(...) to read it; after the call assert the returned plaintext equals the original, then reload the InstanceConfiguration from the DB and assert its stored value is no longer the legacy ciphertext (i.e., has been re-encrypted with the current key) and that decrypt_data_with_status(persisted_value) reports used_legacy False, ensuring get_configuration_value performs write-on-read re-encryption and persists the upgraded ciphertext.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/license/utils/instance_value.py`:
- Around line 27-33: The lazy migration should only overwrite the exact row and
only with a successful new ciphertext: capture the row identifier and original
encrypted blob (use item.get("id") or the model PK and item.get("value")), call
decrypt_data_with_status(item.get("value")) and if used_legacy and plaintext
produce new_cipher = encrypt_data(plaintext) and verify new_cipher is non-empty,
then perform a conditional update on InstanceConfiguration using both the PK (or
id) and the original encrypted value (e.g.
InstanceConfiguration.objects.filter(pk=pk,
value=original_encrypted).update(value=new_cipher)) to avoid clobbering
concurrent changes; wrap encrypt/update in try/except to handle exceptions and
skip update on failure.
---
Duplicate comments:
In `@apps/api/plane/license/utils/encryption.py`:
- Around line 16-19: derive_key currently computes the PBKDF2 salt as
sha256(secret_key) which is still dependent on the secret and does not provide
an independent salt; change derive_key to either (A) generate a true random salt
(e.g. secrets.token_bytes or os.urandom, 16+ bytes) and return both (salt, dk)
so callers can persist the salt with the stored key, or (B) if you need a
deterministic deployment-specific salt, derive the salt using an independent
deployment secret/pepper (read from config/env) and compute salt =
HMAC-SHA256(deployment_secret, b"pbkdf2-salt-context") (not by hashing
secret_key) before calling hashlib.pbkdf2_hmac; update references to derive_key
to handle the returned salt when storing/validating keys and ensure function
name derive_key and locals salt and dk are adjusted accordingly.
---
Nitpick comments:
In `@apps/api/plane/tests/unit/utils/test_encryption.py`:
- Around line 57-81: Add a unit test that exercises the instance_value migration
path by creating an InstanceConfiguration row containing a legacy Fernet
ciphertext (use derive_key_legacy + Fernet to produce legacy_ct) and then
calling get_configuration_value(...) to read it; after the call assert the
returned plaintext equals the original, then reload the InstanceConfiguration
from the DB and assert its stored value is no longer the legacy ciphertext
(i.e., has been re-encrypted with the current key) and that
decrypt_data_with_status(persisted_value) reports used_legacy False, ensuring
get_configuration_value performs write-on-read re-encryption and persists the
upgraded ciphertext.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a4425be-d95f-473e-bdcb-cda80e7dc63f
📒 Files selected for processing (3)
apps/api/plane/license/utils/encryption.pyapps/api/plane/license/utils/instance_value.pyapps/api/plane/tests/unit/utils/test_encryption.py
Comment on lines
+27
to
+33
| plaintext, used_legacy = decrypt_data_with_status(item.get("value")) | ||
| if used_legacy and plaintext: | ||
| try: | ||
| InstanceConfiguration.objects.filter( | ||
| key=item.get("key") | ||
| ).update(value=encrypt_data(plaintext)) | ||
| except Exception: |
Contributor
There was a problem hiding this comment.
Make the lazy migration write conditional on the row you actually read.
Lines 30-32 do a blind update() by key only and inline encrypt_data(plaintext). If another request changes that config between the read at Line 21 and this write, this migration can clobber the newer value; if encrypt_data() fails and returns "", the same path will persist an empty secret.
Suggested fix
- if used_legacy and plaintext:
+ if used_legacy and plaintext:
try:
- InstanceConfiguration.objects.filter(
- key=item.get("key")
- ).update(value=encrypt_data(plaintext))
+ reencrypted_value = encrypt_data(plaintext)
+ if reencrypted_value:
+ InstanceConfiguration.objects.filter(
+ key=item.get("key"),
+ value=item.get("value"),
+ is_encrypted=True,
+ ).update(value=reencrypted_value)
except Exception:
pass🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/plane/license/utils/instance_value.py` around lines 27 - 33, The
lazy migration should only overwrite the exact row and only with a successful
new ciphertext: capture the row identifier and original encrypted blob (use
item.get("id") or the model PK and item.get("value")), call
decrypt_data_with_status(item.get("value")) and if used_legacy and plaintext
produce new_cipher = encrypt_data(plaintext) and verify new_cipher is non-empty,
then perform a conditional update on InstanceConfiguration using both the PK (or
id) and the original encrypted value (e.g.
InstanceConfiguration.objects.filter(pk=pk,
value=original_encrypted).update(value=new_cipher)) to avoid clobbering
concurrent changes; wrap encrypt/update in try/except to handle exceptions and
skip update on failure.
There was a problem hiding this comment.
@orbisai0security can you address code review comments?
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix high severity security issue in
apps/api/plane/license/utils/encryption.py.Vulnerability
V-001apps/api/plane/license/utils/encryption.py:15Description: The encryption utility uses hashlib.pbkdf2_hmac with a hardcoded, trivial salt value (b'salt'). This eliminates the fundamental purpose of salting in key derivation, which is to prevent precomputation attacks and ensure that identical passwords produce different derived keys. Since this is an open-source project, the hardcoded salt is publicly known, allowing attackers to precompute rainbow tables that work against all encrypted values in any Plane deployment.
Evidence
Exploitation scenario: An attacker who obtains encrypted data from the database (e.g., through SQL injection in another component, backup exposure, or insider access) can use the known hardcoded salt b'salt' to precompute.
Scanner confirmation: multi_agent_ai rule
V-001flagged this pattern.Production code: This file is in the production codebase, not test-only code.
Threat Model Context
This API endpoint appears to be publicly accessible. This is a Node.js library - vulnerabilities affect downstream consumers who use this package.
Changes
apps/api/plane/license/utils/encryption.pyVerification
Security Invariant
Regression test
This test guards against regressions — it's useful independent of the code change above.
Automated security fix by OrbisAI Security
Summary by CodeRabbit
New Features
Bug Fixes
Tests