fix(semantic-layers): mask nested and union secrets in layer configuration - #43827
fix(semantic-layers): mask nested and union secrets in layer configuration#43827mikebridge wants to merge 1 commit into
Conversation
…ation Hardening follow-up to apache#43474, which added top-level masking of write-only configuration fields on the semantic-layer read endpoints. That masker scans only the schema's top-level `properties`, so a secret nested inside an object, a discriminated union, or a list — e.g. a Snowflake `auth.password` SecretStr in a `password`/`key` auth union — is still returned in the clear. This extends apache#43474's masking to walk the connector's published `get_configuration_schema()` recursively, keeping its established behavior (mask only fields the schema marks secret via `writeOnly` / `SecretStr`, reveal everything else, and fail closed by masking the whole payload when the schema is unavailable) while now catching secrets at any depth. * New `superset/semantic_layers/masking.py`: a schema-directed walker (`mask_configuration` / `unmask_configuration`) that resolves `$ref` into `$defs`, descends objects, discriminated unions (anyOf/oneOf/allOf), and lists, and — for a key described by several union variants — masks it if any variant marks it secret. `_serialize_layer` and the update round-trip now delegate to it, so read-masking and the masked-value passthrough become recursive together (a nested masked value echoed back on update is restored from the stored config, not written as the mask string). * Addresses Amin's review Finding 1: free-form keys governed by `additionalProperties` are classified against every union variant, not just the first, so a variant that marks such a key secret can no longer be overridden by an earlier variant that would reveal it. * Addresses Amin's Finding 5: masking covers the stored configuration payload only, never a schema a provider builds from it. The module docstring records the provider contract that `get_configuration_schema` / `get_runtime_schema` responses (returned to clients verbatim, e.g. the `runtime_schema` endpoint) must not echo configuration values back into the schema. Tests: masking unit tests to 100% package coverage — nested/union/list secret masking (the closed gap), reveal-of-undescribed-and-non-secret fields (matching apache#43474), the additionalProperties-divergence case (Finding 1), and the fail-closed paths (unregistered type, schema raises, non-dict schema, pathological recursion depth). apache#43474's own api/update tests pass unchanged against the delegating functions. Shortcut: sc-119467 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
Code Review Agent Run #65670cActionable Suggestions - 0Additional Suggestions - 3
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43827 +/- ##
===========================================
+ Coverage 57.47% 79.42% +21.94%
===========================================
Files 2894 2895 +1
Lines 167820 167912 +92
Branches 38863 38886 +23
===========================================
+ Hits 96453 133361 +36908
+ Misses 70468 32051 -38417
- Partials 899 2500 +1601
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Reviewed at bd9b7e3a3579d7efdfdf173499251d7c8abcea13. Comment only — I can't approve, so this is not an approval and not a request for changes. CI is green at this head (deduped newest-run-per-name: 48 success / 12 skipped / 3 neutral / 0 failure).
The gap this closes is real, and I reproduced both halves
I ran the merged #43474 masker and this one side by side against real pydantic-generated schemas (not by reading the source).
#43474 on this PR's own DemoConfig fixture — every top-level property is anyOf/oneOf/$ref-shaped, so secret_keys comes out empty and the if not secret_keys: return config branch returns the whole payload verbatim:
{"account": "acme", "token": "TOP-SECRET", "auth": {"kind": "password", "password": "NESTED-SECRET"}}
This PR, same input: token and auth.password both XXXXXXXXXX.
I also ran a broader realistic model — Annotated[Union[PwAuth, KeyAuth], Field(discriminator="kind")], list[Cred], dict[str, Cred], Optional[list[Cred]], list[SecretStr], Optional[SecretStr] — and every secret was masked at every depth, with unmask_configuration(stored, masked) == stored round-tripping losslessly. Optional[SecretStr] (anyOf + no top-level writeOnly) and a nested model behind $ref are the two ordinary declarations that leaked before; both are covered now. The $ref cycle guard, the _UNION_KEYS walk, the object/list recursion, and the union-conservative _combine_masked are all doing what the docstrings say. Nice piece of work, and the test module is unusually thorough — it covers the fail-closed paths (no provider, schema raises, non-dict schema, depth limit, additionalProperties divergence), not just the happy path.
I also confirmed _serialize_layer is the only client-facing emitter of the stored column: the other read of SemanticView.configuration in api.py (~L797) only builds a dedupe key, and _serialize_semantic_layer doesn't include it. The write path is genuinely rewired — update.py::_unmask_configuration delegates, and the old top-level-only dict comprehension is gone.
What I'd like to raise
The theme of all four inline notes is the same: the module's stated fail-closed posture and the implemented posture disagree, and there are still positions where a schema-marked secret is revealed. I don't think any of them is a shipping blocker, and for the biggest one I think the docstring is the thing that's wrong, not the code — but on a redaction control that ambiguity is worth removing before merge rather than after.
I measured this ladder of degenerate schemas against the same config ({"account", "password", "auth": {"user", "password"}}):
get_configuration_schema() returns |
result |
|---|---|
| raises | everything masked ✅ |
None (non-dict) |
everything masked ✅ |
{} |
entire config in the clear |
{"type": "object"} |
entire config in the clear |
{"title": "C"} |
entire config in the clear |
The first two fail closed; the last three fail open, silently. That is the closest surviving relative of #43474's if not secret_keys: return config, and it's the one I'd most like to see closed (inline note 3) — it's a one-line guard.
Smaller notes
_mask_alland_mask_valueboth reveal falsy scalars ("",0,False). Consistent with #43474 and defensible ("an empty value hides nothing"), but note it applies even on the total fail-closed path, where by construction nothing is known about the field. Probably fine; flagging only so it's a deliberate choice.- The two
unmask_configurationlimitations you document in the docstring (can't set a literalPASSWORD_MASK; lists matched by index, so reordering a credential list while echoing a mask can move a stored secret) are the same onesBaseEngineSpec.unmask_encrypted_extracarries. Disclosed, mirrored, and I agree they don't bite the scalar credential shapes in use — noting them only as read-and-accepted. - The bito bot raised two of these independently (the docstring, and the
additionalPropertiesunion gate) plus a third I agree with:api.pydropped thelogger.warningthe old_mask_configurationemitted when the schema was unavailable. Now that fail-closed happens silently insidemask_configuration, an operator seeing an entirely-masked configuration has no way to tell whether it's an unregistered extension, a raising provider, or a genuinely all-secret payload. Alogger.warningin each of the three fail-closed branches would be worth restoring. - The branch is 15 commits behind master (ahead 1,
diverged). Nothing in the base looks like it touches this code, but a rebase before merge would be tidy. - The description cites "Review Finding 1" and "Review Finding 5" from a numbered review I don't have. I'm not assuming 2–4 were addressed or dropped — if any of them are still open against this file, it'd help to say so in the description.
Thanks for chasing the nested/union case rather than stopping at the top-level fix; Optional[SecretStr] alone would have kept that hole open indefinitely.
|
|
||
| Fail-closed posture: when the schema cannot say which fields are secret — | ||
| the layer's type has no registered provider (extension not loaded), schema | ||
| generation fails, or a key is not described by the schema — every scalar |
There was a problem hiding this comment.
The module docstring contradicts the implemented posture, in the one clause a reader of a redaction control will trust most.
"when the schema cannot say which fields are secret — the layer's type has no registered provider, schema generation fails, or a key is not described by the schema — every scalar value in the affected subtree is masked rather than exposed."
The first two clauses are true. The third is not: _mask_object reveals undescribed keys (L219-224, else item), and test_reveals_keys_the_schema_does_not_describe_but_masks_marked_secrets asserts exactly that. Measured against a provider whose schema declares account/token/auth but not legacy_password:
in : {"account":"acme","token":"TOK","auth":{...},"legacy_password":"OLD-SECRET"}
out: {"account":"acme","token":"XXXXXXXXXX","auth":{...masked...},"legacy_password":"OLD-SECRET"}
Same at nested depth: an undeclared auth.legacy_token comes back in the clear.
I think the code is right and the docstring is wrong. The PR description states the intent plainly — "keeping #43474's established behavior: mask only fields the schema marks secret, reveal everything else" — and strict fail-closed here would mask legitimate non-secret fields every time the stored payload outlives a schema revision, which is a real UX regression for a routine schema evolution. So this is a docs fix, not a behavior change.
But please make it explicit, because as written the docstring promises a security property the module does not provide, and the next person to touch this will either weaken the code to match the docs or trust a guarantee that isn't there. Something like: fail closed when the schema as a whole is unusable (no provider, generation raises, non-dict); within a usable schema, reveal anything it does not mark secret.
| # only masked where the schema marks it, matching #43474). | ||
| masked[key] = ( | ||
| _mask_against(item, additional_schemas, defs) | ||
| if additional_schemas and all_variants_classify_extra |
There was a problem hiding this comment.
all_variants_classify_extra inverts this block's own stated rule, and leaves "Review Finding 1" only half-fixed.
The comment two lines up says: "If any variant does not describe such keys with a schema (no dict additionalProperties), the key is unclassifiable there, so fail closed and mask it." The code does the opposite — when all_variants_classify_extra is False it takes the else item branch and reveals.
Measured, on a union where one variant marks free-form keys secret and the sibling is simply silent about them:
anyOf: [ {properties:{host}, additionalProperties:{format:password, writeOnly:true}},
{properties:{host}} ]
value: {"host":"h","extra_cred":"LEAKY"}
-> {"host":"h","extra_cred":"LEAKY"} # revealed
Add additionalProperties: {"type":"string"} to the second variant and the same value masks correctly. So the leak isn't triggered by two variants disagreeing — it's triggered by one variant not saying anything, which is the more common shape.
This is the same principle _combine_masked's docstring states ("a secret nested inside a single union branch is masked even when a sibling branch would have revealed the same key") and the same one the PR description claims for Finding 1 ("a variant marking such a key secret can't be overridden by an earlier variant that would reveal it"). Here a variant marking the key secret is overridden — by a variant that says nothing at all.
Suggested fix is to drop the extra gate:
masked[key] = _mask_against(item, additional_schemas, defs) if additional_schemas else itemA silent variant means "unconstrained", not "not secret", so treating it as unclassifiable and masking is the safe direction. _combine_masked already handles the divergence. Worth a test alongside test_additionalproperties_divergent_union_variants_mask_conservatively, which only covers the both-variants-classify case.
(Independently flagged by the bito bot as CWE-200 on this hunk; I confirmed it by execution.)
| return _mask_all(configuration) | ||
| if not isinstance(schema, dict): | ||
| return _mask_all(configuration) | ||
| return _mask_value(configuration, schema, schema.get("$defs", {})) |
There was a problem hiding this comment.
Three fail-closed guards, but a fourth degenerate schema falls through to a full reveal.
mask_configuration fails closed on no provider, on a raising get_configuration_schema, and on a non-dict schema. It does not fail closed on a dict schema that describes nothing. Measured against {"account":"acme","password":"S3CRET","auth":{"user":"bob","password":"NESTED"}}:
| schema returned | result |
|---|---|
| raises | all masked ✅ |
None |
all masked ✅ |
{} |
whole config in the clear |
{"type": "object"} |
whole config in the clear |
{"title": "C"} |
whole config in the clear |
{} and None are two spellings of the same failure and they land on opposite sides of the fence. This is structurally the same shape as #43474's if not secret_keys: return config — the branch this PR is here to remove — just relocated from "no secret keys found" to "no keys found at all".
Cheap to close:
if not isinstance(schema, dict) or not schema:
return _mask_all(configuration)and, if you want the stronger version, treat a top-level schema that yields no _object_variants the same way — a provider that can't describe its own configuration as an object hasn't told you anything about it.
| ) -> dict[str, Any]: | ||
| """Mask a dict value against the object schemas it may conform to.""" | ||
| variants = _object_variants(schema, defs) | ||
| if not variants: |
There was a problem hiding this comment.
An unresolvable $ref reveals a subtree the schema does mark secret.
_resolve_ref returns {} for a missing or cyclic $defs target, _object_variants then returns [], and this branch reveals the whole subtree. Measured — same Config as above, with $defs emptied but properties.auth still {"$ref": "#/$defs/Auth"}:
{"account":"acme","token":"XXXXXXXXXX","api_key":"XXXXXXXXXX",
"auth":{"user":"bob","password":"NESTED-SECRET"}}
Auth.password is writeOnly: true in the schema the provider intended to publish. It comes back in the clear only because the reference couldn't be followed.
I recognise this is deliberate — test_unresolvable_ref_reveals_its_subtree codifies it, and the reasoning ("a real provider schema resolves its refs") is fair. But it isn't the same case as "the schema doesn't describe this key": here the schema did try to describe it and the walker couldn't follow. That's a schema-resolution failure, and every other schema-resolution failure in this module fails closed.
Worth distinguishing "$ref present but unresolvable" (→ _mask_all) from "no object schema at this position" (→ reveal). _resolve_ref already knows the difference; it just flattens both into {}. Low likelihood, but the cost of being wrong is a nested credential in a GET response, and the fix is local.
Same argument applies to _mask_list's untyped-array reveal (L254) if the array schema came from an unresolvable $ref.
| key: PASSWORD_MASK if key in secret_keys and value else value | ||
| for key, value in config.items() | ||
| } | ||
| return mask_configuration(layer.type, config) |
There was a problem hiding this comment.
The old _mask_configuration logged a logger.warning naming the layer type whenever it hit the fail-closed path. That's gone, and mask_configuration fails closed silently in all three of its branches (cls is None, get_configuration_schema raising, non-dict schema).
The behavior is right, but the observability regressed: an operator looking at a fully-XXXXXXXXXX configuration can no longer distinguish "the extension isn't loaded" from "the provider's schema call is throwing" from "this payload really is all secrets" — and the first two are outages that will otherwise present as a confusing UI. A logger.warning in each fail-closed branch of masking.mask_configuration, carrying layer_type and the reason, would restore it. (Also raised by the bito bot.)
2 participants
SUMMARY
Hardening follow-up to #43474, which added masking of write-only
configuration fields on the semantic-layer read endpoints. That masker scans
only the schema's top-level
properties, so a secret nested inside anobject, a discriminated union, or a list is still returned in the clear — for
example a Snowflake
auth.passwordSecretStrinside apassword/keyauthunion leaks on
GET /api/v1/semantic_layer/<uuid>.This extends #43474 (it does not replace it): the masking now walks the
connector's published
get_configuration_schema()recursively, keeping#43474's established behavior — mask only fields the schema marks secret
(
writeOnly/SecretStr), reveal everything else, and fail closed by maskingthe whole payload when the schema can't be loaded — while catching secrets at
any depth.
What changed
superset/semantic_layers/masking.py: a schema-directed walker(
mask_configuration/unmask_configuration) that resolves$refinto$defsand descends objects, unions (anyOf/oneOf/allOf), and lists.For a key described by several union variants it masks the key if any
variant marks it secret.
_serialize_layer(read) andUpdateSemanticLayerCommand's round-trip(write) now delegate to it, so masking and the masked-value passthrough
become recursive together: a nested masked value a client echoes back on
update is restored from the stored config, not written as the mask string.
additionalPropertiesareclassified against every union variant, not just the first, so a variant
marking such a key secret can't be overridden by an earlier variant that
would reveal it.
get_configuration_schema/get_runtime_schemaresponses (returned toclients verbatim, e.g. the
runtime_schemaendpoint) must not echoconfiguration values back into the schema — masking covers the stored
payload, not a schema built from it.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — API masking change, no UI surface.
Before: a nested/union secret (e.g.
configuration.auth.password) is returnedin the clear by the read endpoints. After: it is
XXXXXXXXXX, like thetop-level secrets #43474 already masked.
TESTING INSTRUCTIONS
New/adapted masking unit tests to 100% package coverage: nested/union/list
secret masking (the closed gap), reveal of undescribed and non-secret fields
(matching #43474), the
additionalProperties-divergence case (Finding 1), andevery fail-closed path (unregistered type, schema raises, non-dict schema,
pathological recursion depth). #43474's own
api_test/update_testpassunchanged against the delegating functions.
ADDITIONAL INFORMATION
SEMANTIC_LAYERS(development, default off)Credit: builds directly on the masking pattern and provider-schema source
established in #43474.
🤖 Generated with Claude Code