From bf27324cf4e093c601424f1e76bb3da7fb9a9fab Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 11 Oct 2026 23:11:08 -0400 Subject: [PATCH] fix(store): check model-level headers at the models.yml door (RIG-5287) rejectCredentialModels checked providers..headers only. The SDK also reads providers..models[].headers and providers..modelOverrides..headers, merges them onto the provider's headers, and resolves each value through resolveConfigValue. A literal secret at either site passed the door. The header rule moves into rejectCredentialHeaders, and all three sites call it, with the offending path named (models[i] or modelOverrides.). The design record gains a dated amendment naming the extra sites. Co-authored-by: Matt Wilkinson --- .../design.md | 13 ++ go/internal/store/agent_config.go | 71 ++++++++--- go/internal/store/agent_config_test.go | 117 +++++++++++++----- 3 files changed, 153 insertions(+), 48 deletions(-) diff --git a/docs/designs/agent/compass-agent-config-passthrough/design.md b/docs/designs/agent/compass-agent-config-passthrough/design.md index 7e99c518d..b7ecf100d 100644 --- a/docs/designs/agent/compass-agent-config-passthrough/design.md +++ b/docs/designs/agent/compass-agent-config-passthrough/design.md @@ -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..headers.*`. The installed SDK +> (`@oh-my-pi/pi-coding-agent@18.0.11`) also reads +> `providers..models[].headers` (`mergeCustomModelHeaders`, +> `custom-models.ts`) and `providers..modelOverrides..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 diff --git a/go/internal/store/agent_config.go b/go/internal/store/agent_config.go index d38c40f2a..f4285bfb0 100644 --- a/go/internal/store/agent_config.go +++ b/go/internal/store/agent_config.go @@ -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 { @@ -976,40 +976,71 @@ func yamlMapEntries(node any) []yamlMapEntry { // provider surface (CP-4): providers..apiKey, or providers..headers. 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. diff --git a/go/internal/store/agent_config_test.go b/go/internal/store/agent_config_test.go index 1ff4c12d3..e009ebc11 100644 --- a/go/internal/store/agent_config_test.go +++ b/go/internal/store/agent_config_test.go @@ -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. @@ -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..apiKey, and a models.yml with a -// providers..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..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"}, @@ -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. { @@ -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