Skip to content

Commit 630383c

Browse files
committed
fix(config): a PUT could set keys the schema marks read-only
A config patch carries an untyped half for keys without a dedicated field, and applyExtra passed it straight to the low-level setter with no patchability check. Three surfaces then disagreed about the same key: `slmcode config set` refused it, the Studio's settings form never offered it, and a PUT /api/config carrying it in the untyped half set it anyway. Three keys were reachable. mcp_servers is the one that matters: MCP servers are external processes whose tools agents can call, so a request able to register one is tool-execution surface — and the Studio panel that lists them says "configured in config.yaml", because file-only is exactly what the read-only flag was declaring. Mutating the check away makes the test print the registered attacker server, which is the shape of the thing. skills_dirs loads prompt content into agents; context_role_budget is benign. The check belongs on the patch path, not on Config.Set: that is the setter env-var and config-file loading both go through, and a read-only key still has to be loadable from the file that declares it. The flag is a statement about remote editing, so that is where it now holds — verified through the real HTTP endpoint as well as the patch type. Found by walking every schema key through Set/Get and ApplyPatch. Nothing had checked that the schema and the code agreed about which keys are writable, and the same sweep pins that every settable key actually round-trips — a key the settings form offers but which does nothing is the other half of this bug. Also pins the credential file permissions. authstore.Save had no test at all, and the permissions on auth.json (0600) and the directory above it (0750) are the whole protection on a shared machine. The transient temp file matters as much: atomicfile writes to a temp in the same directory and renames, so a temp created world-readable would expose the key for the length of one write however tight the final mode is. os.CreateTemp opens at 0600 and the Chmod that follows only widens, which is the safe order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UiLDxnEu1Gf8Q5hWAFhaAd
1 parent 1860d40 commit 630383c

5 files changed

Lines changed: 344 additions & 0 deletions

File tree

‎docs/changelog.md‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,29 @@ A defect that comes back is one defect with another attempt, folded by the same
295295
rule the Fixes tab uses — the summary and the panel must never disagree about
296296
how many things went wrong.
297297

298+
### Fixed — `PUT /api/config` could set keys the schema marks read-only
299+
300+
A config patch carries an untyped half for keys without a dedicated field, and
301+
that half went straight to the low-level setter with no patchability check. So
302+
three surfaces disagreed about the same key: `slmcode config set` refused it,
303+
the Studio's settings form never offered it, and a `PUT /api/config` carrying it
304+
set it anyway.
305+
306+
Three keys were reachable that way. `mcp_servers` is the one that matters: MCP
307+
servers are external processes whose tools agents can call, so a request able to
308+
register one is tool-execution surface — and the Studio panel listing them says
309+
*"configured in config.yaml"*, because file-only is exactly what the read-only
310+
flag was declaring. `skills_dirs` loads prompt content into agents;
311+
`context_role_budget` is benign.
312+
313+
The check goes on the patch path rather than on `Config.Set`, which must keep
314+
accepting these — it is the setter env-var and config-file loading both use, and
315+
a read-only key still has to be loadable from the file that declares it. The
316+
flag is a statement about *remote* editing, so that is where it now holds.
317+
318+
Found by walking every schema key through `Set`/`Get` and `ApplyPatch`. Nothing
319+
had checked that the schema and the code agreed about which keys are writable.
320+
298321
### Fixed — a language-specialised tester was given the wrong finish contract
299322

300323
Per-task routing puts `go-tester` / `python-tester` on a verification task

