codeant-ai-for-open-source[bot] commented on code in PR #41843:
URL: https://github.com/apache/superset/pull/41843#discussion_r3565537278
##########
superset/security/manager.py:
##########
@@ -1818,17 +1866,32 @@ 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")
+ # Build display names from the raw parts: Table.__str__ URL-encodes
+ # each segment, and the renderer encodes the whole value again, so
+ # using it here would double-encode. Sorted for deterministic links.
+ return _render_permission_instructions_link(
+ table_names=",".join(
+ sorted(
+ ".".join(
+ part
+ for part in (table.catalog, table.schema, table.table)
+ if part
+ )
+ for table in tables
+ )
+ ),
+ )
Review Comment:
**Suggestion:** The table-name link rendering loses identifier boundaries
for names that contain dots. This code joins raw `catalog/schema/table` parts
with `.` and then URL-encodes the whole combined string, so a literal dot
inside an identifier is indistinguishable from the separator dot. As a result,
access-request links can be prefilled with the wrong table names for valid SQL
identifiers like `foo.bar`. Preserve per-segment escaping (including dots) and
avoid re-encoding already-escaped table segments. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ /v1/query errors send ambiguous table_names for dotted identifiers.
- ⚠️ Explore charts with denied tables generate misleading request URLs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a permission instructions link with table_names templating in
config so
`_render_permission_instructions_link` uses it: set
`PERMISSION_INSTRUCTIONS_LINK =
"https://access.internal/request?tables={table_names}"` in the Flask config
read by
`get_conf()` (superset/security/manager.py:124) and rendered in
`_render_permission_instructions_link`
(superset/security/manager.py:128-161, especially
line 143 where `link = get_conf().get("PERMISSION_INSTRUCTIONS_LINK")` is
obtained and
lines 152-160 where placeholders are replaced via `quote(str(value),
safe="")`).
2. Use or create a physical SQL table whose identifier contains a literal
dot (per engine
rules), for example a Postgres table named `"foo.bar"` in schema `public`.
When Superset
parses queries, it constructs `Table` objects from catalog/schema/table
components via the
dataclass in superset/sql/parse.py:154-163, and its `__str__` method at
superset/sql/parse.py:164-175 URL-encodes each segment and replaces `.` with
`%2E`
specifically to preserve segment boundaries in string form.
3. Have a Gamma/viewer issue a query against that dotted-name table through
a standard
entry point that enforces access control, such as the Explore API endpoint
`POST
/api/v1/query/` implemented in `Api.query` (superset/views/api.py:60-18).
The endpoint
builds a `QueryContext` and calls `query_context.raise_for_access()`
(superset/views/api.py:12-16), which internally delegates to
`SupersetSecurityManager.raise_for_access` and performs per-table permission
checks; when
the user lacks access, the code in superset/security/manager.py:3880-48
collects denied
`Table` objects into the `denied` set and raises
`SupersetSecurityException(self.get_table_access_error_object(denied))`.
4. Observe how the access-request link is built for the denied tables:
`get_table_access_error_object` (superset/security/manager.py:1852-168)
constructs a
`SupersetError` with `extra={"link": self.get_table_access_link(tables),
"tables":
[str(table) for table in tables]}`. `get_table_access_link`
(superset/security/manager.py:170-195) computes `table_names` by joining raw
`catalog`,
`schema`, and `table` parts with `"."` (`".".join(part for part in
(table.catalog,
table.schema, table.table) if part)`), then joins multiple tables with
commas and passes
that combined string into `_render_permission_instructions_link`. Because
`_render_permission_instructions_link` calls `quote(str(value), safe="")` on
the entire
combined string (superset/security/manager.py:152-160), and Python’s
`urllib.parse.quote`
never encodes the `.` character, both separator dots and literal dots inside
identifiers
remain `"."` in the final `table_names` URL parameter (e.g.
`public.foo.bar`), unlike
`Table.__str__` which encodes internal dots as `%2E`. Downstream
access-request tools
receiving `table_names` cannot distinguish segment separators from
identifier-internal
dots, so a table originally named `foo.bar` in schema `public` is
indistinguishable from a
three-part path `public.foo.bar`, making it easy for the prefilled
access-request context
to reference the wrong object or misparse the denied tables.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=85ef3ae1b99e49fbb57d6acfc4a1c1da&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=85ef3ae1b99e49fbb57d6acfc4a1c1da&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:** 1883:1894
**Comment:**
*Logic Error: The table-name link rendering loses identifier boundaries
for names that contain dots. This code joins raw `catalog/schema/table` parts
with `.` and then URL-encodes the whole combined string, so a literal dot
inside an identifier is indistinguishable from the separator dot. As a result,
access-request links can be prefilled with the wrong table names for valid SQL
identifiers like `foo.bar`. Preserve per-segment escaping (including dots) and
avoid re-encoding already-escaped table segments.
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=3b94a1a6c6e96a2b930d43ebefd5ec8b5c8fd3d80940a9eab28f1175e4003433&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41843&comment_hash=3b94a1a6c6e96a2b930d43ebefd5ec8b5c8fd3d80940a9eab28f1175e4003433&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]
