fix(store): descend through non-string-keyed nodes in models.yml credential denylist (RIG-3116) - #826
Open
rigel-mintaka wants to merge 2 commits into
Open
Conversation
…ential denylist (RIG-3116)
`rejectCredentialModels` — the store-door CP-4 guard keeping provider
credentials off the config bundle — descended into `providers`, each
provider, and `headers` via direct `.(map[string]any)` assertions. yaml.v3
decodes a mapping to `map[any]any` the moment any key in it is non-string, so
a non-string sibling key flipped the enclosing node's Go type and every
assertion on it failed OPEN: the walk skipped the credential-bearing sibling
and the secret rode the bundle un-rejected.
This is the same fail-open bug class RIG-2968's round-2 fix closed for the
settings/profile-settings credential walk (via `yamlMapIndex`), left unclosed
here because this door ENUMERATES arbitrary provider/header names rather than
doing keyed lookups over known paths. Surfaced by the RIG-2968 round-3
adversarial review's cross-member parity check; three shapes proven accepted
end-to-end through `validateAndHashConfigBundle`:
- provider-level shield: `providers.openai.{0: junk, apiKey: …}` hides the apiKey.
- providers-level shield: `providers.{0: junk, openai: {apiKey: …}}` skips ALL providers.
- headers-level shield: `providers.x.headers.{0: junk, Authorization: …}` skips the literal-secret header.
Fix: add `yamlMapEntries`, the enumeration analog of `yamlMapIndex` — it
iterates both map shapes and returns a collision-safe slice (a non-string key
that stringifies to a sibling's name cannot overwrite it). Route
`rejectCredentialModels`' descent through `yamlMapEntries` (providers, headers)
and `yamlMapIndex` (per-provider apiKey/headers lookup), so no sibling can flip
a node and shield a credential leaf. Both credential doors now share one
convention: detect credentials regardless of map shape, tolerate benign
non-string keys, reject only credentials.
`parseYAMLMapping` already fails CLOSED on a top-level non-string key, so the
three inner descents were the only fail-open sites.
Tests: three red-first reject cases (one per shield site, each verified to
redden when its defending helper's `map[any]any` branch is neutered) plus an
accept-symmetry case (a nested non-string key with no credential leaf must
still be ACCEPTED).
Refs RIG-3116
Co-authored-by: Matt Wilkinson <matt@rigel.build>
|
Compass engineering docs preview: https://compass-server-rig-3116-mode.compass-eng-docs.pages.dev Deployed from |
…dential denylist (RIG-3116) Adds the combined-shield reject case from the round-1 review (Low #1): a non-string sibling at BOTH the `providers` node AND the provider node simultaneously. Cases (a)/(b) already cover each flip point in isolation via `yamlMapIndex` / `yamlMapEntries`; the two helpers descend linearly with no shared state, so the combined case cannot regress unless a singleton does. Pinned explicitly to defend the composition against a future refactor that couples the descents. Review-loop disposition for PR #826 (round 1, clean pass at floor — 0 high, 0 medium): Low #1 fixed here; Low #2 (non-string header VALUE skip, a pre-existing byte-for-byte-unchanged value-shape axis) deferred to RIG-3134. Spec-impact: none (test-only; production behavior unchanged). Refs RIG-3116 Refs RIG-3134 Co-authored-by: Matt Wilkinson <matt@rigel.build>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
rejectCredentialModels— the store-door CP-4 guard keeping providercredentials off the config bundle — descended into
providers, eachprovider, and
headersvia direct.(map[string]any)assertions. yaml.v3decodes a mapping to
map[any]anythe moment any key in it is non-string, soa non-string sibling key flipped the enclosing node's Go type and every
assertion on it failed OPEN: the walk skipped the credential-bearing sibling
and the secret rode the bundle un-rejected.
This is the same fail-open bug class RIG-2968's round-2 fix closed for the
settings/profile-settings credential walk (via
yamlMapIndex), left unclosedhere because this door ENUMERATES arbitrary provider/header names rather than
doing keyed lookups over known paths. Surfaced by the RIG-2968 round-3
adversarial review's cross-member parity check; three shapes proven accepted
end-to-end through
validateAndHashConfigBundle:providers.openai.{0: junk, apiKey: …}hides the apiKey.providers.{0: junk, openai: {apiKey: …}}skips ALL providers.providers.x.headers.{0: junk, Authorization: …}skips the literal-secret header.Fix: add
yamlMapEntries, the enumeration analog ofyamlMapIndex— ititerates both map shapes and returns a collision-safe slice (a non-string key
that stringifies to a sibling's name cannot overwrite it). Route
rejectCredentialModels' descent throughyamlMapEntries(providers, headers)and
yamlMapIndex(per-provider apiKey/headers lookup), so no sibling can flipa node and shield a credential leaf. Both credential doors now share one
convention: detect credentials regardless of map shape, tolerate benign
non-string keys, reject only credentials.
parseYAMLMappingalready fails CLOSED on a top-level non-string key, so thethree inner descents were the only fail-open sites.
Tests: three red-first reject cases (one per shield site, each verified to
redden when its defending helper's
map[any]anybranch is neutered) plus anaccept-symmetry case (a nested non-string key with no credential leaf must
still be ACCEPTED).
Refs RIG-3116
Co-authored-by: Matt Wilkinson matt@rigel.build