Skip to content

fix(store): check model-level headers at the models.yml door (RIG-5287) #2227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattwilkinsonn merged 1 commit into from
Oct 12, 2026
Merged

fix(store): check model-level headers at the models.yml door (RIG-5287) #2227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions docs/designs/agent/compass-agent-config-passthrough/design.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,19 @@
> in 17.1.8 and to be re-verified at upgrade time, not a load-bearing claim
> for the shipped mechanism.
>
> **Amendment (2026-10-11) — model-level headers are the same credential
> surface.** CP-4 and T1 name `providers.<name>.headers.*`. The installed SDK
> (`@oh-my-pi/pi-coding-agent@18.0.11`) also reads
> `providers.<name>.models[].headers` (`mergeCustomModelHeaders`,
> `custom-models.ts`) and `providers.<name>.modelOverrides.<id>.headers`
> (`createLiveConfigHeaders`, `model-config-values.ts`). Both merge onto the
> model's headers and resolve each value through the same `resolveConfigValue`.
> The door applies the CP-4 header rule at all three sites. It also refuses a
> header value that is not a string (the schema admits only strings) and a
> non-mapping `providers`, `modelOverrides` or `headers` node: the schema
> coerces a list at a record-typed node into an index-keyed record, so a list
> still reaches the registry.
>
> Freezes on merge; later changes supersede by citation, never rewrite
> (convention: `../compass-0.5/design.md:10-12`). Tracked as RIG-1678. Extends
> the frozen RIG-1568 record
Expand Down
71 changes: 51 additions & 20 deletions go/internal/store/agent_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ func validateSettingsMember(joined string, r io.Reader) ([]byte, error) {
}