‎pkg/authstore/perms_test.go‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
package authstore
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"runtime"
7+
"strings"
8+
"testing"
9+
)
10+
11+
// ── Credentials on disk ──────────────────────────────────────────────────
12+
//
13+
// auth.json holds provider API keys. The permissions on it and on the directory
14+
// above it are the whole protection on a shared machine, and nothing pinned
15+
// them — Save had no test at all.
16+
//
17+
// The transient temp file matters as much as the final one: atomicfile writes
18+
// to a temp in the same directory and renames, so a temp created world-readable
19+
// would expose the key for the length of one write however tight the final mode
20+
// is. os.CreateTemp opens at 0600 and the Chmod that follows only ever widens
21+
// from there, which is the safe order — this pins it.
22+
23+
func TestSavedCredentialsAreOwnerOnly(t *testing.T) {
24+
if runtime.GOOS == "windows" {
25+
t.Skip("POSIX permission bits")
26+
}
27+
slm := filepath.Join(t.TempDir(), ".slmcode")
28+
29+
if err := Set(slm, "openai", "sk-test-secret"); err != nil {
30+
t.Fatalf("Set: %v", err)
31+
}
32+
33+
fi, err := os.Stat(Path(slm))
34+
if err != nil {
35+
t.Fatalf("stat auth.json: %v", err)
36+
}
37+
if got := fi.Mode().Perm(); got != 0o600 {
38+
t.Errorf("auth.json mode = %04o, want 0600 — the key is readable by other accounts", got)
39+
}
40+
41+
di, err := os.Stat(slm)
42+
if err != nil {
43+
t.Fatalf("stat .slmcode: %v", err)
44+
}
45+
if got := di.Mode().Perm(); got&0o007 != 0 {
46+
t.Errorf(".slmcode mode = %04o, want no world bits — the directory holds auth.json", got)
47+
}
48+
}
49+
50+
// Save is the exported entry point and had no coverage; a round trip proves it
51+
// both writes and is readable back.
52+
func TestSaveRoundTrips(t *testing.T) {
53+
slm := filepath.Join(t.TempDir(), ".slmcode")
54+
if err := Save(slm, &Store{Keys: map[string]string{"openai": "sk-a", "groq": "gsk-b"}}); err != nil {
55+
t.Fatalf("Save: %v", err)
56+
}
57+
for provider, want := range map[string]string{"openai": "sk-a", "groq": "gsk-b"} {
58+
if got, ok := Get(slm, provider); !ok || got != want {
59+
t.Errorf("Get(%q) = %q/%v, want %q", provider, got, ok, want)
60+
}
61+
}
62+
if _, ok := Get(slm, "anthropic"); ok {
63+
t.Error("Get invented a key that was never stored")
64+
}
65+
}
66+
67+
// A nil store must produce a valid empty file rather than a panic or a file
68+
// containing "null", which Load would then have to special-case.
69+
func TestSavingNothingProducesAValidEmptyStore(t *testing.T) {
70+
slm := filepath.Join(t.TempDir(), ".slmcode")
71+
if err := Save(slm, nil); err != nil {
72+
t.Fatalf("Save(nil): %v", err)
73+
}
74+
b, err := os.ReadFile(Path(slm))
75+
if err != nil {
76+
t.Fatal(err)
77+
}
78+
if strings.Contains(string(b), "null") {
79+
t.Errorf("Save(nil) wrote a null store: %s", b)
80+
}
81+
s, err := Load(slm)
82+
if err != nil || s == nil || s.Keys == nil {
83+
t.Errorf("Load after Save(nil) = %+v, %v", s, err)
84+
}
85+
}
86+
87+
// A key must never end up in an error message or anywhere else that gets
88+
// logged — the store is the only place it belongs.
89+
func TestErrorsNeverCarryTheKey(t *testing.T) {
90+
// An unwritable directory is the realistic failure.
91+
base := t.TempDir()
92+
blocked := filepath.Join(base, "blocked")
93+
if err := os.WriteFile(blocked, []byte("not a directory"), 0o600); err != nil {
94+
t.Fatal(err)
95+
}
96+
err := Set(filepath.Join(blocked, ".slmcode"), "openai", "sk-super-secret-value")
97+
if err == nil {
98+
t.Skip("the platform allowed the write; nothing to assert")
99+
}
100+
if strings.Contains(err.Error(), "sk-super-secret-value") {
101+
t.Errorf("the API key leaked into an error message: %v", err)
102+
}
103+
}

