From 766b65635fb075ef2e893adb5abf49083872cfc3 Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer B (MiniMax)" Date: Sun, 14 Jun 2026 11:05:01 +0000 Subject: [PATCH 1/4] feat(provisioner#24): generic template-asset channel (RFC #2843) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PIECE 1 / RFC #2843 / #24 — generic, non-secret template asset channel that replaces the per-template SEO patch (deleted in #23) AND scales to ANY template (not just the seo-agent). Per RFC §4.2 transport option (a): template assets (config.yaml + prompts/ + agent-skills/) are materialized from a template identity (resolved by the fetcher) onto the persisted data volume at provision and on every boot. The SM config-bundle path is no longer the transport for non-secret assets. NEW API (workspace-server/internal/provisioner): type TemplateAssetFetcher interface { Load(ctx, templateIdentity string) (map[string][]byte, error) } A SaaS workspace passes a Gitea shallow-clone impl (transport option (a) per RFC §4.2). The fetcher resolves templateIdentity → the template repo + ref → fetches config.yaml + prompts/* + agent-skills/* → returns the asset map. Self-host callers (Docker provisioner) leave the fetcher nil and fall through to the existing local TemplatePath path — the RFC's opt-in contract: nil fetcher = no behavior change for self-host. NEW FIELDS on WorkspaceConfig: TemplateIdentity string // opaque token (e.g. claudius-v1.2.3) TemplateAssetFetcher TemplateAssetFetcher // nil = self-host default WIRING (collectCPConfigFiles in cp_provisioner.go): 1. TemplatePath walk (existing local-dir path) — unchanged 2. cfg.ConfigFiles (caller overrides) — unchanged 3. NEW: cfg.TemplateAssetFetcher.Load(ctx, TemplateIdentity) when BOTH are set, seeds the bundle with the fetched assets. Caller's cfg.ConfigFiles win on conflict (the fetcher is called BEFORE the ConfigFiles loop so any caller entry overwrites the fetcher's). 4. Fail-closed: a transport error from the fetcher ABORTS the provision rather than regressing to stub-mode /configs (same contract as the persisted-bundle provider in #2831 PIECE 1 — no silent regressions). CRITICAL preserve/blast-radius axis (PM_flagged_in_#24_clarify): this fetcher ONLY materializes TEMPLATE ASSETS — config + prompts + skills. It does NOT touch agent-owned state (memory, ~/.claude/sessions, /workspace) — those live on separate volumes and are reconciled by the boot entrypoint, NOT by this collect path. CR2+Researcher will scrutinize this; the comment in cp_provisioner.go calls it out explicitly. TEST SURFACE (5 new tests in template_assets_test.go): - TestCollectCPConfigFiles_MergesFetcherAssets: happy path (fetcher returns assets, bundle includes them). - TestCollectCPConfigFiles_CallerWinsOnConflict: caller's cfg.ConfigFiles entry overwrites the fetcher's (no-clobber policy). - TestCollectCPConfigFiles_FetcherErrorAborts: a transport error surfaces as a provision abort (fail-closed). - TestCollectCPConfigFiles_NilFetcherIsNoop: nil fetcher = no-op (self-host default; behavior unchanged). - TestCollectCPConfigFiles_EmptyIdentityNoop: a wired fetcher is NOT called with empty identity (programming- error guard — Gitea can't resolve an empty ref). go test ./internal/provisioner/ -> 0.071s clean (all 5 new + all existing pass). go test ./internal/handlers/ -> 25.328s clean (no regressions). Refs RFC #2843 #24, #23 (predecessor), #2831 PIECE 1 (predecessor pattern: PersistedBundleProvider in approvals.go). NOT in this PR (follow-ups in #25+): - main.go wiring of a real Gitea fetcher for SaaS. - Reconcile-on-boot path (the existing PersistedBundleProvider in approvals.go is the model for that; this PR is the consumer hook). - Self-repair-on-boot for missing/stub /configs (RFC §4.3). --- .../internal/provisioner/cp_provisioner.go | 39 ++++ .../internal/provisioner/provisioner.go | 16 ++ .../internal/provisioner/template_assets.go | 63 ++++++ .../provisioner/template_assets_test.go | 204 ++++++++++++++++++ 4 files changed, 322 insertions(+) create mode 100644 workspace-server/internal/provisioner/template_assets.go create mode 100644 workspace-server/internal/provisioner/template_assets_test.go diff --git a/workspace-server/internal/provisioner/cp_provisioner.go b/workspace-server/internal/provisioner/cp_provisioner.go index ca87f659c..48d02f899 100644 --- a/workspace-server/internal/provisioner/cp_provisioner.go +++ b/workspace-server/internal/provisioner/cp_provisioner.go @@ -446,6 +446,45 @@ func collectCPConfigFiles(cfg WorkspaceConfig) (map[string]string, error) { return nil, err } } + + // 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 merge into + // the bundle. Caller's cfg.ConfigFiles win on conflict + // (the merge happens BEFORE this loop, so any caller + // overrides are preserved). The fetch is OPT-IN: nil + // fetcher = no-op (self-host default; falls through to + // the local TemplatePath path above). + // + // CRITICAL preserve/blast-radius axis (PM-flagged): + // this fetcher ONLY materializes template assets (config + + // prompts + skills). It does NOT touch agent-owned state + // (memory, ~/.claude/sessions, /workspace) — those live + // on separate volumes and are reconciled by the boot + // entrypoint, NOT by this collect path. 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 != "" { + assets, fetchErr := cfg.TemplateAssetFetcher.Load(context.Background(), cfg.TemplateIdentity) + if fetchErr != nil { + return nil, fmt.Errorf("collectCPConfigFiles: fetch template assets (RFC #2843 #24): %w", fetchErr) + } + for name, data := range assets { + if err := addFile(name, data); err != nil { + return nil, err + } + } + } + for name, data := range cfg.ConfigFiles { + if err := addFile(name, data); err != nil { + return nil, err + } + } if len(files) == 0 { return nil, nil } 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..07333fc0d --- /dev/null +++ b/workspace-server/internal/provisioner/template_assets.go @@ -0,0 +1,63 @@ +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. +// +// 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" + +// 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 caller can +// re-encode if needed for the SM wire format; the generic +// channel does not require encoding). +// +// 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). +type TemplateAssetFetcher interface { + Load(ctx context.Context, templateIdentity string) (map[string][]byte, error) +} + +// defaultTemplateAssetFetcher is the in-tree fallback used +// when no provider is wired (self-host default — the +// Docker provisioner reads from a local TemplatePath +// directory; SaaS wires a Gitea fetcher in main.go). +// Currently this is a stub that returns nil — the existing +// TemplatePath path in collectCPConfigFiles continues to +// handle the local-dir case. Wire a real impl in #24's +// follow-ups (the SaaS-side Gitea fetch). +type defaultTemplateAssetFetcher struct{} + +// Load on defaultTemplateAssetFetcher returns (nil, nil) — +// "no assets to add." The existing collectCPConfigFiles +// TemplatePath walk handles the local-dir case for self-host. +// SaaS workspaces wire a real fetcher (Gitea shallow clone) +// in main.go per RFC #2843 §4.2 transport option (a). +func (defaultTemplateAssetFetcher) Load(_ context.Context, _ string) (map[string][]byte, error) { + return nil, nil +} 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..cf22a4f95 --- /dev/null +++ b/workspace-server/internal/provisioner/template_assets_test.go @@ -0,0 +1,204 @@ +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 MERGE with cfg.ConfigFiles (caller wins +// on conflict — same pattern as the persisted-bundle +// provider in #2831 PIECE 1). +// 3. A transport error on the fetcher ABORTS the provision +// (fail-closed; never regresses to stub /configs). +// 4. Nil fetcher = no-op (self-host default; the existing +// TemplatePath local-dir path still works). +// 5. 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" + "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 bundle includes +// them alongside any caller-supplied cfg.ConfigFiles. +// Fails if a future refactor stops calling cfg.TemplateAssetFetcher.Load +// from collectCPConfigFiles when the fetcher + identity are set. +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, err := collectCPConfigFiles(cfg) + if err != nil { + t.Fatalf("collectCPConfigFiles: %v", err) + } + // All 3 fetched assets land in the bundle. + wantKeys := []string{ + "config.yaml", + "prompts/system.md", + "agent-skills/seo-audit/SKILL.md", + } + for _, wk := range wantKeys { + if _, ok := files[wk]; !ok { + t.Errorf("expected %q in bundle, got keys: %v", wk, keysOfBundle(files)) + } + } + // 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_CallerWinsOnConflict asserts the +// no-clobber policy: when the caller supplies a +// cfg.ConfigFiles["config.yaml"] AND the fetcher returns the +// same key, the caller's value wins (matches the persisted- +// bundle provider pattern in #2831 PIECE 1). +func TestCollectCPConfigFiles_CallerWinsOnConflict(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, err := collectCPConfigFiles(cfg) + if err != nil { + t.Fatalf("collectCPConfigFiles: %v", err) + } + // Decode the base64-encoded bundle value. + decoded, decErr := base64DecodeFirst(files["config.yaml"]) + if decErr != nil { + t.Fatalf("decode config.yaml: %v", decErr) + } + if string(decoded) != "# caller override" { + t.Errorf("expected caller override to win, got bundle config.yaml=%q", string(decoded)) + } +} + +// 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) + } +} + +// base64DecodeFirst is a small helper that decodes the +// first base64-encoded value in the bundle, returning the +// raw bytes. Test-side helper to avoid pulling base64 +// decoding into the production path. +func base64DecodeFirst(v string) ([]byte, error) { + decoded, err := base64.StdEncoding.DecodeString(v) + if err != nil { + return nil, err + } + return decoded, nil +} + +// keysOfBundle returns the sorted keys of a map for stable +// test output. Local helper to avoid coupling to the test +// files' key-ordering conventions. +func keysOfBundle(m map[string]string) []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) +} -- 2.52.0 From 7bcc3b5f725b72cfca5e33d0a0c58b4ed1b9597f Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer B (MiniMax)" Date: Sun, 14 Jun 2026 12:24:42 +0000 Subject: [PATCH 2/4] fix(provisioner#24): blast-radius allowlist + TemplateAssets transport split MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #2845 RC #11688 + RC #11690 + addendum (Researcher + Reviewer-CR2) on head 766b6563. Three load-bearing fixes: 1. REMOVED unused defaultTemplateAssetFetcher stub (RC #11688, lint blocker). The stub was a confusing artifact: it claimed to be a 'self-host fallback' but returned (nil, nil), so the existing nil- fetcher path in collectCPConfigFiles already handled that case. Removing the stub eliminates the golangci-lint 'unused' failure and tightens the surface. 2. ADDED IsCPTemplateAssetPath allowlist (RC #11690 blast-radius guard). Every key in a fetcher's output is gated by this function at the addAsset boundary in collectCPConfigFiles. Allowed: config.yaml, prompts/*, agent-skills/*. Rejected (provision aborts): MEMORY.md, USER.md, CLAUDE.md, .claude/sessions/*, plus the existing traversal/absolute/symlink guards. Without this guard a malicious or buggy fetcher could smuggle curated-memory or runtime-state paths into the bundle. 3. SPLIT TemplateAssets onto a SEPARATE wire field (addendum from Reviewer-CR2). The prior impl merged fetched assets into ConfigFiles, which is the bundle the CP stages through AWS Secrets Manager (cpConfigFilesMaxBytes = 256 KiB cap, wrong layer for non-secret assets per core-devops 10:13 SM-inventory RCA). The split lets a future CP route non-secret assets through a non-SM transport (Gitea asset pin, S3 non-secret bucket, etc.) without a wire-shape change. The TemplateAssets field travels on cpProvisionRequest as template_assets (omitempty), base64-encoded at marshal. NEW TESTS: - TestIsCPTemplateAssetPath_Allows* (3): pin the allowlist - TestIsCPTemplateAssetPath_Rejects* (4): pin the exclusions (MEMORY.md, USER.md, CLAUDE.md, .claude/sessions/*) - TestIsCPTemplateAssetPath_RejectsAbsoluteAndTraversal - TestIsCPTemplateAssetPath_NormalizesSlashes - TestCollectCPConfigFiles_RejectsFetcherAssetOutsideAllowlist (7 subcases: MEMORY.md, USER.md, CLAUDE.md, .claude/sessions/, /etc/passwd, adapter.py, Dockerfile) - TestCollectCPConfigFiles_FetcherAssetsRawBytes - TestCollectCPConfigFiles_NoAssetsWhenNoFetcher - TestCollectCPConfigFiles_PreservesCallerConfigFiles - TestStart_SendsTemplateAssetsOnSeparateField: end-to-end wire-shape test — fetched assets land in TemplateAssets, NOT ConfigFiles. Catches any future refactor that re-merges. - TestStart_AbortsOnFetcherAssetOutsideAllowlist: end-to-end blast-radius test — MEMORY.md aborts the provision before the CP is contacted. REFACTORED: collectCPConfigFiles returns (configFiles map[string]string, templateAssets map[string][]byte, error). Updated the existing TestStart_CollectsConfigFiles, TestStart_SendsTemplateAndGeneratedConfigFiles, and cp_provisioner_config_size_test.go callers to the new signature. go test ./internal/provisioner/ -> clean (0.05s) go test ./internal/handlers/ -> clean (25.25s, no regressions) go vet ./... -> clean go build ./... -> clean Refs #2831 PIECE 1, RFC #2843 #24, PR #2845. Addresses #11688, #11690, #11691. --- .../internal/provisioner/cp_provisioner.go | 115 ++++-- .../cp_provisioner_config_size_test.go | 6 +- .../provisioner/cp_provisioner_test.go | 105 ++++- .../internal/provisioner/template_assets.go | 116 ++++-- .../provisioner/template_assets_test.go | 363 +++++++++++++++--- 5 files changed, 585 insertions(+), 120 deletions(-) diff --git a/workspace-server/internal/provisioner/cp_provisioner.go b/workspace-server/internal/provisioner/cp_provisioner.go index 48d02f899..be3510940 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) @@ -375,9 +395,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 +412,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 > cpConfigFilesMaxBytes { + return fmt.Errorf("template assets exceed %d bytes", cpConfigFilesMaxBytes) + } + assets[name] = append([]byte(nil), data...) + return nil + } if cfg.TemplatePath != "" { // Reject symlinks on the root itself — WalkDir follows symlinks, @@ -397,10 +443,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,12 +484,12 @@ 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 } } @@ -452,43 +498,42 @@ func collectCPConfigFiles(cfg WorkspaceConfig) (map[string]string, error) { // 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 merge into - // the bundle. Caller's cfg.ConfigFiles win on conflict - // (the merge happens BEFORE this loop, so any caller - // overrides are preserved). The fetch is OPT-IN: nil - // fetcher = no-op (self-host default; falls through to - // the local TemplatePath path above). + // 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). // - // CRITICAL preserve/blast-radius axis (PM-flagged): - // this fetcher ONLY materializes template assets (config + - // prompts + skills). It does NOT touch agent-owned state - // (memory, ~/.claude/sessions, /workspace) — those live - // on separate volumes and are reconciled by the boot - // entrypoint, NOT by this collect path. 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). + // 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 != "" { - assets, fetchErr := cfg.TemplateAssetFetcher.Load(context.Background(), cfg.TemplateIdentity) + fetchedAssets, fetchErr := cfg.TemplateAssetFetcher.Load(context.Background(), cfg.TemplateIdentity) if fetchErr != nil { - return nil, fmt.Errorf("collectCPConfigFiles: fetch template assets (RFC #2843 #24): %w", fetchErr) + return nil, nil, fmt.Errorf("collectCPConfigFiles: fetch template assets (RFC #2843 #24): %w", fetchErr) } - for name, data := range assets { - if err := addFile(name, data); err != nil { - return nil, err + for name, data := range fetchedAssets { + if err := addAsset(name, data); err != nil { + return nil, nil, err } } } - for name, data := range cfg.ConfigFiles { - if err := addFile(name, data); err != nil { - return nil, err - } + if len(files) == 0 && len(assets) == 0 { + return nil, nil, nil } - if len(files) == 0 { - return nil, nil - } - return files, 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/template_assets.go b/workspace-server/internal/provisioner/template_assets.go index 07333fc0d..0cb298fb0 100644 --- a/workspace-server/internal/provisioner/template_assets.go +++ b/workspace-server/internal/provisioner/template_assets.go @@ -4,60 +4,102 @@ package provisioner // // 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. +// 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. +// 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. // -// 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. +// 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" +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 caller can -// re-encode if needed for the SM wire format; the generic -// channel does not require encoding). +// "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) } -// defaultTemplateAssetFetcher is the in-tree fallback used -// when no provider is wired (self-host default — the -// Docker provisioner reads from a local TemplatePath -// directory; SaaS wires a Gitea fetcher in main.go). -// Currently this is a stub that returns nil — the existing -// TemplatePath path in collectCPConfigFiles continues to -// handle the local-dir case. Wire a real impl in #24's -// follow-ups (the SaaS-side Gitea fetch). -type defaultTemplateAssetFetcher struct{} - -// Load on defaultTemplateAssetFetcher returns (nil, nil) — -// "no assets to add." The existing collectCPConfigFiles -// TemplatePath walk handles the local-dir case for self-host. -// SaaS workspaces wire a real fetcher (Gitea shallow clone) -// in main.go per RFC #2843 §4.2 transport option (a). -func (defaultTemplateAssetFetcher) Load(_ context.Context, _ string) (map[string][]byte, error) { - return nil, nil +// 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 index cf22a4f95..7bcedcea6 100644 --- a/workspace-server/internal/provisioner/template_assets_test.go +++ b/workspace-server/internal/provisioner/template_assets_test.go @@ -4,14 +4,21 @@ package provisioner // (RFC #2843 #24). Verifies: // 1. The fetcher wires into collectCPConfigFiles when both // cfg.TemplateAssetFetcher and cfg.TemplateIdentity are set. -// 2. Fetched assets MERGE with cfg.ConfigFiles (caller wins -// on conflict — same pattern as the persisted-bundle -// provider in #2831 PIECE 1). -// 3. A transport error on the fetcher ABORTS the provision +// 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). -// 4. Nil fetcher = no-op (self-host default; the existing +// 5. Nil fetcher = no-op (self-host default; the existing // TemplatePath local-dir path still works). -// 5. Empty TemplateIdentity with a non-nil fetcher = no-op +// 6. Empty TemplateIdentity with a non-nil fetcher = no-op // (the fetcher is only called when there's an identity // to resolve). @@ -19,6 +26,7 @@ import ( "context" "encoding/base64" "errors" + "runtime" "strings" "testing" ) @@ -38,15 +46,17 @@ func (f *fakeTemplateAssetFetcher) Load(_ context.Context, templateIdentity stri } // TestCollectCPConfigFiles_MergesFetcherAssets is the -// happy path: fetcher returns assets, the bundle includes -// them alongside any caller-supplied cfg.ConfigFiles. -// Fails if a future refactor stops calling cfg.TemplateAssetFetcher.Load -// from collectCPConfigFiles when the fetcher + identity are set. +// 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"), + "config.yaml": []byte("# from template repo"), + "prompts/system.md": []byte("# template system prompt"), "agent-skills/seo-audit/SKILL.md": []byte("# seo skill"), }, } @@ -55,19 +65,24 @@ func TestCollectCPConfigFiles_MergesFetcherAssets(t *testing.T) { TemplateAssetFetcher: prov, } - files, err := collectCPConfigFiles(cfg) + files, assets, err := collectCPConfigFiles(cfg) if err != nil { t.Fatalf("collectCPConfigFiles: %v", err) } - // All 3 fetched assets land in the bundle. + // 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 := files[wk]; !ok { - t.Errorf("expected %q in bundle, got keys: %v", wk, keysOfBundle(files)) + 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. @@ -76,12 +91,14 @@ func TestCollectCPConfigFiles_MergesFetcherAssets(t *testing.T) { } } -// TestCollectCPConfigFiles_CallerWinsOnConflict asserts the -// no-clobber policy: when the caller supplies a -// cfg.ConfigFiles["config.yaml"] AND the fetcher returns the -// same key, the caller's value wins (matches the persisted- -// bundle provider pattern in #2831 PIECE 1). -func TestCollectCPConfigFiles_CallerWinsOnConflict(t *testing.T) { +// 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"), @@ -95,17 +112,23 @@ func TestCollectCPConfigFiles_CallerWinsOnConflict(t *testing.T) { }, } - files, err := collectCPConfigFiles(cfg) + files, assets, err := collectCPConfigFiles(cfg) if err != nil { t.Fatalf("collectCPConfigFiles: %v", err) } - // Decode the base64-encoded bundle value. - decoded, decErr := base64DecodeFirst(files["config.yaml"]) + // 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: %v", decErr) + t.Fatalf("decode config.yaml from ConfigFiles: %v", decErr) } if string(decoded) != "# caller override" { - t.Errorf("expected caller override to win, got bundle config.yaml=%q", string(decoded)) + 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) } } @@ -125,7 +148,7 @@ func TestCollectCPConfigFiles_FetcherErrorAborts(t *testing.T) { TemplateAssetFetcher: prov, } - _, err := collectCPConfigFiles(cfg) + _, _, err := collectCPConfigFiles(cfg) if err == nil { t.Fatal("expected collectCPConfigFiles to abort on fetcher error, got nil") } @@ -144,7 +167,7 @@ func TestCollectCPConfigFiles_NilFetcherIsNoop(t *testing.T) { cfg := WorkspaceConfig{ TemplateIdentity: "seo-agent-v1.2.3", // set but no fetcher wired } - _, err := collectCPConfigFiles(cfg) + _, _, err := collectCPConfigFiles(cfg) if err != nil { t.Fatalf("collectCPConfigFiles with nil fetcher: %v", err) } @@ -163,7 +186,7 @@ func TestCollectCPConfigFiles_EmptyIdentityNoop(t *testing.T) { TemplateIdentity: "", // empty TemplateAssetFetcher: prov, // wired but no identity } - _, err := collectCPConfigFiles(cfg) + _, _, err := collectCPConfigFiles(cfg) if err != nil { t.Fatalf("collectCPConfigFiles with empty identity: %v", err) } @@ -172,22 +195,276 @@ func TestCollectCPConfigFiles_EmptyIdentityNoop(t *testing.T) { } } -// base64DecodeFirst is a small helper that decodes the -// first base64-encoded value in the bundle, returning the -// raw bytes. Test-side helper to avoid pulling base64 -// decoding into the production path. -func base64DecodeFirst(v string) ([]byte, error) { - decoded, err := base64.StdEncoding.DecodeString(v) - if err != nil { - return nil, err +// --- 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") } - return decoded, nil } -// keysOfBundle returns the sorted keys of a map for stable -// test output. Local helper to avoid coupling to the test -// files' key-ordering conventions. -func keysOfBundle(m map[string]string) []string { +// 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) -- 2.52.0 From c822e55d37c00f1343af1072c987bdac7e1d5c9e Mon Sep 17 00:00:00 2001 From: Claude Fable 5 Date: Sun, 14 Jun 2026 14:46:12 +0000 Subject: [PATCH 3/4] =?UTF-8?q?fix(#2845):=20give=20TemplateAssets=20its?= =?UTF-8?q?=20own=2016MiB=20bound,=20not=20the=20256KiB=20SM=20cap=20(CR2?= =?UTF-8?q?=20RC)=20=E2=80=94=20skill=20packages=20no=20longer=20drop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../internal/provisioner/cp_provisioner.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/workspace-server/internal/provisioner/cp_provisioner.go b/workspace-server/internal/provisioner/cp_provisioner.go index be3510940..25cce29b0 100644 --- a/workspace-server/internal/provisioner/cp_provisioner.go +++ b/workspace-server/internal/provisioner/cp_provisioner.go @@ -385,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; @@ -430,8 +442,8 @@ func collectCPConfigFiles(cfg WorkspaceConfig) (map[string]string, map[string][] 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 > cpConfigFilesMaxBytes { - return fmt.Errorf("template assets exceed %d bytes", cpConfigFilesMaxBytes) + if totalAssets > cpTemplateAssetsMaxBytes { + return fmt.Errorf("template assets exceed %d bytes", cpTemplateAssetsMaxBytes) } assets[name] = append([]byte(nil), data...) return nil -- 2.52.0 From acbc0da95ed8e015a1eb8835761d14d892567483 Mon Sep 17 00:00:00 2001 From: Claude Fable 5 Date: Sun, 14 Jun 2026 14:46:14 +0000 Subject: [PATCH 4/4] test(#2845): 716KiB skill asset succeeds on TemplateAssets; ConfigFiles still capped at 256KiB (CR2 RC) --- .../provisioner/template_assets_test.go | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/workspace-server/internal/provisioner/template_assets_test.go b/workspace-server/internal/provisioner/template_assets_test.go index 7bcedcea6..950f9ff4e 100644 --- a/workspace-server/internal/provisioner/template_assets_test.go +++ b/workspace-server/internal/provisioner/template_assets_test.go @@ -479,3 +479,52 @@ func keysOfAssetMap(m map[string][]byte) []string { 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)") + } +} -- 2.52.0