// validateModelsMember validates the top-level models.yml member: a YAML mapping
// with no credentialed provider surface (apiKey, or a headers.* literal secret).
// with no credentialed provider surface (see rejectCredentialModels).
func validateModelsMember(joined string, r io.Reader) ([]byte, error) {
content, err := io.ReadAll(r)
if err != nil {
Expand Down Expand Up @@ -976,40 +976,71 @@ func yamlMapEntries(node any) []yamlMapEntry {
// provider surface (CP-4): providers.<name>.apiKey, or providers.<name>.headers.<h> set
// to a NON-env-indirection literal (a pinned secret). An env-referenced header passes.
// The door can't see the container env, so "is an env reference" is a syntactic judgment.
// A header value that is not a string, or a headers node that is not a mapping, is
// refused rather than skipped: the SDK schema admits only string values.
// Model definitions and model overrides carry headers too, checked the same way.
func rejectCredentialModels(mapping map[string]any, joined string) error {
providers, present := mapping["providers"]
if !present {
if !present || providers == nil {
return nil
}
for _, p := range yamlMapEntries(providers) {
// The SDK schema coerces a list at a record-typed node into an index-keyed
// record, so a non-mapping node here would skip the walk; refuse it instead.
entries := yamlMapEntries(providers)
if entries == nil {
return fmt.Errorf("%w: models member %q sets providers to a non-mapping (providers must map names to provider configs)", ErrInvalidArgument, joined)
}
for _, p := range entries {
if v, present := yamlMapIndex(p.val, "apiKey"); present && v != nil {
return fmt.Errorf("%w: models member %q sets providers.%s.apiKey (provider credentials never ride the config bundle)", ErrInvalidArgument, joined, p.key)
}
headers, ok := yamlMapIndex(p.val, "headers")
if !ok || headers == nil {
continue
}
// The SDK types headers as a string map, yet sends a list's items as
// headers named by index, so a non-mapping headers node is refused.
entries := yamlMapEntries(headers)
if entries == nil {
return fmt.Errorf("%w: models member %q sets providers.%s.headers to a non-mapping (headers must map names to env references)", ErrInvalidArgument, joined, p.key)
if err := rejectCredentialHeaders(p.val, joined, "providers."+p.key); err != nil {
return err
}
for _, hdr := range entries {
s, ok := hdr.val.(string)
if !ok {
return fmt.Errorf("%w: models member %q sets providers.%s.headers.%s to a non-string value (pin it to an env reference instead)", ErrInvalidArgument, joined, p.key, hdr.key)
models, _ := yamlMapIndex(p.val, "models")
list, _ := models.([]any)
for i, m := range list {
if err := rejectCredentialHeaders(m, joined, fmt.Sprintf("providers.%s.models[%d]", p.key, i)); err != nil {
return err
}
if !isEnvIndirection(s) {
return fmt.Errorf("%w: models member %q sets providers.%s.headers.%s to a literal secret (pin it to an env reference instead)", ErrInvalidArgument, joined, p.key, hdr.key)
}
overrides, _ := yamlMapIndex(p.val, "modelOverrides")
overrideEntries := yamlMapEntries(overrides)
if overrides != nil && overrideEntries == nil {
return fmt.Errorf("%w: models member %q sets providers.%s.modelOverrides to a non-mapping (overrides must map model ids to overrides)", ErrInvalidArgument, joined, p.key)
}
for _, o := range overrideEntries {
if err := rejectCredentialHeaders(o.val, joined, "providers."+p.key+".modelOverrides."+o.key); err != nil {
return err
}
}
}
return nil
}

// rejectCredentialHeaders checks the headers node under owner (a provider, model
// definition, or model override, named by path). Each value must be a string env
// reference; the SDK sends a headers list's items under index names, so a
// non-mapping headers node is refused.
func rejectCredentialHeaders(owner any, joined, path string) error {
headers, ok := yamlMapIndex(owner, "headers")
if !ok || headers == nil {
return nil
}
entries := yamlMapEntries(headers)
if entries == nil {
return fmt.Errorf("%w: models member %q sets %s.headers to a non-mapping (headers must map names to env references)", ErrInvalidArgument, joined, path)
}
for _, hdr := range entries {
s, ok := hdr.val.(string)
if !ok {
return fmt.Errorf("%w: models member %q sets %s.headers.%s to a non-string value (pin it to an env reference instead)", ErrInvalidArgument, joined, path, hdr.key)
}
if !isEnvIndirection(s) {
return fmt.Errorf("%w: models member %q sets %s.headers.%s to a literal secret (pin it to an env reference instead)", ErrInvalidArgument, joined, path, hdr.key)
}
}
return nil
}

// envIndirectionPattern matches a bare env-var name — the syntactic form the SDK
// treats as an env reference (resolveConfigValue: env-name-first). A value that
// matches is indirection (passes); anything else is a pinned literal.
Expand Down
117 changes: 89 additions & 28 deletions go/internal/store/agent_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ func TestValidateConfigBundleAcceptsNewMembers(t *testing.T) {
{"models.yml headers !command indirection", []tarEntry{{name: "models.yml", content: "providers:\n x:\n headers:\n Authorization: \"!op read secret\"\n"}}},
{"models.yml empty headers mapping", []tarEntry{{name: "models.yml", content: "providers:\n x:\n headers: {}\n"}}},
{"models.yml null headers", []tarEntry{{name: "models.yml", content: "providers:\n x:\n headers: ~\n"}}},
{"models.yml model-level headers env reference", []tarEntry{{name: "models.yml", content: "providers:\n x:\n models:\n - id: m\n headers:\n X-Org: MY_ORG_ENV\n modelOverrides:\n m:\n headers:\n Authorization: \"!op read secret\"\n"}}},
// Symmetric to the shielded-credential models.yml reject cases: a nested
// non-string key with NO credential leaf must still be ACCEPTED. Guards
// against yamlMapEntries over-rejecting benign models config.
Expand Down Expand Up @@ -491,15 +492,10 @@ func TestValidateConfigBundleRejectsNewMembers(t *testing.T) {

// TestValidateConfigBundleRejectsCredentialKeys pins the credential denylist at
// the store door: a settings/config.yml setting a credential-marked key, a
// models.yml with providers.<name>.apiKey, and a models.yml with a
// providers.<name>.headers.* literal secret are each rejected with the offending
// key path named in the error; an env-referenced header value passes.
// models.yml with providers.<name>.apiKey, and a provider-level literal-secret
// header are each rejected with the offending key path named in the error.
func TestValidateConfigBundleRejectsCredentialKeys(t *testing.T) {
cases := []struct {
name string
member tarEntry
wantSub string // substring the error must name
}{
cases := []bundleRejectCase{
{
name: "settings credential key",
member: tarEntry{name: "settings/config.yml", content: "auth:\n broker:\n token: sekret\n"},
Expand Down Expand Up @@ -562,6 +558,48 @@ func TestValidateConfigBundleRejectsCredentialKeys(t *testing.T) {
member: tarEntry{name: "models.yml", content: "providers:\n 0: junk\n openai:\n 1: junk\n apiKey: sk-secret\n"},
wantSub: "providers.openai.apiKey",
},
{
// The only denylist entry whose leaf is a RECORD not a scalar — the
// first case where yamlPathIsSet terminates on a map node. Pins the
// door against a future change that treats a container leaf as "not
// set". Added by the SDK 18.x bump.
name: "settings record-valued credential leaf",
member: tarEntry{name: "settings/config.yml", content: "images:\n urls:\n credentials:\n s3: {key: v}\n"},
wantSub: "images.urls.credentials",
},
}
assertBundleRejects(t, cases)
}

type bundleRejectCase struct {
name string
member tarEntry
wantSub string // substring the error must name
}

// assertBundleRejects runs each case through the store door and requires an
// ErrInvalidArgument naming the offending key.
func assertBundleRejects(t *testing.T, cases []bundleRejectCase) {
t.Helper()
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
b := buildBundle(t, gzip.DefaultCompression, time.Unix(1000, 0), tc.member)
_, _, err := validateAndHashConfigBundle(b)
if !errors.Is(err, ErrInvalidArgument) {
t.Fatalf("want ErrInvalidArgument, got %v", err)
}
if !strings.Contains(err.Error(), tc.wantSub) {
t.Fatalf("error %q does not name offending key %q", err, tc.wantSub)
}
})
}
}

// TestValidateConfigBundleRejectsModelsHeaderShapes pins the models.yml header
// rule at every site the SDK reads headers from (provider, model definition,
// model override) and the non-mapping nodes the SDK schema would coerce.
func TestValidateConfigBundleRejectsModelsHeaderShapes(t *testing.T) {
assertBundleRejects(t, []bundleRejectCase{
// The SDK schema types every header value as a string, so a value of any
// other shape is not checked by isEnvIndirection; reject it here.
{
Expand Down Expand Up @@ -591,28 +629,51 @@ func TestValidateConfigBundleRejectsCredentialKeys(t *testing.T) {
member: tarEntry{name: "models.yml", content: "providers:\n x:\n headers:\n - \"Bearer sk-live-123\"\n"},
wantSub: "providers.x.headers",
},
// Model definitions and model overrides carry their own headers; the SDK
// merges them onto the model's headers and sends them the same way.
{
// The only denylist entry whose leaf is a RECORD not a scalar — the
// first case where yamlPathIsSet terminates on a map node. Pins the
// door against a future change that treats a container leaf as "not
// set". Added by the SDK 18.x bump.
name: "settings record-valued credential leaf",
member: tarEntry{name: "settings/config.yml", content: "images:\n urls:\n credentials:\n s3: {key: v}\n"},
wantSub: "images.urls.credentials",
name: "models model-definition header literal secret",
member: tarEntry{name: "models.yml", content: "providers:\n x:\n models:\n - id: m\n headers:\n Authorization: \"Bearer sk-live-123\"\n"},
wantSub: "providers.x.models[0].headers.Authorization",
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
b := buildBundle(t, gzip.DefaultCompression, time.Unix(1000, 0), tc.member)
_, _, err := validateAndHashConfigBundle(b)
if !errors.Is(err, ErrInvalidArgument) {
t.Fatalf("want ErrInvalidArgument, got %v", err)
}
if !strings.Contains(err.Error(), tc.wantSub) {
t.Fatalf("error %q does not name offending key %q", err, tc.wantSub)
}
})
}
{
name: "models model-definition header non-string value",
member: tarEntry{name: "models.yml", content: "providers:\n x:\n models:\n - id: ok\n - id: m\n headers:\n X-Key: 1234567890\n"},
wantSub: "providers.x.models[1].headers.X-Key",
},
{
name: "models model-override header literal secret",
member: tarEntry{name: "models.yml", content: "providers:\n x:\n modelOverrides:\n m:\n headers:\n Authorization: \"Bearer sk-live-123\"\n"},
wantSub: "providers.x.modelOverrides.m.headers.Authorization",
},
{
name: "models model-override headers list",
member: tarEntry{name: "models.yml", content: "providers:\n x:\n modelOverrides:\n m:\n headers:\n - \"Bearer sk-live-123\"\n"},
wantSub: "providers.x.modelOverrides.m.headers",
},
{
name: "models model-override header shielded by non-string sibling",
member: tarEntry{name: "models.yml", content: "providers:\n x:\n modelOverrides:\n 0: junk\n m:\n headers:\n Authorization: \"Bearer sk-live-123\"\n"},
wantSub: "providers.x.modelOverrides.m.headers.Authorization",
},
{
name: "models model-definition header shielded by non-string sibling in the item",
member: tarEntry{name: "models.yml", content: "providers:\n x:\n models:\n - id: m\n 0: junk\n headers:\n Authorization: \"Bearer sk-live-123\"\n"},
wantSub: "providers.x.models[0].headers.Authorization",
},
// The SDK schema coerces a list at providers or modelOverrides into an
// index-keyed record, so a list there still reaches the registry.
{
name: "models modelOverrides list",
member: tarEntry{name: "models.yml", content: "providers:\n x:\n modelOverrides:\n - headers:\n Authorization: \"Bearer sk-live-123\"\n"},
wantSub: "providers.x.modelOverrides",
},
{
name: "models providers list",
member: tarEntry{name: "models.yml", content: "providers:\n - apiKey: sk-live-123\n"},
wantSub: "sets providers to a non-mapping",
},
})
}

// TestCredentialKeysMatchSchema guards the generated denylist against a
Expand Down
Loading