codeant-ai-for-open-source[bot] commented on code in PR #42796: URL: https://github.com/apache/superset/pull/42796#discussion_r3749949745
########## superset/utils/error_sanitization.py: ########## @@ -0,0 +1,148 @@ +# 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. +""" +Redaction of error details for embedded (guest token) viewers. + +Errors raised while running a query are relayed verbatim to the client so chart +authors can fix them. For an embedded viewer that detail is both unusable and +sensitive: engine errors routinely quote catalog, schema, table and column +names of the underlying warehouse. Guest responses therefore carry a generic +message unless the error is one Superset authored itself. +""" + +from __future__ import annotations + +import dataclasses +from typing import Any + +from flask_babel import lazy_gettext as _ + +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType + +GENERIC_ERROR_MESSAGE = _("An error occurred while fetching the data.") + +# Error types Superset raises on its own, describing an access decision, a +# malformed payload or a client-side condition. Their messages are written by +# Superset rather than echoed from the database, so they survive redaction. +SAFE_ERROR_TYPES = frozenset( + { + SupersetErrorType.FRONTEND_CSRF_ERROR, + SupersetErrorType.FRONTEND_NETWORK_ERROR, + SupersetErrorType.FRONTEND_TIMEOUT_ERROR, + SupersetErrorType.TABLE_SECURITY_ACCESS_ERROR, + SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR, + SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR, + SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR, + SupersetErrorType.MISSING_OWNERSHIP_ERROR, + SupersetErrorType.USER_ACTIVITY_SECURITY_ACCESS_ERROR, + SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR, + SupersetErrorType.CHART_SECURITY_ACCESS_ERROR, + SupersetErrorType.OAUTH2_REDIRECT, + SupersetErrorType.OAUTH2_REDIRECT_ERROR, + SupersetErrorType.BACKEND_TIMEOUT_ERROR, + SupersetErrorType.SQLLAB_TIMEOUT_ERROR, + SupersetErrorType.RESULT_TOO_LARGE_ERROR, + SupersetErrorType.INVALID_PAYLOAD_FORMAT_ERROR, + SupersetErrorType.INVALID_PAYLOAD_SCHEMA_ERROR, + SupersetErrorType.MARSHMALLOW_ERROR, + } +) + +# Statuses that report an authentication, authorization or routing decision +# rather than a query failure, so their bare-string messages survive redaction. +SAFE_STATUSES = frozenset({401, 403, 404, 429}) + + +def is_sanitization_required() -> bool: + """ + Whether the principal of the current request is an embedded guest viewer. + """ + # pylint: disable=import-outside-toplevel + from superset import security_manager + + return security_manager.is_guest_user() + + +def sanitize_error_message(message: str) -> str: + """ + Replace an error message with a generic one for embedded guest viewers. + """ + if not is_sanitization_required(): + return message + return str(GENERIC_ERROR_MESSAGE) + + +def sanitize_superset_error(error: SupersetError) -> SupersetError: + """ + Replace a ``SupersetError`` with a generic one for embedded guest viewers. + + ``extra`` is dropped along with the message: it carries engine names and, for + some error types, the offending SQL. + """ + if not is_sanitization_required() or error.error_type in SAFE_ERROR_TYPES: Review Comment: Yes, this is relevant. The allowlist currently returns the original error, so `extra` remains exposed to guest users. This conflicts with the goal of dropping engine/provider details and can leak upstream OAuth response content or metadata. The sanitizer should preserve the allowlisted message and type but sanitize `extra` as well—for example, retain only explicitly required OAuth redirect fields (`url`, `tab_id`, and `redirect_uri`) and drop all other fields. For allowlisted types without a documented client requirement, set `extra=None`. The existing OAuth test currently asserts that the full `extra` payload is preserved, so it should be updated to verify that only approved fields remain and add a regression case containing `extra["error"]`. This should also cover the async serialized-error path, since it uses the same sanitizer. -- 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]
