fix: scope deck_layers base-filter bypass to guests, harden viz migration, fix Explore layer preview lag

rusackas · claude · rusackas · commit 1a2bee4954c0 · 2026-08-06T00:03:15.000-07:00
- superset/charts/api.py: only skip the chart base filter for the deck_layers
  container-based lookup when the caller is an embedded guest; an ordinary
  logged-in user could otherwise read an arbitrary chart's params/datasource
  by naming it in a deck_multi container's deck_slices they can edit.
- superset/migrations/shared/migrate_viz/base.py: back up a stored
  query_context with "queries": null, or a non-object query_context, through
  the same FULL_CONTEXT_BAK_KEY wholesale path used for a missing "queries"
  key, so downgrade can tell "no context" apart from "context had a null/odd
  queries" instead of discarding the slice's datasource/form_data, and so a
  non-dict context doesn't raise mid-migration and leave the row
  half-migrated.
- Multi.tsx: fall back to per-chart reads for any deck_slices id missing
  from the container's persisted deck_layers response, so a layer just
  added in Explore (but not yet saved) still previews instead of waiting
  for a save.

Co-Authored-By: Claude Opus 4.8 &lt;noreply@anthropic.com&gt;
diff --git a/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.test.tsx b/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.test.tsx
@@ -636,3 +636,87 @@ test('does not include parent_slice_id when parent has no slice_id', async () =>
     expect(formData.parent_slice_id).toBeUndefined();
   });
 });
+
+test('falls back to a per-chart read for a layer missing from the persisted deck_slices', async () => {
+  // A saved container's `deck_slices` on the server can lag the in-memory
+  // Explore selection (e.g. layer 3 was just added but not saved yet), so
+  // the bulk deck_layers response only resolves layers 1 and 2. Layer 3
+  // must still be fetched (and previewed) via a per-chart read rather than
+  // silently dropped until the chart is saved.
+  jest.clearAllMocks();
+  featuresByVizType = {};
+  const parentSliceId = 50;
+
+  (SupersetClient.get as jest.Mock).mockImplementation(
+    ({ endpoint }: { endpoint: string }) => {
+      if (endpoint === `/api/v1/chart/${parentSliceId}/deck_layers/`) {
+        return Promise.resolve({
+          json: {
+            result: [1, 2].map(sliceId => ({
+              slice_id: sliceId,
+              viz_type: SUBSLICES[sliceId].vizType,
+              datasource_id: 1,
+              datasource_type: 'table',
+              params: JSON.stringify({
+                viz_type: SUBSLICES[sliceId].vizType,
+                datasource: 'test_datasource',
+              }),
+            })),
+          },
+        });
+      }
+      const subslice = { vizType: 'deck_scatter' };
+      return Promise.resolve({
+        json: {
+          result: {
+            viz_type: subslice.vizType,
+            datasource_id: 1,
+            datasource_type: 'table',
+            params: JSON.stringify({
+              viz_type: subslice.vizType,
+              datasource: 'test_datasource',
+            }),
+          },
+        },
+      });
+    },
+  );
+  (SupersetClient.post as jest.Mock).mockImplementation(
+    ({ jsonPayload }: { jsonPayload: { form_data: { viz_type: string } } }) =>
+      Promise.resolve({
+        json: {
+          result: [
+            { data: featuresByVizType[jsonPayload.form_data.viz_type] || [] },
+          ],
+        },
+      }),
+  );
+
+  const props = {
+    ...baseMockProps,
+    formData: {
+      ...baseMockProps.formData,
+      slice_id: parentSliceId,
+      deck_slices: [1, 2, 3],
+    },
+  };
+
+  renderWithProviders(<DeckMulti {...props} />);
+
+  await waitFor(() =>
+    expect(SupersetClient.get).toHaveBeenCalledWith(
+      expect.objectContaining({ endpoint: '/api/v1/chart/3' }),
+    ),
+  );
+  expect(SupersetClient.get).toHaveBeenCalledWith(
+    expect.objectContaining({
+      endpoint: `/api/v1/chart/${parentSliceId}/deck_layers/`,
+    }),
+  );
+  expect(SupersetClient.get).not.toHaveBeenCalledWith(
+    expect.objectContaining({ endpoint: '/api/v1/chart/1' }),
+  );
+  expect(SupersetClient.get).not.toHaveBeenCalledWith(
+    expect.objectContaining({ endpoint: '/api/v1/chart/2' }),
+  );
+});
diff --git a/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx b/superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx
@@ -606,12 +606,26 @@ const DeckMulti = (props: DeckMultiProps) => {
       })
         .then(({ json }) => {
           const layers = ((json as JsonObject).result || []) as JsonObject[];
-          return layers
+          const resolved = layers
             .map(layer => toLayerFormData(layer.slice_id, layer))
             .filter(
               (slice): slice is { slice_id: number } & JsonObject =>
                 slice !== null && sliceIds.includes(slice.slice_id),
             );
+          // The container's persisted deck_slices can lag the in-memory
+          // Explore selection (e.g. a layer just added but not yet saved),
+          // so it won't be in the bulk response above. Fall back to
+          // per-chart reads for whichever requested ids weren't resolved,
+          // so newly selected layers still preview before saving.
+          const resolvedIds = new Set(resolved.map(slice => slice.slice_id));
+          const missingIds = sliceIds.filter(id => !resolvedIds.has(id));
+          if (missingIds.length === 0) {
+            return resolved;
+          }
+          return fetchSubslicesPerChart(missingIds).then(extra => [
+            ...resolved,
+            ...extra,
+          ]);
         })
         .catch(() => fetchSubslicesPerChart(sliceIds));
     },
