fix(re-encrypt): handle non-id PKs and make command idempotent#40079
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40079 +/- ##
==========================================
+ Coverage 64.08% 64.12% +0.04%
==========================================
Files 2590 2590
Lines 137982 138011 +29
Branches 32008 32012 +4
==========================================
+ Hits 88419 88499 +80
+ Misses 48045 47993 -52
- Partials 1518 1519 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if previous_secret_key is None: | ||
| click.secho("A previous secret key must be provided", err=True) | ||
| sys.exit(1) | ||
| secrets_migrator = SecretsMigrator(previous_secret_key=previous_secret_key) | ||
| try: | ||
| secrets_migrator.run() | ||
| except ValueError as exc: | ||
| click.secho( | ||
| f"An error occurred, " | ||
| f"probably an invalid previous secret key was provided. Error:[{exc}]", | ||
| err=True, | ||
| "No previous secret key provided; nothing to re-encrypt.", | ||
| fg="yellow", | ||
| ) | ||
| return |
There was a problem hiding this comment.
Suggestion: The missing-key guard only checks for None, so an empty PREVIOUS_SECRET_KEY (for example an empty env var) is treated as a valid key and the migrator runs with "", causing avoidable decryption failures and exit 1. Treat blank/falsy values as missing in this branch so the command stays idempotent and no-op when no usable previous key is provided. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ CLI `re_encrypt_secrets` fails when PREVIOUS_SECRET_KEY is empty.
- ⚠️ Rotation scripts misbehave if env var left blank.Steps of Reproduction ✅
1. Configure Superset with `PREVIOUS_SECRET_KEY` set to an empty string (e.g., via
environment) so `current_app.config["PREVIOUS_SECRET_KEY"] == ""` when
`re_encrypt_secrets()` in `superset/cli/update.py:113-133` is invoked without the
`--previous_secret_key` option.
2. In `re_encrypt_secrets`, line 114 assigns `previous_secret_key = previous_secret_key or
current_app.config.get("PREVIOUS_SECRET_KEY")`; with no CLI argument and an empty config
value this expression yields `""`, not `None`, so the guard `if previous_secret_key is
None:` at line 117 is bypassed and the early-return path is not taken.
3. The function constructs `SecretsMigrator(previous_secret_key=previous_secret_key)` at
line 123, passing the empty string into `SecretsMigrator.__init__`
(`superset/utils/encrypt.py:103-110`), and then calls `stats = secrets_migrator.run()`
within the transaction.
4. Inside `SecretsMigrator.run()` and `_re_encrypt_row()`
(`superset/utils/encrypt.py:180-259`), existing ciphertexts are unreadable under both the
current key and the empty previous key, so `stats.failed` is incremented and `run()`
raises at lines 59-63, causing `re_encrypt_secrets` to hit the `except` at lines 126-128
and exit with status 1 instead of the intended "nothing to re-encrypt" no-op for
missing/blank previous keys.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/cli/update.py
**Line:** 117:122
**Comment:**
*Incorrect Condition Logic: The missing-key guard only checks for `None`, so an empty `PREVIOUS_SECRET_KEY` (for example an empty env var) is treated as a valid key and the migrator runs with `""`, causing avoidable decryption failures and exit 1. Treat blank/falsy values as missing in this branch so the command stays idempotent and no-op when no usable previous key is provided.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| secrets_migrator = SecretsMigrator(previous_secret_key=previous_secret_key) | ||
| try: | ||
| stats = secrets_migrator.run() | ||
| except Exception as exc: # pylint: disable=broad-except | ||
| click.secho(f"Re-encryption failed: {exc}", err=True) |
There was a problem hiding this comment.
Suggestion: Exception handling starts after SecretsMigrator construction, so any failure during initialization (for example DB/engine access issues) bypasses the new failure path and leaks as an uncaught exception instead of the intended deterministic CLI error/exit behavior. Include migrator creation inside the same try block. [logic error]
Severity Level: Major ⚠️
- ⚠️ Initialization errors surface as uncaught tracebacks in CLI.
- ⚠️ DevOps scripts cannot rely on uniform failure message.Steps of Reproduction ✅
1. Misconfigure the database/engine so that accessing
`superset.db.engine.url.get_dialect()` in `SecretsMigrator.__init__`
(`superset/utils/encrypt.py:103-110`) raises an exception when the CLI command is run.
2. Invoke the `re_encrypt_secrets` Click command defined in
`superset/cli/update.py:113-133` (e.g., via `superset re-encrypt-secrets`), which
evaluates `secrets_migrator = SecretsMigrator(previous_secret_key=previous_secret_key)` at
line 123.
3. The misconfiguration causes `SecretsMigrator.__init__` to raise before returning, but
this happens prior to the `try:` block that starts at line 124, so the broad `except
Exception as exc` at line 126 does not execute.
4. The initialization exception propagates out of `re_encrypt_secrets` as an uncaught
error, emitting a raw traceback instead of the intended deterministic "Re-encryption
failed: ..." message and controlled exit status used for failures arising inside
`secrets_migrator.run()`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/cli/update.py
**Line:** 123:127
**Comment:**
*Logic Error: Exception handling starts after `SecretsMigrator` construction, so any failure during initialization (for example DB/engine access issues) bypasses the new failure path and leaks as an uncaught exception instead of the intended deterministic CLI error/exit behavior. Include migrator creation inside the same `try` block.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #591658
Actionable Suggestions - 3
-
tests/integration_tests/utils/encrypt_tests.py - 1
- Incorrect test logic for re-encryption · Line 205-242
-
superset/utils/encrypt.py - 2
- Blind exception catch too broad · Line 245-245
- Avoid raising vanilla Exception class · Line 319-319
Review Details
-
Files reviewed - 4 · Commit Range:
bee410c..bee410c- superset/cli/update.py
- superset/utils/encrypt.py
- tests/integration_tests/cli_tests.py
- tests/integration_tests/utils/encrypt_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| def test_re_encrypt_row_uses_pk_columns(self): | ||
| """ | ||
| Verify SecretsMigrator builds UPDATE statements targeting the table's | ||
| actual primary key columns rather than a hardcoded `id` column. | ||
| Regression guard for tables like `semantic_layers` whose PK is `uuid`. | ||
| """ | ||
| from unittest.mock import MagicMock | ||
|
|
||
| from sqlalchemy.engine import make_url | ||
|
|
||
| dialect = make_url("sqlite://").get_dialect() | ||
| migrator = SecretsMigrator(self.app.config["SECRET_KEY"]) | ||
| migrator._dialect = dialect # noqa: SLF001 | ||
|
|
||
| field = encrypted_field_factory.create(String(1024)) | ||
| ciphertext = field.process_bind_param("hunter2", dialect) | ||
|
|
||
| conn = MagicMock() | ||
| row = {"uuid": b"\x00" * 16, "configuration": ciphertext} | ||
| stats = ReEncryptStats() | ||
|
|
||
| migrator._re_encrypt_row( # noqa: SLF001 | ||
| conn, | ||
| row, | ||
| "semantic_layers", | ||
| {"configuration": field}, | ||
| ["uuid"], | ||
| stats, | ||
| ) | ||
|
|
||
| assert conn.execute.call_count == 1 | ||
| stmt = str(conn.execute.call_args.args[0]) | ||
| assert "WHERE uuid = :_pk_uuid" in stmt | ||
| assert "id" not in stmt.split("WHERE", 1)[1] | ||
| kwargs = conn.execute.call_args.kwargs | ||
| assert kwargs["_pk_uuid"] == row["uuid"] | ||
| assert "configuration" in kwargs | ||
| assert stats == ReEncryptStats(re_encrypted=1, skipped=0, failed=0) |
There was a problem hiding this comment.
The test sets previous_secret_key equal to the current SECRET_KEY and encrypts data with the current key, so the method correctly skips re-encryption since the current key can decrypt. However, the test asserts re-encryption occurred. To properly test UPDATE statement building with PK columns, use a distinct previous key and encrypt the data with it.
Code Review Run #591658
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| return | ||
| except Exception: | ||
| raise Exception from ex # pylint: disable=broad-exception-raised | ||
| except Exception as prev_ex: # pylint: disable=broad-except |
There was a problem hiding this comment.
Avoid catching bare Exception. Catch specific exception types like ValueError or InvalidToken to handle only expected errors.
Code suggestion
Check the AI-generated fix before applying
| except Exception as prev_ex: # pylint: disable=broad-except | |
| except (ValueError, Exception) as prev_ex: # pylint: disable=broad-except |
Code Review Run #591658
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| stats.failed, | ||
| ) | ||
| if stats.failed: | ||
| raise Exception( # pylint: disable=broad-exception-raised |
There was a problem hiding this comment.
Create a custom exception class instead of raising a generic Exception. Define a specific exception type like ReEncryptionError for better error handling.
Code suggestion
Check the AI-generated fix before applying
from flask import Flask
+
+
+class ReEncryptionError(Exception):
+ """Exception raised when re-encryption fails."""
@@ -318,8 +322,8 @@
if stats.failed:
- raise Exception( # pylint: disable=broad-exception-raised
- f"Re-encryption failed for {stats.failed} value(s); "
- "transaction rolled back"
- )
+ error_msg = f"Re-encryption failed for {stats.failed} value(s); transaction rolled back"
+ raise ReEncryptionError(error_msg)
Code Review Run #591658
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Vitor-Avila
left a comment
Vitor-Avila
left a comment
There was a problem hiding this comment.
Haven't manually tested, but overall LGTM! Thanks for all the improvements to this flow
|
Bito Automatic Review Skipped – PR Already Merged |
2 participants
SUMMARY
Fixes several issues in superset re-encrypt-secrets that surfaced with the addition of the semantic_layers table (PK =
uuid, with an encrypted configuration column):SELECT id, ... FROM <table> and WHERE id = :id. It now derives the actual primary key column(s) from the Table metadata, so tables likesemantic_layers(whose PK isuuid) can be rotated without error.NULLvalues are counted separately from "already current" skips so operators can tell them apart. On failure, the transaction rolls back and the count of failed values is surfaced.PREVIOUS_SECRET_KEY→ exit 0 with a yellow "nothing to re-encrypt" notice (was exit 1). This lets scheduled rotation scripts run unconditionally after a rotation is complete without starting to fail.BEFORE
AFTER
Successful rotation:
No-op (SECRET_KEY already decrypts values successfully):
Invalid secrets (neither
SECRET_KEYnorPREVIOUS_SECRET_KEYdecrypts current values):ADDITIONAL INFORMATION