codeant-ai-for-open-source[bot] commented on code in PR #42479:
URL: https://github.com/apache/superset/pull/42479#discussion_r3663781763
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:
##########
@@ -320,11 +320,39 @@ const Chart = (props: ChartProps) => {
);
useLayoutEffect(() => {
- if (isExpanded && descriptionRef.current) {
- setDescriptionHeight(descriptionRef.current.offsetHeight);
- } else {
+ if (!isExpanded || !descriptionRef.current) {
setDescriptionHeight(0);
+ return undefined;
}
+
+ let isDescriptionHeightSet = false;
+ const initialHeight = descriptionRef.current.offsetHeight;
+ if (initialHeight > 0) {
+ setDescriptionHeight(initialHeight);
+ isDescriptionHeightSet = true;
+ }
+
+ if (typeof ResizeObserver !== 'undefined') {
+ const observer = new ResizeObserver(entries => {
+ for (const entry of entries) {
+ if (entry.target === descriptionRef.current) {
+ const height = (entry.target as HTMLElement).offsetHeight;
+ if (height > 0 || !isDescriptionHeightSet) {
+ setDescriptionHeight(height);
+ isDescriptionHeightSet = true;
+ }
+ }
+ }
+ });
+
+ observer.observe(descriptionRef.current);
+
+ return () => {
+ observer.disconnect();
+ };
+ }
+
+ return undefined;
}, [isExpanded]);
Review Comment:
**Suggestion:** The effect only reruns when `isExpanded` changes. If
`slice.description_markdown` is removed while the chart remains expanded, the
description element is unmounted but this effect does not run again, so the
observer is not disconnected and `descriptionHeight` retains the old value. The
chart then continues to subtract the height of a description that is no longer
rendered. Include the description presence/content or the observed element
identity in the lifecycle handling, and reset the height when the element
disappears. [stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Expanded charts retain space for removed descriptions.
- ⚠️ Chart rendering uses stale dimensions after slice updates.
- ⚠️ Detached description observers remain registered until unmount.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a dashboard chart with `expandedSlices[props.id]` set to `true`;
`Chart.tsx:209-211` derives `isExpanded`, and `Chart.tsx:786-795` mounts the
description
element when `slice.description_markdown` is truthy.
2. Allow the effect at `Chart.tsx:322-356` to measure the description and
register a
`ResizeObserver`; the positive measurement is stored in `descriptionHeight`
at
`Chart.tsx:329-343`.
3. Update the chart's slice entity so `description_markdown` becomes empty
while
`isExpanded` remains true. Slice entities are replaced through
`updateSlices()` at
`src/dashboard/actions/sliceEntities.ts:118-121`, and the fetched slice
shape includes
`description_markdown` at `src/dashboard/actions/sliceEntities.ts:101-107`.
4. React removes the `<aside>` at `Chart.tsx:786-795`, but the effect does
not rerun
because its dependency array only contains `isExpanded` at `Chart.tsx:356`;
therefore its
cleanup does not disconnect the observer or reset `descriptionHeight`, and
`getChartHeight()` continues subtracting the stale value at
`Chart.tsx:395-396`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0b717162d00b4e329da63d679c99b838&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=0b717162d00b4e329da63d679c99b838&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-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx
**Line:** 356:356
**Comment:**
*Stale Reference: The effect only reruns when `isExpanded` changes. If
`slice.description_markdown` is removed while the chart remains expanded, the
description element is unmounted but this effect does not run again, so the
observer is not disconnected and `descriptionHeight` retains the old value. The
chart then continues to subtract the height of a description that is no longer
rendered. Include the description presence/content or the observed element
identity in the lifecycle handling, and reset the height when the element
disappears.
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%2F42479&comment_hash=28b5f99bbb6ca7d3c367467dadeef288e6e214ca0beb21d85373a84ed28b0a04&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42479&comment_hash=28b5f99bbb6ca7d3c367467dadeef288e6e214ca0beb21d85373a84ed28b0a04&reaction=dislike'>👎</a>
##########
superset/security/manager.py:
##########
@@ -4803,6 +4803,11 @@ def is_editor(self, resource: Model) -> bool:
editor_subject_ids = set(get_extra_editor_subject_ids(resource))
if hasattr(resource, "editors"):
editor_subject_ids.update(s.id for s in resource.editors)
+
+ # Fallback for models like Query and SavedQuery that use 'user_id'
+ if hasattr(resource, "user_id") and resource.user_id is not None:
+ editor_subject_ids.add(resource.user_id)
Review Comment:
**Suggestion:** The fallback adds an `ab_user.id` directly to a set of
`Subject.id` values. These are separate primary-key namespaces, so an SQL Lab
query owner will usually not match their own user subject and will still be
denied editorship. Conversely, if a user's primary-key value happens to equal
another user's, role's, or group's subject ID, this can grant editorship to the
wrong principal. Resolve the `Subject` for `resource.user_id` and add its
subject ID instead. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ SQL Lab owners can still receive 403 errors when creating charts.
- ❌ Explore navigation can fail for authorized query creators.
- ⚠️ Subject-ID collisions can grant editorship to unrelated principals.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a non-admin SQL Lab user and execute a query. The `Query` model
stores the
creator's `ab_user.id` in `user_id` at `superset/models/sql_lab.py:39-49`;
`SavedQuery`
does the same at `superset/models/sql_lab.py:37-40`.
2. Click SQL Lab's Create chart flow, which eventually performs resource
ownership checks
through `SecurityManager.is_editor()` at
`superset/security/manager.py:4778-4811`.
3. `is_editor()` obtains the current user's subject IDs using
`get_user_subject_ids()` at
`superset/security/manager.py:4793-4794`. That helper returns `subjects.id`
values at
`superset/subjects/utils.py:82-91`, including the user's USER subject
selected through
`Subject.user_id` at `superset/subjects/utils.py:46`.
4. The new fallback instead inserts `resource.user_id` directly at
`superset/security/manager.py:4807-4809`. `Subject.id` is an independent
primary key at
`superset/subjects/models.py:48`, while `Subject.user_id` separately
references
`ab_user.id` at `superset/subjects/models.py:55-60`; therefore the creator's
user ID
generally does not equal the creator's subject ID, so the intersection at
`superset/security/manager.py:4811` remains empty and the owner can still
receive the
ownership denial.
5. If an `ab_user.id` happens to equal a subject ID belonging to another
user, role, or
group, the same direct integer comparison can instead make that unrelated
principal appear
to own the query. Resolve the USER subject by `Subject.user_id` (for example
through
`get_user_subject(resource.user_id)`) and add its `Subject.id`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=546c18a30a744fa08037e860209f6eae&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=546c18a30a744fa08037e860209f6eae&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:** 4807:4809
**Comment:**
*Security: The fallback adds an `ab_user.id` directly to a set of
`Subject.id` values. These are separate primary-key namespaces, so an SQL Lab
query owner will usually not match their own user subject and will still be
denied editorship. Conversely, if a user's primary-key value happens to equal
another user's, role's, or group's subject ID, this can grant editorship to the
wrong principal. Resolve the `Subject` for `resource.user_id` and add its
subject ID instead.
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%2F42479&comment_hash=be7083af2f2bc9084564afbf36aab9729a7cb288a73d12e8eb60856ffd78cfae&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42479&comment_hash=be7083af2f2bc9084564afbf36aab9729a7cb288a73d12e8eb60856ffd78cfae&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]
