codeant-ai-for-open-source[bot] commented on code in PR #39999:
URL: https://github.com/apache/superset/pull/39999#discussion_r3214991376
##########
superset/initialization/__init__.py:
##########
@@ -663,6 +663,38 @@ def log_default_secret_key_warning() -> None:
logger.error("Refusing to start due to insecure SECRET_KEY")
sys.exit(1)
+ def check_guest_token_secret(self) -> None:
+ """Refuse to start with default guest JWT secret when embedding is
enabled."""
+ default_secret = "test-guest-secret-change-me" # noqa: S105
+ feature_flags = {
+ **self.config.get("DEFAULT_FEATURE_FLAGS", {}),
+ **self.config.get("FEATURE_FLAGS", {}),
+ }
+ if not feature_flags.get("EMBEDDED_SUPERSET"):
+ return
Review Comment:
**Suggestion:** The new startup guard checks `EMBEDDED_SUPERSET` from raw
config dicts only, but the rest of the codebase evaluates feature flags through
`is_feature_enabled`/feature flag manager (which can be overridden by
`GET_FEATURE_FLAGS_FUNC` or `IS_FEATURE_ENABLED_FUNC`). In deployments that
enable embedding through those hooks, this check will incorrectly skip
enforcement and allow startup with the default guest JWT secret, leaving guest
tokens forgeable. Evaluate the embedding flag via the feature-flag manager path
so this security check uses the same effective flag value as runtime auth
logic. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Embedded dashboards can start with default guest JWT secret.
- ❌ Guest JWT tokens forgeable when dynamic embedding feature flag enabled.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure dynamic feature flags via the documented hook in
`superset/config.py:20-42`
by setting `GET_FEATURE_FLAGS_FUNC` or `IS_FEATURE_ENABLED_FUNC` in
`superset_config.py`
so that it returns `EMBEDDED_SUPERSET=True` even though
`DEFAULT_FEATURE_FLAGS`/`FEATURE_FLAGS` (defined around
`superset/config.py:18-24`) do not
contain an `EMBEDDED_SUPERSET` entry.
2. Leave `GUEST_TOKEN_JWT_SECRET` at its default value
`"test-guest-secret-change-me"`
(the value used in `SupersetAppInitializer.check_guest_token_secret` at
`superset/initialization/__init__.py:668`) and start the Superset app, which
calls
`SupersetAppInitializer.init_app` at
`superset/initialization/__init__.py:769-783`.
3. During startup, `init_app` calls `self.configure_feature_flags()` at
`superset/initialization/__init__.py:781`, which initializes
`feature_flag_manager` (see
`superset/utils/feature_flag_manager.py:29-35` where
`GET_FEATURE_FLAGS_FUNC`/`IS_FEATURE_ENABLED_FUNC` are wired), but
`check_guest_token_secret` at `superset/initialization/__init__.py:666-676`
evaluates
embedding with a raw merged config dict (`feature_flags = {...}` and `if not
feature_flags.get("EMBEDDED_SUPERSET"):`). Because `EMBEDDED_SUPERSET` is
only added by
the dynamic hook and not present in `DEFAULT_FEATURE_FLAGS`/`FEATURE_FLAGS`,
this
condition evaluates to falsy and the method returns early without logging or
exiting,
allowing the server to start with the default guest secret.
4. At runtime, embedded access control uses the feature-flag manager path:
`superset/security/manager.py:26-32` and `:3510-3513` call
`is_feature_enabled("EMBEDDED_SUPERSET")` from `superset/__init__.py:33-40`,
which
delegates to `feature_flag_manager.is_feature_enabled` in
`superset/utils/feature_flag_manager.py:47-57` that in turn honors
`GET_FEATURE_FLAGS_FUNC`/`IS_FEATURE_ENABLED_FUNC`. Thus, embedded
dashboards are
effectively enabled while `parse_jwt_guest_token` at
`superset/security/manager.py:13-23`
still decodes tokens with the default `GUEST_TOKEN_JWT_SECRET`, leaving
guest JWTs
forgeable despite the intended startup guard.
```
</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%2Finitialization%2F__init__.py%0A%2A%2ALine%3A%2A%2A%20669%3A674%0A%2A%2AComment%3A%2A%2A%0A%09%2ASecurity%3A%20The%20new%20startup%20guard%20checks%20%60EMBEDDED_SUPERSET%60%20from%20raw%20config%20dicts%20only%2C%20but%20the%20rest%20of%20the%20codebase%20evaluates%20feature%20flags%20through%20%60is_feature_enabled%60%2Ffeature%20flag%20manager%20%28which%20can%20be%20overridden%20by%20%60GET_FEATURE_FLAGS_FUNC%60%20or%20%60IS_FEATURE_ENABLED_FUNC%60%29.%20In%20deployments%20that%20enable%20embedding%20through%20those%20hooks%2C%20this%20check%20will%20incorrectly%20skip%20enforcement%20and%20allow%20startup%20with%20the%20default%20guest%20JWT%20secret%2C%20leaving%20guest%20tokens%20forgeable.%20Evaluate%20the%20embedding%20flag%20via%20the%20feature-flag%20manager%20path%20so%20this%20security%20check%20uses%20t
he%20same%20effective%20flag%20value%20as%20runtime%20auth%20logic.%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%2Finitialization%2F__init__.py%0A%2A%2ALine%3A%2A%2A%20669%3A674%0A%2A%2AComment%3A%2A%2A%0A%09%2ASecurity%3A%20The%20new%20startup%20guard%20checks%20%60EMBEDDED_SUPERSET%60%20from%20raw%20config%20dicts%20on
ly%2C%20but%20the%20rest%20of%20the%20codebase%20evaluates%20feature%20flags%20through%20%60is_feature_enabled%60%2Ffeature%20flag%20manager%20%28which%20can%20be%20overridden%20by%20%60GET_FEATURE_FLAGS_FUNC%60%20or%20%60IS_FEATURE_ENABLED_FUNC%60%29.%20In%20deployments%20that%20enable%20embedding%20through%20those%20hooks%2C%20this%20check%20will%20incorrectly%20skip%20enforcement%20and%20allow%20startup%20with%20the%20default%20guest%20JWT%20secret%2C%20leaving%20guest%20tokens%20forgeable.%20Evaluate%20the%20embedding%20flag%20via%20the%20feature-flag%20manager%20path%20so%20this%20security%20check%20uses%20the%20same%20effective%20flag%20value%20as%20runtime%20auth%20logic.%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%2
0ask%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/initialization/__init__.py
**Line:** 669:674
**Comment:**
*Security: The new startup guard checks `EMBEDDED_SUPERSET` from raw
config dicts only, but the rest of the codebase evaluates feature flags through
`is_feature_enabled`/feature flag manager (which can be overridden by
`GET_FEATURE_FLAGS_FUNC` or `IS_FEATURE_ENABLED_FUNC`). In deployments that
enable embedding through those hooks, this check will incorrectly skip
enforcement and allow startup with the default guest JWT secret, leaving guest
tokens forgeable. Evaluate the embedding flag via the feature-flag manager path
so this security check uses the same effective flag value as runtime auth logic.
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%2F39999&comment_hash=54343cba9cd02be07de3aa0764eba21a0591a098185a427f6027c3c5ea9e7b4c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39999&comment_hash=54343cba9cd02be07de3aa0764eba21a0591a098185a427f6027c3c5ea9e7b4c&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]
