codeant-ai-for-open-source[bot] commented on code in PR #40079:
URL: https://github.com/apache/superset/pull/40079#discussion_r3230488439
##########
superset/cli/update.py:
##########
@@ -115,15 +115,19 @@ def re_encrypt_secrets(previous_secret_key: Optional[str]
= None) -> None:
"PREVIOUS_SECRET_KEY"
)
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
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ CLI `re_encrypt_secrets` fails when PREVIOUS_SECRET_KEY is empty.
- ⚠️ Rotation scripts misbehave if env var left blank.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fcli%2Fupdate.py%0A%2A%2ALine%3A%2A%2A%20117%3A122%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncorrect%20Condition%20Logic%3A%20The%20missing-key%20guard%20only%20checks%20for%20%60None%60%2C%20so%20an%20empty%20%60PREVIOUS_SECRET_KEY%60%20%28for%20example%20an%20empty%20env%20var%29%20is%20treated%20as%20a%20valid%20key%20and%20the%20migrator%20runs%20with%20%60%22%22%60%2C%20causing%20avoidable%20decryption%20failures%20and%20exit%201.%20Treat%20blank%2Ffalsy%20values%20as%20missing%20in%20this%20branch%20so%20the%20command%20stays%20idempotent%20and%20no-op%20when%20no%20usable%20previous%20key%20is%20provided.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%2
0fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fcli%2Fupdate.py%0A%2A%2ALine%3A%2A%2A%20117%3A122%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncorrect%20Condition%20Logic%3A%20The%20missing-key%20guard%20only%20checks%20for%20%60None%60%2C%20so%20an%20empty%20%60PREVIOUS_SECRET_KEY%60%20%28for%20example%20an%20empty%20env%20var%29%20is%20treated%20as%20a%20valid%20key%20and%20the%20migrator%20runs%20with%20%60%22%22%60%2C%20causing%20avoidable%20decryption%20failures%20and%20exit%201.%20Treat%20blank%2Ffalsy%20values%20as%20missing%20in%20thi
s%20branch%20so%20the%20command%20stays%20idempotent%20and%20no-op%20when%20no%20usable%20previous%20key%20is%20provided.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40079&comment_hash=ed7f83109fad10921c0ba64c53d2194bf35fd2b6efca2291025cde7d5df84e18&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40079&comment_hash=ed7f83109fad10921c0ba64c53d2194bf35fd2b6efca2291025cde7d5df84e18&reaction=dislike'>👎</a>
##########
superset/cli/update.py:
##########
@@ -115,15 +115,19 @@ def re_encrypt_secrets(previous_secret_key: Optional[str]
= None) -> None:
"PREVIOUS_SECRET_KEY"
)
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
+ 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)
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Initialization errors surface as uncaught tracebacks in CLI.
- ⚠️ DevOps scripts cannot rely on uniform failure message.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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()`.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fcli%2Fupdate.py%0A%2A%2ALine%3A%2A%2A%20123%3A127%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Exception%20handling%20starts%20after%20%60SecretsMigrator%60%20construction%2C%20so%20any%20failure%20during%20initialization%20%28for%20example%20DB%2Fengine%20access%20issues%29%20bypasses%20the%20new%20failure%20path%20and%20leaks%20as%20an%20uncaught%20exception%20instead%20of%20the%20intended%20deterministic%20CLI%20error%2Fexit%20behavior.%20Include%20migrator%20creation%20inside%20the%20same%20%60try%60%20block.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C
%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fcli%2Fupdate.py%0A%2A%2ALine%3A%2A%2A%20123%3A127%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Exception%20handling%20starts%20after%20%60SecretsMigrator%60%20construction%2C%20so%20any%20failure%20during%20initialization%20%28for%20example%20DB%2Fengine%20access%20issues%29%20bypasses%20the%20new%20failure%20path%20and%20leaks%20as%20an%20uncaught%20exception%20instead%20of%20the%20intended%20deterministic%20CLI%20error%2Fexit%20behavior.%20Include%20migrator%20creation%20inside%20the%20same%20%60try%60%20block.%0A%0AValidate%20the%20correctness%20of%20the%20fla
gged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40079&comment_hash=c9fc03ba055c66e26f3f5144d0aa7706adfba09cf43a0549244c4e87d4836280&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40079&comment_hash=c9fc03ba055c66e26f3f5144d0aa7706adfba09cf43a0549244c4e87d4836280&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