‎pkg/config/patch.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,20 @@ func (p Patch) WithValues(values map[string]any) Patch {
103103
// needs to report a bad value (that is what `slmcode config set` does).
104104
func (c *Config) applyExtra(p Patch) {
105105
for _, k := range sortedKeys(p.extra) {
106+
// The schema's read-only flag is a statement about REMOTE editing, and
107+
// this is the remote-editing path. Without the check, three surfaces
108+
// disagreed about the same key: `slmcode config set` refused it, the
109+
// Studio's form never offered it, and a PUT /api/config carrying it in
110+
// the untyped half set it anyway.
111+
//
112+
// That mattered most for `mcp_servers`. MCP servers are external
113+
// processes whose tools agents can call, so a request that could
114+
// register one is tool-execution surface — and the Studio panel that
115+
// shows them says "configured in config.yaml", because file-only is
116+
// exactly what the flag was declaring.
117+
if _, patchable := PatchableField(k); !patchable {
118+
continue
119+
}
106120
if err := c.Set(k, p.extra[k]); err != nil {
107121
continue
108122
}
Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
package config
2+
3+
import (
4+
"encoding/json"
5+
"fmt"
6+
"strings"
7+
"testing"
8+
)
9+
10+
// ── Every schema key must actually be settable ───────────────────────────
11+
//
12+
// The schema is what `slmcode config set`, `config show --all`, the Studio's
13+
// settings form and the config docs are all generated from. A key that appears
14+
// there but does not round-trip through Set/Get is a setting the user is
15+
// offered, types a value into, and which then does nothing — with no error to
16+
// tell them so.
17+
//
18+
// Nothing checked that the two agreed. This walks every patchable key and
19+
// proves it.
20+
21+
// sampleFor returns a value the schema itself says is valid for a field.
22+
func sampleFor(f FieldSchema) (any, bool) {
23+
if len(f.Enum) > 0 {
24+
for _, v := range f.Enum {
25+
if strings.TrimSpace(v) != "" {
26+
return v, true
27+
}
28+
}
29+
return nil, false
30+
}
31+
switch f.Type {
32+
case "string":
33+
return "round-trip-probe", true
34+
case "int":
35+
return 7, true
36+
case "float":
37+
return 0.25, true
38+
case "bool":
39+
return true, true
40+
case "duration":
41+
return "90s", true
42+
case "string[]":
43+
return []string{"alpha", "beta"}, true
44+
default:
45+
// map / list shapes have no single safe sample; the typed patch tests
46+
// cover those individually.
47+
return nil, false
48+
}
49+
}
50+
51+
func TestEverySettableSchemaKeyRoundTrips(t *testing.T) {
52+
for _, f := range Schema() {
53+
if _, patchable := PatchableField(f.Key); !patchable {
54+
continue
55+
}
56+
sample, ok := sampleFor(f)
57+
if !ok {
58+
continue
59+
}
60+
t.Run(f.Key, func(t *testing.T) {
61+
c := Default(t.TempDir())
62+
if err := c.Set(f.Key, sample); err != nil {
63+
t.Fatalf("Set(%q, %v) on a %s field: %v", f.Key, sample, f.Type, err)
64+
}
65+
got, ok := c.Get(f.Key)
66+
if !ok {
67+
t.Fatalf("Get(%q) reported the key does not exist, but Set accepted it", f.Key)
68+
}
69+
// Compared as rendered text: Set may normalize (a duration, a
70+
// lower-cased enum), and the contract is that the value STUCK, not
71+
// that it survived byte-for-byte.
72+
if strings.TrimSpace(render(got)) == "" && strings.TrimSpace(render(sample)) != "" {
73+
t.Errorf("Set(%q, %v) was accepted but Get returned empty — the key does nothing",
74+
f.Key, sample)
75+
}
76+
})
77+
}
78+
}
79+
80+
// A key the schema marks read-only must not be settable through a PATCH.
81+
//
82+
// Config.Set itself must keep accepting them — it is the low-level setter that
83+
// env-var and config-file loading both go through, and a read-only key still
84+
// has to be loadable from the file that declares it. The flag is a statement
85+
// about REMOTE editing, so the place it has to hold is the patch path.
86+
//
87+
// It did not. `slmcode config set` refused these keys and the Studio's form
88+
// never offered them, but a PUT /api/config carrying one in the untyped half of
89+
// the patch set it anyway — three surfaces disagreeing about the same key.
90+
func TestAPatchCannotSetAReadOnlyKey(t *testing.T) {
91+
for _, f := range Schema() {
92+
if _, patchable := PatchableField(f.Key); patchable {
93+
continue
94+
}
95+
sample, ok := sampleFor(f)
96+
if !ok {
97+
continue
98+
}
99+
t.Run(f.Key, func(t *testing.T) {
100+
c := Default(t.TempDir())
101+
before, _ := c.Get(f.Key)
102+
103+
body, err := json.Marshal(map[string]any{f.Key: sample})
104+
if err != nil {
105+
t.Fatal(err)
106+
}
107+
var p Patch
108+
if err := json.Unmarshal(body, &p); err != nil {
109+
t.Fatalf("decode patch: %v", err)
110+
}
111+
c.ApplyPatch(p)
112+
113+
if after, _ := c.Get(f.Key); render(before) != render(after) {
114+
t.Errorf("a patch set the read-only key %q (%v → %v)", f.Key, before, after)
115+
}
116+
})
117+
}
118+
}
119+
120+
// The most consequential of them, stated on its own so the reason survives.
121+
//
122+
// MCP servers are external processes whose tools agents can call, so a request
123+
// that could register one is tool-execution surface. The Studio panel that
124+
// lists them says "configured in config.yaml" — file-only is exactly what the
125+
// read-only flag was declaring.
126+
func TestAPatchCannotRegisterAnMCPServer(t *testing.T) {
127+
c := Default(t.TempDir())
128+
var p Patch
129+
body := `{"mcp_servers":[{"name":"attacker","command":"/bin/sh","args":["-c","id"]}]}`
130+
if err := json.Unmarshal([]byte(body), &p); err != nil {
131+
t.Fatalf("decode patch: %v", err)
132+
}
133+
c.ApplyPatch(p)
134+
if len(c.MCPServers) != 0 {
135+
t.Errorf("a patch registered %d MCP server(s): %+v", len(c.MCPServers), c.MCPServers)
136+
}
137+
}
138+
139+
// A patchable key must still go through, or the check would have turned the
140+
// settings form read-only.
141+
func TestAPatchStillSetsTheKeysItShould(t *testing.T) {
142+
c := Default(t.TempDir())
143+
var p Patch
144+
if err := json.Unmarshal([]byte(`{"max_parallel":5}`), &p); err != nil {
145+
t.Fatal(err)
146+
}
147+
c.ApplyPatch(p)
148+
if c.MaxParallel != 5 {
149+
t.Errorf("max_parallel = %d, want the patch applied", c.MaxParallel)
150+
}
151+
}
152+
153+
// Every schema key must resolve through Field(), including via any alias —
154+
// `config set` looks the key up there before it looks anywhere else.
155+
func TestEverySchemaKeyResolves(t *testing.T) {
156+
for _, f := range Schema() {
157+
if _, ok := Field(f.Key); !ok {
158+
t.Errorf("Schema lists %q but Field() cannot resolve it", f.Key)
159+
}
160+
if got := CanonicalKey(f.Key); got != f.Key {
161+
t.Errorf("CanonicalKey(%q) = %q — the schema lists a non-canonical key", f.Key, got)
162+
}
163+
}
164+
}
165+
166+
func render(v any) string {
167+
if v == nil {
168+
return ""
169+
}
170+
return strings.TrimSpace(fmt.Sprintf("%v", v))
171+
}

‎pkg/server/configure_test.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,3 +147,36 @@ func TestAnExplicitEndpointIsNotSecondGuessed(t *testing.T) {
147147
t.Errorf("the scan reached an endpoint nobody named: %s", rec.Body.String())
148148
}
149149
}
150+
151+
// PUT /api/config carries an untyped half, and a key the schema marks read-only
152+
// used to travel in it — so a request could register an MCP server, which is an
153+
// external process whose tools agents can call. The Studio panel that lists
154+
// them says "configured in config.yaml", because file-only is what the flag was
155+
// declaring.
156+
func TestPutConfigCannotRegisterAnMCPServer(t *testing.T) {
157+
s, _ := configureServer(t)
158+
body := `{"mcp_servers":[{"name":"attacker","command":"/bin/sh","args":["-c","id"]}]}`
159+
160+
rec := httptest.NewRecorder()
161+
s.Handler().ServeHTTP(rec, newAPIRequest(http.MethodPut, "/api/config", strings.NewReader(body)))
162+
if rec.Code >= 500 {
163+
t.Fatalf("status=%d body=%s", rec.Code, rec.Body.String())
164+
}
165+
if got := len(s.cfg().MCPServers); got != 0 {
166+
t.Errorf("a config PUT registered %d MCP server(s): %+v", got, s.cfg().MCPServers)
167+
}
168+
}
169+
170+
// The same request must still apply the keys it is allowed to.
171+
func TestPutConfigStillAppliesPatchableKeys(t *testing.T) {
172+
s, _ := configureServer(t)
173+
rec := httptest.NewRecorder()
174+
s.Handler().ServeHTTP(rec, newAPIRequest(http.MethodPut, "/api/config",
175+
strings.NewReader(`{"max_parallel":5}`)))
176+
if rec.Code != http.StatusOK {
177+
t.Fatalf("status=%d body=%s", rec.Code, rec.Body.String())
178+
}
179+
if got := s.cfg().MaxParallel; got != 5 {
180+
t.Errorf("max_parallel = %d, want the patch applied", got)
181+
}
182+
}

0 commit comments

Comments
 (0)