codeant-ai-for-open-source[bot] commented on code in PR #42796: URL: https://github.com/apache/superset/pull/42796#discussion_r3721271242
########## 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: **Suggestion:** The allowlist returns the original `SupersetError`, including `extra`, so guest responses can still expose sensitive values. For example, `OAuth2TokenRefreshError` uses an allowlisted type but stores the upstream OAuth response text in `extra["error"]`; return a sanitized error without `extra` even for allowlisted types, or explicitly remove sensitive fields while preserving only the data required by the client. [security] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Embedded OAuth responses can expose provider error details. - ⚠️ Async job errors preserve sensitive OAuth response fields. - ⚠️ Error payloads may disclose credentials or provider metadata. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ec55f9bf6ae643fdb33b2288e752fa36&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=ec55f9bf6ae643fdb33b2288e752fa36&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/utils/error_sanitization.py **Line:** 96:96 **Comment:** *Security: The allowlist returns the original `SupersetError`, including `extra`, so guest responses can still expose sensitive values. For example, `OAuth2TokenRefreshError` uses an allowlisted type but stores the upstream OAuth response text in `extra["error"]`; return a sanitized error without `extra` even for allowlisted types, or explicitly remove sensitive fields while preserving only the data required by the client. 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%2F42796&comment_hash=07010d19c98bce40daf8a36ab5abd59a326d87c6a31e68fc679b925546df5098&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42796&comment_hash=07010d19c98bce40daf8a36ab5abd59a326d87c6a31e68fc679b925546df5098&reaction=dislike'>👎</a> ########## superset/views/error_handling.py: ########## @@ -74,12 +79,20 @@ def json_error_response( ) -> FlaskResponse: payload = payload or {} + if isinstance(error_details, SupersetError): + error_details = [error_details] + if isinstance(error_details, list): - payload["errors"] = [dataclasses.asdict(error) for error in error_details] - elif isinstance(error_details, SupersetError): - payload["errors"] = [dataclasses.asdict(error_details)] + payload["errors"] = [ + dataclasses.asdict(error) + for error in sanitize_superset_errors(error_details) + ] elif isinstance(error_details, str): - payload["error"] = error_details + payload["error"] = ( + error_details + if status in SAFE_STATUSES + else sanitize_error_message(error_details) + ) Review Comment: **Suggestion:** Preserving every bare-string message for statuses in `SAFE_STATUSES` bypasses the guest redaction policy. Existing callers use status 404 for messages containing database and table names, such as the core view's “Table ... wasn't found in the database ...” response, so an embedded viewer can still receive warehouse metadata. Restrict this bypass to known authorization/routing messages or sanitize these messages based on their originating error type rather than status alone. [security] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Legacy Explore errors disclose warehouse identifiers. - ⚠️ Embedded viewers can observe supplied database metadata. - ⚠️ Status alone cannot distinguish routing and backend errors. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5baf32f24cd949a0b3d46305638edc2f&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=5baf32f24cd949a0b3d46305638edc2f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/views/error_handling.py **Line:** 90:95 **Comment:** *Security: Preserving every bare-string message for statuses in `SAFE_STATUSES` bypasses the guest redaction policy. Existing callers use status 404 for messages containing database and table names, such as the core view's “Table ... wasn't found in the database ...” response, so an embedded viewer can still receive warehouse metadata. Restrict this bypass to known authorization/routing messages or sanitize these messages based on their originating error type rather than status alone. 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%2F42796&comment_hash=a5bc7690d67a1b4b6f3d6071b25609627c08e501d9f8a2250fd49e25510650cf&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42796&comment_hash=a5bc7690d67a1b4b6f3d6071b25609627c08e501d9f8a2250fd49e25510650cf&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]
