fitzee commented on code in PR #43944:
URL: https://github.com/apache/superset/pull/43944#discussion_r3952895861
##########
superset-frontend/src/embedded/index.tsx:
##########
@@ -225,18 +233,18 @@ function start() {
}
root.render(<EmbeddedApp />);
},
- err => {
+ (error: unknown) => {
// something is most likely wrong with the guest token; reset the
guard
// so a rehandshake with a valid token can retry.
- logging.error(err);
- showFailureMessage(
- t(
- 'Something went wrong with embedded authentication. Check the
dev console for details.',
- ),
- );
+ // A refresh while the request is in flight makes attribution
ambiguous.
+ const size =
+ requestTokenSize === guestTokenSize ? requestTokenSize : undefined;
+ logging.error('Embedded authentication failed', size);
+ showFailureMessage(guestAuthenticationMessage(size, error));
started = false;
Review Comment:
Investigated; I am not changing the authentication guard. The existing
started guard prevents a second /me/roles request while the first is pending; a
token refresh updates the client but does not dispatch another request.
Resetting the guard on failure is necessary for a subsequent token message to
retry. In 9f22eaebcd I strengthened the pending-refresh test to verify that
next-token retry, while retaining generic messaging when size attribution is
ambiguous. Both focused embedded suites pass (39 tests). Leaving this disputed
thread open for reviewer confirmation rather than changing pre-existing
auth/retry behavior in a diagnostics-only PR.
##########
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:
Clarified both points in the PR body. This was development-build-only
tracing; production already had debug disabled. The loss of embedded
Switchboard tracing is intentional callsite containment, with safe size
diagnostics retained. The published Switchboard implementation and other
debug-enabled consumers are unchanged; library-wide redaction is a separate
follow-up, not included here. No production-exposure/backport claim. Focused
embedded suites pass 39 tests, including the debug-disabled assertion.
##########
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:
Addressed in 9f22eaebcd with matching positive-safe-integer normalization
and paired Python/TypeScript boundary cases. Strings, booleans,
fractional/non-finite values, and values above 2^53-1 disable the budget
safely. One detail: Number.isSafeInteger(16384.0) is true, so integral floats
remain accepted on both sides rather than introducing a new mismatch. Frontend
tests pass (39 total); mypy and frontend type checking pass. Python regression
tests are added, but local pytest cannot import conftest because paramiko is
missing.
##########
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:
Added 494 in 9f22eaebcd for cases where that header-too-large status is
surfaced, still requiring measured oversized-token evidence. Tests verify 494
without size evidence stays generic. I kept 413 generic: RFC 9110 defines it
for request content size, not specifically headers, so an oversized token does
not establish the cause. Added an explicit 413 generic-fallback test. All 39
focused tests pass. Leaving this thread open for your confirmation of that
conservative 413 choice. Reference:
https://www.rfc-editor.org/rfc/rfc9110.html#name-413-content-too-large
##########
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 = (
Review Comment:
Fixed in 9f22eaebcd: validate the runtime setting before numeric comparison.
String and other invalid budgets disable diagnostics (matching the browser),
rather than raising or coercing configuration. Docs explicitly require
converting environment-variable strings to integers to activate the budget.
Added helper normalization cases and issuance regression cases for string/bool
settings preserving HTTP 200, response/token, and grants. Mypy passes; local
pytest remains blocked during conftest import by missing paramiko, so those
Python runtime tests are not claimed as passing.
--
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]
