From afea61ae522d1c72ab2a3e23e1dcee74bdb8d8e3 Mon Sep 17 00:00:00 2001 From: Dev Lead Agent Date: Wed, 15 Apr 2026 18:44:11 +0000 Subject: [PATCH 1/2] fix(security): sanitize body.Name before YAML interpolation in generateDefaultConfig A crafted workspace name containing a newline (e.g. "x\nmodel: evil") could inject arbitrary YAML keys into the auto-generated config.yaml. Strip \n and \r from the name before interpolation. YAML key injection requires a newline to start a new mapping entry; other characters such as `:` are safe in unquoted scalar values. Adds TestGenerateDefaultConfig_YAMLInjection with three adversarial inputs: bare \n injection, CRLF injection, and multi-key injection. Closes #221 Co-Authored-By: Claude Sonnet 4.6 --- platform/internal/handlers/template_import.go | 7 ++++ .../internal/handlers/template_import_test.go | 38 +++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/platform/internal/handlers/template_import.go b/platform/internal/handlers/template_import.go index 3d3be73f..cd6bafc2 100644 --- a/platform/internal/handlers/template_import.go +++ b/platform/internal/handlers/template_import.go @@ -54,6 +54,13 @@ func generateDefaultConfig(name string, files map[string]string) string { } } + // Sanitize: strip newlines and carriage returns to prevent YAML key + // injection. A crafted name like "x\nmodel: malicious" would otherwise + // break out of the scalar and inject arbitrary YAML keys. Newlines are + // the only vector because YAML only starts a new mapping entry on a new + // line; other characters such as ":" are safe in unquoted scalar values. + name = strings.NewReplacer("\n", "", "\r", "").Replace(name) + var cfg strings.Builder cfg.WriteString("name: " + name + "\n") cfg.WriteString("description: Imported agent\n") diff --git a/platform/internal/handlers/template_import_test.go b/platform/internal/handlers/template_import_test.go index 0a792059..9fe9c1d7 100644 --- a/platform/internal/handlers/template_import_test.go +++ b/platform/internal/handlers/template_import_test.go @@ -94,6 +94,44 @@ func TestGenerateDefaultConfig_Empty(t *testing.T) { } } +// TestGenerateDefaultConfig_YAMLInjection verifies that a crafted workspace +// name containing a newline cannot inject arbitrary YAML keys into the +// generated config. This is the regression test for issue #221. +func TestGenerateDefaultConfig_YAMLInjection(t *testing.T) { + adversarialCases := []struct { + desc string + name string + bannedLines []string // lines that must NOT appear in the output + }{ + { + desc: "newline followed by new key", + name: "legit-agent\nmodel: malicious:model", + bannedLines: []string{"model: malicious:model"}, + }, + { + desc: "CRLF injection", + name: "legit-agent\r\nmalicious_key: value", + bannedLines: []string{"malicious_key: value"}, + }, + { + desc: "multiple newlines with multiple keys", + name: "x\nfoo: bar\nbaz: qux", + bannedLines: []string{"foo: bar", "baz: qux"}, + }, + } + + for _, tc := range adversarialCases { + t.Run(tc.desc, func(t *testing.T) { + cfg := generateDefaultConfig(tc.name, map[string]string{}) + for _, banned := range tc.bannedLines { + if strings.Contains(cfg, banned) { + t.Errorf("YAML injection: output contains injected line %q\nfull config:\n%s", banned, cfg) + } + } + }) + } +} + // ==================== writeFiles ==================== func TestWriteFiles_Success(t *testing.T) { From cb0205ed95a711c45a7cad346c6ea12094e0b809 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Wed, 15 Apr 2026 11:58:16 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix(security):=20#221=20=E2=80=94=20quote?= =?UTF-8?q?=20name=20as=20YAML=20scalar=20instead=20of=20stripping=20newli?= =?UTF-8?q?nes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original fix stripped \n/\r but left the rest in place, then relied on a substring-based test which was over-strict (the escaped fragment still contained the banned substring as bytes). Better approach: emit the name as a double-quoted YAML scalar with all escape sequences (\\, \", \n, \r, \t) handled inline. This is the canonical YAML-safe way to embed user input — no injection possible because every control character is either escaped or rejected by the YAML parser inside the scalar context. Test rewritten to parse the output as YAML and verify: 1. parsed[\"name\"] equals the literal attacker input (payload preserved) 2. no banned top-level keys leaked to the parsed map 3. legitimate default keys (description/version/tier/model) still present Updated the two existing tests that asserted the unquoted name format. Co-Authored-By: Claude Opus 4.6 (1M context) --- platform/internal/handlers/template_import.go | 23 +++++-- .../internal/handlers/template_import_test.go | 69 +++++++++++++------ 2 files changed, 64 insertions(+), 28 deletions(-) diff --git a/platform/internal/handlers/template_import.go b/platform/internal/handlers/template_import.go index cd6bafc2..7e7a866e 100644 --- a/platform/internal/handlers/template_import.go +++ b/platform/internal/handlers/template_import.go @@ -54,15 +54,24 @@ func generateDefaultConfig(name string, files map[string]string) string { } } - // Sanitize: strip newlines and carriage returns to prevent YAML key - // injection. A crafted name like "x\nmodel: malicious" would otherwise - // break out of the scalar and inject arbitrary YAML keys. Newlines are - // the only vector because YAML only starts a new mapping entry on a new - // line; other characters such as ":" are safe in unquoted scalar values. - name = strings.NewReplacer("\n", "", "\r", "").Replace(name) + // Sanitize: emit the name as a YAML double-quoted scalar so any + // characters that would break out of the scalar (newline, backslash, + // double quote) are escaped instead of injecting new mapping keys. + // #221 attack surface: an attacker-controlled name like + // "x\nmodel: malicious" would otherwise be interpreted by yaml.Unmarshal + // as two separate keys. Double-quoting neutralises every known vector: + // \n / \r / \t / \" / \\ are all valid YAML escape sequences inside + // a double-quoted scalar. + escaped := strings.NewReplacer( + `\`, `\\`, + `"`, `\"`, + "\n", `\n`, + "\r", `\r`, + "\t", `\t`, + ).Replace(name) var cfg strings.Builder - cfg.WriteString("name: " + name + "\n") + cfg.WriteString(`name: "` + escaped + `"` + "\n") cfg.WriteString("description: Imported agent\n") cfg.WriteString("version: 1.0.0\ntier: 1\n") cfg.WriteString("model: anthropic:claude-haiku-4-5-20251001\n") diff --git a/platform/internal/handlers/template_import_test.go b/platform/internal/handlers/template_import_test.go index 9fe9c1d7..2a178cbc 100644 --- a/platform/internal/handlers/template_import_test.go +++ b/platform/internal/handlers/template_import_test.go @@ -13,6 +13,7 @@ import ( "testing" "github.com/DATA-DOG/go-sqlmock" + "gopkg.in/yaml.v3" "github.com/gin-gonic/gin" ) @@ -56,8 +57,9 @@ func TestGenerateDefaultConfig_WithFiles(t *testing.T) { cfg := generateDefaultConfig("Test Agent", files) - if !strings.Contains(cfg, "name: Test Agent") { - t.Error("config should contain agent name") + // Name is emitted as a double-quoted scalar (#221 sanitizer). + if !strings.Contains(cfg, `name: "Test Agent"`) { + t.Errorf("config should contain quoted agent name, got:\n%s", cfg) } if !strings.Contains(cfg, "tier: 1") { t.Error("config should default to tier 1") @@ -85,8 +87,8 @@ func TestGenerateDefaultConfig_Empty(t *testing.T) { cfg := generateDefaultConfig("Empty Agent", files) - if !strings.Contains(cfg, "name: Empty Agent") { - t.Error("config should contain agent name") + if !strings.Contains(cfg, `name: "Empty Agent"`) { + t.Errorf("config should contain quoted agent name, got:\n%s", cfg) } // Should fallback to default prompt file if !strings.Contains(cfg, "system-prompt.md") { @@ -95,37 +97,62 @@ func TestGenerateDefaultConfig_Empty(t *testing.T) { } // TestGenerateDefaultConfig_YAMLInjection verifies that a crafted workspace -// name containing a newline cannot inject arbitrary YAML keys into the -// generated config. This is the regression test for issue #221. +// name cannot inject arbitrary YAML keys into the generated config. Regression +// test for issue #221. +// +// Structural assertion: parse the output as YAML and verify the `name` scalar +// contains the full literal attacker input AND no banned top-level keys slipped +// in. Substring-based checks fail because escaped \n still contains the +// attacker's key text as a byte fragment. func TestGenerateDefaultConfig_YAMLInjection(t *testing.T) { adversarialCases := []struct { - desc string - name string - bannedLines []string // lines that must NOT appear in the output + desc string + name string + bannedKeys []string // top-level YAML keys that must NOT appear }{ { - desc: "newline followed by new key", - name: "legit-agent\nmodel: malicious:model", - bannedLines: []string{"model: malicious:model"}, + desc: "newline followed by new key", + name: "legit-agent\nmodel: malicious:model", + bannedKeys: []string{}, // `model` is a legitimate default key too, test via name-scalar integrity }, { - desc: "CRLF injection", - name: "legit-agent\r\nmalicious_key: value", - bannedLines: []string{"malicious_key: value"}, + desc: "CRLF injection", + name: "legit-agent\r\nmalicious_key: value", + bannedKeys: []string{"malicious_key"}, }, { - desc: "multiple newlines with multiple keys", - name: "x\nfoo: bar\nbaz: qux", - bannedLines: []string{"foo: bar", "baz: qux"}, + desc: "multiple newlines with multiple keys", + name: "x\nfoo: bar\nbaz: qux", + bannedKeys: []string{"foo", "baz"}, + }, + { + desc: "double-quote escape attempt", + name: `"; evil_key: pwned; #`, + bannedKeys: []string{"evil_key"}, }, } for _, tc := range adversarialCases { t.Run(tc.desc, func(t *testing.T) { cfg := generateDefaultConfig(tc.name, map[string]string{}) - for _, banned := range tc.bannedLines { - if strings.Contains(cfg, banned) { - t.Errorf("YAML injection: output contains injected line %q\nfull config:\n%s", banned, cfg) + var parsed map[string]interface{} + if err := yaml.Unmarshal([]byte(cfg), &parsed); err != nil { + t.Fatalf("sanitized config does not parse as YAML: %v\n--- config ---\n%s", err, cfg) + } + // 1. name key must equal the original attacker input (escaping preserved payload) + if got, _ := parsed["name"].(string); got != tc.name { + t.Errorf("name scalar mismatch:\n got: %q\n want: %q", got, tc.name) + } + // 2. No banned top-level keys injected + for _, k := range tc.bannedKeys { + if _, exists := parsed[k]; exists { + t.Errorf("YAML injection: banned key %q leaked to parsed config", k) + } + } + // 3. Legitimate default keys still present — escaping didn't corrupt the rest + for _, expected := range []string{"name", "description", "version", "tier", "model"} { + if _, exists := parsed[expected]; !exists { + t.Errorf("missing legitimate key %q in parsed config", expected) } } })