codeant-ai-for-open-source[bot] commented on code in PR #41843:
URL: https://github.com/apache/superset/pull/41843#discussion_r3541087620
##########
superset/security/manager.py:
##########
@@ -1640,17 +1683,20 @@ def get_table_access_error_object(self, tables:
set["Table"]) -> SupersetError:
},
)
- def get_table_access_link( # pylint: disable=unused-argument
- self, tables: set["Table"]
- ) -> Optional[str]:
+ def get_table_access_link(self, tables: set["Table"]) -> Optional[str]:
"""
Return the access link for the denied SQL tables.
+ The configured ``PERMISSION_INSTRUCTIONS_LINK`` may template the denied
+ table names (and the current username) into the access URL.
+
:param tables: The set of denied SQL tables
:returns: The access URL
"""
- return get_conf().get("PERMISSION_INSTRUCTIONS_LINK")
+ return _render_permission_instructions_link(
+ table_names=",".join(str(table) for table in tables),
+ )
Review Comment:
**Suggestion:** `Table.__str__()` already URL-encodes table parts, but this
value is encoded again in `_render_permission_instructions_link`, which causes
double-encoding for tables containing special characters (for example spaces or
literal dots in identifiers). Build `table_names` from raw table parts
(catalog/schema/table) and let the helper perform the only encoding pass.
[double interpolation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ /api/v1/query access links double-encode complex table names.
- ⚠️ Request-access URLs misrepresent tables with spaces or dots.
- ⚠️ Viewers see confusing, malformed table identifiers in links.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure PERMISSION_INSTRUCTIONS_LINK with a {table_names} placeholder,
as exercised
in the helper tests at
`superset/tests/unit_tests/security/test_permission_instructions_link.py:119-137`
where
the template `"https://acme.example.com/req?tables={table_names}"` is used.
2. Note that the table objects used in permission checks are instances of
`superset.sql.parse.Table` imported in `superset/security/manager.py:89`,
whose `__str__`
implementation at `superset/sql/parse.py:94-115` constructs a string by
joining
URL-encoded parts (`urllib.parse.quote(part, safe="")`) with `"."`, so
special characters
like spaces or literal dots inside identifiers are already percent-encoded in
`str(table)`.
3. When a user issues a POST request to the query API endpoint
`/api/v1/query/`
(implemented in `superset/views/api.py:60-18`), and the query references one
or more SQL
tables that the current user is not allowed to access,
`query_context.raise_for_access()`
ultimately calls `SupersetSecurityManager.raise_for_access`, which computes
a `denied` set
of `Table` objects and, on failure, raises
`SupersetSecurityException(self.get_table_access_error_object(denied))` as
shown in
`superset/security/manager.py:3700-3725`.
4. Inside `SupersetSecurityManager.get_table_access_error_object` at
`superset/security/manager.py:120-135`, the error object’s `extra["link"]`
is populated
via `self.get_table_access_link(tables)`, and `get_table_access_link` at
`superset/security/manager.py:137-150` builds
`table_names=",".join(str(table) for table
in tables)` and passes it to `_render_permission_instructions_link`;
`_render_permission_instructions_link` at
`superset/security/manager.py:127-160` then
applies `quote(str(value), safe="")` again to the already-encoded
`table_names`, so a
table like `Table("Quarterly Sales", "public")` yields
`public.sales%2520data` (space
double-encoded to `%2520`) in the returned link, producing a double-encoded
and
potentially unusable access-request URL in the API error payload.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f45d6d95a4904903a1db608469bf2321&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=f45d6d95a4904903a1db608469bf2321&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/security/manager.py
**Line:** 1697:1699
**Comment:**
*Double Interpolation: `Table.__str__()` already URL-encodes table
parts, but this value is encoded again in
`_render_permission_instructions_link`, which causes double-encoding for tables
containing special characters (for example spaces or literal dots in
identifiers). Build `table_names` from raw table parts (catalog/schema/table)
and let the helper perform the only encoding pass.
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%2F41843&comment_hash=35bdeae369eca04fc09f2a5ccc44f94e495497fc8646e366e1f4d7a3ff977e39&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41843&comment_hash=35bdeae369eca04fc09f2a5ccc44f94e495497fc8646e366e1f4d7a3ff977e39&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]
