codeant-ai-for-open-source[bot] commented on code in PR #33924:
URL: https://github.com/apache/superset/pull/33924#discussion_r3600400196
##########
superset/jinja_context.py:
##########
@@ -305,6 +311,67 @@ def url_param(
self.cache_key_wrapper(result)
return result
+ def get_guest_user_attribute(
+ self,
+ attribute_name: str,
+ default: JsonValue = None,
+ add_to_cache_keys: bool = True,
+ escape_result: bool = True,
+ ) -> JsonValue:
+ """
+ Get a specific user attribute from guest user.
+
+ This function retrieves attributes from the guest user token and
supports
+ all JSON-native types (string, number, boolean, array, object, null).
+
+ Args:
+ attribute_name: Name of the attribute to retrieve
+ default: Default value if attribute not found (can be any
JSON-native type)
+ add_to_cache_keys: Whether the value should be included in the
cache key
+ escape_result: Escape string values (and strings inside lists)
using
+ the database dialect's quoting so they are safe to interpolate
+ into SQL, mirroring ``url_param``. Enabled by default;
non-string
+ JSON types are returned unchanged. Set to False for the raw
value.
+
+ Returns:
+ The attribute value from the guest user token, or the default
value.
+ Can be any JSON-native type: string, number, boolean, array,
object, or
+ null.
+
+ Examples:
+ {{ get_guest_user_attribute('department') }} # Returns:
"Engineering"
+ {{ get_guest_user_attribute('is_admin') }} # Returns: True
+ {{ get_guest_user_attribute('permissions') }} # Returns: ["read",
"write"]
+ {{ get_guest_user_attribute('config') }} # Returns: {"theme":
"dark"}
+ {{ get_guest_user_attribute('missing', 'default') }} # Returns:
"default"
+ """
+
+ # The macro only applies to guest users (embedded). is_guest_user()
+ # handles the feature-flag and request-context checks internally.
+ if not security_manager.is_guest_user():
+ return default
+
+ token = g.user.guest_token
+ user_attributes = token.get("user", {}).get("attributes") or {}
+
+ # Only add to the cache key if the attribute exists in the guest token
+ if attribute_name not in user_attributes:
+ return default
Review Comment:
**Suggestion:** `get_guest_user_attribute` returns `default` immediately for
non-guest users and missing attributes, which bypasses the method’s own
escaping path even when `escape_result=True`. This can break SQL rendering (or
allow injection) if the default contains quotes or comes from a non-escaped
source. Route the default through the same escaping logic before returning so
the method’s safety contract is consistent across all branches. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Jinja guest attribute macro bypasses SQL escaping for defaults.
⚠️ Embedded dashboards using default attributes risk unsafe SQL.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. SQL Lab and other templated queries render Jinja SQL via
`_render_sql_template` in
`superset/sql/execution/executor.py:13-29`, which calls
`get_template_processor(database=self.database)` and then
`BaseTemplateProcessor.process_template(sql, **template_params)` to evaluate
macros in
`superset/jinja_context.py`.
2. `JinjaTemplateProcessor.set_context` registers the
`get_guest_user_attribute` macro in
the Jinja context at `superset/jinja_context.py:970-1021` by mapping
`"get_guest_user_attribute"` to `partial(safe_proxy,
extra_cache.get_guest_user_attribute)`, so it is available in any templated
SQL executed
through this pipeline.
3. Inside `ExtraCache.get_guest_user_attribute`
(`superset/jinja_context.py:314-373`), the
method returns early for non-guest users and missing attributes: the branch
`if not
security_manager.is_guest_user(): return default` and the branch `if
attribute_name not in
user_attributes: return default` (lines 351-359) return the `default`
argument directly
without passing it through `_escape_value` or any dialect quoting.
4. Execute a templated SQL query in SQL Lab such as `SELECT '{{
get_guest_user_attribute("tenant", default_value) }}'` where `default_value`
contains
quotes or SQL fragments derived from another unescaped source; for a regular
(non-guest)
session `security_manager.is_guest_user()` is False, so the macro returns
`default`
unchanged, and the rendered SQL string includes this value verbatim,
bypassing the
dialect-quoting logic in `_escape_value`
(`superset/jinja_context.py:416-435`) even though
`escape_result=True`, breaking the macro's safety expectations and allowing
unsafe content
to flow into SQL.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f5558b1aff0548668f23bce652dc29ac&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f5558b1aff0548668f23bce652dc29ac&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/jinja_context.py
**Line:** 351:359
**Comment:**
*Security: `get_guest_user_attribute` returns `default` immediately for
non-guest users and missing attributes, which bypasses the method’s own
escaping path even when `escape_result=True`. This can break SQL rendering (or
allow injection) if the default contains quotes or comes from a non-escaped
source. Route the default through the same escaping logic before returning so
the method’s safety contract is consistent across all branches.
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%2F33924&comment_hash=7c24efd31b98534dafb202d91580c64462aab851c5ed7c2f042ee0c98e1ac0c3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F33924&comment_hash=7c24efd31b98534dafb202d91580c64462aab851c5ed7c2f042ee0c98e1ac0c3&reaction=dislike'>👎</a>
##########
superset/jinja_context.py:
##########
@@ -305,6 +311,67 @@ def url_param(
self.cache_key_wrapper(result)
return result
+ def get_guest_user_attribute(
+ self,
+ attribute_name: str,
+ default: JsonValue = None,
+ add_to_cache_keys: bool = True,
+ escape_result: bool = True,
+ ) -> JsonValue:
+ """
+ Get a specific user attribute from guest user.
+
+ This function retrieves attributes from the guest user token and
supports
+ all JSON-native types (string, number, boolean, array, object, null).
+
+ Args:
+ attribute_name: Name of the attribute to retrieve
+ default: Default value if attribute not found (can be any
JSON-native type)
+ add_to_cache_keys: Whether the value should be included in the
cache key
+ escape_result: Escape string values (and strings inside lists)
using
+ the database dialect's quoting so they are safe to interpolate
+ into SQL, mirroring ``url_param``. Enabled by default;
non-string
+ JSON types are returned unchanged. Set to False for the raw
value.
+
+ Returns:
+ The attribute value from the guest user token, or the default
value.
+ Can be any JSON-native type: string, number, boolean, array,
object, or
+ null.
+
+ Examples:
+ {{ get_guest_user_attribute('department') }} # Returns:
"Engineering"
+ {{ get_guest_user_attribute('is_admin') }} # Returns: True
+ {{ get_guest_user_attribute('permissions') }} # Returns: ["read",
"write"]
+ {{ get_guest_user_attribute('config') }} # Returns: {"theme":
"dark"}
+ {{ get_guest_user_attribute('missing', 'default') }} # Returns:
"default"
+ """
+
+ # The macro only applies to guest users (embedded). is_guest_user()
+ # handles the feature-flag and request-context checks internally.
+ if not security_manager.is_guest_user():
+ return default
+
+ token = g.user.guest_token
+ user_attributes = token.get("user", {}).get("attributes") or {}
+
+ # Only add to the cache key if the attribute exists in the guest token
+ if attribute_name not in user_attributes:
+ return default
+
+ result = user_attributes[attribute_name]
+ if add_to_cache_keys and result is not None:
+ # Use json.dumps for consistent serialization of all JSON-native
types
+ cache_value = json.dumps(result, sort_keys=True)
+ self.cache_key_wrapper(
+ f"guest_user_attribute:{attribute_name}:{cache_value}"
+ )
+ # Guest attributes are interpolated into the rendered SQL, so escape
+ # string values (and strings within lists) with the dialect's quoting
+ # by default, mirroring url_param. Non-string JSON types pass through.
+ if escape_result:
+ result = self._escape_value(result)
+ return result
Review Comment:
**Suggestion:** The new macro advertises JSON object support, but escaping
only delegates to `_escape_value`, which does not escape strings nested inside
dictionaries. That leaves nested attribute strings unescaped when templates
access object members (for example, `get_guest_user_attribute(...)[...]`) and
interpolate them into SQL. Either recursively escape string values in dict
structures or restrict this macro to scalar/list string-safe outputs when
`escape_result=True`. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Nested JSON guest attributes can inject unescaped strings into SQL.
⚠️ Multi-tenant dashboards using JSON attributes risk query compromise.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Guest embedded dashboards and other guest user queries are rendered via
the same Jinja
pipeline as SQL Lab: `_render_sql_template` in
`superset/sql/execution/executor.py:13-29`
calls `get_template_processor`, which instantiates a
`JinjaTemplateProcessor` and injects
`get_guest_user_attribute` into the context
(`superset/jinja_context.py:970-1021`).
2. For guest users, `ExtraCache.get_guest_user_attribute` at
`superset/jinja_context.py:314-373` loads the guest token from
`g.user.guest_token`, reads
`user_attributes = token.get("user", {}).get("attributes") or {}` (lines
354-355), and if
the attribute exists assigns `result = user_attributes[attribute_name]`
(line 361).
3. The method then applies escaping only via `_escape_value` when
`escape_result` is True
(lines 368-372), but `_escape_value` (`superset/jinja_context.py:416-435`)
explicitly
escapes only strings and lists of strings, returning dictionaries unchanged;
thus a JSON
object attribute like `{"tenant": {"id": "foo' OR 1=1 --"}}` is returned as
a dict without
any nested string escaping even though `escape_result=True`.
4. A SQL template that uses an object attribute and indexes into it (for
example, `WHERE
tenant_id = '{{ get_guest_user_attribute("tenant")["id"] }}'`) causes
`get_guest_user_attribute` to return the dict unchanged; Jinja then
evaluates `["id"]` to
a raw string, which is interpolated directly into the SQL without going
through dialect
quoting, so malicious or malformed nested strings in JSON attributes can
reach the final
SQL despite the macro being invoked with escaping enabled.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=28c8303a38cb4b28bd50c1f5a0e6ed59&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=28c8303a38cb4b28bd50c1f5a0e6ed59&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/jinja_context.py
**Line:** 368:373
**Comment:**
*Security: The new macro advertises JSON object support, but escaping
only delegates to `_escape_value`, which does not escape strings nested inside
dictionaries. That leaves nested attribute strings unescaped when templates
access object members (for example, `get_guest_user_attribute(...)[...]`) and
interpolate them into SQL. Either recursively escape string values in dict
structures or restrict this macro to scalar/list string-safe outputs when
`escape_result=True`.
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%2F33924&comment_hash=cbc031ebe16dafa6dc711492c36a876c260a00abe453e1a7ffd219e136521a28&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F33924&comment_hash=cbc031ebe16dafa6dc711492c36a876c260a00abe453e1a7ffd219e136521a28&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]
