This is an automated email from the ASF dual-hosted git repository.
sha174n pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 4d8dfdd1520 fix(sqllab): restrict TabStateView updates to owned
columns and queries (#43992)
4d8dfdd1520 is described below
commit 4d8dfdd1520d9b26c0b713b0d707481d36526665
Author: Shaitan <[email protected]>
AuthorDate: Wed Sep 9 14:03:30 2026 +0100
fix(sqllab): restrict TabStateView updates to owned columns and queries
(#43992)
Co-authored-by: sha174n <[email protected]>
Co-authored-by: Claude Opus 4.8 <[email protected]>
---
superset/views/sql_lab/views.py | 65 ++++++-
.../views/test_sql_lab_tab_state_views.py | 203 ++++++++++++++++++++-
2 files changed, 253 insertions(+), 15 deletions(-)
diff --git a/superset/views/sql_lab/views.py b/superset/views/sql_lab/views.py
index 7a12f5b20df..a4a85091207 100644
--- a/superset/views/sql_lab/views.py
+++ b/superset/views/sql_lab/views.py
@@ -21,7 +21,7 @@ from flask import request, Response
from flask_appbuilder import expose
from flask_appbuilder.security.decorators import has_access, has_access_api
from flask_babel import gettext as __
-from sqlalchemy import and_
+from sqlalchemy import and_, or_
from superset import db
from superset.models.sql_lab import Query, TableSchema, TabState
@@ -51,6 +51,29 @@ def _get_tab_user_id(tab_state_id: int) -> int | None:
return
db.session.query(TabState.user_id).filter_by(id=tab_state_id).scalar()
+# Columns a client may set through ``TabStateView.put``: the fields the SQL Lab
+# editor auto-sync sends, plus ``saved_query_id``. Identity columns (``id``,
+# ``user_id``) are excluded so a tab state stays bound to its creating user,
and
+# ``active`` is excluded because ``activate`` owns it -- that endpoint rewrites
+# the flag across all of the user's tabs to keep exactly one of them active.
+_TAB_STATE_PUT_FIELDS = frozenset(
+ {
+ "label",
+ "database_id",
+ "catalog",
+ "schema",
+ "sql",
+ "query_limit",
+ "latest_query_id",
+ "autorun",
+ "template_params",
+ "hide_left_bar",
+ "saved_query_id",
+ "extra_json",
+ }
+)
+
+
class TabStateView(BaseSupersetView):
@has_access_api
@expose("/", methods=("POST",))
@@ -146,14 +169,33 @@ class TabStateView(BaseSupersetView):
@has_access_api
@expose("<int:tab_state_id>", methods=("PUT",))
def put(self, tab_state_id: int) -> FlaskResponse:
+ user_id = get_user_id()
tab_user_id = _get_tab_user_id(tab_state_id)
if tab_user_id is None:
return Response(status=404)
- if tab_user_id != get_user_id():
+ if tab_user_id != user_id:
return Response(status=403)
try:
- fields = {k: json.loads(v) for k, v in
request.form.to_dict().items()}
+ fields = {
+ k: json.loads(v)
+ for k, v in request.form.to_dict().items()
+ if k in _TAB_STATE_PUT_FIELDS
+ }
+ # Drop latest_query_id only when it points at a query owned by a
+ # different user; the caller's own and unowned queries are fine.
+ if (latest_query_id := fields.get("latest_query_id")) is not None:
+ owned_by_other = (
+ db.session.query(Query.id)
+ .filter(
+ Query.client_id == latest_query_id,
+ Query.user_id.isnot(None),
+ Query.user_id != user_id,
+ )
+ .first()
+ )
+ if owned_by_other:
+ del fields["latest_query_id"]
db.session.query(TabState).filter_by(id=tab_state_id).update(fields)
db.session.commit()
return json_success(json.dumps(tab_state_id))
@@ -165,16 +207,27 @@ class TabStateView(BaseSupersetView):
@expose("<int:tab_state_id>/migrate_query", methods=("POST",))
def migrate_query(self, tab_state_id: int) -> FlaskResponse:
try:
+ user_id = get_user_id()
tab_user_id = _get_tab_user_id(tab_state_id)
if tab_user_id is None:
return Response(status=404)
- if tab_user_id != get_user_id():
+ if tab_user_id != user_id:
return Response(status=403)
client_id = json.loads(request.form["queryId"])
- db.session.query(Query).filter_by(client_id=client_id).update(
- {"sql_editor_id": tab_state_id}
+ rebound = (
+ db.session.query(Query)
+ .filter(
+ Query.client_id == client_id,
+ or_(Query.user_id == user_id, Query.user_id.is_(None)),
+ )
+ .update({"sql_editor_id": tab_state_id})
)
+ if not rebound:
+ # No query the caller may rebind matches this client_id, so
+ # report the miss instead of a success the client would use to
+ # update its own state.
+ return Response(status=404)
db.session.commit()
return json_success(json.dumps(tab_state_id))
except Exception as ex: # pylint: disable=broad-except
diff --git a/tests/unit_tests/views/test_sql_lab_tab_state_views.py
b/tests/unit_tests/views/test_sql_lab_tab_state_views.py
index d8ca5d84521..5b55f53ee2f 100644
--- a/tests/unit_tests/views/test_sql_lab_tab_state_views.py
+++ b/tests/unit_tests/views/test_sql_lab_tab_state_views.py
@@ -25,12 +25,40 @@ authenticated HTTP session. Only the ownership-check logic
inside the view
methods themselves is under test here.
"""
+import pytest
+from flask import current_app
from sqlalchemy.orm.session import Session
from superset.models.sql_lab import Query, TabState
from superset.views.sql_lab.views import TabStateView
[email protected](autouse=True)
+def tab_state_tables(session: Session) -> None:
+ """``TabState`` and ``Query`` share the declarative metadata."""
+ TabState.metadata.create_all(session.get_bind()) # pylint:
disable=no-member
+
+
+def _add_query(
+ session: Session,
+ *,
+ client_id: str,
+ user_id: int | None,
+ sql_editor_id: str,
+ sql: str = "SELECT 1",
+) -> Query:
+ query = Query(
+ client_id=client_id,
+ database_id=1,
+ user_id=user_id,
+ sql_editor_id=sql_editor_id,
+ sql=sql,
+ )
+ session.add(query)
+ session.flush()
+ return query
+
+
def _create_tab_state_and_query(
session: Session,
*,
@@ -47,15 +75,12 @@ def _create_tab_state_and_query(
session.add(tab_state)
session.flush()
- query = Query(
+ query = _add_query(
+ session,
client_id=latest_query_client_id,
- database_id=1,
user_id=owner_id,
sql_editor_id=str(tab_state.id),
- sql="SELECT 1",
)
- session.add(query)
- session.flush()
return tab_state, query
@@ -71,8 +96,6 @@ def test_delete_query_rejects_update_from_non_owning_user(
rejected before either the ``TabState`` update or the ``Query`` row
deletion happens.
"""
- TabState.metadata.create_all(session.get_bind()) # pylint:
disable=no-member
-
owner_id = 2
other_user_id = 1
@@ -111,8 +134,6 @@ def test_put_rejects_update_from_non_owning_user(session:
Session, mocker) -> No
is included for contrast with ``delete_query`` above -- the guard exists
elsewhere in this class, it's just missing on the ``delete_query`` path.
"""
- TabState.metadata.create_all(session.get_bind()) # pylint:
disable=no-member
-
owner_id = 2
other_user_id = 1
@@ -127,3 +148,167 @@ def test_put_rejects_update_from_non_owning_user(session:
Session, mocker) -> No
response = TabStateView.put.__wrapped__(view, tab_state.id)
assert response.status_code == 403
+
+
+def test_put_ignores_columns_outside_the_allowlist(session: Session, mocker)
-> None:
+ """
+ ``put`` writes only the client-updatable columns; identity columns such
+ as ``user_id`` are not accepted from the request body, so the tab state
+ stays bound to its creating user while allowed fields still apply.
+ """
+ owner_id = 2
+ tab_state, _query = _create_tab_state_and_query(
+ session, owner_id=owner_id, latest_query_client_id="owner-query-1"
+ )
+ session.commit()
+
+ mocker.patch("superset.views.sql_lab.views.get_user_id",
return_value=owner_id)
+
+ view = TabStateView()
+ with current_app.test_request_context(
+ method="PUT", data={"label": '"renamed"', "user_id": "1"}
+ ):
+ TabStateView.put.__wrapped__(view, tab_state.id)
+
+ session.expire_all()
+ refreshed = session.query(TabState).filter_by(id=tab_state.id).one()
+ assert refreshed.label == "renamed"
+ assert refreshed.user_id == owner_id
+
+
+def test_put_writes_nothing_for_a_payload_of_only_denied_columns(
+ session: Session, mocker
+) -> None:
+ """
+ A payload made up entirely of non-updatable columns is accepted but
+ changes none of them, rather than erroring or falling through to a write.
+ """
+ owner_id = 2
+ tab_state, _query = _create_tab_state_and_query(
+ session, owner_id=owner_id, latest_query_client_id="owner-query-1"
+ )
+ session.commit()
+
+ mocker.patch("superset.views.sql_lab.views.get_user_id",
return_value=owner_id)
+
+ view = TabStateView()
+ with current_app.test_request_context(method="PUT", data={"user_id": "1"}):
+ response = TabStateView.put.__wrapped__(view, tab_state.id)
+
+ assert response.status_code == 200
+
+ session.expire_all()
+ refreshed = session.query(TabState).filter_by(id=tab_state.id).one()
+ assert refreshed.user_id == owner_id
+ assert refreshed.label == "unrelated tab"
+
+
+def test_put_leaves_the_active_flag_to_the_activate_endpoint(
+ session: Session, mocker
+) -> None:
+ """
+ ``activate`` keeps exactly one of a user's tabs active by rewriting the
+ flag across all of their rows, so ``put`` does not accept ``active``.
+ Sending it leaves the stored flag alone while allowed fields still apply.
+ """
+ owner_id = 2
+ tab_state, _query = _create_tab_state_and_query(
+ session, owner_id=owner_id, latest_query_client_id="owner-query-1"
+ )
+ tab_state.active = False
+ session.commit()
+
+ mocker.patch("superset.views.sql_lab.views.get_user_id",
return_value=owner_id)
+
+ view = TabStateView()
+ with current_app.test_request_context(
+ method="PUT", data={"active": "true", "label": '"renamed"'}
+ ):
+ TabStateView.put.__wrapped__(view, tab_state.id)
+
+ session.expire_all()
+ refreshed = session.query(TabState).filter_by(id=tab_state.id).one()
+ assert refreshed.active is False
+ assert refreshed.label == "renamed"
+
+
+def test_put_only_accepts_latest_query_id_owned_by_caller(
+ session: Session, mocker
+) -> None:
+ """
+ A ``latest_query_id`` is stored only when it references the caller's own
+ query. A client_id belonging to another user is dropped, so the tab's
+ pointer keeps referencing the caller's own query.
+ """
+ owner_id = 2
+ other_user_id = 1
+ tab_state, _query = _create_tab_state_and_query(
+ session, owner_id=owner_id, latest_query_client_id="owner-query-1"
+ )
+ _add_query(
+ session,
+ client_id="foreign-query-1",
+ user_id=other_user_id,
+ sql_editor_id="999",
+ sql="SELECT secret",
+ )
+ session.commit()
+
+ mocker.patch("superset.views.sql_lab.views.get_user_id",
return_value=owner_id)
+
+ view = TabStateView()
+ with current_app.test_request_context(
+ method="PUT", data={"latest_query_id": '"foreign-query-1"'}
+ ):
+ TabStateView.put.__wrapped__(view, tab_state.id)
+
+ session.expire_all()
+ refreshed = session.query(TabState).filter_by(id=tab_state.id).one()
+ assert refreshed.latest_query_id == "owner-query-1"
+
+
[email protected](
+ "query_owner_id,expected_status,rebound",
+ [
+ pytest.param(1, 404, False, id="another_users_query_is_left_alone"),
+ pytest.param(2, 200, True, id="own_query_is_rebound"),
+ pytest.param(None, 200, True, id="unowned_query_is_rebound"),
+ ],
+)
+def test_migrate_query_scopes_the_rebind_to_the_caller(
+ session: Session,
+ mocker,
+ query_owner_id: int | None,
+ expected_status: int,
+ rebound: bool,
+) -> None:
+ """
+ ``migrate_query`` rebinds a query to a tab only when the caller owns that
+ query or nobody does. A client_id owned by another user is left untouched
+ and the caller gets a 404 rather than a success it would apply locally.
+ """
+ owner_id = 2
+ tab_state, _query = _create_tab_state_and_query(
+ session, owner_id=owner_id, latest_query_client_id="owner-query-1"
+ )
+ _add_query(
+ session,
+ client_id="target-query",
+ user_id=query_owner_id,
+ sql_editor_id="88",
+ )
+ session.commit()
+
+ mocker.patch("superset.views.sql_lab.views.get_user_id",
return_value=owner_id)
+
+ view = TabStateView()
+ with current_app.test_request_context(
+ method="POST", data={"queryId": '"target-query"'}
+ ):
+ response = TabStateView.migrate_query.__wrapped__(view, tab_state.id)
+
+ assert response.status_code == expected_status
+
+ session.expire_all()
+ target = session.query(Query).filter_by(client_id="target-query").one()
+ assert target.sql_editor_id == (str(tab_state.id) if rebound else "88")
