codeant-ai-for-open-source[bot] commented on code in PR #40815: URL: https://github.com/apache/superset/pull/40815#discussion_r3365638793
########## superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/useMatrixifyAllowedValues.ts: ########## @@ -0,0 +1,145 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { useEffect, useMemo, useState } from 'react'; +import { SupersetClient } from '../../..'; +import { getMatrixifyConfig, MatrixifyFormData } from '../../types/matrixify'; + +export type MatrixifyAllowedValuesStatus = 'success' | 'loading' | 'error'; + +export interface MatrixifyAllowedValuesState { + status: MatrixifyAllowedValuesStatus; + /** + * Map of dimension column name -> set of string-normalized values the + * current viewer is allowed to see (RLS applied by the backend). + */ + allowedByColumn: Record<string, Set<string>>; +} + +/** + * Collect the distinct dimension columns referenced by the matrixify axes. + */ +function getDimensionColumns(formData: MatrixifyFormData): string[] { + const config = getMatrixifyConfig(formData as any); + if (!config) { + return []; + } + const columns = new Set<string>(); + [config.rows, config.columns].forEach(axis => { + if (axis.mode === 'dimensions' && axis.dimension?.dimension) { + columns.add(axis.dimension.dimension); + } + }); + return Array.from(columns); +} + +/** + * Fetch the distinct values a viewer is permitted to see for a column. This + * reuses the datasource ``/column/<col>/values/`` endpoint, which applies the + * requesting user's row-level security filters server-side. + */ +async function fetchAllowedValues( + datasource: string, + column: string, + signal: AbortSignal, +): Promise<any[]> { + const [id, type] = String(datasource).split('__'); + const endpoint = `/api/v1/datasource/${type}/${id}/column/${encodeURIComponent( + column, + )}/values/`; + const { json } = await SupersetClient.get({ endpoint, signal }); + return json?.result || []; +} + +/** + * Resolve, per render, which dimension values the current viewer is allowed to + * see. Matrixify axis values are frozen into ``formData`` at design time, so + * without this the grid would be built from the chart author's RLS context and + * leak values (as subplot headers + empty cells) to restricted viewers. The + * renderer intersects the stored values against the returned allow-list. + * + * Fails closed: while loading the grid must not render, and on error the + * allow-list is treated as empty rather than falling back to the unfiltered + * (leaking) list. + */ +export function useMatrixifyAllowedValues( + formData: MatrixifyFormData, +): MatrixifyAllowedValuesState { + const datasource = (formData as any)?.datasource as string | undefined; + // Serialize the dimension columns to a primitive key. ``formData`` is a fresh + // object on most renders, so the effect must depend on this stable string + // rather than the (always-new) array, otherwise it would refetch in a loop. + const columnsKey = useMemo( + () => JSON.stringify([...getDimensionColumns(formData)].sort()), + [formData], + ); + + const [state, setState] = useState<MatrixifyAllowedValuesState>({ + status: columnsKey === '[]' ? 'success' : 'loading', + allowedByColumn: {}, + }); Review Comment: **Suggestion:** The hook keeps the previous `success` state when `datasource`/dimension columns change because `useState` initializer runs only on mount, so the first render after a config change can still look resolved and allow the grid to be built before the new allow-list request completes. That creates a fail-open frame where stale or unfiltered axis values can be rendered. Track a request key (e.g., datasource + columns) in state and treat mismatched keys as `loading` immediately, or reset state synchronously when inputs change, so renders never use stale `success` data. [stale reference] <details> <summary><b>Severity Level:</b> Critical π¨</summary> ```mdx - β Matrixify RLS gating fails during axis reconfiguration. - β Restricted dimension values displayed briefly despite RLS. - β οΈ Embedded viewers may see stale unauthorized headers. - β οΈ Harder to guarantee fail-closed behavior for matrixify. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. In the matrixify chart pipeline, `MatrixifyGridRenderer` at `superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.tsx:13-25` calls `useMatrixifyAllowedValues(formData)` and derives `allowedStatus` / `allowedByColumn`, then builds `sanitizedFormData` and `grid` when `allowedStatus === 'success'` (lines 24β31 and 52β59 from `MatrixifyGridRenderer.tsx`). 2. On initial mount with a given configuration (e.g., `matrixify_dimension_rows.dimension = 'region'`), `useMatrixifyAllowedValues` computes `columnsKey` from `getDimensionColumns(formData)` (lines 9β11 in `useMatrixifyAllowedValues.ts` snippet) and initializes state once via `useState` at `useMatrixifyAllowedValues.ts:93-96`, setting `status: 'loading'` (non-empty `columnsKey`) and an empty `allowedByColumn`, then the effect at `useMatrixifyAllowedValues.ts:19-63` fetches allowed values and sets `state = { status: 'success', allowedByColumn }`. 3. After this first fetch resolves, `MatrixifyGridRenderer` re-renders with `allowedStatus === 'success'` and a populated `allowedByColumn`, so `sanitizedFormData` (lines 29β49 in `MatrixifyGridRenderer.tsx`) filters dimension values using the allow-list, `grid` is built when `allowedStatus === 'success'` (lines 52β59), and the main layout renders because the `allowedStatus === 'loading'` / `=== 'error'` early returns at lines 90β103 in `MatrixifyGridRenderer.tsx` do not trigger. 4. Now change `formData` for the same mounted component instance so that `datasource` and/or the dimension columns change (e.g., `matrixify_dimension_rows.dimension` switches from `'region'` to `'country'`), as happens in Explore when a user edits axis controls: on that render, `columnsKey` (lines 9β11) updates, but the existing `state` from `useState` (lines 93β96) is reused because React does not re-run the initializer, so `allowedStatus` remains `'success'` and `allowedByColumn` still reflects the previous columns; `MatrixifyGridRenderer` therefore sees `allowedStatus === 'success'`, skips the loading/error guards (lines 90β103), and builds `sanitizedFormData` using a stale allow-listβany new dimension not present in `allowedByColumn` is left unfiltered (lines 35β40), causing a frame where the grid is rendered from stale/unfiltered axis values before the effect (lines 19β63) runs and sets `status` back to `'loading'`. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9dfae412385c484faac5a85a627ef8d9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9dfae412385c484faac5a85a627ef8d9&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/packages/superset-ui-core/src/chart/components/Matrixify/useMatrixifyAllowedValues.ts **Line:** 93:96 **Comment:** *Stale Reference: The hook keeps the previous `success` state when `datasource`/dimension columns change because `useState` initializer runs only on mount, so the first render after a config change can still look resolved and allow the grid to be built before the new allow-list request completes. That creates a fail-open frame where stale or unfiltered axis values can be rendered. Track a request key (e.g., datasource + columns) in state and treat mismatched keys as `loading` immediately, or reset state synchronously when inputs change, so renders never use stale `success` data. 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%2F40815&comment_hash=46f6e2030632497afd8bb58d95eecc74062deb5f090f38ebc929c00bb7bea990&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40815&comment_hash=46f6e2030632497afd8bb58d95eecc74062deb5f090f38ebc929c00bb7bea990&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]
