codeant-ai-for-open-source[bot] commented on code in PR #40654:
URL: https://github.com/apache/superset/pull/40654#discussion_r3407888749
##########
superset/utils/encrypt.py:
##########
@@ -19,17 +19,76 @@
from dataclasses import dataclass
from typing import Any, Optional
-from flask import Flask
+from flask import current_app, Flask
from flask_babel import lazy_gettext as _
from sqlalchemy import Table, text, TypeDecorator
from sqlalchemy.engine import Connection, Dialect, Row
from sqlalchemy_utils import EncryptedType as SqlaEncryptedType
+from sqlalchemy_utils.types.encrypted.encrypted_type import (
+ AesEngine,
+ AesGcmEngine,
+ EncryptionDecryptionBaseEngine,
+)
class EncryptedType(SqlaEncryptedType):
cache_ok = True
+# Named encryption engines selectable via the
``SQLALCHEMY_ENCRYPTED_FIELD_ENGINE``
+# config. "aes" (AES-CBC) is the historical default; "aes-gcm" is authenticated
+# encryption (recommended for new deployments). NOTE: switching an existing
+# deployment from "aes" to "aes-gcm" requires re-encrypting all stored secrets
+# first — see the SIP referenced in the docs. Changing this on a populated
+# database without that migration will make existing secrets undecryptable.
+ENCRYPTION_ENGINES: dict[str, type[EncryptionDecryptionBaseEngine]] = {
+ "aes": AesEngine,
+ "aes-gcm": AesGcmEngine,
+}
+
+# The historical fallback engine when the config does not name one.
+DEFAULT_ENCRYPTION_ENGINE_NAME = "aes"
+
+# Engines whose ciphertext is authenticated: a successful decrypt is
+# cryptographic proof the value is genuinely in that form. AES-GCM carries an
+# authentication tag; AES-CBC does not, so a CBC "success" can be coincidental.
+# Classification logic (the migrator's idempotency fast path) must let an
+# authenticated decrypt win over an unauthenticated one, never the reverse.
+AUTHENTICATED_ENGINES: frozenset[type[EncryptionDecryptionBaseEngine]] =
frozenset(
+ {AesGcmEngine}
+)
+
+
+def _is_authenticated_engine(engine: type[EncryptionDecryptionBaseEngine]) ->
bool:
+ return engine in AUTHENTICATED_ENGINES
+
+
+def resolve_encryption_engine(engine_name: str) ->
type[EncryptionDecryptionBaseEngine]:
+ """Resolve a configured engine name to its engine class, fail-closed.
+
+ The value is normalized (trimmed, lower-cased, underscores → hyphens) so it
+ matches the case-insensitive CLI ``click.Choice``. An unrecognized name
+ raises so a misconfiguration fails at field-construction (startup) rather
+ than silently degrading to unauthenticated AES-CBC — which would let an
+ operator who typo'd ``"aes_gcm"`` believe they had authenticated
encryption,
+ and, after a GCM migration, write new secrets as CBC into a GCM database.
+
+ The offending value is deliberately kept out of the error message: it comes
+ from the app config (which also holds ``SECRET_KEY``), and static analysis
+ flags interpolating config-sourced values into logs/errors as potential
+ clear-text secret exposure. The set of valid engine names is enough to
+ diagnose a typo.
+ """
+ normalized = engine_name.strip().lower().replace("_", "-")
Review Comment:
**Suggestion:** `resolve_encryption_engine` assumes `engine_name` is always
a string and immediately calls `.strip()`. If a deployment sets
`SQLALCHEMY_ENCRYPTED_FIELD_ENGINE` to a non-string (for example `None` in a
custom config override), this raises `AttributeError` during model
initialization instead of the intended fail-closed `ValueError` with valid
engine options. Add an explicit type check and raise the same controlled
`ValueError` path for non-string values. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Superset fails starting when engine config set to None.
- ⚠️ All encrypted columns unusable until configuration type corrected.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Superset instance so that `SQLALCHEMY_ENCRYPTED_FIELD_ENGINE`
is set to a
non-string value (for example `None`) in the Flask app config, overriding
the default
string literal defined in `superset/config.py:290-320`.
2. Start Superset so that `AppInitializer.configure_db_encrypt` in
`superset/initialization/__init__.py:978-982` runs and calls
`encrypted_field_factory.init_app(self.superset_app)`, which stores
`self.superset_app.config` and instantiates `SQLAlchemyUtilsAdapter` as the
concrete
adapter in `superset/utils/encrypt.py:153-162`.
3. During model setup, when an encrypted column is constructed, e.g.
`password =
Column(encrypted_field_factory.create(String(1024)))` in
`superset/models/core.py:160-167`, `encrypted_field_factory.create`
delegates to
`SQLAlchemyUtilsAdapter.create` in `superset/utils/encrypt.py:120-146`,
where `engine_name
= app_config.get("SQLALCHEMY_ENCRYPTED_FIELD_ENGINE",
DEFAULT_ENCRYPTION_ENGINE_NAME)` at
lines 138-140 evaluates to `None`.
4. Still in `SQLAlchemyUtilsAdapter.create`,
`resolve_encryption_engine(engine_name)` is
called at `superset/utils/encrypt.py:144`; inside
`resolve_encryption_engine` at line 82
the code `normalized = engine_name.strip().lower().replace("_", "-")`
executes, causing
`AttributeError: 'NoneType' object has no attribute 'strip'` instead of the
intended
controlled `ValueError` listing valid engines, and Superset fails to start
or load models
that use encrypted fields.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bff0165e55dd467ca684d78a78d9dd70&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bff0165e55dd467ca684d78a78d9dd70&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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/utils/encrypt.py
**Line:** 82:82
**Comment:**
*Type Error: `resolve_encryption_engine` assumes `engine_name` is
always a string and immediately calls `.strip()`. If a deployment sets
`SQLALCHEMY_ENCRYPTED_FIELD_ENGINE` to a non-string (for example `None` in a
custom config override), this raises `AttributeError` during model
initialization instead of the intended fail-closed `ValueError` with valid
engine options. Add an explicit type check and raise the same controlled
`ValueError` path for non-string values.
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%2F40654&comment_hash=bb164f03ebb75910fe2ba039cd455d8892366b2753610de56f51578e6dc7fdcd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40654&comment_hash=bb164f03ebb75910fe2ba039cd455d8892366b2753610de56f51578e6dc7fdcd&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]
