diff --git a/workspace-server/internal/handlers/approvals.go b/workspace-server/internal/handlers/approvals.go index 1f091afa..dcce896d 100644 --- a/workspace-server/internal/handlers/approvals.go +++ b/workspace-server/internal/handlers/approvals.go @@ -116,6 +116,9 @@ func (h *ApprovalsHandler) ListAll(c *gin.Context) { "created_at": createdAt, }) } + if err := rows.Err(); err != nil { + log.Printf("ListPendingApprovals rows.Err: %v", err) + } c.JSON(http.StatusOK, approvals) } @@ -155,6 +158,9 @@ func (h *ApprovalsHandler) List(c *gin.Context) { "created_at": createdAt, }) } + if err := rows.Err(); err != nil { + log.Printf("ListApprovals rows.Err workspace=%s: %v", workspaceID, err) + } c.JSON(http.StatusOK, approvals) } diff --git a/workspace-server/internal/handlers/instructions.go b/workspace-server/internal/handlers/instructions.go index 2e8e89ac..6f53421b 100644 --- a/workspace-server/internal/handlers/instructions.go +++ b/workspace-server/internal/handlers/instructions.go @@ -248,6 +248,9 @@ func (h *InstructionsHandler) Resolve(c *gin.Context) { b.WriteString(content) b.WriteString("\n\n") } + if err := rows.Err(); err != nil { + log.Printf("ResolveInstructions rows.Err workspace=%s: %v", workspaceID, err) + } c.JSON(http.StatusOK, gin.H{ "workspace_id": workspaceID, @@ -258,6 +261,7 @@ func (h *InstructionsHandler) Resolve(c *gin.Context) { func scanInstructions(rows interface { Next() bool Scan(dest ...interface{}) error + Err() error }) []Instruction { var instructions []Instruction for rows.Next() { @@ -269,6 +273,9 @@ func scanInstructions(rows interface { } instructions = append(instructions, inst) } + if err := rows.Err(); err != nil { + log.Printf("scanInstructions rows.Err: %v", err) + } if instructions == nil { instructions = []Instruction{} } diff --git a/workspace-server/internal/handlers/tokens.go b/workspace-server/internal/handlers/tokens.go index e63eff29..c41f8c51 100644 --- a/workspace-server/internal/handlers/tokens.go +++ b/workspace-server/internal/handlers/tokens.go @@ -67,6 +67,9 @@ func (h *TokenHandler) List(c *gin.Context) { } tokens = append(tokens, t) } + if err := rows.Err(); err != nil { + log.Printf("ListTokens rows.Err workspace=%s: %v", workspaceID, err) + } c.JSON(http.StatusOK, gin.H{ "tokens": tokens, diff --git a/workspace-server/internal/handlers/workspace_provision.go b/workspace-server/internal/handlers/workspace_provision.go index 7900945c..821b313b 100644 --- a/workspace-server/internal/handlers/workspace_provision.go +++ b/workspace-server/internal/handlers/workspace_provision.go @@ -805,6 +805,9 @@ func loadWorkspaceSecrets(ctx context.Context, workspaceID string) (map[string]s envVars[k] = string(decrypted) } } + if err := globalRows.Err(); err != nil { + log.Printf("Provisioner: global_secrets rows.Err workspace=%s: %v", workspaceID, err) + } } wsRows, err := db.DB.QueryContext(ctx, `SELECT key, encrypted_value, encryption_version FROM workspace_secrets WHERE workspace_id = $1`, workspaceID) @@ -823,6 +826,9 @@ func loadWorkspaceSecrets(ctx context.Context, workspaceID string) (map[string]s envVars[k] = string(decrypted) } } + if err := wsRows.Err(); err != nil { + log.Printf("Provisioner: workspace_secrets rows.Err workspace=%s: %v", workspaceID, err) + } } return envVars, "" } diff --git a/workspace-server/internal/provisioner/cp_provisioner.go b/workspace-server/internal/provisioner/cp_provisioner.go index 4b3786a8..dfd1afe5 100644 --- a/workspace-server/internal/provisioner/cp_provisioner.go +++ b/workspace-server/internal/provisioner/cp_provisioner.go @@ -4,12 +4,14 @@ import ( "bytes" "context" "database/sql" + "encoding/base64" "encoding/json" "fmt" "io" "log" "net/http" "os" + "path/filepath" "strings" "time" @@ -156,6 +158,11 @@ type cpProvisionRequest struct { Tier int `json:"tier"` PlatformURL string `json:"platform_url"` Env map[string]string `json:"env"` + // ConfigFiles are template + generated config files to write into the + // 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 map[string]string `json:"config_files,omitempty"` } type cpProvisionResponse struct { @@ -179,6 +186,16 @@ func (p *CPProvisioner) Start(ctx context.Context, cfg WorkspaceConfig) (string, } env["ADMIN_TOKEN"] = p.adminToken } + // Collect template files and generated configs, with OFFSEC-010 guards: + // - Rejects symlinks at the template root (prevents bypass via symlink traversal) + // - Skips symlinks during WalkDir (prevents /etc/passwd etc. inclusion) + // - Validates all paths are relative and non-escaping + // - Caps total size at 12 KiB to prevent payload bloat + configFiles, err := collectCPConfigFiles(cfg) + if err != nil { + return "", fmt.Errorf("cp provisioner: collect config files: %w", err) + } + req := cpProvisionRequest{ OrgID: p.orgID, WorkspaceID: cfg.WorkspaceID, @@ -186,6 +203,7 @@ func (p *CPProvisioner) Start(ctx context.Context, cfg WorkspaceConfig) (string, Tier: cfg.Tier, PlatformURL: cfg.PlatformURL, Env: env, + ConfigFiles: configFiles, } body, err := json.Marshal(req) @@ -237,6 +255,94 @@ func (p *CPProvisioner) Start(ctx context.Context, cfg WorkspaceConfig) (string, return result.InstanceID, nil } +const cpConfigFilesMaxBytes = 12 << 10 + +// 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; +// shipping arbitrary files (e.g. adapter.py, Dockerfile) is both unnecessary +// and a potential data-exfiltration surface. +func isCPTemplateConfigFile(name string) bool { + name = filepath.ToSlash(filepath.Clean(name)) + return name == "config.yaml" || strings.HasPrefix(name, "prompts/") +} + +func collectCPConfigFiles(cfg WorkspaceConfig) (map[string]string, error) { + files := make(map[string]string) + total := 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, "/../") { + return fmt.Errorf("invalid config file path %q", name) + } + total += len(data) + if total > cpConfigFilesMaxBytes { + return fmt.Errorf("config files exceed %d bytes", cpConfigFilesMaxBytes) + } + files[name] = base64.StdEncoding.EncodeToString(data) + return nil + } + + if cfg.TemplatePath != "" { + // Reject symlinks on the root itself — WalkDir follows symlinks, + // so a symlink TemplatePath that escapes the intended root directory + // 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) + } + if rootInfo.Mode()&os.ModeSymlink != 0 { + return 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 { + return walkErr + } + // Skip symlinks — WalkDir follows them by default, which means + // a symlink inside the template dir pointing to /etc/passwd + // would be traversed even though the resulting relative-path + // check would correctly reject it. Defense-in-depth: don't + // follow symlinks at all. (OFFSEC-010) + if d.Type()&os.ModeSymlink != 0 { + return nil + } + if d.IsDir() { + return nil + } + info, err := d.Info() + if err != nil { + return err + } + if !info.Mode().IsRegular() { + return nil + } + rel, err := filepath.Rel(cfg.TemplatePath, path) + if err != nil { + return err + } + if !isCPTemplateConfigFile(rel) { + return nil + } + data, err := os.ReadFile(path) + if err != nil { + return err + } + return addFile(rel, data) + }) + if 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 + } + return files, nil +} // Stop terminates the workspace's EC2 instance via the control plane. // // Looks up the actual EC2 instance_id from the workspaces table before diff --git a/workspace-server/internal/provisioner/cp_provisioner_test.go b/workspace-server/internal/provisioner/cp_provisioner_test.go index 4d8a6795..cde5ea1c 100644 --- a/workspace-server/internal/provisioner/cp_provisioner_test.go +++ b/workspace-server/internal/provisioner/cp_provisioner_test.go @@ -1,11 +1,15 @@ package provisioner import ( + "bytes" "context" + "encoding/base64" "encoding/json" "io" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "testing" "time" @@ -279,6 +283,105 @@ func TestStart_TransportFailureSurfaces(t *testing.T) { } } +// TestStart_CollectsConfigFiles — verify that collectCPConfigFiles is called and +// its result is included in the cpProvisionRequest sent to the control plane. +// Tests the OFFSEC-010 wiring: the function's symlink guards are only effective +// if the call site actually invokes it. +func TestStart_CollectsConfigFiles(t *testing.T) { + tmpl := t.TempDir() + if err := os.WriteFile(filepath.Join(tmpl, "config.yaml"), []byte("name: test\n"), 0o600); err != nil { + t.Fatal(err) + } + // adapter.py is within the size limit but is NOT config.yaml or prompts/, + // so isCPTemplateConfigFile must exclude it from the transport. + if err := os.WriteFile(filepath.Join(tmpl, "adapter.py"), bytes.Repeat([]byte("x"), cpConfigFilesMaxBytes), 0o600); err != nil { + t.Fatal(err) + } + + 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: "python", + Tier: 1, + PlatformURL: "http://tenant", + TemplatePath: tmpl, + ConfigFiles: map[string][]byte{"generated.json": []byte(`{"key":"value"}`)}, + }) + if err != nil { + t.Fatalf("Start: %v", err) + } + + // config.yaml from TemplatePath must be base64-encoded in ConfigFiles + if len(gotBody.ConfigFiles) == 0 { + t.Fatal("ConfigFiles is empty: collectCPConfigFiles was not called") + } + + // Find config.yaml entry and verify it's valid base64 + correct content + var foundTemplate, foundGenerated bool + for name, encoded := range gotBody.ConfigFiles { + decoded, err := base64.StdEncoding.DecodeString(encoded) + if err != nil { + t.Errorf("ConfigFiles[%q] is not valid base64: %v", name, err) + continue + } + if name == "config.yaml" && string(decoded) == "name: test\n" { + foundTemplate = true + } + if name == "generated.json" && string(decoded) == `{"key":"value"}` { + foundGenerated = true + } + } + if !foundTemplate { + t.Errorf("ConfigFiles missing config.yaml from TemplatePath") + } + if !foundGenerated { + t.Errorf("ConfigFiles missing generated.json from ConfigFiles") + } + // adapter.py must NOT be in ConfigFiles — isCPTemplateConfigFile filters it out + for name := range gotBody.ConfigFiles { + if name == "adapter.py" { + t.Errorf("adapter.py should not be in ConfigFiles — isCPTemplateConfigFile must filter it out") + } + } +} + +// 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. +func TestStart_SymlinkTemplatePathError(t *testing.T) { + // Create a temp file and a symlink pointing to it + tmp := t.TempDir() + realFile := filepath.Join(tmp, "real") + if err := os.WriteFile(realFile, []byte("data"), 0o600); err != nil { + t.Fatal(err) + } + symlink := filepath.Join(tmp, "template_link") + if err := os.Symlink(realFile, symlink); err != nil { + t.Fatal(err) + } + + p := &CPProvisioner{baseURL: "http://unused", orgID: "org-1", httpClient: &http.Client{Timeout: time.Second}} + _, err := p.Start(context.Background(), WorkspaceConfig{ + WorkspaceID: "ws-1", + Runtime: "python", + TemplatePath: symlink, // symlink root → OFFSEC-010 guard should fire + }) + if err == nil { + t.Fatal("expected error for symlink TemplatePath, got nil") + } + if !strings.Contains(err.Error(), "symlink") { + t.Errorf("error should mention symlink, got %q", err.Error()) + } +} + // TestStop_SendsBothAuthHeaders — verify #118/#130 compliance on the // teardown path. Any call to /cp/workspaces/:id must carry both the // platform-wide shared secret AND the per-tenant admin token, or the @@ -842,3 +945,67 @@ func TestIsRunning_EmptyInstanceIDReturnsFalse(t *testing.T) { t.Errorf("IsRunning with empty instance_id should return running=false, got true") } } + +// TestCollectCPConfigFiles_SkipsSymlinks — WalkDir follows symlinks by default, +// but collectCPConfigFiles must skip them so a symlink inside a template dir +// pointing outside (e.g. ln -s /etc snapshot) cannot be traversed. +// Verifies OFFSEC-010 defense-in-depth fix. (OFFSEC-010) +func TestCollectCPConfigFiles_SkipsSymlinks(t *testing.T) { + tmpl := t.TempDir() + // Write a real file that should be included. + if err := os.WriteFile(filepath.Join(tmpl, "config.yaml"), []byte("name: real\n"), 0o600); err != nil { + t.Fatal(err) + } + // Create a subdir with a file that will be symlinked-outside. + sensitiveDir := t.TempDir() + if err := os.WriteFile(filepath.Join(sensitiveDir, "secret.txt"), []byte("SENSITIVE\n"), 0o600); err != nil { + t.Fatal(err) + } + // Symlink inside template dir pointing to outside path. + symlinkPath := filepath.Join(tmpl, "snapshot") + if err := os.Symlink(sensitiveDir, symlinkPath); err != nil { + t.Fatal(err) + } + + files, err := collectCPConfigFiles(WorkspaceConfig{TemplatePath: tmpl}) + if err != nil { + t.Fatalf("collectCPConfigFiles: %v", err) + } + if files == nil { + t.Fatal("files should not be nil") + } + // config.yaml must be present. + if _, ok := files["config.yaml"]; !ok { + t.Errorf("config.yaml missing from files") + } + // The symlinked path must NOT be included (even though WalkDir would + // traverse it, the d.Type()&os.ModeSymlink guard skips the entry). + for k := range files { + if strings.Contains(k, "snapshot") || strings.Contains(k, "secret") { + t.Errorf("symlink path %q should not be in files — OFFSEC-010 regression", k) + } + } +} + +// TestCollectCPConfigFiles_RejectsRootSymlink — if cfg.TemplatePath itself is +// a symlink, WalkDir would follow it to an arbitrary directory, bypassing the +// cfg.TemplatePath boundary. The function must reject this case explicitly. +// (OFFSEC-010) +func TestCollectCPConfigFiles_RejectsRootSymlink(t *testing.T) { + real := t.TempDir() + if err := os.WriteFile(filepath.Join(real, "config.yaml"), []byte("name: real\n"), 0o600); err != nil { + t.Fatal(err) + } + link := filepath.Join(t.TempDir(), "template-link") + if err := os.Symlink(real, link); err != nil { + t.Fatal(err) + } + + _, err := collectCPConfigFiles(WorkspaceConfig{TemplatePath: link}) + if err == nil { + t.Error("collectCPConfigFiles with symlink TemplatePath should return error") + } + if err != nil && !strings.Contains(err.Error(), "symlink") { + t.Errorf("expected symlink-related error, got: %v", err) + } +}