fix(explore): apply RLS to matrixify dimension values at render#40815
fix(explore): apply RLS to matrixify dimension values at render#40815msyavuz wants to merge 1 commit into
Conversation
dosubot
Bot
added
authentication:row-level-security
|
| 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):
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext2. Re-extract strings and sync .po files:
./scripts/translations/babel_update.shThis 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):
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.poFor 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.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const [state, setState] = useState<MatrixifyAllowedValuesState>({ | ||
| status: columnsKey === '[]' ? 'success' : 'loading', | ||
| allowedByColumn: {}, | ||
| }); |
There was a problem hiding this 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]
Severity Level: Critical 🚨
- ❌ 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.Steps of Reproduction ✅
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'`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
|
The flagged issue is correct. The To resolve this, you should track the // 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 |
Codecov Report❌ Patch coverage is
❌ 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 coverage. Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #49b0f3
Actionable Suggestions - 2
-
superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/useMatrixifyAllowedValues.ts - 2
- CWE--20: Unsafe Type Cast · Line 84-84
- CWE-1321: Object injection sink with dynamic key · Line 124-130
Additional Suggestions - 3
-
superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/useMatrixifyAllowedValues.ts - 1
-
CWE--20: Unsafe Type Cast · Line 39-39The `getDimensionColumns` function casts `formData as any` when calling `getMatrixifyConfig`, which accepts `MatrixifyFormData & any`. This pattern should use a more specific type projection to maintain type safety.
-
-
superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.test.tsx - 2
-
Incomplete assertion for loading indicator · Line 438-438Test name 'shows a loading indicator' implies verification of the Loading component rendering, but only checks grid cells === 0. Add explicit assertion to verify Loading is present, matching the documented behavior in MatrixifyGridRenderer.tsx lines 221-227.
Code suggestion
--- a/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.test.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.test.tsx @@ -455,6 +455,8 @@ test('shows a loading indicator while the RLS allow-list resolves', () => { // The grid is never built while resolution is in flight expect(mockGenerateMatrixifyGrid).not.toHaveBeenCalled(); const gridCells = container.querySelectorAll('[data-testid^="grid-cell-"]'); expect(gridCells).toHaveLength(0); + // Verify the loading indicator is actually rendered + expect(container.querySelector('[class*="loading"]') ?? container.querySelector('[class*="Loading"]')).toBeInTheDocument(); });
-
Test missing error message assertion · Line 461-481The error-state test verifies no cells render but doesn't assert the user-facing error message (`GridMessage` with 'Unable to verify access to dimension values.') is displayed. Per rule [6262], tests should verify the actual UX behavior, not just absence of content.
-
Review Details
-
Files reviewed - 4 · Commit Range:
498adc9..498adc9- superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.test.tsx
- superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.tsx
- superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/useMatrixifyAllowedValues.test.ts
- superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/useMatrixifyAllowedValues.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| export function useMatrixifyAllowedValues( | ||
| formData: MatrixifyFormData, | ||
| ): MatrixifyAllowedValuesState { | ||
| const datasource = (formData as any)?.datasource as string | undefined; |
There was a problem hiding this comment.
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.
Citations
- Rule Violated: dev-standard.mdc:16
Code Review Run #49b0f3
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| if (controller.signal.aborted) { | ||
| return; | ||
| } | ||
| const allowedByColumn: Record<string, Set<string>> = {}; | ||
| results.forEach(([column, values]) => { | ||
| allowedByColumn[column] = new Set(values.map(v => String(v))); | ||
| }); |
There was a problem hiding this comment.
Object injection sink at line 129: column used as dynamic object key without validation. This could enable prototype pollution (CWE-1321) if column contains malicious values from formData. Validate that column is a safe identifier before property assignment.
Code suggestion
Check the AI-generated fix before applying
| if (controller.signal.aborted) { | |
| return; | |
| } | |
| const allowedByColumn: Record<string, Set<string>> = {}; | |
| results.forEach(([column, values]) => { | |
| allowedByColumn[column] = new Set(values.map(v => String(v))); | |
| }); | |
| 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))); | |
| }); |
Code Review Run #49b0f3
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
1 participant
SUMMARY
Matrixify (subplot grid) builds its row/column axes from dimension values frozen into
formDataat 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.
useMatrixifyAllowedValueshook 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.MatrixifyGridRendererdrops any frozen value not in the allow-list before generating the grid, so forbidden subplots and their labels are never emitted.formDataidentity changes each render.Applies in all contexts (explore, dashboard, embedded) since
MatrixifyGridRendereris the single render path viaSuperChart.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
regiontoUS) for a non-admin/embedded role.US,EU,APAC).EU,APAC(empty cells). After: onlyUSrenders.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
Follow-ups (not in this PR): confirm the values endpoint is reachable under an embedded guest token;
topnmode currently intersects rather than re-ranking per viewer, so a restricted viewer may see fewer than N subplots.