You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "msyavuz (via GitHub)" <gi...@apache.org> on 2026/06/05 21:58:21 UTC
[PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
msyavuz opened a new pull request, #40815:
URL: https://github.com/apache/superset/pull/40815
### SUMMARY
Matrixify (subplot grid) builds its row/column axes from dimension **values frozen into `formData` at design time**. The cell data queries already enforce RLS, but the *grid structure itself* — which subplots exist and their header labels — is generated client-side from that frozen list with no per-viewer check. So an RLS-restricted viewer (notably an embedded guest) gets a subplot, with a value label, for **every** value the chart author could see — leaking the value identifiers and producing a grid of empty cells. This made the feature hard to use in embedded contexts.
Fix: resolve the allowed dimension values **per render** and intersect the stored axis values against them before building the grid.
- New `useMatrixifyAllowedValues` hook fetches the RLS-filtered distinct values for each dimension axis via the existing `/datasource/<type>/<id>/column/<col>/values/` endpoint, which applies the *requesting viewer's* RLS server-side.
- `MatrixifyGridRenderer` drops any frozen value not in the allow-list before generating the grid, so forbidden subplots and their labels are never emitted.
- **Fails closed**: the grid renders nothing until the allow-list resolves (loading spinner), and on fetch error it shows a message rather than the unfiltered list. The grid is never built from unfiltered values.
- No fetch for metrics-only matrixify. Effect is keyed on primitives so it won't refetch-loop when `formData` identity changes each render.
Applies in all contexts (explore, dashboard, embedded) since `MatrixifyGridRenderer` is the single render path via `SuperChart`.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — behavior change is per-viewer RLS filtering of which subplots appear; not easily captured as a static screenshot without an RLS-configured fixture.
### TESTING INSTRUCTIONS
1. Create a dataset with an RLS rule restricting a dimension (e.g. `region` to `US`) for a non-admin/embedded role.
2. As admin, build a chart with Matrixify enabled, rows = that dimension, selecting values the admin can see (e.g. `US`, `EU`, `APAC`).
3. View the chart (or embed it) as the restricted role/guest token.
4. **Before:** subplots/headers appear for `EU`, `APAC` (empty cells). **After:** only `US` renders.
Automated: `npm run test -- packages/superset-ui-core/src/chart/components/Matrixify` (hook + renderer suites: fetch/normalize, fail-closed, loading/error gates, value filtering).
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags: No
- [x] Changes UI — RLS-restricted viewers now see fewer/empty-free matrixify subplots; adds loading/error states to the grid
- [ ] Includes DB Migration: No
- [ ] Introduces new feature or API: No — reuses the existing datasource column-values endpoint
- [ ] Removes existing feature or API: No
Follow-ups (not in this PR): confirm the values endpoint is reachable under an embedded **guest token**; `topn` mode currently intersects rather than re-ranking per viewer, so a restricted viewer may see fewer than N subplots.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40815:
URL: https://github.com/apache/superset/pull/40815#discussion_r3797421952
##########
superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/useMatrixifyAllowedValues.ts:
##########
@@ -0,0 +1,182 @@
+/**
+ * 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>>;
+}
+
+export interface MatrixifyAllowedValuesFormData extends MatrixifyFormData {
+ /** ``${id}__${type}`` identifier the column-values endpoint is keyed by */
+ datasource?: string;
+}
+
+/**
+ * A resolution tagged with the request it belongs to, so a result fetched for a
+ * previous datasource/axis is never mistaken for the current one.
+ */
+interface ResolvedAllowedValues extends MatrixifyAllowedValuesState {
+ key: string;
+}
+
+const NO_ALLOWED_VALUES: Record<string, Set<string>> = {};
+const NO_DIMENSIONS_KEY = '[]';
+
+/**
+ * Collect the distinct dimension columns referenced by the matrixify axes.
+ */
+function getDimensionColumns(formData: MatrixifyFormData): string[] {
+ const config = getMatrixifyConfig(formData);
+ 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<unknown[]> {
+ 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: MatrixifyAllowedValuesFormData,
+): MatrixifyAllowedValuesState {
+ const { datasource } = formData;
+ // 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],
+ );
+ // Identifies the resolution this render requires; state tagged with any other
+ // key describes a datasource/axis the viewer is no longer looking at.
+ const fetchKey = `${datasource ?? ''}|${columnsKey}`;
+
+ const [resolved, setResolved] = useState<ResolvedAllowedValues | null>(null);
+
+ useEffect(() => {
+ const dimensionColumns: string[] = JSON.parse(columnsKey);
+
+ // Metrics-only matrixify has no dimension axes: nothing to resolve.
+ if (dimensionColumns.length === 0) {
Review Comment:
**Suggestion:** When the axes temporarily become metrics-only, this effect returns without clearing `resolved`. If the chart switches back to the same dimension axis, `fetchKey` matches the old resolution, so the hook reports `success` and the renderer builds one frame using the stale allow-list before the new request completes. This can expose values that are no longer permitted after an axis or authorization-context change; invalidate the previous resolution whenever the dimension set becomes empty or otherwise force a loading state for the new request. [stale reference]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Axis changes can reuse an outdated RLS allow-list.
- ⚠️ Previously allowed dimension labels may briefly reappear.
- ⚠️ Fresh authorization results arrive only after rendering.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4f5a5e8ed57a4dfabbf102ac275c2e9c&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=4f5a5e8ed57a4dfabbf102ac275c2e9c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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:** 117:118
**Comment:**
*Stale Reference: When the axes temporarily become metrics-only, this effect returns without clearing `resolved`. If the chart switches back to the same dimension axis, `fetchKey` matches the old resolution, so the hook reports `success` and the renderer builds one frame using the stale allow-list before the new request completes. This can expose values that are no longer permitted after an axis or authorization-context change; invalidate the previous resolution whenever the dimension set becomes empty or otherwise force a loading state for the new request.
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=c5179e629c21cdef609ee81ba53ef9621031433f9b416d0909454490f96efae7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40815&comment_hash=c5179e629c21cdef609ee81ba53ef9621031433f9b416d0909454490f96efae7&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.tsx:
##########
@@ -121,10 +142,48 @@ function MatrixifyGridRenderer({
height,
hooks,
}: MatrixifyGridRendererProps) {
- // Generate grid structure from form data
+ // Resolve which dimension values the current viewer is allowed to see, with
+ // row-level security applied server-side. 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 to restricted viewers.
+ const { status: allowedStatus, allowedByColumn } =
+ useMatrixifyAllowedValues(formData);
Review Comment:
**Suggestion:** The renderer receives the resolved datasource object through its `datasource` prop, but passes only `formData` to the allow-list hook. In render paths where raw form data does not include the serialized `id__type` datasource value, the hook reports an error and the chart permanently fails closed despite having a valid datasource. Pass the resolved datasource identifier to the hook or derive the endpoint from the supplied datasource prop. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Matrixify charts fail closed when raw form data omits datasource.
- ⚠️ Valid resolved datasource metadata is ignored.
- ❌ Explore, dashboard, or embedded matrix rendering can be replaced by an access-error message.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=de1577cd5f8d47788962db8ca8b5eef9&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=de1577cd5f8d47788962db8ca8b5eef9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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/MatrixifyGridRenderer.tsx
**Line:** 150:150
**Comment:**
*Api Mismatch: The renderer receives the resolved datasource object through its `datasource` prop, but passes only `formData` to the allow-list hook. In render paths where raw form data does not include the serialized `id__type` datasource value, the hook reports an error and the chart permanently fails closed despite having a valid datasource. Pass the resolved datasource identifier to the hook or derive the endpoint from the supplied datasource prop.
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=1bf4d9e9a75f83460f8e65455bf16c9bd37d6471f77f04fcc682f67972508a84&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40815&comment_hash=1bf4d9e9a75f83460f8e65455bf16c9bd37d6471f77f04fcc682f67972508a84&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40815:
URL: https://github.com/apache/superset/pull/40815#discussion_r4018367598
##########
superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/useMatrixifyAllowedValues.ts:
##########
@@ -0,0 +1,182 @@
+/**
+ * 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>>;
+}
+
+export interface MatrixifyAllowedValuesFormData extends MatrixifyFormData {
+ /** ``${id}__${type}`` identifier the column-values endpoint is keyed by */
+ datasource?: string;
+}
+
+/**
+ * A resolution tagged with the request it belongs to, so a result fetched for a
+ * previous datasource/axis is never mistaken for the current one.
+ */
+interface ResolvedAllowedValues extends MatrixifyAllowedValuesState {
+ key: string;
+}
+
+const NO_ALLOWED_VALUES: Record<string, Set<string>> = {};
+const NO_DIMENSIONS_KEY = '[]';
+
+/**
+ * Collect the distinct dimension columns referenced by the matrixify axes.
+ */
+function getDimensionColumns(formData: MatrixifyFormData): string[] {
+ const config = getMatrixifyConfig(formData);
+ 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<unknown[]> {
+ 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: MatrixifyAllowedValuesFormData,
+): MatrixifyAllowedValuesState {
+ const { datasource } = formData;
+ // 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],
+ );
+ // Identifies the resolution this render requires; state tagged with any other
+ // key describes a datasource/axis the viewer is no longer looking at.
+ const fetchKey = `${datasource ?? ''}|${columnsKey}`;
+
+ const [resolved, setResolved] = useState<ResolvedAllowedValues | null>(null);
+
+ useEffect(() => {
+ const dimensionColumns: string[] = JSON.parse(columnsKey);
+
+ // Metrics-only matrixify has no dimension axes: nothing to resolve.
+ if (dimensionColumns.length === 0) {
Review Comment:
Agreed, this one's real. If the axes go metrics-only and then back to the same dimension, `resolved.key` still matches the new `fetchKey` from before the detour, so the memo reports `success` with the old `allowedByColumn` while the fresh fetch is still in flight. Might be worth tagging `resolved` with something that busts on that intermediate state too, not just the key.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-5006030679
CI's got two real failures here right now, `babel-extract` (a source string got reworded, invalidating existing translations) and the ts coverage gate on the new hook. Mind sorting those out before this goes further? Holler if you want me to jump in on the translation bits or anything else.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-4635891366
<!-- translation-regression-bot -->
## ⚠️ Translation Regression Detected
A source change in this PR renamed or reworded strings, invalidating existing translations (they are now `#, fuzzy`) in `ar`, `ca`, `cs`, `de`, `fi`, `fr`, `it`, `ja`, `ko`, `lv`, `mi`, `nl`, `pt`, `ru`, `sk`, `sl`, `th`, `tr`, `uk`. Please resolve the affected `.po` files before merging.
_Note: intentionally **deleting** a translatable string is not a regression and is not flagged here — only translations invalidated by a renamed/reworded source string are._
| Language | Fuzzy before | Fuzzy after | New |
|----------|-------------:|------------:|----:|
| `ar` | 1136 | 1137 | +1 |
| `ca` | 815 | 816 | +1 |
| `cs` | 787 | 788 | +1 |
| `de` | 997 | 998 | +1 |
| `fi` | 4823 | 4824 | +1 |
| `fr` | 1014 | 1015 | +1 |
| `it` | 1667 | 1668 | +1 |
| `ja` | 322 | 323 | +1 |
| `ko` | 1521 | 1522 | +1 |
| `lv` | 295 | 296 | +1 |
| `mi` | 807 | 808 | +1 |
| `nl` | 1143 | 1144 | +1 |
| `pt` | 1839 | 1840 | +1 |
| `ru` | 293 | 294 | +1 |
| `sk` | 774 | 775 | +1 |
| `sl` | 963 | 964 | +1 |
| `th` | 4823 | 4824 | +1 |
| `tr` | 786 | 787 | +1 |
| `uk` | 293 | 294 | +1 |
### How to fix
**1. Install dependencies** (if not already set up):
```bash
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext
```
**2. Re-extract strings and sync `.po` files:**
```bash
./scripts/translations/babel_update.sh
```
This rewrites `superset/translations/messages.pot` from the current source files and merges the changes into every `.po` file. Strings whose `msgid` changed will be marked `#, fuzzy`.
**3. Resolve the fuzzy entries** in the affected language files (`ar`, `ca`, `cs`, `de`, `fi`, `fr`, `it`, `ja`, `ko`, `lv`, `mi`, `nl`, `pt`, `ru`, `sk`, `sl`, `th`, `tr`, `uk`):
```bash
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.po
```
For each fuzzy entry, either rewrite the `msgstr` to match the new string and remove the `#, fuzzy` line, or clear the `msgstr` to `""` if you cannot provide a translation.
**4. Commit your changes to the `.po` files.**
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-4635949251
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40815?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `97.10145%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 64.14%. Comparing base ([`108e40c`](https://app.codecov.io/gh/apache/superset/commit/108e40cbb61af67d72c4836d568d23ecf4a461a7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`498adc9`](https://app.codecov.io/gh/apache/superset/commit/498adc9722ee9c9c4742df31e330cddb38c6972c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 18 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/40815?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../components/Matrixify/useMatrixifyAllowedValues.ts](https://app.codecov.io/gh/apache/superset/pull/40815?src=pr&el=tree&filepath=superset-frontend%2Fpackages%2Fsuperset-ui-core%2Fsrc%2Fchart%2Fcomponents%2FMatrixify%2FuseMatrixifyAllowedValues.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvY2hhcnQvY29tcG9uZW50cy9NYXRyaXhpZnkvdXNlTWF0cml4aWZ5QWxsb3dlZFZhbHVlcy50cw==) | 95.65% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40815?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
:x: Your project check has failed because the head coverage (99.95%) is below the target coverage (100.00%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40815 +/- ##
==========================================
+ Coverage 64.13% 64.14% +0.01%
==========================================
Files 2666 2667 +1
Lines 143979 144047 +68
Branches 33105 33120 +15
==========================================
+ Hits 92337 92402 +65
- Misses 50034 50037 +3
Partials 1608 1608
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40815/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/40815/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `67.77% <97.10%> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/40815?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40815:
URL: https://github.com/apache/superset/pull/40815#discussion_r4018365106
##########
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:
This is the stale-state race I flagged on my first pass, fixed since: the hook now derives status by comparing `resolved.key` against the live `fetchKey` (see `useMatrixifyAllowedValues.ts` on `e266594`), so a render with a new axis/datasource reports `loading` immediately instead of reusing the old `success` state. Resolving.
##########
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;
Review Comment:
Same round of cleanup, `datasource` is a typed field on `MatrixifyAllowedValuesFormData` now instead of `(formData as any)?.datasource`. No more `any` cast on this path. Resolving.
##########
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: {},
+ });
+
+ useEffect(() => {
+ const dimensionColumns: string[] = JSON.parse(columnsKey);
+
+ // Metrics-only matrixify has no dimension axes: nothing to resolve.
+ if (dimensionColumns.length === 0) {
+ setState({ status: 'success', allowedByColumn: {} });
+ return undefined;
+ }
+
+ if (!datasource) {
+ setState({ status: 'error', allowedByColumn: {} });
+ return undefined;
+ }
+
+ const controller = new AbortController();
+ setState(prev => ({ ...prev, status: 'loading' }));
+
+ (async () => {
+ try {
+ const results = await Promise.all(
+ dimensionColumns.map(column =>
+ fetchAllowedValues(datasource, column, controller.signal).then(
+ values => [column, values] as const,
+ ),
+ ),
+ );
+ if (controller.signal.aborted) {
+ return;
+ }
+ const allowedByColumn: Record<string, Set<string>> = {};
+ results.forEach(([column, values]) => {
+ allowedByColumn[column] = new Set(values.map(v => String(v)));
+ });
Review Comment:
Think this one's a false positive. `column` only ever comes from `getDimensionColumns`, real dataset column names pulled from the chart's own `matrixify_dimension_rows`/`_columns` config, not user input. And assigning `allowedByColumn['__proto__']` on a plain object literal just swaps that one object's own prototype link, it doesn't touch `Object.prototype` globally. Resolving.
##########
superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.tsx:
##########
@@ -121,10 +142,48 @@ function MatrixifyGridRenderer({
height,
hooks,
}: MatrixifyGridRendererProps) {
- // Generate grid structure from form data
+ // Resolve which dimension values the current viewer is allowed to see, with
+ // row-level security applied server-side. 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 to restricted viewers.
+ const { status: allowedStatus, allowedByColumn } =
+ useMatrixifyAllowedValues(formData);
Review Comment:
Also looks like a false positive: the `formData` passed in is `chartProps.rawFormData` (`SuperChart.tsx`), which always carries the base `datasource` string on every chart render path, explore, dashboard, or embedded. The `datasource` prop on the renderer is a separate resolved-metadata object, not the id/type string this hook needs, so there's nothing to swap. Resolving.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40815:
URL: https://github.com/apache/superset/pull/40815#discussion_r4018367223
##########
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;
Review Comment:
<!-- Bito Reply -->
The user's update correctly addresses the reviewer's concern by replacing the unsafe `(formData as any)?.datasource` type cast with a properly typed `datasource` field in `MatrixifyAllowedValuesFormData`. This change improves type safety and eliminates the potential for runtime errors associated with the previous `any` cast.
##########
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: {},
+ });
+
+ useEffect(() => {
+ const dimensionColumns: string[] = JSON.parse(columnsKey);
+
+ // Metrics-only matrixify has no dimension axes: nothing to resolve.
+ if (dimensionColumns.length === 0) {
+ setState({ status: 'success', allowedByColumn: {} });
+ return undefined;
+ }
+
+ if (!datasource) {
+ setState({ status: 'error', allowedByColumn: {} });
+ return undefined;
+ }
+
+ const controller = new AbortController();
+ setState(prev => ({ ...prev, status: 'loading' }));
+
+ (async () => {
+ try {
+ const results = await Promise.all(
+ dimensionColumns.map(column =>
+ fetchAllowedValues(datasource, column, controller.signal).then(
+ values => [column, values] as const,
+ ),
+ ),
+ );
+ if (controller.signal.aborted) {
+ return;
+ }
+ const allowedByColumn: Record<string, Set<string>> = {};
+ results.forEach(([column, values]) => {
+ allowedByColumn[column] = new Set(values.map(v => String(v)));
+ });
Review Comment:
<!-- Bito Reply -->
The assessment that this is a false positive is correct. Since the `column` keys are derived from internal configuration rather than direct user input, and the assignment `allowedByColumn[column]` on a plain object literal does not affect the global `Object.prototype`, the risk of prototype pollution is not present in this specific implementation. The suggestion can be safely ignored.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-4635942320
<!-- Bito Reply -->
The flagged issue is correct. The `useMatrixifyAllowedValues` hook relies on `useState` initialized only on mount, which causes it to retain stale `success` state when the `formData` (and thus the `columnsKey`) changes. This creates a race condition where the grid can be rendered using stale, unfiltered data before the new request completes.
To resolve this, you should track the `columnsKey` in state or use a `useEffect` that resets the status to `loading` whenever the `columnsKey` changes. Here is a concise implementation pattern:
```typescript
// Inside useMatrixifyAllowedValues.ts
const [state, setState] = useState({ status: 'loading', allowedByColumn: {} });
const columnsKey = getDimensionColumns(formData).join(',');
useEffect(() => {
setState(prev => ({ ...prev, status: 'loading' }));
// ... trigger fetch ...
}, [columnsKey]);
```
I have checked the available PR context, and there are no other actionable review comments to address at this time. Would you like me to perform any further analysis on this PR?
**superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/useMatrixifyAllowedValues.ts**
```
const [state, setState] = useState({ status: 'loading', allowedByColumn: {} });
const columnsKey = getDimensionColumns(formData).join(',');
useEffect(() => {
setState(prev => ({ ...prev, status: 'loading' }));
// ... trigger fetch ...
}, [columnsKey]);
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-5018548553
One thing I'm curious about: does the embedded guest role actually get `can_get_column_values` on `Datasource`? That's a distinct permission from what Public/Gamma get by default, so if it's missing, the exact embedded case this targets would just render a blank grid instead of leaking values, which is safe but might mean the fix doesn't actually help out of the box. Worth confirming before merge.
Also, `babel-extract` is red on the new translatable string, looks like it needs `./scripts/translations/babel_update.sh`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-4635879696
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40815:
URL: https://github.com/apache/superset/pull/40815#discussion_r3365706041
##########
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;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE--20: Unsafe Type Cast</b></div>
<div id="fix">
The `datasource` property is accessed via `(formData as any)?.datasource` but `MatrixifyFormData` does not declare this field. Using `any` bypasses type checking and could mask runtime errors if the property is missing. Either add `datasource?: string` to the interface or use a documented type projection.
</div>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a href="https://github.com/apache/superset/blob/498adc9/.cursor/rules/dev-standard.mdc#L16">dev-standard.mdc:16</a>
</li>
</ul>
</details>
<small><i>Code Review Run #49b0f3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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: {},
+ });
+
+ useEffect(() => {
+ const dimensionColumns: string[] = JSON.parse(columnsKey);
+
+ // Metrics-only matrixify has no dimension axes: nothing to resolve.
+ if (dimensionColumns.length === 0) {
+ setState({ status: 'success', allowedByColumn: {} });
+ return undefined;
+ }
+
+ if (!datasource) {
+ setState({ status: 'error', allowedByColumn: {} });
+ return undefined;
+ }
+
+ const controller = new AbortController();
+ setState(prev => ({ ...prev, status: 'loading' }));
+
+ (async () => {
+ try {
+ const results = await Promise.all(
+ dimensionColumns.map(column =>
+ fetchAllowedValues(datasource, column, controller.signal).then(
+ values => [column, values] as const,
+ ),
+ ),
+ );
+ if (controller.signal.aborted) {
+ return;
+ }
+ const allowedByColumn: Record<string, Set<string>> = {};
+ results.forEach(([column, values]) => {
+ allowedByColumn[column] = new Set(values.map(v => String(v)));
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-1321: Object injection sink with dynamic key</b></div>
<div id="fix">
Object injection sink at line 129: `column` used as dynamic object key without validation. This could enable prototype pollution ([CWE-1321](https://cwe.mitre.org/data/definitions/1321.html)) if `column` contains malicious values from formData. Validate that `column` is a safe identifier before property assignment.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
if (controller.signal.aborted) {
return;
}
// Validate column names to prevent object injection attacks (CWE-1321)
const safeColumnPattern = /^[_a-zA-Z][_a-zA-Z0-9]*$/;
const allowedByColumn: Record<string, Set<string>> = {};
results.forEach(([column, values]) => {
if (!safeColumnPattern.test(column)) {
console.warn(`Invalid column name skipped: ${column}`);
return;
}
allowedByColumn[column] = new Set(values.map(v => String(v)));
});
````
</div>
</details>
</div>
<small><i>Code Review Run #49b0f3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-6487401882
## <a href="https://codeant.ai/" title="CodeAnt AI"><img src="https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/codeant-block-purple-v2.svg" width="28" height="28" align="absmiddle" style="vertical-align:middle" alt="CodeAnt" /></a> CodeAnt PR Risk
**🟡 Medium Risk**
- The PR needs attention before merging.
- After switching to metrics-only and back, the hook can reuse an old allow-list for one render, briefly exposing dimension labels before fresh values resolve.
**Assessed commit:** `e2afb1463d89`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-5099501923
Thanks for digging into this, @msyavuz. This is a legit leak and failing closed is the right call.
One thing to check on: `useMatrixifyAllowedValues`'s state only resets inside the effect, so if `formData`'s dimension column changes after a prior fetch resolved, one render can use the stale `allowedByColumn` (missing the new column, so effectively unfiltered) before status flips back to loading. Worth confirming/fixing that gap since the whole point here is never rendering unfiltered, even briefly. :)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-4988600205
Lookin' good. Only thing I'd want cleaned up before merging is the handful of `any` casts in `useMatrixifyAllowedValues`/`MatrixifyGridRenderer`, since we've been trying to keep new code `any`-free.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-4635893167
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 498adc9722ee9c9c4742df31e330cddb38c6972c |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a23467dccde9d00099a9f2f |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-40815--superset-docs-preview.netlify.app](https://deploy-preview-40815--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctLDoMgEADQu8xaIqFGQ2_QY0wZiJPwC6CRNt5ddm_z_pBLCrnBG2CCb8Fo9uFQO57Hb3Z8zQFb4YtdF8VXQRxsrJyiONEfto5FNvvUPzTeiuq1rBsZQ1aTlFJr1E45uB8) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-40815--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-5448102850
@msyavuz this looks better now! Thanks for cleaning up the `any` casts and the stale-allowlist race from my last pass.
This still says "Fixes #43258", but per your reply to @mikemazara this PR only covers the RLS leak, not the dashboard-filter/25-cap issue #43258 is actually about. Mind dropping that closing link so merging doesn't quietly close it out from under his follow-up?
Also never got your take on whether the embedded guest role actually has `can_get_column_values` on `Datasource` by default. That's the case this PR is tackling, so I wanted to make sure it isn't just going to render the "unable to verify" message for them.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "mikemazara (via GitHub)" <gi...@apache.org>.
mikemazara commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-5317600544
@msyavuz happy to see this PR open, resolving this at render is the right place for it. I filed #43258 about a related case
and I think your hook is close to covering both.
`fetchAllowedValues` hits the values endpoint with no filters, so the allow-list picks up
dataset-level RLS but not the viewer's dashboard filters. On 6.1.0 with a matrixified chart
(69 members, `all` mode so capped at 25): for a restricted user the endpoint returns 2 values
and the intersection nicely collapses the grid to 2 panels. For an admin it returns all ~70,
so nothing gets intersected away and 6 members that do have data still never render — their
narrowing comes from a native filter, not the dataset. Same dashboard, different behaviour
depending on role.
Would you be open to passing the chart's effective filters into that fetch (or swapping it for
a chart-data query)? Or would you rather keep this focused and I follow up on top?
Separately, the intersection can't help with members that `MAX_ALL_DIMENSION_VALUES = 25` drops
before the fetch — that probably needs its own fix.
Happy to test a branch, I have a repro.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-5317898074
> @msyavuz happy to see this PR open, resolving this at render is the right place for it. I filed https://github.com/apache/superset/issues/43258 about a related case
> and I think your hook is close to covering both.
>
> fetchAllowedValues hits the values endpoint with no filters, so the allow-list picks up
> dataset-level RLS but not the viewer's dashboard filters. On 6.1.0 with a matrixified chart
> (69 members, all mode so capped at 25): for a restricted user the endpoint returns 2 values
> and the intersection nicely collapses the grid to 2 panels. For an admin it returns all ~70,
> so nothing gets intersected away and 6 members that do have data still never render — their
> narrowing comes from a native filter, not the dataset. Same dashboard, different behaviour
> depending on role.
>
> Would you be open to passing the chart's effective filters into that fetch (or swapping it for
> a chart-data query)? Or would you rather keep this focused and I follow up on top?
>
> Separately, the intersection can't help with members that MAX_ALL_DIMENSION_VALUES = 25 drops
> before the fetch — that probably needs its own fix.
>
> Happy to test a branch, I have a repro.
Hey, thanks for the issue. I'll clean this up and push it forward by asking for some reviews. If you already have an approach and a branch i think it's best if i keep this focused and you follow up with your pr on top since i don't have much bandwidth to work on this. Thank you again @mikemazara!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(explore): apply RLS to matrixify dimension values at render [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40815:
URL: https://github.com/apache/superset/pull/40815#issuecomment-5319644474
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org