rusackas commented on code in PR #44036:
URL: https://github.com/apache/superset/pull/44036#discussion_r3974750977
##########
superset/security/manager.py:
##########
@@ -5349,11 +5349,24 @@ def validate_guest_token_resources(resources:
GuestTokenResources) -> None:
embedded =
EmbeddedDashboardDAO.find_by_id(str(resource["id"]))
if not embedded:
raise EmbeddedDashboardNotFoundError()
+ dashboard = embedded.dashboard
elif not dashboard.embedded:
# A raw dashboard id must still reference an embedded
dashboard;
# otherwise a guest token could be scoped to a
non-embedded one.
raise EmbeddedDashboardNotFoundError()
+ # The caller minting the token must themselves be entitled to
+ # the dashboard being scoped. `grant_guest_token` is a
+ # coarse, instance-wide permission -- without this check, an
+ # operator who narrows it to a non-Admin role (a realistic
+ # "embedding backend service" grant) would let that
+ # principal mint a fully valid guest token for *any*
+ # embedded dashboard, not just ones they have access to.
+ try:
+ self.raise_for_access(dashboard=dashboard)
+ except SupersetSecurityException as ex:
+ raise EmbeddedDashboardAccessDeniedError() from ex
Review Comment:
Good catch, fixed in 9de81a7. The endpoint now translates that denial to a
403 (the exception already carried the status, it just wasn't in the endpoint's
except list), and there's an API-level test pinning it so @safe can't quietly
turn it back into a 500.
##########
superset/commands/semantic_layer/update.py:
##########
@@ -71,6 +92,26 @@ def _unmask_configuration(
except (TypeError, ValueError):
existing_configuration = {}
+ masked_keys = {
+ key
+ for key, value in new_configuration.items()
+ if value == PASSWORD_MASK and key in existing_configuration
+ }
+ # `.get(key)` alone can't tell "key absent from storage" apart from "key
+ # present and stored as None" -- both return None -- so a newly
+ # introduced key with an explicit None value would be misread as
+ # unchanged and let a masked secret slip through alongside it. A
+ # sentinel default makes that distinction explicit.
+ if masked_keys and any(
Review Comment:
Good catch, fixed in 9de81a7. Removed keys now count as a configuration
change too, so the masked round-trip only passes when the submitted key set and
values match what's stored exactly. Dropping a key with a fresh secret is still
allowed, and both cases have tests.
--
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]
