codeant-ai-for-open-source[bot] commented on code in PR #45151: URL: https://github.com/apache/superset/pull/45151#discussion_r4233587556
########## docker/lillio/superset_config.py: ########## @@ -0,0 +1,408 @@ +# 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 logging +import os as _os +import threading as _threading +from copy import deepcopy as _deepcopy +from datetime import timedelta as _timedelta +from typing import Any, cast +from urllib.parse import quote as _urlquote + +from celery.schedules import crontab as _crontab +from celery.signals import worker_init as _worker_init +from celery.signals import worker_ready as _worker_ready +from celery.signals import worker_shutdown as _worker_shutdown +from flask_caching.backends.rediscache import RedisCache as _RedisCache +from jinja2 import pass_context as _pass_context +from jinja2.runtime import Context as _JinjaContext, Undefined as _JinjaUndefined +from redis import Redis as _Redis +from redis.backoff import ExponentialBackoff as _ExponentialBackoff +from redis.exceptions import BusyLoadingError as _BusyLoadingError +from redis.exceptions import ConnectionError as _RedisConnectionError +from redis.exceptions import TimeoutError as _RedisTimeoutError +from redis.retry import Retry as _Retry +from superset import config as _superset_config + +logger = logging.getLogger(__name__) +_jinja_logger = logging.getLogger("superset.jinja_context") + +# --------------------------------------------------------------------------- +# File-based Celery worker health check +# Mirrors the implementation in helm/superset/templates/_helpers.tpl so ECS +# can use a simple file-mtime check instead of `celery inspect ping`, which +# requires loading the full Flask app on every health check interval. +# --------------------------------------------------------------------------- +_HC_READY_FILE = "/tmp/celery_worker_ready" # noqa: S108 +_HC_ALIVE_FILE = "/tmp/celery_worker_alive" # noqa: S108 +_HC_HEARTBEAT_INTERVAL = 10 # seconds +_hc_liveness_thread = None +_hc_liveness_stop_event = None + + +@_worker_ready.connect +def _hc_create_ready_file(sender: Any = None, **kwargs: Any) -> None: + try: + open(_HC_READY_FILE, "w").close() # noqa: WPS515 + except Exception as exc: # noqa: BLE001 + logger.warning("health-check: could not create ready file: %r", exc) + + +@_worker_shutdown.connect +def _hc_remove_ready_file(sender: Any = None, **kwargs: Any) -> None: + global _hc_liveness_thread, _hc_liveness_stop_event # noqa: PLW0603 + if _hc_liveness_stop_event: + _hc_liveness_stop_event.set() + if _hc_liveness_thread: + _hc_liveness_thread.join(timeout=5) + for path in (_HC_READY_FILE, _HC_ALIVE_FILE): + try: + if _os.path.exists(path): + _os.remove(path) + except Exception as exc: # noqa: BLE001 + logger.warning("health-check: could not remove %s: %r", path, exc) + + +@_worker_init.connect +def _hc_start_liveness_heartbeat(sender: Any = None, **kwargs: Any) -> None: + global _hc_liveness_thread, _hc_liveness_stop_event # noqa: PLW0603 + _hc_liveness_stop_event = _threading.Event() + + def _beat() -> None: + while not _hc_liveness_stop_event.is_set(): + try: + with open(_HC_ALIVE_FILE, "w") as fh: + fh.write(str(_os.getpid())) + except Exception as exc: # noqa: BLE001 + logger.warning("health-check: could not update alive file: %r", exc) + _hc_liveness_stop_event.wait(_HC_HEARTBEAT_INTERVAL) + + _hc_liveness_thread = _threading.Thread(target=_beat, daemon=True) + _hc_liveness_thread.start() + + +FEATURE_FLAGS = { + "ALERT_REPORTS": True, + "DATE_RANGE_TIMESHIFTS_ENABLED": True, + "EMBEDDED_SUPERSET": True, + "ENABLE_TEMPLATE_PROCESSING": True, + "GLOBAL_ASYNC_QUERIES": True, + "PLAYWRIGHT_REPORTS_AND_THUMBNAILS": True, + "SQLLAB_FORCE_RUN_ASYNC": True, + # A core migration (add_granular_export_permissions) unconditionally + # deletes the legacy "can_csv on Superset" permission when it runs, + # regardless of this flag. With the flag left off, the runtime code + # still checks for that now-deleted permission, so CSV/chart export + # can never be granted to any role. Turning this on switches the + # runtime checks to the granular permissions the migration actually + # created (can_export_data / can_export_image / can_copy_clipboard). + "GRANULAR_EXPORT_CONTROLS": True, +} + +GUEST_TOKEN_JWT_SECRET = _os.environ["SUPERSET__GUEST_TOKEN_JWT_SECRET"] + + +def _register_cache_key(ctx: _JinjaContext, value: Any) -> None: + """ + Fold ``value`` into the query's cache key via the sibling ``cache_key_wrapper`` + macro that Superset's ``ExtraCache`` exposes in the same template context. + + Without this, a per-guest/per-user value returned by a JINJA_CONTEXT_ADDONS + function never varies the cache key, so every viewer's query result is + cached under the same key: the first value computed "wins" and gets served + to everyone else until the cache entry expires. + """ + cache_key_wrapper = ctx.resolve("cache_key_wrapper") + # Jinja's base Undefined defines __call__ (it raises when actually invoked), + # so `callable(...)` alone can't tell "missing from context" apart from + # "a real callable" -- check for Undefined explicitly. + if callable(cache_key_wrapper) and not isinstance( + cache_key_wrapper, _JinjaUndefined + ): + hashable_value = tuple(value) if isinstance(value, list) else value + cache_key_wrapper(hashable_value) + + +def _get_template_user() -> Any: + """Return the request or Celery-task user used by Superset. + + Async chart tasks restore their user with ``override_user()``, which writes + to ``flask.g.user``. ``flask_login.current_user`` requires a request context + and therefore resolves incorrectly in a Celery worker. + """ + try: + from flask import g + + if (user := getattr(g, "user", None)) is not None: + return user + except RuntimeError: + # Synchronous template rendering outside an application context can + # still use Flask-Login's request-local proxy. + pass + + from flask_login import current_user + + return current_user + + +@_pass_context +def get_user_attribute(ctx: _JinjaContext, attr: str, default: Any = None) -> Any: + try: + value = getattr(_get_template_user(), attr, default) + except Exception: # noqa: BLE001 + value = default + _register_cache_key(ctx, value) + return value Review Comment: **Suggestion:** Guest-token values are returned verbatim; when a template interpolates an attacker-controlled string into SQL, it can terminate the literal and change the query. **Assessment:** ๐ด `Critical` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Sql injection` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7a7be6e6a16440af906ff0b643856e2b&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=7a7be6e6a16440af906ff0b643856e2b&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:** docker/lillio/superset_config.py **Line:** 168:169 **Comment:** *Sql Injection: Guest-token values are returned verbatim; when a template interpolates an attacker-controlled string into SQL, it can terminate the literal and change the query. 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%2F45151&comment_hash=eab3f56cd909f28e01a47b5a887ca2dfacff1a288f8c92bedbdebf35c1d3ba58&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45151&comment_hash=eab3f56cd909f28e01a47b5a887ca2dfacff1a288f8c92bedbdebf35c1d3ba58&reaction=dislike'>๐</a> ########## docker/lillio/superset_config.py: ########## @@ -0,0 +1,408 @@ +# 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 logging +import os as _os +import threading as _threading +from copy import deepcopy as _deepcopy +from datetime import timedelta as _timedelta +from typing import Any, cast +from urllib.parse import quote as _urlquote + +from celery.schedules import crontab as _crontab +from celery.signals import worker_init as _worker_init +from celery.signals import worker_ready as _worker_ready +from celery.signals import worker_shutdown as _worker_shutdown +from flask_caching.backends.rediscache import RedisCache as _RedisCache +from jinja2 import pass_context as _pass_context +from jinja2.runtime import Context as _JinjaContext, Undefined as _JinjaUndefined +from redis import Redis as _Redis +from redis.backoff import ExponentialBackoff as _ExponentialBackoff +from redis.exceptions import BusyLoadingError as _BusyLoadingError +from redis.exceptions import ConnectionError as _RedisConnectionError +from redis.exceptions import TimeoutError as _RedisTimeoutError +from redis.retry import Retry as _Retry +from superset import config as _superset_config + +logger = logging.getLogger(__name__) +_jinja_logger = logging.getLogger("superset.jinja_context") + +# --------------------------------------------------------------------------- +# File-based Celery worker health check +# Mirrors the implementation in helm/superset/templates/_helpers.tpl so ECS +# can use a simple file-mtime check instead of `celery inspect ping`, which +# requires loading the full Flask app on every health check interval. +# --------------------------------------------------------------------------- +_HC_READY_FILE = "/tmp/celery_worker_ready" # noqa: S108 +_HC_ALIVE_FILE = "/tmp/celery_worker_alive" # noqa: S108 +_HC_HEARTBEAT_INTERVAL = 10 # seconds +_hc_liveness_thread = None +_hc_liveness_stop_event = None + + +@_worker_ready.connect +def _hc_create_ready_file(sender: Any = None, **kwargs: Any) -> None: + try: + open(_HC_READY_FILE, "w").close() # noqa: WPS515 + except Exception as exc: # noqa: BLE001 + logger.warning("health-check: could not create ready file: %r", exc) + + +@_worker_shutdown.connect +def _hc_remove_ready_file(sender: Any = None, **kwargs: Any) -> None: + global _hc_liveness_thread, _hc_liveness_stop_event # noqa: PLW0603 + if _hc_liveness_stop_event: + _hc_liveness_stop_event.set() + if _hc_liveness_thread: + _hc_liveness_thread.join(timeout=5) + for path in (_HC_READY_FILE, _HC_ALIVE_FILE): + try: + if _os.path.exists(path): + _os.remove(path) + except Exception as exc: # noqa: BLE001 + logger.warning("health-check: could not remove %s: %r", path, exc) + + +@_worker_init.connect +def _hc_start_liveness_heartbeat(sender: Any = None, **kwargs: Any) -> None: + global _hc_liveness_thread, _hc_liveness_stop_event # noqa: PLW0603 + _hc_liveness_stop_event = _threading.Event() + + def _beat() -> None: + while not _hc_liveness_stop_event.is_set(): + try: + with open(_HC_ALIVE_FILE, "w") as fh: + fh.write(str(_os.getpid())) + except Exception as exc: # noqa: BLE001 + logger.warning("health-check: could not update alive file: %r", exc) + _hc_liveness_stop_event.wait(_HC_HEARTBEAT_INTERVAL) + + _hc_liveness_thread = _threading.Thread(target=_beat, daemon=True) + _hc_liveness_thread.start() + + +FEATURE_FLAGS = { + "ALERT_REPORTS": True, + "DATE_RANGE_TIMESHIFTS_ENABLED": True, + "EMBEDDED_SUPERSET": True, + "ENABLE_TEMPLATE_PROCESSING": True, + "GLOBAL_ASYNC_QUERIES": True, + "PLAYWRIGHT_REPORTS_AND_THUMBNAILS": True, + "SQLLAB_FORCE_RUN_ASYNC": True, + # A core migration (add_granular_export_permissions) unconditionally + # deletes the legacy "can_csv on Superset" permission when it runs, + # regardless of this flag. With the flag left off, the runtime code + # still checks for that now-deleted permission, so CSV/chart export + # can never be granted to any role. Turning this on switches the + # runtime checks to the granular permissions the migration actually + # created (can_export_data / can_export_image / can_copy_clipboard). + "GRANULAR_EXPORT_CONTROLS": True, +} + +GUEST_TOKEN_JWT_SECRET = _os.environ["SUPERSET__GUEST_TOKEN_JWT_SECRET"] + + +def _register_cache_key(ctx: _JinjaContext, value: Any) -> None: + """ + Fold ``value`` into the query's cache key via the sibling ``cache_key_wrapper`` + macro that Superset's ``ExtraCache`` exposes in the same template context. + + Without this, a per-guest/per-user value returned by a JINJA_CONTEXT_ADDONS + function never varies the cache key, so every viewer's query result is + cached under the same key: the first value computed "wins" and gets served + to everyone else until the cache entry expires. + """ + cache_key_wrapper = ctx.resolve("cache_key_wrapper") + # Jinja's base Undefined defines __call__ (it raises when actually invoked), + # so `callable(...)` alone can't tell "missing from context" apart from + # "a real callable" -- check for Undefined explicitly. + if callable(cache_key_wrapper) and not isinstance( + cache_key_wrapper, _JinjaUndefined + ): + hashable_value = tuple(value) if isinstance(value, list) else value + cache_key_wrapper(hashable_value) + + +def _get_template_user() -> Any: + """Return the request or Celery-task user used by Superset. + + Async chart tasks restore their user with ``override_user()``, which writes + to ``flask.g.user``. ``flask_login.current_user`` requires a request context + and therefore resolves incorrectly in a Celery worker. + """ + try: + from flask import g + + if (user := getattr(g, "user", None)) is not None: + return user + except RuntimeError: + # Synchronous template rendering outside an application context can + # still use Flask-Login's request-local proxy. + pass + + from flask_login import current_user + + return current_user + + +@_pass_context +def get_user_attribute(ctx: _JinjaContext, attr: str, default: Any = None) -> Any: + try: + value = getattr(_get_template_user(), attr, default) + except Exception: # noqa: BLE001 + value = default + _register_cache_key(ctx, value) + return value + + +@_pass_context +def guest_attr(ctx: _JinjaContext, attr: str, default: Any = None) -> Any: + try: + user = _get_template_user() + if getattr(user, "is_guest_user", False): + token_user = user.guest_token.get("user", {}) + value = token_user.get(attr, default) + else: + value = default + except Exception as exc: # noqa: BLE001 + _jinja_logger.warning("[guest_attr] ERROR attr=%r exc=%r", attr, exc) + value = default + _register_cache_key(ctx, value) + return value + + +JINJA_CONTEXT_ADDONS = { + "get_user_attribute": get_user_attribute, + "guest_attr": guest_attr, +} + + +def GUEST_TOKEN_VALIDATOR_HOOK(body: dict[str, Any]) -> bool: # noqa: N802 + try: + from flask import request as flask_request + + raw_user = (flask_request.json or {}).get("user", {}) + allowed_keys = {"username", "first_name", "last_name"} + custom_fields = {k: v for k, v in raw_user.items() if k not in allowed_keys} + if custom_fields: + body.setdefault("user", {}).update(custom_fields) + except Exception as exc: # noqa: BLE001 + logger.warning("GUEST_TOKEN_VALIDATOR_HOOK error: %r", exc) + return True + + +ENABLE_PROXY_FIX = True +DEBUG = False + +TALISMAN_ENABLED = True +TALISMAN_CONFIG = cast(dict[str, Any], _deepcopy(_superset_config.TALISMAN_CONFIG)) +TALISMAN_CONFIG["content_security_policy"]["frame-ancestors"] = [ + "'self'", + "https://app.lillio.com";, + "https://staging-app.lillio.com";, + "https://*.herokuapp.com";, Review Comment: **Suggestion:** This allows any Heroku tenant to frame authenticated Superset pages; with X-Frame-Options disabled, malicious pages can overlay controls and trick users into actions. **Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Security` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c1157b24f56a41d49891cd925da2467f&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=c1157b24f56a41d49891cd925da2467f&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:** docker/lillio/superset_config.py **Line:** 217:217 **Comment:** *Security: This allows any Heroku tenant to frame authenticated Superset pages; with X-Frame-Options disabled, malicious pages can overlay controls and trick users into actions. 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%2F45151&comment_hash=34132d821936f1cab3a527a525a123f1c325364356d0aab9b6a7ca7a60d8468f&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45151&comment_hash=34132d821936f1cab3a527a525a123f1c325364356d0aab9b6a7ca7a60d8468f&reaction=dislike'>๐</a> ########## docker/lillio/custom_sso_security_manager.py: ########## @@ -0,0 +1,85 @@ +# 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. +"""Okta OIDC user-info mapping for Superset.""" + +import logging +from typing import Any + +from flask import request +from flask_appbuilder import expose +from flask_appbuilder.security.views import AuthOAuthView, WerkzeugResponse +from superset.security import SupersetSecurityManager + +logger = logging.getLogger(__name__) + + +class LillioAuthOAuthView(AuthOAuthView): + """Keep the IdP issuer out of Flask-AppBuilder's internal state JWT.""" + + @expose("/login/") + @expose("/login/<provider>") + def login(self, provider: str | None = None) -> WerkzeugResponse: + # Okta-initiated login appends ``iss`` to this request. FAB serializes + # query parameters with ``flat=False``, which turns it into a list in + # the internal state JWT. PyJWT rejects that because ``iss`` is a + # registered claim whose value must be a string. The IdP issuer is + # already defined by server_metadata_url and is not state we need to + # preserve across the OAuth redirect. + if "iss" not in request.args: + return super().login(provider) + + original_args = request.args + sanitized_args = original_args.copy() + sanitized_args.pop("iss", None) + request.args = sanitized_args + try: + return super().login(provider) + finally: + request.args = original_args + + +class CustomSsoSecurityManager(SupersetSecurityManager): + authoauthview = LillioAuthOAuthView + + def oauth_user_info(self, provider: str, response: Any = None) -> dict[str, Any]: + if provider != "okta": + return {} + + remote = self.appbuilder.sm.oauth_remotes[provider] + + info: dict[str, Any] = {} + try: + info = remote.userinfo() or {} + except Exception as exc: # noqa: BLE001 + logger.warning("okta userinfo() failed: %r", exc) + + groups = info.get("groups") + if groups is None and response: + try: + groups = (remote.parse_id_token(response) or {}).get("groups") + except Exception as exc: # noqa: BLE001 + logger.warning("okta parse_id_token failed: %r", exc) + + email = info.get("email") + logger.debug("Okta OIDC user %s groups=%s", email, groups) + return { + "username": email, + "email": email, + "first_name": info.get("given_name", ""), + "last_name": info.get("family_name", ""), + "role_keys": groups or [], + } Review Comment: **Suggestion:** If userinfo fails or lacks email, this ignores email claims in the ID token and returns a None username, preventing valid OAuth users from being matched or registered. **Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Api mismatch` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2c303395723242f18c5c2e1bb5a27d46&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=2c303395723242f18c5c2e1bb5a27d46&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:** docker/lillio/custom_sso_security_manager.py **Line:** 77:85 **Comment:** *Api Mismatch: If userinfo fails or lacks email, this ignores email claims in the ID token and returns a None username, preventing valid OAuth users from being matched or registered. 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%2F45151&comment_hash=dd21931932827512e94a48886712d85dfbfd9fcb635f3c828e31e719cd612ae3&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45151&comment_hash=dd21931932827512e94a48886712d85dfbfd9fcb635f3c828e31e719cd612ae3&reaction=dislike'>๐</a> ########## docker/lillio/superset_config.py: ########## @@ -0,0 +1,408 @@ +# 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 logging +import os as _os +import threading as _threading +from copy import deepcopy as _deepcopy +from datetime import timedelta as _timedelta +from typing import Any, cast +from urllib.parse import quote as _urlquote + +from celery.schedules import crontab as _crontab +from celery.signals import worker_init as _worker_init +from celery.signals import worker_ready as _worker_ready +from celery.signals import worker_shutdown as _worker_shutdown +from flask_caching.backends.rediscache import RedisCache as _RedisCache +from jinja2 import pass_context as _pass_context +from jinja2.runtime import Context as _JinjaContext, Undefined as _JinjaUndefined +from redis import Redis as _Redis +from redis.backoff import ExponentialBackoff as _ExponentialBackoff +from redis.exceptions import BusyLoadingError as _BusyLoadingError +from redis.exceptions import ConnectionError as _RedisConnectionError +from redis.exceptions import TimeoutError as _RedisTimeoutError +from redis.retry import Retry as _Retry +from superset import config as _superset_config + +logger = logging.getLogger(__name__) +_jinja_logger = logging.getLogger("superset.jinja_context") + +# --------------------------------------------------------------------------- +# File-based Celery worker health check +# Mirrors the implementation in helm/superset/templates/_helpers.tpl so ECS +# can use a simple file-mtime check instead of `celery inspect ping`, which +# requires loading the full Flask app on every health check interval. +# --------------------------------------------------------------------------- +_HC_READY_FILE = "/tmp/celery_worker_ready" # noqa: S108 +_HC_ALIVE_FILE = "/tmp/celery_worker_alive" # noqa: S108 +_HC_HEARTBEAT_INTERVAL = 10 # seconds +_hc_liveness_thread = None +_hc_liveness_stop_event = None + + +@_worker_ready.connect +def _hc_create_ready_file(sender: Any = None, **kwargs: Any) -> None: + try: + open(_HC_READY_FILE, "w").close() # noqa: WPS515 + except Exception as exc: # noqa: BLE001 + logger.warning("health-check: could not create ready file: %r", exc) + + +@_worker_shutdown.connect +def _hc_remove_ready_file(sender: Any = None, **kwargs: Any) -> None: + global _hc_liveness_thread, _hc_liveness_stop_event # noqa: PLW0603 + if _hc_liveness_stop_event: + _hc_liveness_stop_event.set() + if _hc_liveness_thread: + _hc_liveness_thread.join(timeout=5) + for path in (_HC_READY_FILE, _HC_ALIVE_FILE): + try: + if _os.path.exists(path): + _os.remove(path) + except Exception as exc: # noqa: BLE001 + logger.warning("health-check: could not remove %s: %r", path, exc) + + +@_worker_init.connect +def _hc_start_liveness_heartbeat(sender: Any = None, **kwargs: Any) -> None: + global _hc_liveness_thread, _hc_liveness_stop_event # noqa: PLW0603 + _hc_liveness_stop_event = _threading.Event() + + def _beat() -> None: + while not _hc_liveness_stop_event.is_set(): + try: + with open(_HC_ALIVE_FILE, "w") as fh: + fh.write(str(_os.getpid())) + except Exception as exc: # noqa: BLE001 + logger.warning("health-check: could not update alive file: %r", exc) + _hc_liveness_stop_event.wait(_HC_HEARTBEAT_INTERVAL) + + _hc_liveness_thread = _threading.Thread(target=_beat, daemon=True) + _hc_liveness_thread.start() + + +FEATURE_FLAGS = { + "ALERT_REPORTS": True, + "DATE_RANGE_TIMESHIFTS_ENABLED": True, + "EMBEDDED_SUPERSET": True, + "ENABLE_TEMPLATE_PROCESSING": True, + "GLOBAL_ASYNC_QUERIES": True, + "PLAYWRIGHT_REPORTS_AND_THUMBNAILS": True, + "SQLLAB_FORCE_RUN_ASYNC": True, + # A core migration (add_granular_export_permissions) unconditionally + # deletes the legacy "can_csv on Superset" permission when it runs, + # regardless of this flag. With the flag left off, the runtime code + # still checks for that now-deleted permission, so CSV/chart export + # can never be granted to any role. Turning this on switches the + # runtime checks to the granular permissions the migration actually + # created (can_export_data / can_export_image / can_copy_clipboard). + "GRANULAR_EXPORT_CONTROLS": True, +} + +GUEST_TOKEN_JWT_SECRET = _os.environ["SUPERSET__GUEST_TOKEN_JWT_SECRET"] + + +def _register_cache_key(ctx: _JinjaContext, value: Any) -> None: + """ + Fold ``value`` into the query's cache key via the sibling ``cache_key_wrapper`` + macro that Superset's ``ExtraCache`` exposes in the same template context. + + Without this, a per-guest/per-user value returned by a JINJA_CONTEXT_ADDONS + function never varies the cache key, so every viewer's query result is + cached under the same key: the first value computed "wins" and gets served + to everyone else until the cache entry expires. + """ + cache_key_wrapper = ctx.resolve("cache_key_wrapper") + # Jinja's base Undefined defines __call__ (it raises when actually invoked), + # so `callable(...)` alone can't tell "missing from context" apart from + # "a real callable" -- check for Undefined explicitly. + if callable(cache_key_wrapper) and not isinstance( + cache_key_wrapper, _JinjaUndefined + ): + hashable_value = tuple(value) if isinstance(value, list) else value + cache_key_wrapper(hashable_value) Review Comment: **Suggestion:** Superset only collects these cache contributions when its template scanner finds a recognized macro; it does not recognize these helper names, so their results still collide across viewers. **Assessment:** ๐ด `Critical` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Cache` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4b47bfe5ff5043bba01f4ff1e606a8ab&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=4b47bfe5ff5043bba01f4ff1e606a8ab&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:** docker/lillio/superset_config.py **Line:** 136:137 **Comment:** *Cache: Superset only collects these cache contributions when its template scanner finds a recognized macro; it does not recognize these helper names, so their results still collide across viewers. 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%2F45151&comment_hash=b454975b4a0cb98f1ec964584190ce0975b7cc81348fccfec7c0bfd8ca311ee6&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45151&comment_hash=b454975b4a0cb98f1ec964584190ce0975b7cc81348fccfec7c0bfd8ca311ee6&reaction=dislike'>๐</a> ########## docker/lillio/superset_config.py: ########## @@ -0,0 +1,408 @@ +# 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 logging +import os as _os +import threading as _threading +from copy import deepcopy as _deepcopy +from datetime import timedelta as _timedelta +from typing import Any, cast +from urllib.parse import quote as _urlquote + +from celery.schedules import crontab as _crontab +from celery.signals import worker_init as _worker_init +from celery.signals import worker_ready as _worker_ready +from celery.signals import worker_shutdown as _worker_shutdown +from flask_caching.backends.rediscache import RedisCache as _RedisCache +from jinja2 import pass_context as _pass_context +from jinja2.runtime import Context as _JinjaContext, Undefined as _JinjaUndefined +from redis import Redis as _Redis +from redis.backoff import ExponentialBackoff as _ExponentialBackoff +from redis.exceptions import BusyLoadingError as _BusyLoadingError +from redis.exceptions import ConnectionError as _RedisConnectionError +from redis.exceptions import TimeoutError as _RedisTimeoutError +from redis.retry import Retry as _Retry +from superset import config as _superset_config + +logger = logging.getLogger(__name__) +_jinja_logger = logging.getLogger("superset.jinja_context") + +# --------------------------------------------------------------------------- +# File-based Celery worker health check +# Mirrors the implementation in helm/superset/templates/_helpers.tpl so ECS +# can use a simple file-mtime check instead of `celery inspect ping`, which +# requires loading the full Flask app on every health check interval. +# --------------------------------------------------------------------------- +_HC_READY_FILE = "/tmp/celery_worker_ready" # noqa: S108 +_HC_ALIVE_FILE = "/tmp/celery_worker_alive" # noqa: S108 +_HC_HEARTBEAT_INTERVAL = 10 # seconds +_hc_liveness_thread = None +_hc_liveness_stop_event = None + + +@_worker_ready.connect +def _hc_create_ready_file(sender: Any = None, **kwargs: Any) -> None: + try: + open(_HC_READY_FILE, "w").close() # noqa: WPS515 + except Exception as exc: # noqa: BLE001 + logger.warning("health-check: could not create ready file: %r", exc) + + +@_worker_shutdown.connect +def _hc_remove_ready_file(sender: Any = None, **kwargs: Any) -> None: + global _hc_liveness_thread, _hc_liveness_stop_event # noqa: PLW0603 + if _hc_liveness_stop_event: + _hc_liveness_stop_event.set() + if _hc_liveness_thread: + _hc_liveness_thread.join(timeout=5) + for path in (_HC_READY_FILE, _HC_ALIVE_FILE): + try: + if _os.path.exists(path): + _os.remove(path) + except Exception as exc: # noqa: BLE001 + logger.warning("health-check: could not remove %s: %r", path, exc) + + +@_worker_init.connect +def _hc_start_liveness_heartbeat(sender: Any = None, **kwargs: Any) -> None: + global _hc_liveness_thread, _hc_liveness_stop_event # noqa: PLW0603 + _hc_liveness_stop_event = _threading.Event() + + def _beat() -> None: + while not _hc_liveness_stop_event.is_set(): + try: + with open(_HC_ALIVE_FILE, "w") as fh: + fh.write(str(_os.getpid())) + except Exception as exc: # noqa: BLE001 + logger.warning("health-check: could not update alive file: %r", exc) + _hc_liveness_stop_event.wait(_HC_HEARTBEAT_INTERVAL) + + _hc_liveness_thread = _threading.Thread(target=_beat, daemon=True) + _hc_liveness_thread.start() + + +FEATURE_FLAGS = { + "ALERT_REPORTS": True, + "DATE_RANGE_TIMESHIFTS_ENABLED": True, + "EMBEDDED_SUPERSET": True, + "ENABLE_TEMPLATE_PROCESSING": True, + "GLOBAL_ASYNC_QUERIES": True, + "PLAYWRIGHT_REPORTS_AND_THUMBNAILS": True, + "SQLLAB_FORCE_RUN_ASYNC": True, + # A core migration (add_granular_export_permissions) unconditionally + # deletes the legacy "can_csv on Superset" permission when it runs, + # regardless of this flag. With the flag left off, the runtime code + # still checks for that now-deleted permission, so CSV/chart export + # can never be granted to any role. Turning this on switches the + # runtime checks to the granular permissions the migration actually + # created (can_export_data / can_export_image / can_copy_clipboard). + "GRANULAR_EXPORT_CONTROLS": True, +} + +GUEST_TOKEN_JWT_SECRET = _os.environ["SUPERSET__GUEST_TOKEN_JWT_SECRET"] + + +def _register_cache_key(ctx: _JinjaContext, value: Any) -> None: + """ + Fold ``value`` into the query's cache key via the sibling ``cache_key_wrapper`` + macro that Superset's ``ExtraCache`` exposes in the same template context. + + Without this, a per-guest/per-user value returned by a JINJA_CONTEXT_ADDONS + function never varies the cache key, so every viewer's query result is + cached under the same key: the first value computed "wins" and gets served + to everyone else until the cache entry expires. + """ + cache_key_wrapper = ctx.resolve("cache_key_wrapper") + # Jinja's base Undefined defines __call__ (it raises when actually invoked), + # so `callable(...)` alone can't tell "missing from context" apart from + # "a real callable" -- check for Undefined explicitly. + if callable(cache_key_wrapper) and not isinstance( + cache_key_wrapper, _JinjaUndefined + ): + hashable_value = tuple(value) if isinstance(value, list) else value + cache_key_wrapper(hashable_value) + + +def _get_template_user() -> Any: + """Return the request or Celery-task user used by Superset. + + Async chart tasks restore their user with ``override_user()``, which writes + to ``flask.g.user``. ``flask_login.current_user`` requires a request context + and therefore resolves incorrectly in a Celery worker. + """ + try: + from flask import g + + if (user := getattr(g, "user", None)) is not None: + return user + except RuntimeError: + # Synchronous template rendering outside an application context can + # still use Flask-Login's request-local proxy. + pass + + from flask_login import current_user + + return current_user + + +@_pass_context +def get_user_attribute(ctx: _JinjaContext, attr: str, default: Any = None) -> Any: + try: + value = getattr(_get_template_user(), attr, default) + except Exception: # noqa: BLE001 + value = default + _register_cache_key(ctx, value) + return value + + +@_pass_context +def guest_attr(ctx: _JinjaContext, attr: str, default: Any = None) -> Any: + try: + user = _get_template_user() + if getattr(user, "is_guest_user", False): + token_user = user.guest_token.get("user", {}) + value = token_user.get(attr, default) + else: + value = default + except Exception as exc: # noqa: BLE001 + _jinja_logger.warning("[guest_attr] ERROR attr=%r exc=%r", attr, exc) + value = default + _register_cache_key(ctx, value) + return value + + +JINJA_CONTEXT_ADDONS = { + "get_user_attribute": get_user_attribute, + "guest_attr": guest_attr, +} + + +def GUEST_TOKEN_VALIDATOR_HOOK(body: dict[str, Any]) -> bool: # noqa: N802 + try: + from flask import request as flask_request + + raw_user = (flask_request.json or {}).get("user", {}) + allowed_keys = {"username", "first_name", "last_name"} + custom_fields = {k: v for k, v in raw_user.items() if k not in allowed_keys} + if custom_fields: + body.setdefault("user", {}).update(custom_fields) + except Exception as exc: # noqa: BLE001 + logger.warning("GUEST_TOKEN_VALIDATOR_HOOK error: %r", exc) + return True + + +ENABLE_PROXY_FIX = True +DEBUG = False + +TALISMAN_ENABLED = True +TALISMAN_CONFIG = cast(dict[str, Any], _deepcopy(_superset_config.TALISMAN_CONFIG)) +TALISMAN_CONFIG["content_security_policy"]["frame-ancestors"] = [ + "'self'", + "https://app.lillio.com";, + "https://staging-app.lillio.com";, + "https://*.herokuapp.com";, +] +TALISMAN_CONFIG["force_https"] = False +TALISMAN_CONFIG["session_cookie_secure"] = True +TALISMAN_CONFIG["session_cookie_samesite"] = "None" +TALISMAN_CONFIG["frame_options"] = None + +PERMANENT_SESSION_LIFETIME = _timedelta(hours=8) +SESSION_COOKIE_HTTPONLY = True +SESSION_COOKIE_SECURE = True +SESSION_COOKIE_SAMESITE = "None" +SESSION_SERVER_SIDE = True +SESSION_TYPE = "redis" +SESSION_USE_SIGNER = True + +# flask-session docs recommend retry with ExponentialBackoff for Redis-backed +# sessions so that transient TimeoutError / ConnectionError on stale SSL +# connections reconnect transparently instead of propagating to Flask-Session +# and redirecting the user to /login/. +# See: https://flask-session.readthedocs.io/en/latest/config.html#retries +_session_redis_retry = _Retry( + _ExponentialBackoff(), + retries=3, +) +SESSION_REDIS = _Redis( + host=_os.environ["REDIS_HOST"], + port=int(_os.environ.get("REDIS_PORT", "6379")), + password=_os.environ.get("REDIS_PASSWORD"), + db=1, + ssl=_os.environ.get("REDIS_SSL", "true").lower() == "true", + ssl_cert_reqs="required", + socket_keepalive=True, + retry=_session_redis_retry, + retry_on_error=[_BusyLoadingError, _RedisConnectionError, _RedisTimeoutError], +) + +_redis_host = _os.environ["REDIS_HOST"] +_redis_port = int(_os.environ.get("REDIS_PORT", "6379")) +_redis_password = _os.environ["REDIS_PASSWORD"] +_redis_password_url = _urlquote(_redis_password, safe="") + + +# DB 0 โ Celery broker only (matches Superset's REDIS_CELERY_DB default) +# DB 1 โ everything else: caches, results, rate-limiter, async-events +# (matches Superset's REDIS_RESULTS_DB default) +_BROKER_DB = 0 +_CACHE_DB = 1 + + +def _rediss_url(database: int) -> str: + return ( + f"rediss://:{_redis_password_url}@{_redis_host}:{_redis_port}/{database}" + "?ssl_cert_reqs=required" + ) + + +class CeleryConfig: # pylint: disable=too-few-public-methods + broker_url = _rediss_url(_BROKER_DB) + result_backend = _rediss_url(_CACHE_DB) + imports = ( + "superset.sql_lab", + "superset.tasks.scheduler", + "superset.tasks.thumbnails", + "superset.tasks.cache", + "superset.tasks.slack", + ) + worker_prefetch_multiplier = 1 + task_acks_late = True + task_annotations = { + "sql_lab.get_sql_results": {"rate_limit": "100/s"}, + } + beat_schedule = { + "reports.scheduler": { + "task": "reports.scheduler", + "schedule": _crontab(minute="*", hour="*"), + }, + "reports.prune_log": { + "task": "reports.prune_log", + "schedule": _crontab(minute=0, hour=0), + }, + } Review Comment: **Suggestion:** This production beat schedule omits version-history pruning; capture is enabled by default, so old rows are never pruned and the metadata database grows indefinitely. **Assessment:** ๐ `Major` ยท ๐ `Occurrence: Often` ยท ๐ท๏ธ `Incomplete implementation` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a33bebd8888c4e2fabd92e893e90bf04&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=a33bebd8888c4e2fabd92e893e90bf04&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:** docker/lillio/superset_config.py **Line:** 288:297 **Comment:** *Incomplete Implementation: This production beat schedule omits version-history pruning; capture is enabled by default, so old rows are never pruned and the metadata database grows indefinitely. 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%2F45151&comment_hash=7a88a1f0cc1f05eebe57a050bf7ec6d8509cc13f3014b8033e96ccea5277bb3b&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45151&comment_hash=7a88a1f0cc1f05eebe57a050bf7ec6d8509cc13f3014b8033e96ccea5277bb3b&reaction=dislike'>๐</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
