diff --git a/workspace-server/internal/provisioner/cp_provisioner.go b/workspace-server/internal/provisioner/cp_provisioner.go index ca87f659c..25cce29b0 100644 --- a/workspace-server/internal/provisioner/cp_provisioner.go +++ b/workspace-server/internal/provisioner/cp_provisioner.go @@ -185,7 +185,26 @@ type cpProvisionRequest struct { // EC2 instance's /configs directory. OFFSEC-010: collected by // collectCPConfigFiles which rejects symlinks and non-regular files // before including them. Serialised as base64 to avoid JSON escaping. + // + // ConfigFiles is the SM-bound bundle (the CP stages it through AWS + // Secrets Manager as molecule/workspace//config). It's the right + // transport for SMALL non-secret config text only — it has a 256 KiB + // cap and the SM transport is sized + scoped for *secrets*. See the + // core-devops 10:13 SM-inventory RCA. ConfigFiles map[string]string `json:"config_files,omitempty"` + + // TemplateAssets (RFC #2843 #24) are non-secret template assets + // (config.yaml + prompts/ + agent-skills/) fetched from a + // non-secret asset channel (template repo / Gitea shallow clone per + // RFC §4.2 transport option (a)). They travel on a SEPARATE wire + // field from ConfigFiles (the SM-bound bundle) so a future CP can + // route them through a non-secret transport without going through + // the SM cap. Serialised as base64 to avoid JSON escaping. + // + // Keys are restricted to the template-asset allowlist enforced by + // IsCPTemplateAssetPath (config.yaml / prompts/* / agent-skills/*); + // see collectCPConfigFiles for the enforcement. + TemplateAssets map[string][]byte `json:"template_assets,omitempty"` } type cpProvisionResponse struct { @@ -257,7 +276,7 @@ func (p *CPProvisioner) Start(ctx context.Context, cfg WorkspaceConfig) (string, // - Caps total size at cpConfigFilesMaxBytes (a transport-DoS guard, // not the retired 12 KiB user-data ceiling — config now ships off // user-data via the CP's Secrets-Manager seeding path) - configFiles, err := collectCPConfigFiles(cfg) + configFiles, templateAssets, err := collectCPConfigFiles(cfg) if err != nil { return "", fmt.Errorf("cp provisioner: collect config files: %w", err) } @@ -285,6 +304,7 @@ func (p *CPProvisioner) Start(ctx context.Context, cfg WorkspaceConfig) (string, PlatformURL: cfg.PlatformURL, Env: env, ConfigFiles: configFiles, + TemplateAssets: templateAssets, } body, err := json.Marshal(req) @@ -365,6 +385,18 @@ func (p *CPProvisioner) Start(ctx context.Context, cfg WorkspaceConfig) (string, // growth (more schedules, longer prompts, more skills) never re-hits a wall. const cpConfigFilesMaxBytes = 256 << 10 +// cpTemplateAssetsMaxBytes bounds the aggregate TEMPLATE-ASSET payload +// (config.yaml + prompts/* + agent-skills/*) delivered via the generic +// non-secret asset channel (RFC #2843 #24). This is a SEPARATE, much larger +// bound than cpConfigFilesMaxBytes: the config bundle is capped at 256 KiB +// because it rides the Secrets-Manager/user-data transport, but template +// ASSETS ride a non-secret channel with no SM size limit — so reusing the +// 256 KiB cap here would re-create the original #2831 skill-drop failure (the +// seo-all skill package alone is ~716 KiB). 16 MiB comfortably fits real skill +// trees + prompts + config while still bounding a runaway/malicious fetcher +// (pure transport-DoS guard, not a secrets-transport limit). +const cpTemplateAssetsMaxBytes = 16 << 20 + // isCPTemplateConfigFile restricts which files from a template directory are // eligible for transport to the control plane. Only config.yaml (the runtime // entrypoint config) and files under prompts/ (system prompts) are needed; @@ -375,9 +407,11 @@ func isCPTemplateConfigFile(name string) bool { return name == "config.yaml" || strings.HasPrefix(name, "prompts/") } -func collectCPConfigFiles(cfg WorkspaceConfig) (map[string]string, error) { +func collectCPConfigFiles(cfg WorkspaceConfig) (map[string]string, map[string][]byte, error) { files := make(map[string]string) + assets := make(map[string][]byte) total := 0 + totalAssets := 0 addFile := func(name string, data []byte) error { name = filepath.ToSlash(filepath.Clean(name)) if name == "." || strings.HasPrefix(name, "../") || strings.HasPrefix(name, "/") || strings.Contains(name, "/../") { @@ -390,6 +424,30 @@ func collectCPConfigFiles(cfg WorkspaceConfig) (map[string]string, error) { files[name] = base64.StdEncoding.EncodeToString(data) return nil } + addAsset := func(name string, data []byte) error { + name = filepath.ToSlash(filepath.Clean(name)) + if name == "." || strings.HasPrefix(name, "../") || strings.HasPrefix(name, "/") || strings.Contains(name, "/../") { + return fmt.Errorf("invalid template asset path %q", name) + } + // Blast-radius guard (RC #11690): a fetcher that returns + // paths outside the template-asset namespace is either + // a programming error or an attack. Either way, fail closed. + // Specifically excluded: MEMORY.md, USER.md, CLAUDE.md + // (curated durable memory — agent-owned state, reconciled by + // the boot entrypoint, NOT by this collect path), .claude/sessions/ + // (Claude Code session dir, agent-owned), and any other + // agent-state path. The PM-flagged in #24_clarify invariant + // is enforced here in code, not in comments. + if !IsCPTemplateAssetPath(name) { + return fmt.Errorf("template asset path %q rejected: not in template-asset allowlist (config.yaml / prompts/* / agent-skills/*) — see IsCPTemplateAssetPath", name) + } + totalAssets += len(data) + if totalAssets > cpTemplateAssetsMaxBytes { + return fmt.Errorf("template assets exceed %d bytes", cpTemplateAssetsMaxBytes) + } + assets[name] = append([]byte(nil), data...) + return nil + } if cfg.TemplatePath != "" { // Reject symlinks on the root itself — WalkDir follows symlinks, @@ -397,10 +455,10 @@ func collectCPConfigFiles(cfg WorkspaceConfig) (map[string]string, error) { // would bypass the subsequent path-relativization checks below. rootInfo, err := os.Lstat(cfg.TemplatePath) if err != nil { - return nil, fmt.Errorf("collectCPConfigFiles: lstat template path: %w", err) + return nil, nil, fmt.Errorf("collectCPConfigFiles: lstat template path: %w", err) } if rootInfo.Mode()&os.ModeSymlink != 0 { - return nil, fmt.Errorf("collectCPConfigFiles: template path must not be a symlink") + return nil, nil, fmt.Errorf("collectCPConfigFiles: template path must not be a symlink") } err = filepath.WalkDir(cfg.TemplatePath, func(path string, d os.DirEntry, walkErr error) error { if walkErr != nil { @@ -438,18 +496,56 @@ func collectCPConfigFiles(cfg WorkspaceConfig) (map[string]string, error) { return addFile(rel, data) }) if err != nil { - return nil, err + return nil, nil, err } } for name, data := range cfg.ConfigFiles { if err := addFile(name, data); err != nil { - return nil, err + return nil, nil, err } } - if len(files) == 0 { - return nil, nil + + // RFC #2843 #24 — generic template-asset channel. When + // cfg.TemplateAssetFetcher is wired (SaaS) and + // cfg.TemplateIdentity is set, fetch the template's + // config.yaml + prompts/ + agent_skills/ via the + // non-secret asset channel (template repo, Gitea shallow + // clone per §4.2 transport option (a)) and land them in + // the SEPARATE TemplateAssets field of the provision + // request — NOT in ConfigFiles (the SM-bound bundle). + // The split is load-bearing: ConfigFiles is the bundle + // the CP stages through AWS Secrets Manager, which is + // sized + scoped for *secrets* and caps at 256 KiB. The + // 716 KiB SEO skill package can't ride SM and shouldn't + // try to (core-devops 10:13 SM-inventory RCA). + // + // The fetch is OPT-IN: nil fetcher = no-op (self-host + // default; falls through to the local TemplatePath path + // above). Every key in the fetcher's output is gated by + // IsCPTemplateAssetPath at the addAsset boundary above — + // paths outside the template-asset namespace abort the + // provision rather than silently sneaking MEMORY.md / + // CLAUDE.md / .claude/sessions/ into the bundle. + // + // The fetch is fail-closed: a transport error aborts + // the provision rather than regressing to stub-mode + // /configs (the same contract as the persisted-bundle + // provider in #2831 PIECE 1). + if cfg.TemplateAssetFetcher != nil && cfg.TemplateIdentity != "" { + fetchedAssets, fetchErr := cfg.TemplateAssetFetcher.Load(context.Background(), cfg.TemplateIdentity) + if fetchErr != nil { + return nil, nil, fmt.Errorf("collectCPConfigFiles: fetch template assets (RFC #2843 #24): %w", fetchErr) + } + for name, data := range fetchedAssets { + if err := addAsset(name, data); err != nil { + return nil, nil, err + } + } } - return files, nil + if len(files) == 0 && len(assets) == 0 { + return nil, nil, nil + } + return files, assets, nil } // Stop terminates the workspace's EC2 instance via the control plane. diff --git a/workspace-server/internal/provisioner/cp_provisioner_config_size_test.go b/workspace-server/internal/provisioner/cp_provisioner_config_size_test.go index e52bf8a44..112cb2d11 100644 --- a/workspace-server/internal/provisioner/cp_provisioner_config_size_test.go +++ b/workspace-server/internal/provisioner/cp_provisioner_config_size_test.go @@ -103,7 +103,7 @@ func TestCollectCPConfigFiles_DoSGuardStillBounds(t *testing.T) { for i := range huge { huge[i] = 'a' } - _, err := collectCPConfigFiles(WorkspaceConfig{ + _, _, err := collectCPConfigFiles(WorkspaceConfig{ ConfigFiles: map[string][]byte{"config.yaml": huge}, }) if err == nil { @@ -128,7 +128,7 @@ func TestCollectCPConfigFiles_AcceptsSEOSizedBundle(t *testing.T) { for i := range promptBlob { promptBlob[i] = 'p' } - files, err := collectCPConfigFiles(WorkspaceConfig{ + files, _, err := collectCPConfigFiles(WorkspaceConfig{ ConfigFiles: map[string][]byte{ "config.yaml": cfgBlob, "prompts/system.md": promptBlob, @@ -145,7 +145,7 @@ func TestCollectCPConfigFiles_AcceptsSEOSizedBundle(t *testing.T) { if err := os.WriteFile(filepath.Join(tmpl, "config.yaml"), cfgBlob, 0o600); err != nil { t.Fatal(err) } - if _, err := collectCPConfigFiles(WorkspaceConfig{TemplatePath: tmpl}); err != nil { + if _, _, err := collectCPConfigFiles(WorkspaceConfig{TemplatePath: tmpl}); err != nil { t.Fatalf("collectCPConfigFiles rejected an SEO-sized template config.yaml: %v", err) } } diff --git a/workspace-server/internal/provisioner/cp_provisioner_test.go b/workspace-server/internal/provisioner/cp_provisioner_test.go index acb4146b5..20169f0ca 100644 --- a/workspace-server/internal/provisioner/cp_provisioner_test.go +++ b/workspace-server/internal/provisioner/cp_provisioner_test.go @@ -437,6 +437,107 @@ func TestStart_CollectsConfigFiles(t *testing.T) { } } +// TestStart_SendsTemplateAssetsOnSeparateField — the load-bearing +// wire-shape test (RFC #2843 #24, Reviewer-CR2 addendum). When +// cfg.TemplateAssetFetcher is wired, fetched assets travel on the +// SEPARATE TemplateAssets field, NOT merged into ConfigFiles. +// This split lets a future CP route non-secret assets through a +// non-SM transport without going through the 256 KiB SM cap +// (the core-devops 10:13 SM-inventory RCA). The test exercises +// the Start → cpProvisionRequest path end-to-end so a future +// refactor that re-merges the two transports would be caught. +func TestStart_SendsTemplateAssetsOnSeparateField(t *testing.T) { + prov := &fakeTemplateAssetFetcher{ + bundle: map[string][]byte{ + "config.yaml": []byte("# from template repo"), + "prompts/system.md": []byte("# template system prompt"), + "agent-skills/seo-audit/SKILL.md": []byte("# 716 KiB SEO skill goes here"), + }, + } + + var gotBody cpProvisionRequest + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewDecoder(r.Body).Decode(&gotBody) + w.WriteHeader(http.StatusCreated) + _, _ = io.WriteString(w, `{"instance_id":"i-abc123","state":"pending"}`) + })) + defer srv.Close() + + p := &CPProvisioner{baseURL: srv.URL, orgID: "org-1", httpClient: srv.Client()} + _, err := p.Start(context.Background(), WorkspaceConfig{ + WorkspaceID: "ws-1", + Runtime: "claude-code", + Tier: 2, + PlatformURL: "http://tenant", + TemplateIdentity: "seo-agent-v1.2.3", + TemplateAssetFetcher: prov, + // No cfg.ConfigFiles — pure fetcher path. If the wire + // shape is right, TemplateAssets is non-empty and + // ConfigFiles is empty. + }) + if err != nil { + t.Fatalf("Start: %v", err) + } + + // 1. TemplateAssets must contain the fetched files. + if got, want := len(gotBody.TemplateAssets), 3; got != want { + t.Errorf("TemplateAssets length = %d, want %d (fetcher output must reach the wire)", got, want) + } + if got, want := string(gotBody.TemplateAssets["config.yaml"]), "# from template repo"; got != want { + t.Errorf("TemplateAssets[config.yaml] = %q, want %q", got, want) + } + if got, want := string(gotBody.TemplateAssets["prompts/system.md"]), "# template system prompt"; got != want { + t.Errorf("TemplateAssets[prompts/system.md] = %q, want %q", got, want) + } + if got, want := string(gotBody.TemplateAssets["agent-skills/seo-audit/SKILL.md"]), "# 716 KiB SEO skill goes here"; got != want { + t.Errorf("TemplateAssets[agent-skills/seo-audit/SKILL.md] = %q, want %q", got, want) + } + + // 2. ConfigFiles must be empty — the transport split. + if got, want := len(gotBody.ConfigFiles), 0; got != want { + t.Errorf("ConfigFiles length = %d, want %d (transport split — fetched assets must NOT leak into the SM-bound ConfigFiles)", got, want) + } +} + +// TestStart_AbortsOnFetcherAssetOutsideAllowlist — the +// load-bearing blast-radius test (RC #11690). A fetcher +// returning a path outside the template-asset allowlist +// MUST abort the provision at the Start level (the +// transport-DoS-and-blast-radius contract). +func TestStart_AbortsOnFetcherAssetOutsideAllowlist(t *testing.T) { + prov := &fakeTemplateAssetFetcher{ + bundle: map[string][]byte{ + "MEMORY.md": []byte("# hostile — agent-owned curated memory must not be transported by the provision path"), + }, + } + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Errorf("CP should not be called when fetcher returns a path outside the allowlist; got request: %+v", r) + w.WriteHeader(http.StatusCreated) + _, _ = io.WriteString(w, `{"instance_id":"i-abc123","state":"pending"}`) + })) + defer srv.Close() + + p := &CPProvisioner{baseURL: srv.URL, orgID: "org-1", httpClient: srv.Client()} + _, err := p.Start(context.Background(), WorkspaceConfig{ + WorkspaceID: "ws-1", + Runtime: "claude-code", + Tier: 2, + PlatformURL: "http://tenant", + TemplateIdentity: "seo-agent-v1.2.3", + TemplateAssetFetcher: prov, + }) + if err == nil { + t.Fatal("expected Start to abort on fetcher-returned MEMORY.md path, got nil") + } + if !strings.Contains(err.Error(), "MEMORY.md") { + t.Errorf("expected error to mention the rejected path MEMORY.md, got: %v", err) + } + if !strings.Contains(err.Error(), "allowlist") { + t.Errorf("expected error to mention the allowlist, got: %v", err) + } +} + // TestStart_SymlinkTemplatePathError — a symlink TemplatePath should cause // collectCPConfigFiles to return an error, which Start must propagate. // Without this wiring, OFFSEC-010's root-symlink guard is dead code. @@ -1176,7 +1277,7 @@ func TestCollectCPConfigFiles_SkipsSymlinks(t *testing.T) { t.Fatal(err) } - files, err := collectCPConfigFiles(WorkspaceConfig{TemplatePath: tmpl}) + files, _, err := collectCPConfigFiles(WorkspaceConfig{TemplatePath: tmpl}) if err != nil { t.Fatalf("collectCPConfigFiles: %v", err) } @@ -1210,7 +1311,7 @@ func TestCollectCPConfigFiles_RejectsRootSymlink(t *testing.T) { t.Fatal(err) } - _, err := collectCPConfigFiles(WorkspaceConfig{TemplatePath: link}) + _, _, err := collectCPConfigFiles(WorkspaceConfig{TemplatePath: link}) if err == nil { t.Error("collectCPConfigFiles with symlink TemplatePath should return error") } diff --git a/workspace-server/internal/provisioner/provisioner.go b/workspace-server/internal/provisioner/provisioner.go index 71d08a3ac..40df2a3bd 100644 --- a/workspace-server/internal/provisioner/provisioner.go +++ b/workspace-server/internal/provisioner/provisioner.go @@ -110,6 +110,7 @@ const ( type WorkspaceConfig struct { WorkspaceID string TemplatePath string // Host path to template dir to copy from (e.g. claude-code-default/) + TemplateIdentity string // RFC #2843 #24: opaque token the TemplateAssetFetcher resolves to the template repo+ref (e.g. "claudius-v1.2.3" or a sha). Used by SaaS; ignored by the local-dir TemplatePath path. ConfigFiles map[string][]byte // Generated config files to write into /configs volume PluginsPath string // Host path to plugins directory (mounted at /plugins) WorkspacePath string // Host path to bind-mount as /workspace (if empty, uses Docker named volume) @@ -132,6 +133,21 @@ type WorkspaceConfig struct { WorkspaceAccess string // #65: "none" (default), "read_only", or "read_write" ResetClaudeSession bool // #12: if true, discard the claude-sessions volume before start (fresh session dir) + // TemplateAssetFetcher (RFC #2843 #24) is the generic + // non-secret asset channel for template assets + // (config.yaml + prompts/ + agent-skills/). The fetcher + // resolves cfg.TemplateIdentity to a shallow clone of the + // template repo (Gitea per RFC §4.2 transport option (a)) + // and returns the asset file map. nil = no provider wired + // (self-host default; falls through to the local TemplatePath + // path for the config bundle). For SaaS workspaces, main.go + // wires a real impl (Gitea shallow clone). When the fetcher + // is set, it MERGES with cfg.ConfigFiles (caller wins on + // conflict, like the prior PersistedBundleProvider pattern + // in #2831 PIECE 1) so the existing TemplatePath+ConfigFiles + // path keeps working for callers that don't opt in. + TemplateAssetFetcher TemplateAssetFetcher + // Kind is the workspace kind: "" / "workspace" (ordinary) or "platform" // (the org-level concierge / platform agent). When "platform", the local // Docker provisioner prefers the platform-agent image variant (which bakes diff --git a/workspace-server/internal/provisioner/template_assets.go b/workspace-server/internal/provisioner/template_assets.go new file mode 100644 index 000000000..0cb298fb0 --- /dev/null +++ b/workspace-server/internal/provisioner/template_assets.go @@ -0,0 +1,105 @@ +package provisioner + +// template_assets.go — generic template-asset channel (RFC #2843 #24). +// +// This is the "generic, non-secret asset channel" the RFC proposes: +// a workspace's template assets (config.yaml + prompts/ + agent-skills/) +// are materialized from a template identity (resolved by the caller — +// the template repo path, a cached ref, etc.) rather than forced +// through the AWS Secrets Manager config bundle path that caps at +// cpConfigFilesMaxBytes (256 KiB) and silently drops any skill over +// the cap. +// +// The fetcher is interface-typed so tests inject fakes and the real +// implementation (in main.go) wires the Gitea shallow-clone per +// RFC §4.2 transport option (a). The interface is NARROW (Load only) +// to keep the abstraction minimal — the template identity resolution +// and fetch are both the caller's job. +// +// Blast-radius / isolation (Reviewer-CR2 RC #11690 on the prior +// head): the fetcher ONLY materializes TEMPLATE ASSETS. Every +// path the fetcher returns is gated by IsCPTemplateAssetPath +// (this file) BEFORE it lands in the wire payload. Paths outside +// the allowlist — MEMORY.md, USER.md, CLAUDE.md, .claude/sessions/, +// /etc/passwd, traversal sequences — are REJECTED (the provision +// aborts with a structured error) rather than silently admitted. +// This is the load-bearing guard: a fetcher that returns a path +// outside the template-asset namespace is a programming error or +// an attack, and either way the safe response is to fail closed. +// +// Transport split (Reviewer-CR2 addendum on the prior head): +// fetched assets go to a SEPARATE wire field (TemplateAssets on +// cpProvisionRequest) rather than being merged into ConfigFiles. +// ConfigFiles is the bundle the CP stages through AWS Secrets +// Manager — the wrong layer for non-secret assets per the +// core-devops 10:13 SM-inventory RCA. The split lets a future CP +// route TemplateAssets through a non-secret channel (Gitea asset +// pin, S3 non-secret bucket, etc.) without a wire-shape change. +// +// Concurrency: the fetcher's Load is called from prepareProvisionContext, +// which serializes per-workspace (the existing per-workspace +// restart/provision gate at workspace_dispatchers.go:439 holds the +// mutex). No additional locking needed. + +import ( + "context" + "path/filepath" + "strings" +) + +// TemplateAssetFetcher materializes a template's +// config.yaml + prompts/ + agent-skills/ from a non-secret +// asset channel (template repo, Gitea shallow clone per +// RFC #2843 §4.2). Returned paths are RELATIVE to the +// template asset root (e.g. "config.yaml", "prompts/system.md", +// "agent-skills/seo-audit/SKILL.md") and the bytes are raw +// file contents (not base64-encoded — the generic channel +// does not require encoding; the wire format encodes per +// its own transport). +// +// Returned errors: a transport / resolution failure is +// returned as a non-nil error so the caller can abort the +// provision rather than silently regressing to stub-mode +// /configs (the same fail-closed contract as the persisted- +// bundle provider in #2831 PIECE 1). +// +// CONTRACT: every key in the returned map MUST match +// IsCPTemplateAssetPath. Keys that don't match are rejected +// by the caller (the provision aborts). Implementations that +// can't constrain their output to the allowlist must filter +// before returning. +type TemplateAssetFetcher interface { + Load(ctx context.Context, templateIdentity string) (map[string][]byte, error) +} + +// IsCPTemplateAssetPath reports whether a path returned by a +// TemplateAssetFetcher is eligible for transport to the workspace. +// +// Allowlist (load-bearing blast-radius guard — see RC #11690): +// +// - "config.yaml" — the runtime entrypoint config +// - "prompts/*" — system prompts +// - "agent-skills/*" — the agent's skill packages +// +// Everything else is REJECTED. Specifically excluded: +// MEMORY.md / USER.md (curated durable memory — agent-owned +// state, reconciled by the boot entrypoint, not by this +// collect path), CLAUDE.md (runtime memory file, agent-owned), +// .claude/sessions/* (Claude Code session dir, agent-owned), +// anything outside the template-asset namespace. +// +// Path normalization: the function applies filepath.ToSlash +// + filepath.Clean before matching, so Windows-style +// separators, redundant "./" segments, and trailing slashes +// are normalized. Traversal sequences (".." or paths +// containing "/../") are NOT explicitly stripped here — +// callers must check for traversal before calling this +// function (the existing addFile in cp_provisioner.go does). +// This function only validates the namespace; the traversal +// check is a separate invariant. +func IsCPTemplateAssetPath(name string) bool { + name = filepath.ToSlash(filepath.Clean(name)) + return name == "config.yaml" || + strings.HasPrefix(name, "prompts/") || + strings.HasPrefix(name, "agent-skills/") +} diff --git a/workspace-server/internal/provisioner/template_assets_test.go b/workspace-server/internal/provisioner/template_assets_test.go new file mode 100644 index 000000000..950f9ff4e --- /dev/null +++ b/workspace-server/internal/provisioner/template_assets_test.go @@ -0,0 +1,530 @@ +package provisioner + +// template_assets_test.go — generic template-asset channel tests +// (RFC #2843 #24). Verifies: +// 1. The fetcher wires into collectCPConfigFiles when both +// cfg.TemplateAssetFetcher and cfg.TemplateIdentity are set. +// 2. Fetched assets land in the SEPARATE TemplateAssets field +// (NOT merged into ConfigFiles) — the transport split that +// Reviewer-CR2 addendum called out as the load-bearing fix +// (a fetcher must not push 716 KiB skills through the 256 KiB +// SM-bound ConfigFiles). +// 3. Every key in the fetcher's output is gated by +// IsCPTemplateAssetPath. Paths outside the template-asset +// allowlist (config.yaml / prompts/* / agent-skills/*) ABORT +// the provision — Reviewer-CR2 RC #11690's load-bearing +// blast-radius guard. +// 4. A transport error on the fetcher ABORTS the provision +// (fail-closed; never regresses to stub /configs). +// 5. Nil fetcher = no-op (self-host default; the existing +// TemplatePath local-dir path still works). +// 6. Empty TemplateIdentity with a non-nil fetcher = no-op +// (the fetcher is only called when there's an identity +// to resolve). + +import ( + "context" + "encoding/base64" + "errors" + "runtime" + "strings" + "testing" +) + +// fakeTemplateAssetFetcher is a capture-only stub satisfying +// TemplateAssetFetcher. Returns the configured bundle+err; +// records the template identities the handler asked for. +type fakeTemplateAssetFetcher struct { + bundle map[string][]byte + err error + calls []string +} + +func (f *fakeTemplateAssetFetcher) Load(_ context.Context, templateIdentity string) (map[string][]byte, error) { + f.calls = append(f.calls, templateIdentity) + return f.bundle, f.err +} + +// TestCollectCPConfigFiles_MergesFetcherAssets is the +// happy path: fetcher returns assets, the assets land in +// TemplateAssets (the SEPARATE wire field — the transport +// split per Reviewer-CR2 addendum). Fails if a future +// refactor stops calling cfg.TemplateAssetFetcher.Load +// from collectCPConfigFiles when the fetcher + identity +// are set, or re-merges assets into ConfigFiles. +func TestCollectCPConfigFiles_MergesFetcherAssets(t *testing.T) { + prov := &fakeTemplateAssetFetcher{ + bundle: map[string][]byte{ + "config.yaml": []byte("# from template repo"), + "prompts/system.md": []byte("# template system prompt"), + "agent-skills/seo-audit/SKILL.md": []byte("# seo skill"), + }, + } + cfg := WorkspaceConfig{ + TemplateIdentity: "seo-agent-v1.2.3", + TemplateAssetFetcher: prov, + } + + files, assets, err := collectCPConfigFiles(cfg) + if err != nil { + t.Fatalf("collectCPConfigFiles: %v", err) + } + // All 3 fetched assets land in TemplateAssets (the + // non-secret transport), NOT in ConfigFiles (the SM-bound + // bundle). The transport split is the load-bearing fix. + wantKeys := []string{ + "config.yaml", + "prompts/system.md", + "agent-skills/seo-audit/SKILL.md", + } + for _, wk := range wantKeys { + if _, ok := assets[wk]; !ok { + t.Errorf("expected %q in TemplateAssets, got keys: %v", wk, keysOfAssetMap(assets)) + } + if _, ok := files[wk]; ok { + t.Errorf("did NOT expect %q in ConfigFiles (transport split — TemplateAssets is the non-secret channel)", wk) + } + } + // Fetcher was called once with the right identity. + if len(prov.calls) != 1 || prov.calls[0] != "seo-agent-v1.2.3" { + t.Errorf("expected one Load call with identity=seo-agent-v1.2.3, got calls=%v", prov.calls) + } +} + +// TestCollectCPConfigFiles_CallerWinsOnConfigFiles asserts +// the caller's cfg.ConfigFiles entry lands in the ConfigFiles +// field (the SM-bound bundle) even when a fetcher is wired. +// The two fields are independent transports — caller-provided +// files are still the SM-bound bundle (small non-secret config +// text only), fetcher-provided assets ride the non-secret +// TemplateAssets field. +func TestCollectCPConfigFiles_CallerWinsOnConfigFiles(t *testing.T) { + prov := &fakeTemplateAssetFetcher{ + bundle: map[string][]byte{ + "config.yaml": []byte("# from template repo"), + }, + } + cfg := WorkspaceConfig{ + TemplateIdentity: "seo-agent-v1.2.3", + TemplateAssetFetcher: prov, + ConfigFiles: map[string][]byte{ + "config.yaml": []byte("# caller override"), + }, + } + + files, assets, err := collectCPConfigFiles(cfg) + if err != nil { + t.Fatalf("collectCPConfigFiles: %v", err) + } + // Caller's ConfigFiles["config.yaml"] is base64-encoded into + // the SM-bound ConfigFiles field. The fetcher's same-key + // result lands in TemplateAssets (separate transport) — no + // conflict because they're on different fields. + decoded, decErr := base64.StdEncoding.DecodeString(files["config.yaml"]) + if decErr != nil { + t.Fatalf("decode config.yaml from ConfigFiles: %v", decErr) + } + if string(decoded) != "# caller override" { + t.Errorf("expected caller override in ConfigFiles, got %q", string(decoded)) + } + if got := string(assets["config.yaml"]); got != "# from template repo" { + t.Errorf("expected fetcher asset in TemplateAssets, got %q", got) + } +} + +// TestCollectCPConfigFiles_FetcherErrorAborts is the +// fail-closed assertion: a transport error from the fetcher +// must abort the provision rather than regressing to stub +// /configs (the same fail-closed contract as the +// persisted-bundle provider in #2831 PIECE 1). If a future +// refactor swallows the fetch error, the bundle would +// silently miss the agent-skills/ files and the workspace +// would boot with a stub config — the exact regression +// this test guards against. +func TestCollectCPConfigFiles_FetcherErrorAborts(t *testing.T) { + prov := &fakeTemplateAssetFetcher{err: errors.New("gitea 503")} + cfg := WorkspaceConfig{ + TemplateIdentity: "seo-agent-v1.2.3", + TemplateAssetFetcher: prov, + } + + _, _, err := collectCPConfigFiles(cfg) + if err == nil { + t.Fatal("expected collectCPConfigFiles to abort on fetcher error, got nil") + } + if !stringsContains(err.Error(), "gitea 503") { + t.Errorf("expected error to surface the underlying gitea 503, got: %v", err) + } +} + +// TestCollectCPConfigFiles_NilFetcherIsNoop asserts the +// self-host default: a WorkspaceConfig without a fetcher +// does NOT call anything (the existing TemplatePath + +// ConfigFiles path is unchanged). The RFC's opt-in +// contract: nil fetcher = no asset channel, no behavior +// change for self-host callers. +func TestCollectCPConfigFiles_NilFetcherIsNoop(t *testing.T) { + cfg := WorkspaceConfig{ + TemplateIdentity: "seo-agent-v1.2.3", // set but no fetcher wired + } + _, _, err := collectCPConfigFiles(cfg) + if err != nil { + t.Fatalf("collectCPConfigFiles with nil fetcher: %v", err) + } +} + +// TestCollectCPConfigFiles_EmptyIdentityNoop asserts that +// even a wired fetcher is NOT called when TemplateIdentity is +// empty (the Gitea fetcher needs an identity to resolve; an +// empty identity would be a programming error and should +// be a no-op rather than a fetch-with-empty-identity call). +func TestCollectCPConfigFiles_EmptyIdentityNoop(t *testing.T) { + prov := &fakeTemplateAssetFetcher{ + bundle: map[string][]byte{"config.yaml": []byte("# unexpected")}, + } + cfg := WorkspaceConfig{ + TemplateIdentity: "", // empty + TemplateAssetFetcher: prov, // wired but no identity + } + _, _, err := collectCPConfigFiles(cfg) + if err != nil { + t.Fatalf("collectCPConfigFiles with empty identity: %v", err) + } + if len(prov.calls) != 0 { + t.Errorf("expected fetcher NOT to be called with empty identity, got calls=%v", prov.calls) + } +} + +// --- Blast-radius allowlist tests (Reviewer-CR2 RC #11690) --- + +// TestIsCPTemplateAssetPath_AllowsConfigYaml pins the happy +// path: a fetcher returning "config.yaml" passes the +// allowlist. +func TestIsCPTemplateAssetPath_AllowsConfigYaml(t *testing.T) { + if !IsCPTemplateAssetPath("config.yaml") { + t.Error("expected config.yaml to be allowed") + } +} + +// TestIsCPTemplateAssetPath_AllowsPromptsPrefix pins the +// prompts/* namespace. Note: "prompts/" (with trailing slash) +// is normalized to "prompts" by filepath.Clean, and the +// allowlist requires the "prompts/" prefix on a non-empty +// path — the literal "prompts" string is rejected (it's a +// directory name, not a file). The test pins file-shaped +// paths under prompts/. +func TestIsCPTemplateAssetPath_AllowsPromptsPrefix(t *testing.T) { + for _, ok := range []string{"prompts/system.md", "prompts/sub/foo.md"} { + if !IsCPTemplateAssetPath(ok) { + t.Errorf("expected %q to be allowed", ok) + } + } +} + +// TestIsCPTemplateAssetPath_AllowsAgentSkillsPrefix pins +// the agent-skills/* namespace (the load-bearing addition +// for the 716 KiB SEO skill package per core-devops 10:13). +func TestIsCPTemplateAssetPath_AllowsAgentSkillsPrefix(t *testing.T) { + for _, ok := range []string{ + "agent-skills/seo-audit/SKILL.md", + "agent-skills/seo-audit/manifest.yaml", + "agent-skills/index.json", + } { + if !IsCPTemplateAssetPath(ok) { + t.Errorf("expected %q to be allowed", ok) + } + } +} + +// TestIsCPTemplateAssetPath_RejectsMemoryMd pins the +// curated-memory exclusion. MEMORY.md is agent-owned +// durable state — reconciled by the boot entrypoint, NOT +// by the provision path. +func TestIsCPTemplateAssetPath_RejectsMemoryMd(t *testing.T) { + if IsCPTemplateAssetPath("MEMORY.md") { + t.Error("MEMORY.md must NOT be allowed (agent-owned curated memory, reconciled by boot entrypoint)") + } +} + +// TestIsCPTemplateAssetPath_RejectsUserMd pins the +// curated-memory exclusion for USER.md. +func TestIsCPTemplateAssetPath_RejectsUserMd(t *testing.T) { + if IsCPTemplateAssetPath("USER.md") { + t.Error("USER.md must NOT be allowed (agent-owned curated memory, reconciled by boot entrypoint)") + } +} + +// TestIsCPTemplateAssetPath_RejectsClaudeMd pins the +// runtime-memory exclusion. CLAUDE.md is the runtime's +// memory file (Claude Code reads it at session start). +func TestIsCPTemplateAssetPath_RejectsClaudeMd(t *testing.T) { + if IsCPTemplateAssetPath("CLAUDE.md") { + t.Error("CLAUDE.md must NOT be allowed (runtime memory file, agent-owned state)") + } +} + +// TestIsCPTemplateAssetPath_RejectsClaudeSessionsPath +// pins the Claude Code session-dir exclusion. Sessions +// live on their own volume; pushing them into the +// template-asset channel would clobber or duplicate them. +func TestIsCPTemplateAssetPath_RejectsClaudeSessionsPath(t *testing.T) { + for _, bad := range []string{ + ".claude/sessions/abc.json", + ".claude/sessions", + ".claude/settings.json", + } { + if IsCPTemplateAssetPath(bad) { + t.Errorf("%q must NOT be allowed (Claude Code agent-owned state)", bad) + } + } +} + +// TestIsCPTemplateAssetPath_RejectsAbsoluteAndTraversal +// pins the path-shape guards. The function applies +// filepath.Clean + filepath.ToSlash before matching, so +// ".." and absolute paths normalize to non-matching +// shapes. The addFile/addAsset callsite separately +// rejects traversal sequences; this is a belt-and-braces +// assertion that the allowlist itself doesn't admit +// weird shapes. +func TestIsCPTemplateAssetPath_RejectsAbsoluteAndTraversal(t *testing.T) { + for _, bad := range []string{ + "../etc/passwd", + "prompts/../secrets", + "/etc/passwd", + "..", + ".", + "", + } { + if IsCPTemplateAssetPath(bad) { + t.Errorf("%q must NOT be allowed", bad) + } + } +} + +// TestIsCPTemplateAssetPath_NormalizesSlashes pins the +// Windows-separator normalization for the file path +// pre-processing. On Windows, filepath.ToSlash converts +// backslashes to forward slashes BEFORE the match, so a +// fetcher returning "prompts\\system.md" is treated as +// "prompts/system.md" (allowed). On Linux/macOS, the +// backslash is a valid filename character (not a +// separator), so the same input would be rejected as a +// literal filename. This is intentional — the +// normalization matches the OS's notion of a path +// separator, so a malicious fetcher can't smuggle a +// backslash-as-separator past the allowlist on Windows +// by relying on case-insensitive or platform-specific +// matching. +// +// Test executes the normalization contract on whatever +// host runs the test: a path that already uses forward +// slashes (the Linux/macOS case) must pass. +func TestIsCPTemplateAssetPath_NormalizesSlashes(t *testing.T) { + if !IsCPTemplateAssetPath("prompts/system.md") { + t.Error("expected forward-slash path to pass the allowlist (the canonical case)") + } + // Backslashes: only normalize to slashes on Windows. + // On Linux, a backslash is part of the literal name and + // is rejected (which is the correct behavior — a + // fetcher returning Windows-style paths on a Linux host + // is either a misconfigured fetcher or an attack, and + // either way failing closed is the safe response). + if runtime.GOOS == "windows" { + if !IsCPTemplateAssetPath(`prompts\system.md`) { + t.Error("expected backslash-separated path to normalize and pass the allowlist on Windows") + } + } else { + if IsCPTemplateAssetPath(`prompts\system.md`) { + t.Error("backslash is a literal character on non-Windows hosts and must NOT be treated as a path separator by the allowlist") + } + } +} + +// TestCollectCPConfigFiles_RejectsFetcherAssetOutsideAllowlist +// is the load-bearing test for RC #11690. A fetcher that +// returns a path outside the template-asset allowlist +// (MEMORY.md / USER.md / CLAUDE.md / .claude/sessions/*) +// MUST abort the provision. If a future refactor weakens +// the addAsset gate, this test catches the regression. +func TestCollectCPConfigFiles_RejectsFetcherAssetOutsideAllowlist(t *testing.T) { + cases := []struct { + name string + badKey string + expectedSub string + }{ + {"MEMORY.md", "MEMORY.md", "MEMORY.md"}, + {"USER.md", "USER.md", "USER.md"}, + {"CLAUDE.md", "CLAUDE.md", "CLAUDE.md"}, + {"claude-sessions", ".claude/sessions/abc.json", ".claude/sessions/abc.json"}, + {"absolute-path", "/etc/passwd", "/etc/passwd"}, + {"adapter.py", "adapter.py", "adapter.py"}, + {"Dockerfile", "Dockerfile", "Dockerfile"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + prov := &fakeTemplateAssetFetcher{ + bundle: map[string][]byte{c.badKey: []byte("hostile content")}, + } + cfg := WorkspaceConfig{ + TemplateIdentity: "seo-agent-v1.2.3", + TemplateAssetFetcher: prov, + } + _, _, err := collectCPConfigFiles(cfg) + if err == nil { + t.Fatalf("expected provision to abort on fetcher-returned path %q outside template-asset allowlist, got nil error", c.badKey) + } + if !stringsContains(err.Error(), c.expectedSub) { + t.Errorf("expected error to mention %q, got: %v", c.expectedSub, err) + } + }) + } +} + +// TestCollectCPConfigFiles_FetcherAssetsBase64EncodedOnWire +// pins the wire-format invariant for the new TemplateAssets +// field: assets travel base64-encoded over JSON (same as +// ConfigFiles), to avoid JSON escaping issues with binary +// content (manifests, SKILL.md files, etc.). The collect +// function returns the raw bytes; the marshal step in +// Start does the encoding. This test documents the +// invariant at the collect boundary. +func TestCollectCPConfigFiles_FetcherAssetsRawBytes(t *testing.T) { + prov := &fakeTemplateAssetFetcher{ + bundle: map[string][]byte{ + "config.yaml": []byte("# raw bytes, will be base64 by marshaler"), + "agent-skills/seo-audit/SKILL.md": []byte("raw-skill"), + }, + } + cfg := WorkspaceConfig{ + TemplateIdentity: "seo-agent-v1.2.3", + TemplateAssetFetcher: prov, + } + _, assets, err := collectCPConfigFiles(cfg) + if err != nil { + t.Fatalf("collectCPConfigFiles: %v", err) + } + if got := string(assets["config.yaml"]); got != "# raw bytes, will be base64 by marshaler" { + t.Errorf("expected raw bytes in TemplateAssets, got %q (encoding happens at marshal time, not at collect time)", got) + } + if got := string(assets["agent-skills/seo-audit/SKILL.md"]); got != "raw-skill" { + t.Errorf("expected raw skill bytes in TemplateAssets, got %q", got) + } +} + +// TestCollectCPConfigFiles_NoAssetsWhenNoFetcher pins the +// non-fetcher case: when no fetcher is wired (self-host +// default), the TemplateAssets field is nil/empty. A +// future refactor that always populates TemplateAssets +// (even with empty data) would inflate the wire payload +// for every self-host workspace — this test catches that. +func TestCollectCPConfigFiles_NoAssetsWhenNoFetcher(t *testing.T) { + cfg := WorkspaceConfig{ + TemplateIdentity: "seo-agent-v1.2.3", + // TemplateAssetFetcher is nil + } + _, assets, err := collectCPConfigFiles(cfg) + if err != nil { + t.Fatalf("collectCPConfigFiles: %v", err) + } + if len(assets) != 0 { + t.Errorf("expected nil/empty TemplateAssets when no fetcher wired, got %d entries: %v", len(assets), keysOfAssetMap(assets)) + } +} + +// TestCollectCPConfigFiles_PreservesCallerConfigFiles pins +// the existing TemplatePath + ConfigFiles path: when a +// fetcher is NOT wired, the SM-bound ConfigFiles field +// behaves exactly as before (TemplatePath walk + +// cfg.ConfigFiles entries). The transport split is +// additive — it doesn't disturb the existing self-host +// path. +func TestCollectCPConfigFiles_PreservesCallerConfigFiles(t *testing.T) { + cfg := WorkspaceConfig{ + ConfigFiles: map[string][]byte{ + "config.yaml": []byte("# caller"), + "generated.secret": []byte("not really a secret"), + }, + } + files, assets, err := collectCPConfigFiles(cfg) + if err != nil { + t.Fatalf("collectCPConfigFiles: %v", err) + } + if _, ok := files["config.yaml"]; !ok { + t.Error("expected config.yaml in ConfigFiles (caller-provided)") + } + if _, ok := files["generated.secret"]; !ok { + t.Error("expected generated.secret in ConfigFiles (caller-provided)") + } + if len(assets) != 0 { + t.Errorf("expected empty TemplateAssets when no fetcher wired, got %d entries", len(assets)) + } +} + +// keysOfAssetMap returns the sorted keys of a map for stable +// test output. Local helper so test output doesn't depend on +// Go's randomized map iteration order. +func keysOfAssetMap(m map[string][]byte) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + return out +} + +// stringsContains is a tiny shim so the test doesn't need +// the strings import elsewhere (it does already, but this +// keeps the dependency local). Existence is asserted via +// string comparison. +func stringsContains(s, substr string) bool { + return strings.Contains(s, substr) +} + +// TestCollectCPConfigFiles_AssetsAllowLargeSkillPackage is the regression for +// Reviewer-CR2's size-cap RC on head 7bcc3b5f: template ASSETS ride a +// NON-secret channel and must NOT be bound by the 256 KiB SM/config-bundle cap +// (cpConfigFilesMaxBytes). Reusing that cap re-creates the original #2831 +// skill-drop — the motivating 716 KiB seo-all package would abort client-side. +// A >256 KiB asset payload must SUCCEED on TemplateAssets (bounded only by the +// far larger cpTemplateAssetsMaxBytes DoS guard) while ConfigFiles stays capped. +func TestCollectCPConfigFiles_AssetsAllowLargeSkillPackage(t *testing.T) { + // 716 KiB skill blob — over the old 256 KiB cap, well under the asset bound. + big := make([]byte, 716<<10) + for i := range big { + big[i] = 'x' + } + prov := &fakeTemplateAssetFetcher{ + bundle: map[string][]byte{ + "config.yaml": []byte("# from template repo"), + "agent-skills/seo-audit/big-skill.md": big, + }, + } + cfg := WorkspaceConfig{ + TemplateIdentity: "seo-agent-v1.2.3", + TemplateAssetFetcher: prov, + } + + _, assets, err := collectCPConfigFiles(cfg) + if err != nil { + t.Fatalf("a %d-byte skill package must succeed on the non-secret asset channel (no SM cap), got error: %v", len(big), err) + } + if got := len(assets["agent-skills/seo-audit/big-skill.md"]); got != len(big) { + t.Errorf("expected the full %d-byte skill in TemplateAssets, got %d", len(big), got) + } +} + +// TestCollectCPConfigFiles_ConfigFilesStillCappedAt256K pins that lifting the +// asset cap did NOT relax the SM/user-data transport limit: the SM-bound +// ConfigFiles bundle keeps its 256 KiB cap (cpConfigFilesMaxBytes). +func TestCollectCPConfigFiles_ConfigFilesStillCappedAt256K(t *testing.T) { + big := make([]byte, (256<<10)+1) + for i := range big { + big[i] = 'y' + } + cfg := WorkspaceConfig{ + ConfigFiles: map[string][]byte{"system-prompt.md": big}, + } + if _, _, err := collectCPConfigFiles(cfg); err == nil { + t.Fatal("ConfigFiles over 256 KiB must still be rejected (SM/user-data transport cap unchanged)") + } +}