aminghadersohi commented on code in PR #43944: URL: https://github.com/apache/superset/pull/43944#discussion_r3946510674
########## superset-frontend/src/embedded/guestTokenDiagnostics.ts: ########## @@ -0,0 +1,71 @@ +/** + * 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 { t } from '@apache-superset/core/translation'; + +export type GuestTokenSize = { + tokenBytes: number; + headerBytes: number; + headerBudgetBytes: number | null; + headerBudgetExceeded: boolean; +}; + +/** Measure the encoded token without decoding or retaining credentials. */ +export function measureGuestToken( + token: string, + headerName = 'X-GuestToken', + budget?: number | null, +): GuestTokenSize { + const encoder = new TextEncoder(); + const tokenBytes = encoder.encode(token).length; + // HTTP/1-style accounting: name + ": " + value + CRLF, not wire compression. + const headerBytes = tokenBytes + encoder.encode(headerName).length + 4; + const headerBudgetBytes = + typeof budget === 'number' && Number.isSafeInteger(budget) && budget > 0 + ? budget + : null; + return { + tokenBytes, + headerBytes, + headerBudgetBytes, + headerBudgetExceeded: + headerBudgetBytes !== null && headerBytes > headerBudgetBytes, + }; +} + +/** Diagnose from size evidence only; never inspect or log proxy response bodies. */ +export function guestAuthenticationMessage( + size?: GuestTokenSize, + error?: unknown, +): string { + const status = + typeof error === 'object' && error !== null && 'status' in error + ? error.status + : undefined; + // Explicit auth/server failures have other causes. Some non-JSON proxy + // failures lose their status during parsing, so size remains the evidence. + const possibleHeaderFailure = + status === undefined || status === 400 || status === 431; Review Comment: `431` is exactly the right status to include, and gating on `status === undefined` to catch non-JSON proxy responses is a good call. Question, not a finding: nginx returns the non-standard **494 Request header too large** for this case rather than 431, and some proxies surface 413. Since the message is only ever shown when `headerBudgetExceeded` is already true, widening this to include `494` (and possibly `413`) looks low-risk and would cover the most common self-hosted proxy in front of Superset. Deliberate omission, or worth adding? ########## superset/security/guest_token.py: ########## @@ -40,7 +42,17 @@ def build_guest_token_audit_payload( """ resources = body.get("resources") or [] rls = body.get("rls") or [] + token_bytes = len(token.encode("utf-8")) + # HTTP/1-style accounting: name + colon-space + value + CRLF. + header_bytes = token_bytes + len(header_name.encode("utf-8")) + 4 + budget = ( + header_budget_bytes if header_budget_bytes and header_budget_bytes > 0 else None Review Comment: Minor normalization asymmetry with the frontend. Here the guard is truthiness plus `> 0`; `guestTokenDiagnostics.ts` requires `Number.isSafeInteger(budget) && budget > 0`. So a config of `GUEST_TOKEN_HEADER_MAX_BYTES = 16 * 1024.0` warns in the server log but is silently dropped in the browser, and the operator never sees the targeted message they configured the budget to get. `True` is worse in a funny way — it is `> 0`, so every token "exceeds". One-liner if you think it is worth it: ```python budget = ( header_budget_bytes if isinstance(header_budget_bytes, int) and not isinstance(header_budget_bytes, bool) and header_budget_bytes > 0 else None ) ``` The declared type is already `int | None`, so this is belt-and-braces against a hand-edited `superset_config.py` rather than a real defect. Your call. ########## superset-frontend/src/embedded/index.tsx: ########## @@ -268,18 +287,19 @@ function setupGuestClient(guestToken: string) { window.addEventListener('message', function embeddedPageInitializer(event) { if (!validateMessageEvent(event, bootstrapData.embedded?.allowed_domains)) { - log('ignoring message unrelated to embedded comms', event); + log('ignoring message unrelated to embedded comms'); return; } const port = event.ports?.[0]; if (event.data.handshake === 'port transfer' && port) { - log('message port received', event); + log('message port received'); Switchboard.init({ port, name: 'superset', - debug: debugMode, + // Switchboard debug logs message bodies, including guest-token credentials. + debug: false, Review Comment: Correct fix for this call site, and the comment above it is accurate. Two things worth recording in the PR body, since neither is visible from the diff: 1. **This was dev-build-only.** `debugMode` is `process.env.WEBPACK_MODE === 'development'` (L97), injected by DefinePlugin at `webpack.config.js:147`. Production bundles never had `debug: true`, so this is hardening for local dev against a real deployment, not a shipped credential exposure. Worth stating so this is neither missed nor mistaken for a backport-worthy security fix. 2. **The disclosure site itself is untouched.** `packages/superset-ui-switchboard/src/switchboard.ts:124` still does `this.log('message received', event)`, and `@superset-ui/switchboard` is published — any other consumer passing `debug: true` still logs full message bodies, guest token included. Redacting inside `Switchboard.log()` (message id / `event.data.method` rather than the whole `MessageEvent`) would cover every caller *and* let this line stay `debug: debugMode`, which would preserve Switchboard tracing in dev. As written, the embedded entry point loses that tracing outright. Intentional? Non-blocking either way; a follow-up PR is fine. -- 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]