diff --git a/superset/charts/api.py b/superset/charts/api.py
@@ -489,11 +489,18 @@ def deck_layers(self, pk: int) -> Response:
         if not deck_slice_ids:
             return self.response(200, result=[])
 
-        # The container's own access has already been checked above; the
-        # layers it declares are resolved without the base filter (they
-        # sit on no dashboard of their own), same as the legacy explore_json
-        # pipeline resolved them server-side under the container's access.
-        layers = ChartDAO.find_by_ids(deck_slice_ids, skip_base_filter=True)
+        # The container's own access has already been checked above. Layer
+        # charts sit on no dashboard of their own, so for an embedded guest
+        # (the intended use case, mirroring what the legacy explore_json
+        # pipeline granted server-side) they are resolved without the base
+        # filter. An ordinary logged-in principal is not entitled to read an
+        # arbitrary chart's params/datasource just by naming it in a
+        # container they can edit, so the base filter still applies to them:
+        # a referenced layer they can't otherwise read is silently omitted
+        # below rather than leaked.
+        layers = ChartDAO.find_by_ids(
+            deck_slice_ids, skip_base_filter=security_manager.is_guest_user()
+        )
         layers_by_id = {layer.id: layer for layer in layers}
         result = [
             {
diff --git a/superset/migrations/shared/migrate_viz/base.py b/superset/migrations/shared/migrate_viz/base.py
@@ -171,16 +171,22 @@ def upgrade_slice(cls, slc: Slice) -> None:
             query_context = try_load_json(slc.query_context)
             queries_bak = None
 
-            if query_context:
-                # A stored query_context is expected to carry "queries", but
-                # an atypical/malformed one (e.g. hand-edited via the API)
-                # missing it must not raise here: viz_type was already
+            if isinstance(query_context, dict) and query_context:
+                # A stored query_context is expected to be an object carrying
+                # a non-null "queries" list, but an atypical/malformed one
+                # (e.g. hand-edited via the API) missing that key, or with
+                # "queries": null, must not raise here: viz_type was already
                 # flipped above, so an uncaught exception at this point
                 # would leave the slice half-migrated (new viz_type, but
                 # stale params/query_context in the old shape). Back up the
                 # whole context in that case so downgrade can restore it
                 # verbatim instead of losing it (see FULL_CONTEXT_BAK_KEY).
-                if "queries" in query_context:
+                # Both cases must share this sentinel path rather than
+                # backing up a bare `None` -- that value is indistinguishable
+                # from "no context was ever stored", which would make
+                # downgrade discard the slice's original datasource/form_data
+                # instead of restoring this context.
+                if "queries" in query_context and query_context["queries"] is not None:
                     queries_bak = copy.deepcopy(query_context["queries"])
                 else:
                     queries_bak = {FULL_CONTEXT_BAK_KEY: copy.deepcopy(query_context)}
@@ -190,6 +196,15 @@ def upgrade_slice(cls, slc: Slice) -> None:
 
                 queries = clz._build_query()["queries"]
                 query_context["queries"] = queries
+            elif query_context:
+                # A parseable but non-object query_context (e.g. a bare
+                # number or a JSON list -- both accepted by the schema
+                # validator) can't carry "queries"/"form_data" keys; back it
+                # up wholesale like the cases above and rebuild a fresh one,
+                # rather than raising on membership-testing a non-dict (which
+                # would leave the slice half-migrated, per the note above).
+                queries_bak = {FULL_CONTEXT_BAK_KEY: copy.deepcopy(query_context)}
+                query_context = clz._build_query()
             else:
                 query_context = clz._build_query()
 
diff --git a/tests/integration_tests/charts/api_tests.py b/tests/integration_tests/charts/api_tests.py
@@ -1223,6 +1223,56 @@ def test_get_deck_layers_no_container_access(self):
         db.session.delete(container)
         db.session.commit()
 
+    @pytest.mark.usefixtures("load_energy_table_with_slice")
+    def test_get_deck_layers_omits_inaccessible_layer_for_ordinary_user(self):
+        """
+        Chart API: An ordinary (non-guest) user with access to the deck_multi
+        container must not have an inaccessible layer's params/datasource
+        leaked just because it's named in the container's `deck_slices` --
+        that layer is silently omitted from the result instead.
+        """
+        admin = self.get_user("admin")
+        gamma = self.get_user("gamma")
+        layer_visible = self.insert_chart(
+            "layer visible",
+            [gamma.id],
+            1,
+            viz_type="deck_scatter",
+            params=json.dumps({"viz_type": "deck_scatter"}),
+        )
+        layer_hidden = self.insert_chart(
+            "layer hidden from gamma",
+            [admin.id],
+            1,
+            viz_type="deck_scatter",
+            params=json.dumps({"viz_type": "deck_scatter"}),
+        )
+        container = self.insert_chart(
+            "deck multi container for gamma",
+            [gamma.id],
+            1,
+            viz_type="deck_multi",
+            params=json.dumps(
+                {
+                    "viz_type": "deck_multi",
+                    "deck_slices": [layer_visible.id, layer_hidden.id],
+                }
+            ),
+        )
+        self.login(GAMMA_USERNAME)
+        uri = f"api/v1/chart/{container.id}/deck_layers/"
+        rv = self.get_assert_metric(uri, "deck_layers")
+        assert rv.status_code == 200
+        data = json.loads(rv.data.decode("utf-8"))
+        assert [layer["slice_id"] for layer in data["result"]] == [
+            layer_visible.id,
+        ]
+
+        db.session.delete(layer_visible)
+        db.session.delete(layer_hidden)
+        db.session.delete(container)
+        db.session.commit()
+
     @pytest.mark.usefixtures(
         "load_energy_table_with_slice",
         "load_birth_names_dashboard_with_slices",
diff --git a/tests/unit_tests/migrations/viz/upgrade_malformed_query_context_test.py b/tests/unit_tests/migrations/viz/upgrade_malformed_query_context_test.py
@@ -58,6 +58,69 @@ def test_upgrade_slice_survives_a_query_context_without_queries() -> None:
     assert json.loads(slc.query_context) == original_query_context
 
 
+def test_downgrade_slice_restores_an_original_null_queries() -> None:
+    """An original query_context with "queries": null backs up as a bare
+    `None`, indistinguishable from "no context was ever stored" -- it must
+    be routed through the same FULL_CONTEXT_BAK_KEY wholesale backup as a
+    missing "queries" key so downgrade_slice can tell the two apart and
+    restore the slice's original datasource/form_data instead of discarding
+    them."""
+    source = {"viz_type": "line", "datasource": "1__table", "x_axis_label": "x"}
+    original_query_context = {"datasource": "1__table", "queries": None}
+
+    slc = Slice(
+        viz_type="line",
+        datasource_type="table",
+        params=json.dumps(source),
+        query_context=json.dumps(original_query_context),
+    )
+
+    MigrateLineChart.upgrade_slice(slc)
+    upgraded_params = json.loads(slc.params)
+    assert upgraded_params.get("queries_bak") == {
+        FULL_CONTEXT_BAK_KEY: original_query_context
+    }
+
+    MigrateLineChart.downgrade_slice(slc)
+
+    assert slc.viz_type == "line"
+    assert json.loads(slc.params) == source
+    assert json.loads(slc.query_context) == original_query_context
+
+
+def test_upgrade_slice_survives_a_non_object_query_context() -> None:
+    """A parseable but non-object query_context (e.g. a bare number or a
+    JSON list, both accepted by the schema validator) must not raise when
+    membership-tested for "queries" -- that would leave the slice
+    half-migrated (new viz_type already set, but stale params/query_context
+    in the old shape) since the exception is swallowed by the broad
+    top-level catch."""
+    source = {"viz_type": "line", "datasource": "1__table", "x_axis_label": "x"}
+
+    slc = Slice(
+        viz_type="line",
+        datasource_type="table",
+        params=json.dumps(source),
+        query_context=json.dumps(1),
+    )
+
+    MigrateLineChart.upgrade_slice(slc)
+
+    assert slc.viz_type == "echarts_timeseries_line"
+    upgraded_params = json.loads(slc.params)
+    assert upgraded_params.get("queries_bak") == {FULL_CONTEXT_BAK_KEY: 1}
+    # A fresh query_context was rebuilt rather than left in the old shape.
+    assert json.loads(slc.query_context)["form_data"]["viz_type"] == (
+        "echarts_timeseries_line"
+    )
+
+    MigrateLineChart.downgrade_slice(slc)
+
+    assert slc.viz_type == "line"
+    assert json.loads(slc.params) == source
+    assert json.loads(slc.query_context) == 1
+
+
 def test_downgrade_slice_restores_an_original_empty_queries_list() -> None:
     """An original query_context with "queries": [] backs up as a falsy-but-
     present list, not None -- downgrade_slice must not mistake that for "no
