feat(security): unify workspace_root boundary enforcement across CLI and gateway
Promote workspace_root from channel-specific (magicform) to agents.defaults so both CLI --workspace/--config-dir and gateway metadata overrides are validated against the same immutable boundary. Paths must be relative subdirectories — absolute paths, .., and empty/dot are rejected. The boundary is snapshotted before overlay merges and cannot be widened by workspace config files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
12cab29449
commit
b7872e5423
9 changed files with 591 additions and 42 deletions
|
|
@ -16,6 +16,7 @@ import (
|
||||||
"github.com/sipeed/picoclaw/pkg/bus"
|
"github.com/sipeed/picoclaw/pkg/bus"
|
||||||
"github.com/sipeed/picoclaw/pkg/config"
|
"github.com/sipeed/picoclaw/pkg/config"
|
||||||
"github.com/sipeed/picoclaw/pkg/logger"
|
"github.com/sipeed/picoclaw/pkg/logger"
|
||||||
|
"github.com/sipeed/picoclaw/pkg/pathutil"
|
||||||
"github.com/sipeed/picoclaw/pkg/providers"
|
"github.com/sipeed/picoclaw/pkg/providers"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
@ -38,13 +39,27 @@ func agentCmd(message, sessionKey, model string, debug bool,
|
||||||
return fmt.Errorf("error loading config: %w", err)
|
return fmt.Errorf("error loading config: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Apply workspace-local config overrides from config-dir
|
// Snapshot workspace_root from the base config before any overlay can touch it.
|
||||||
|
workspaceRoot := cfg.Agents.Defaults.WorkspaceRoot
|
||||||
|
|
||||||
|
// Validate and resolve --workspace and --config-dir against workspace_root.
|
||||||
|
var resolveErr error
|
||||||
|
workspace, configDir, resolveErr = validateWorkspacePaths(workspaceRoot, workspace, configDir)
|
||||||
|
if resolveErr != nil {
|
||||||
|
return resolveErr
|
||||||
|
}
|
||||||
|
|
||||||
|
// Apply workspace-local config overrides from config-dir.
|
||||||
|
// mergeAgentDefaults will re-validate any workspace field from the overlay
|
||||||
|
// against the snapshotted workspace_root.
|
||||||
if configDir != "" {
|
if configDir != "" {
|
||||||
wc, wcErr := config.LoadWorkspaceConfig(configDir)
|
wc, wcErr := config.LoadWorkspaceConfig(configDir)
|
||||||
if wcErr != nil {
|
if wcErr != nil {
|
||||||
return fmt.Errorf("error loading workspace config from %s: %w", configDir, wcErr)
|
return fmt.Errorf("error loading workspace config from %s: %w", configDir, wcErr)
|
||||||
}
|
}
|
||||||
cfg.MergeWorkspaceConfig(wc)
|
if mergeErr := cfg.MergeWorkspaceConfig(wc); mergeErr != nil {
|
||||||
|
return fmt.Errorf("error merging workspace config from %s: %w", configDir, mergeErr)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// CLI flags win over workspace config
|
// CLI flags win over workspace config
|
||||||
|
|
@ -52,7 +67,7 @@ func agentCmd(message, sessionKey, model string, debug bool,
|
||||||
cfg.Agents.Defaults.ModelName = model
|
cfg.Agents.Defaults.ModelName = model
|
||||||
}
|
}
|
||||||
|
|
||||||
// Workspace override
|
// Workspace override (already validated above)
|
||||||
if workspace != "" {
|
if workspace != "" {
|
||||||
cfg.Agents.Defaults.Workspace = workspace
|
cfg.Agents.Defaults.Workspace = workspace
|
||||||
os.MkdirAll(workspace, 0o755)
|
os.MkdirAll(workspace, 0o755)
|
||||||
|
|
@ -162,6 +177,27 @@ func applySkillsFilter(cfg *config.Config, skills []string) {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// validateWorkspacePaths resolves --workspace and --config-dir against workspace_root.
|
||||||
|
// Both must be relative subdirectories of workspace_root. Returns the resolved
|
||||||
|
// absolute paths or an error if validation fails.
|
||||||
|
func validateWorkspacePaths(workspaceRoot, workspace, configDir string) (string, string, error) {
|
||||||
|
if workspace != "" {
|
||||||
|
resolved, err := pathutil.ResolveWorkspacePath(workspaceRoot, workspace)
|
||||||
|
if err != nil {
|
||||||
|
return "", "", fmt.Errorf("invalid --workspace: %w", err)
|
||||||
|
}
|
||||||
|
workspace = resolved
|
||||||
|
}
|
||||||
|
if configDir != "" {
|
||||||
|
resolved, err := pathutil.ResolveWorkspacePath(workspaceRoot, configDir)
|
||||||
|
if err != nil {
|
||||||
|
return "", "", fmt.Errorf("invalid --config-dir: %w", err)
|
||||||
|
}
|
||||||
|
configDir = resolved
|
||||||
|
}
|
||||||
|
return workspace, configDir, nil
|
||||||
|
}
|
||||||
|
|
||||||
// copyBootstrapFiles copies recognized bootstrap files (AGENTS.md, IDENTITY.md,
|
// copyBootstrapFiles copies recognized bootstrap files (AGENTS.md, IDENTITY.md,
|
||||||
// SOUL.md, USER.md) from srcDir into the workspace directory.
|
// SOUL.md, USER.md) from srcDir into the workspace directory.
|
||||||
func copyBootstrapFiles(srcDir, workspace string) {
|
func copyBootstrapFiles(srcDir, workspace string) {
|
||||||
|
|
|
||||||
140
cmd/picoclaw/internal/agent/helpers_test.go
Normal file
140
cmd/picoclaw/internal/agent/helpers_test.go
Normal file
|
|
@ -0,0 +1,140 @@
|
||||||
|
package agent
|
||||||
|
|
||||||
|
import (
|
||||||
|
"path/filepath"
|
||||||
|
"strings"
|
||||||
|
"testing"
|
||||||
|
|
||||||
|
"github.com/stretchr/testify/assert"
|
||||||
|
"github.com/stretchr/testify/require"
|
||||||
|
)
|
||||||
|
|
||||||
|
func mustAbs(t *testing.T, path string) string {
|
||||||
|
t.Helper()
|
||||||
|
abs, err := filepath.Abs(path)
|
||||||
|
require.NoError(t, err)
|
||||||
|
return abs
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestValidateWorkspacePaths(t *testing.T) {
|
||||||
|
root := filepath.Join(t.TempDir(), "workspaces")
|
||||||
|
|
||||||
|
absRoot, err := filepath.Abs(root)
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
root string
|
||||||
|
workspace string
|
||||||
|
configDir string
|
||||||
|
wantWS string // expected resolved workspace; empty if error expected
|
||||||
|
wantCD string // expected resolved configDir; empty if error expected
|
||||||
|
wantErr string // substring of expected error; empty means success
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "valid workspace subdirectory",
|
||||||
|
root: root,
|
||||||
|
workspace: "tenant1",
|
||||||
|
wantWS: filepath.Join(absRoot, "tenant1"),
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "valid config-dir subdirectory",
|
||||||
|
root: root,
|
||||||
|
configDir: "tenant1/config",
|
||||||
|
wantCD: filepath.Join(absRoot, "tenant1", "config"),
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "both valid",
|
||||||
|
root: root,
|
||||||
|
workspace: "tenant1",
|
||||||
|
configDir: "tenant1/config",
|
||||||
|
wantWS: filepath.Join(absRoot, "tenant1"),
|
||||||
|
wantCD: filepath.Join(absRoot, "tenant1", "config"),
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "empty flags are fine",
|
||||||
|
root: root,
|
||||||
|
workspace: "",
|
||||||
|
configDir: "",
|
||||||
|
},
|
||||||
|
|
||||||
|
// --- workspace traversal ---
|
||||||
|
{
|
||||||
|
name: "workspace traversal rejected",
|
||||||
|
root: root,
|
||||||
|
workspace: "../escape",
|
||||||
|
wantErr: "invalid --workspace",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "workspace bare dotdot rejected",
|
||||||
|
root: root,
|
||||||
|
workspace: "..",
|
||||||
|
wantErr: "invalid --workspace",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "workspace mid-path traversal rejected",
|
||||||
|
root: root,
|
||||||
|
workspace: "a/../../../etc",
|
||||||
|
wantErr: "invalid --workspace",
|
||||||
|
},
|
||||||
|
|
||||||
|
// --- config-dir traversal ---
|
||||||
|
{
|
||||||
|
name: "config-dir traversal rejected",
|
||||||
|
root: root,
|
||||||
|
configDir: "../escape",
|
||||||
|
wantErr: "invalid --config-dir",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "config-dir absolute path rejected",
|
||||||
|
root: root,
|
||||||
|
configDir: "/etc/passwd",
|
||||||
|
wantErr: "invalid --config-dir",
|
||||||
|
},
|
||||||
|
|
||||||
|
// --- no workspace_root configured (backward compat: falls back to filepath.Abs) ---
|
||||||
|
{
|
||||||
|
name: "workspace without root uses Abs fallback",
|
||||||
|
root: "",
|
||||||
|
workspace: "anything",
|
||||||
|
wantWS: mustAbs(t, "anything"),
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "config-dir without root uses Abs fallback",
|
||||||
|
root: "",
|
||||||
|
configDir: "anything",
|
||||||
|
wantCD: mustAbs(t, "anything"),
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "workspace traversal without root still rejected",
|
||||||
|
root: "",
|
||||||
|
workspace: "../escape",
|
||||||
|
wantErr: "invalid --workspace",
|
||||||
|
},
|
||||||
|
|
||||||
|
// --- workspace resolves to root itself ---
|
||||||
|
{
|
||||||
|
name: "workspace dot rejected",
|
||||||
|
root: root,
|
||||||
|
workspace: ".",
|
||||||
|
wantErr: "invalid --workspace",
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
ws, cd, err := validateWorkspacePaths(tt.root, tt.workspace, tt.configDir)
|
||||||
|
|
||||||
|
if tt.wantErr != "" {
|
||||||
|
require.Error(t, err)
|
||||||
|
assert.True(t, strings.Contains(err.Error(), tt.wantErr),
|
||||||
|
"error %q should contain %q", err.Error(), tt.wantErr)
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
require.NoError(t, err)
|
||||||
|
assert.Equal(t, tt.wantWS, ws)
|
||||||
|
assert.Equal(t, tt.wantCD, cd)
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -1,6 +1,7 @@
|
||||||
{
|
{
|
||||||
"agents": {
|
"agents": {
|
||||||
"defaults": {
|
"defaults": {
|
||||||
|
"workspace_root": "/data/workspaces",
|
||||||
"workspace": "~/.picoclaw/workspace",
|
"workspace": "~/.picoclaw/workspace",
|
||||||
"restrict_to_workspace": true,
|
"restrict_to_workspace": true,
|
||||||
"model_name": "gpt4",
|
"model_name": "gpt4",
|
||||||
|
|
|
||||||
|
|
@ -21,6 +21,7 @@ import (
|
||||||
"unicode/utf8"
|
"unicode/utf8"
|
||||||
|
|
||||||
"github.com/sipeed/picoclaw/pkg/bus"
|
"github.com/sipeed/picoclaw/pkg/bus"
|
||||||
|
"github.com/sipeed/picoclaw/pkg/pathutil"
|
||||||
"github.com/sipeed/picoclaw/pkg/channels"
|
"github.com/sipeed/picoclaw/pkg/channels"
|
||||||
"github.com/sipeed/picoclaw/pkg/commands"
|
"github.com/sipeed/picoclaw/pkg/commands"
|
||||||
"github.com/sipeed/picoclaw/pkg/config"
|
"github.com/sipeed/picoclaw/pkg/config"
|
||||||
|
|
@ -637,6 +638,28 @@ func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage)
|
||||||
workspaceOverride := msg.Metadata["workspace_override"]
|
workspaceOverride := msg.Metadata["workspace_override"]
|
||||||
configDir := msg.Metadata["config_dir"]
|
configDir := msg.Metadata["config_dir"]
|
||||||
|
|
||||||
|
// Defense-in-depth: validate workspace/configDir overrides against workspace_root
|
||||||
|
// even though the originating channel should have already validated them.
|
||||||
|
wsRoot := al.cfg.Agents.Defaults.WorkspaceRoot
|
||||||
|
if workspaceOverride != "" {
|
||||||
|
resolved, err := pathutil.ResolveWorkspacePath(wsRoot, workspaceOverride)
|
||||||
|
if err != nil {
|
||||||
|
logger.WarnCF("agent", "Rejecting workspace_override from metadata",
|
||||||
|
map[string]any{"workspace_override": workspaceOverride, "error": err.Error()})
|
||||||
|
return "", fmt.Errorf("invalid workspace_override in metadata: %w", err)
|
||||||
|
}
|
||||||
|
workspaceOverride = resolved
|
||||||
|
}
|
||||||
|
if configDir != "" {
|
||||||
|
resolved, err := pathutil.ResolveWorkspacePath(wsRoot, configDir)
|
||||||
|
if err != nil {
|
||||||
|
logger.WarnCF("agent", "Rejecting config_dir from metadata",
|
||||||
|
map[string]any{"config_dir": configDir, "error": err.Error()})
|
||||||
|
return "", fmt.Errorf("invalid config_dir in metadata: %w", err)
|
||||||
|
}
|
||||||
|
configDir = resolved
|
||||||
|
}
|
||||||
|
|
||||||
var allowedTools, allowedSkills []string
|
var allowedTools, allowedSkills []string
|
||||||
if v := msg.Metadata["allowed_tools"]; v != "" {
|
if v := msg.Metadata["allowed_tools"]; v != "" {
|
||||||
for _, t := range strings.Split(v, ",") {
|
for _, t := range strings.Split(v, ",") {
|
||||||
|
|
@ -812,7 +835,9 @@ func (al *AgentLoop) runAgentLoop(
|
||||||
map[string]any{"path": configSource, "error": err.Error()})
|
map[string]any{"path": configSource, "error": err.Error()})
|
||||||
} else if wc != nil {
|
} else if wc != nil {
|
||||||
tmpCfg := al.cfg.Clone()
|
tmpCfg := al.cfg.Clone()
|
||||||
tmpCfg.MergeWorkspaceConfig(wc)
|
if err := tmpCfg.MergeWorkspaceConfig(wc); err != nil {
|
||||||
|
return "", fmt.Errorf("workspace config overlay rejected: %w", err)
|
||||||
|
}
|
||||||
if tmpCfg.Agents.Defaults.GetModelName() == "" {
|
if tmpCfg.Agents.Defaults.GetModelName() == "" {
|
||||||
tmpCfg.Agents.Defaults.ModelName = agent.Model
|
tmpCfg.Agents.Defaults.ModelName = agent.Model
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -8,6 +8,6 @@ import (
|
||||||
|
|
||||||
func init() {
|
func init() {
|
||||||
channels.RegisterFactory("magicform", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) {
|
channels.RegisterFactory("magicform", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) {
|
||||||
return NewMagicFormChannel(cfg.Channels.MagicForm, b)
|
return NewMagicFormChannel(cfg.Channels.MagicForm, cfg.Agents.Defaults.WorkspaceRoot, b)
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -8,7 +8,6 @@ import (
|
||||||
"fmt"
|
"fmt"
|
||||||
"io"
|
"io"
|
||||||
"net/http"
|
"net/http"
|
||||||
"path/filepath"
|
|
||||||
"strings"
|
"strings"
|
||||||
"sync"
|
"sync"
|
||||||
"time"
|
"time"
|
||||||
|
|
@ -17,6 +16,7 @@ import (
|
||||||
"github.com/sipeed/picoclaw/pkg/channels"
|
"github.com/sipeed/picoclaw/pkg/channels"
|
||||||
"github.com/sipeed/picoclaw/pkg/config"
|
"github.com/sipeed/picoclaw/pkg/config"
|
||||||
"github.com/sipeed/picoclaw/pkg/logger"
|
"github.com/sipeed/picoclaw/pkg/logger"
|
||||||
|
"github.com/sipeed/picoclaw/pkg/pathutil"
|
||||||
)
|
)
|
||||||
|
|
||||||
// WebhookPayload is the inbound payload from MagicForm.
|
// WebhookPayload is the inbound payload from MagicForm.
|
||||||
|
|
@ -55,6 +55,7 @@ type requestContext struct {
|
||||||
type MagicFormChannel struct {
|
type MagicFormChannel struct {
|
||||||
*channels.BaseChannel
|
*channels.BaseChannel
|
||||||
config config.MagicFormConfig
|
config config.MagicFormConfig
|
||||||
|
workspaceRoot string // effective root: channel-level fallback to global
|
||||||
httpClient *http.Client
|
httpClient *http.Client
|
||||||
requests sync.Map // chatID → *requestContext
|
requests sync.Map // chatID → *requestContext
|
||||||
ctx context.Context
|
ctx context.Context
|
||||||
|
|
@ -62,7 +63,11 @@ type MagicFormChannel struct {
|
||||||
}
|
}
|
||||||
|
|
||||||
// NewMagicFormChannel creates a new MagicForm channel.
|
// NewMagicFormChannel creates a new MagicForm channel.
|
||||||
func NewMagicFormChannel(cfg config.MagicFormConfig, msgBus *bus.MessageBus) (*MagicFormChannel, error) {
|
// globalWorkspaceRoot is the agents.defaults.workspace_root from the base config.
|
||||||
|
// The channel uses its own config.WorkspaceRoot if set, otherwise falls back to
|
||||||
|
// globalWorkspaceRoot. If neither is configured, the constructor returns an error
|
||||||
|
// because workspace overrides cannot be validated without a root boundary.
|
||||||
|
func NewMagicFormChannel(cfg config.MagicFormConfig, globalWorkspaceRoot string, msgBus *bus.MessageBus) (*MagicFormChannel, error) {
|
||||||
base := channels.NewBaseChannel(
|
base := channels.NewBaseChannel(
|
||||||
"magicform",
|
"magicform",
|
||||||
cfg,
|
cfg,
|
||||||
|
|
@ -70,11 +75,21 @@ func NewMagicFormChannel(cfg config.MagicFormConfig, msgBus *bus.MessageBus) (*M
|
||||||
cfg.AllowFrom,
|
cfg.AllowFrom,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
effectiveRoot := cfg.WorkspaceRoot
|
||||||
|
if effectiveRoot == "" {
|
||||||
|
effectiveRoot = globalWorkspaceRoot
|
||||||
|
}
|
||||||
|
if effectiveRoot == "" {
|
||||||
|
return nil, fmt.Errorf("magicform channel requires workspace_root to be configured " +
|
||||||
|
"(set channels.magicform.workspace_root or agents.defaults.workspace_root)")
|
||||||
|
}
|
||||||
|
|
||||||
ctx, cancel := context.WithCancel(context.Background())
|
ctx, cancel := context.WithCancel(context.Background())
|
||||||
|
|
||||||
ch := &MagicFormChannel{
|
ch := &MagicFormChannel{
|
||||||
BaseChannel: base,
|
BaseChannel: base,
|
||||||
config: cfg,
|
config: cfg,
|
||||||
|
workspaceRoot: effectiveRoot,
|
||||||
httpClient: &http.Client{
|
httpClient: &http.Client{
|
||||||
Timeout: 30 * time.Second,
|
Timeout: 30 * time.Second,
|
||||||
},
|
},
|
||||||
|
|
@ -191,31 +206,11 @@ func (c *MagicFormChannel) ServeHTTP(w http.ResponseWriter, r *http.Request) {
|
||||||
go c.processWebhook(c.ctx, payload)
|
go c.processWebhook(c.ctx, payload)
|
||||||
}
|
}
|
||||||
|
|
||||||
// resolveWorkspace validates and resolves the workspace path.
|
// resolveWorkspace validates and resolves the workspace path using the shared
|
||||||
// If workspace_root is configured, the workspace must be a relative path that
|
// pathutil.ResolveWorkspacePath boundary check. The effective workspace root is
|
||||||
// resolves under the root. If workspace_root is not configured, workspace is
|
// determined at construction time (channel-level config with global fallback).
|
||||||
// rejected (no arbitrary path writes allowed).
|
|
||||||
func (c *MagicFormChannel) resolveWorkspace(workspace string) (string, error) {
|
func (c *MagicFormChannel) resolveWorkspace(workspace string) (string, error) {
|
||||||
root := c.config.WorkspaceRoot
|
return pathutil.ResolveWorkspacePath(c.workspaceRoot, workspace)
|
||||||
if root == "" {
|
|
||||||
return "", fmt.Errorf("workspace_root not configured; workspace overrides are not allowed")
|
|
||||||
}
|
|
||||||
|
|
||||||
absRoot, err := filepath.Abs(root)
|
|
||||||
if err != nil {
|
|
||||||
return "", fmt.Errorf("invalid workspace_root: %w", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
// Join the root with the provided workspace (which may be relative)
|
|
||||||
resolved := filepath.Join(absRoot, workspace)
|
|
||||||
resolved = filepath.Clean(resolved)
|
|
||||||
|
|
||||||
// Ensure the resolved path is under the root (prevents ../../../etc traversal)
|
|
||||||
if !strings.HasPrefix(resolved, absRoot+string(filepath.Separator)) && resolved != absRoot {
|
|
||||||
return "", fmt.Errorf("workspace path escapes workspace_root")
|
|
||||||
}
|
|
||||||
|
|
||||||
return resolved, nil
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// verifyToken checks the Authorization Bearer token using constant-time comparison.
|
// verifyToken checks the Authorization Bearer token using constant-time comparison.
|
||||||
|
|
|
||||||
|
|
@ -10,6 +10,7 @@ import (
|
||||||
"github.com/caarlos0/env/v11"
|
"github.com/caarlos0/env/v11"
|
||||||
|
|
||||||
"github.com/sipeed/picoclaw/pkg/fileutil"
|
"github.com/sipeed/picoclaw/pkg/fileutil"
|
||||||
|
"github.com/sipeed/picoclaw/pkg/pathutil"
|
||||||
)
|
)
|
||||||
|
|
||||||
// rrCounter is a global counter for round-robin load balancing across models.
|
// rrCounter is a global counter for round-robin load balancing across models.
|
||||||
|
|
@ -181,6 +182,7 @@ type RoutingConfig struct {
|
||||||
}
|
}
|
||||||
|
|
||||||
type AgentDefaults struct {
|
type AgentDefaults struct {
|
||||||
|
WorkspaceRoot string `json:"workspace_root,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_WORKSPACE_ROOT"`
|
||||||
Workspace string `json:"workspace" env:"PICOCLAW_AGENTS_DEFAULTS_WORKSPACE"`
|
Workspace string `json:"workspace" env:"PICOCLAW_AGENTS_DEFAULTS_WORKSPACE"`
|
||||||
RestrictToWorkspace bool `json:"restrict_to_workspace" env:"PICOCLAW_AGENTS_DEFAULTS_RESTRICT_TO_WORKSPACE"`
|
RestrictToWorkspace bool `json:"restrict_to_workspace" env:"PICOCLAW_AGENTS_DEFAULTS_RESTRICT_TO_WORKSPACE"`
|
||||||
AllowReadOutsideWorkspace bool `json:"allow_read_outside_workspace" env:"PICOCLAW_AGENTS_DEFAULTS_ALLOW_READ_OUTSIDE_WORKSPACE"`
|
AllowReadOutsideWorkspace bool `json:"allow_read_outside_workspace" env:"PICOCLAW_AGENTS_DEFAULTS_ALLOW_READ_OUTSIDE_WORKSPACE"`
|
||||||
|
|
@ -986,9 +988,10 @@ func (c *Config) Clone() *Config {
|
||||||
|
|
||||||
// MergeWorkspaceConfig overlays allowed fields from a workspace config onto this config.
|
// MergeWorkspaceConfig overlays allowed fields from a workspace config onto this config.
|
||||||
// Fields NOT honored (infrastructure-level): Gateway, Heartbeat, Devices, Providers.
|
// Fields NOT honored (infrastructure-level): Gateway, Heartbeat, Devices, Providers.
|
||||||
func (c *Config) MergeWorkspaceConfig(wc *WorkspaceConfig) {
|
// Returns an error if the overlay contains a workspace path that escapes workspace_root.
|
||||||
|
func (c *Config) MergeWorkspaceConfig(wc *WorkspaceConfig) error {
|
||||||
if wc == nil || wc.Config == nil {
|
if wc == nil || wc.Config == nil {
|
||||||
return
|
return nil
|
||||||
}
|
}
|
||||||
src := wc.Config
|
src := wc.Config
|
||||||
|
|
||||||
|
|
@ -998,7 +1001,9 @@ func (c *Config) MergeWorkspaceConfig(wc *WorkspaceConfig) {
|
||||||
}
|
}
|
||||||
|
|
||||||
// agents.defaults: merge non-zero fields
|
// agents.defaults: merge non-zero fields
|
||||||
mergeAgentDefaults(&c.Agents.Defaults, &src.Agents.Defaults)
|
if err := mergeAgentDefaults(&c.Agents.Defaults, &src.Agents.Defaults); err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
|
||||||
// agents.list: replace if workspace has entries
|
// agents.list: replace if workspace has entries
|
||||||
if len(src.Agents.List) > 0 {
|
if len(src.Agents.List) > 0 {
|
||||||
|
|
@ -1017,11 +1022,20 @@ func (c *Config) MergeWorkspaceConfig(wc *WorkspaceConfig) {
|
||||||
|
|
||||||
// session: merge non-zero fields (prevents cross-tenant identity leakage)
|
// session: merge non-zero fields (prevents cross-tenant identity leakage)
|
||||||
mergeSessionConfig(&c.Session, &src.Session)
|
mergeSessionConfig(&c.Session, &src.Session)
|
||||||
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// mergeAgentDefaults copies non-zero fields from src into dst.
|
// mergeAgentDefaults copies non-zero fields from src into dst.
|
||||||
func mergeAgentDefaults(dst, src *AgentDefaults) {
|
// WorkspaceRoot is intentionally NOT copied — it is a security boundary
|
||||||
|
// set by the base config and must not be overridden by workspace overlays.
|
||||||
|
// Returns an error if a workspace overlay attempts to escape the root boundary.
|
||||||
|
func mergeAgentDefaults(dst, src *AgentDefaults) error {
|
||||||
if src.Workspace != "" {
|
if src.Workspace != "" {
|
||||||
|
if dst.WorkspaceRoot != "" {
|
||||||
|
if _, err := pathutil.ResolveWorkspacePath(dst.WorkspaceRoot, src.Workspace); err != nil {
|
||||||
|
return fmt.Errorf("workspace config overlay rejected: %w", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
dst.Workspace = src.Workspace
|
dst.Workspace = src.Workspace
|
||||||
}
|
}
|
||||||
if src.RestrictToWorkspace {
|
if src.RestrictToWorkspace {
|
||||||
|
|
@ -1066,6 +1080,7 @@ func mergeAgentDefaults(dst, src *AgentDefaults) {
|
||||||
if src.MaxMediaSize > 0 {
|
if src.MaxMediaSize > 0 {
|
||||||
dst.MaxMediaSize = src.MaxMediaSize
|
dst.MaxMediaSize = src.MaxMediaSize
|
||||||
}
|
}
|
||||||
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// mergeSessionConfig copies non-zero fields from src into dst.
|
// mergeSessionConfig copies non-zero fields from src into dst.
|
||||||
|
|
|
||||||
110
pkg/pathutil/resolve.go
Normal file
110
pkg/pathutil/resolve.go
Normal file
|
|
@ -0,0 +1,110 @@
|
||||||
|
package pathutil
|
||||||
|
|
||||||
|
import (
|
||||||
|
"fmt"
|
||||||
|
"path/filepath"
|
||||||
|
"strings"
|
||||||
|
)
|
||||||
|
|
||||||
|
// ResolveWorkspacePath resolves path against root and returns an absolute path
|
||||||
|
// that is guaranteed to be a proper subdirectory of root.
|
||||||
|
//
|
||||||
|
// When root is set:
|
||||||
|
// - Empty string and bare "." are rejected (must pick a subdirectory).
|
||||||
|
// - Absolute paths are rejected (must use relative subdirectory names).
|
||||||
|
// - ".." traversal is rejected before any path joining occurs.
|
||||||
|
// - A valid relative path is joined to root and returned as an absolute path.
|
||||||
|
// - A post-join boundary check confirms the result stays within root.
|
||||||
|
//
|
||||||
|
// When root is empty, the function falls back to filepath.Abs(path) for
|
||||||
|
// backward compatibility with callers that don't have a boundary configured.
|
||||||
|
func ResolveWorkspacePath(root, path string) (string, error) {
|
||||||
|
if root == "" {
|
||||||
|
// No boundary configured — resolve raw path as absolute.
|
||||||
|
if path == "" {
|
||||||
|
return "", fmt.Errorf("workspace path is empty")
|
||||||
|
}
|
||||||
|
if containsTraversal(path) {
|
||||||
|
return "", fmt.Errorf("workspace path contains directory traversal")
|
||||||
|
}
|
||||||
|
return filepath.Abs(path)
|
||||||
|
}
|
||||||
|
|
||||||
|
// With a root boundary, path must be a non-empty relative subdirectory.
|
||||||
|
if path == "" {
|
||||||
|
return "", fmt.Errorf("workspace path must be a subdirectory of workspace_root, not root itself")
|
||||||
|
}
|
||||||
|
|
||||||
|
if isAbsoluteOrRooted(path) {
|
||||||
|
return "", fmt.Errorf("workspace path must be relative, got absolute path")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Check traversal before Clean — Clean("foo/..") normalises to "." and
|
||||||
|
// would produce a misleading "not root itself" error instead.
|
||||||
|
if containsTraversal(path) {
|
||||||
|
return "", fmt.Errorf("workspace path contains directory traversal")
|
||||||
|
}
|
||||||
|
|
||||||
|
if filepath.Clean(path) == "." {
|
||||||
|
return "", fmt.Errorf("workspace path must be a subdirectory of workspace_root, not root itself")
|
||||||
|
}
|
||||||
|
|
||||||
|
absRoot, err := filepath.Abs(root)
|
||||||
|
if err != nil {
|
||||||
|
return "", fmt.Errorf("invalid workspace_root: %w", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
resolved := filepath.Clean(filepath.Join(absRoot, path))
|
||||||
|
|
||||||
|
// Belt-and-suspenders: confirm the resolved path is a proper subdirectory
|
||||||
|
// of root even after Clean normalisation.
|
||||||
|
if !strings.HasPrefix(resolved, absRoot+string(filepath.Separator)) {
|
||||||
|
return "", fmt.Errorf("workspace path escapes workspace_root")
|
||||||
|
}
|
||||||
|
|
||||||
|
return resolved, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// containsTraversal detects ".." traversal in all four patterns:
|
||||||
|
//
|
||||||
|
// bare ".." — the path IS ".."
|
||||||
|
// starts with "../" — e.g. ../etc
|
||||||
|
// ends with "/.." — e.g. foo/..
|
||||||
|
// contains "/../" — e.g. foo/../bar
|
||||||
|
//
|
||||||
|
// Both forward-slash and backslash variants are checked so that this
|
||||||
|
// works correctly on Windows where either separator may appear.
|
||||||
|
func containsTraversal(path string) bool {
|
||||||
|
if path == ".." {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, sep := range []string{"/", `\`} {
|
||||||
|
if strings.HasPrefix(path, ".."+sep) {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
if strings.HasSuffix(path, sep+"..") {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
if strings.Contains(path, sep+".."+sep) {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
|
||||||
|
// isAbsoluteOrRooted returns true if the path is absolute (e.g. C:\foo on
|
||||||
|
// Windows, /foo on Unix) or rooted with a leading separator. On Windows,
|
||||||
|
// filepath.IsAbs returns false for Unix-style "/foo" paths, but we still
|
||||||
|
// reject them since they are not relative subdirectory names.
|
||||||
|
func isAbsoluteOrRooted(path string) bool {
|
||||||
|
if filepath.IsAbs(path) {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
// Catch Unix-style rooted paths on Windows (e.g. "/etc/passwd").
|
||||||
|
if len(path) > 0 && (path[0] == '/' || path[0] == '\\') {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
return false
|
||||||
|
}
|
||||||
227
pkg/pathutil/resolve_test.go
Normal file
227
pkg/pathutil/resolve_test.go
Normal file
|
|
@ -0,0 +1,227 @@
|
||||||
|
package pathutil
|
||||||
|
|
||||||
|
import (
|
||||||
|
"path/filepath"
|
||||||
|
"runtime"
|
||||||
|
"strings"
|
||||||
|
"testing"
|
||||||
|
|
||||||
|
"github.com/stretchr/testify/require"
|
||||||
|
)
|
||||||
|
|
||||||
|
func TestResolveWorkspacePath_WithRoot(t *testing.T) {
|
||||||
|
root := filepath.Join(t.TempDir(), "workspaces")
|
||||||
|
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
path string
|
||||||
|
wantErr string // substring of expected error; empty means success
|
||||||
|
wantRel string // expected relative suffix under root (checked on success)
|
||||||
|
}{
|
||||||
|
// --- valid paths ---
|
||||||
|
{
|
||||||
|
name: "relative subdirectory",
|
||||||
|
path: "tenant1",
|
||||||
|
wantRel: "tenant1",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "nested subdirectory",
|
||||||
|
path: "tenant1/project/sub",
|
||||||
|
wantRel: filepath.Join("tenant1", "project", "sub"),
|
||||||
|
},
|
||||||
|
|
||||||
|
// --- empty / dot ---
|
||||||
|
{
|
||||||
|
name: "empty path rejected",
|
||||||
|
path: "",
|
||||||
|
wantErr: "subdirectory of workspace_root",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "dot path rejected",
|
||||||
|
path: ".",
|
||||||
|
wantErr: "subdirectory of workspace_root",
|
||||||
|
},
|
||||||
|
|
||||||
|
// --- traversal: bare .. ---
|
||||||
|
{
|
||||||
|
name: "bare dotdot rejected",
|
||||||
|
path: "..",
|
||||||
|
wantErr: "traversal",
|
||||||
|
},
|
||||||
|
|
||||||
|
// --- traversal: starts with ../ ---
|
||||||
|
{
|
||||||
|
name: "starts with dotdot slash",
|
||||||
|
path: "../escape",
|
||||||
|
wantErr: "traversal",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "starts with dotdot backslash",
|
||||||
|
path: `..\\escape`,
|
||||||
|
wantErr: "traversal",
|
||||||
|
},
|
||||||
|
|
||||||
|
// --- traversal: ends with /.. ---
|
||||||
|
{
|
||||||
|
name: "ends with slash dotdot",
|
||||||
|
path: "foo/..",
|
||||||
|
wantErr: "traversal",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "ends with backslash dotdot",
|
||||||
|
path: `foo\..`,
|
||||||
|
wantErr: "traversal",
|
||||||
|
},
|
||||||
|
|
||||||
|
// --- traversal: contains /../ ---
|
||||||
|
{
|
||||||
|
name: "contains slash dotdot slash",
|
||||||
|
path: "a/../escape",
|
||||||
|
wantErr: "traversal",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "contains backslash dotdot backslash",
|
||||||
|
path: `a\..\escape`,
|
||||||
|
wantErr: "traversal",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "deep traversal",
|
||||||
|
path: "a/b/../../escape",
|
||||||
|
wantErr: "traversal",
|
||||||
|
},
|
||||||
|
|
||||||
|
// --- absolute paths ---
|
||||||
|
{
|
||||||
|
name: "unix absolute path rejected",
|
||||||
|
path: "/etc/passwd",
|
||||||
|
wantErr: "must be relative",
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
if runtime.GOOS == "windows" {
|
||||||
|
tests = append(tests, struct {
|
||||||
|
name string
|
||||||
|
path string
|
||||||
|
wantErr string
|
||||||
|
wantRel string
|
||||||
|
}{
|
||||||
|
name: "windows absolute path rejected",
|
||||||
|
path: `C:\Windows\System32`,
|
||||||
|
wantErr: "must be relative",
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
got, err := ResolveWorkspacePath(root, tt.path)
|
||||||
|
|
||||||
|
if tt.wantErr != "" {
|
||||||
|
if err == nil {
|
||||||
|
t.Fatalf("expected error containing %q, got nil (result: %s)", tt.wantErr, got)
|
||||||
|
}
|
||||||
|
if !strings.Contains(err.Error(), tt.wantErr) {
|
||||||
|
t.Fatalf("expected error containing %q, got: %v", tt.wantErr, err)
|
||||||
|
}
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
absRoot, absErr := filepath.Abs(root)
|
||||||
|
require.NoError(t, absErr)
|
||||||
|
want := filepath.Join(absRoot, tt.wantRel)
|
||||||
|
if got != want {
|
||||||
|
t.Fatalf("expected %q, got %q", want, got)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestResolveWorkspacePath_NoRoot(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
path string
|
||||||
|
wantErr string
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "relative path resolved via Abs",
|
||||||
|
path: "some/dir",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "empty path rejected",
|
||||||
|
path: "",
|
||||||
|
wantErr: "empty",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "traversal rejected even without root",
|
||||||
|
path: "../escape",
|
||||||
|
wantErr: "traversal",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "bare dotdot rejected even without root",
|
||||||
|
path: "..",
|
||||||
|
wantErr: "traversal",
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
got, err := ResolveWorkspacePath("", tt.path)
|
||||||
|
|
||||||
|
if tt.wantErr != "" {
|
||||||
|
if err == nil {
|
||||||
|
t.Fatalf("expected error containing %q, got nil (result: %s)", tt.wantErr, got)
|
||||||
|
}
|
||||||
|
if !strings.Contains(err.Error(), tt.wantErr) {
|
||||||
|
t.Fatalf("expected error containing %q, got: %v", tt.wantErr, err)
|
||||||
|
}
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Without root, should return filepath.Abs(path)
|
||||||
|
want, absErr := filepath.Abs(tt.path)
|
||||||
|
require.NoError(t, absErr)
|
||||||
|
if got != want {
|
||||||
|
t.Fatalf("expected %q, got %q", want, got)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestContainsTraversal(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
path string
|
||||||
|
want bool
|
||||||
|
}{
|
||||||
|
// positive cases
|
||||||
|
{"..", true},
|
||||||
|
{"../foo", true},
|
||||||
|
{`..\\foo`, true},
|
||||||
|
{"foo/..", true},
|
||||||
|
{`foo\..`, true},
|
||||||
|
{"foo/../bar", true},
|
||||||
|
{`foo\..\bar`, true},
|
||||||
|
{"a/b/../../c", true},
|
||||||
|
|
||||||
|
// negative cases — dotdot as part of a name is fine
|
||||||
|
{"..hidden", false},
|
||||||
|
{"foo..bar", false},
|
||||||
|
{"tenant1", false},
|
||||||
|
{"a/b/c", false},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.path, func(t *testing.T) {
|
||||||
|
got := containsTraversal(tt.path)
|
||||||
|
if got != tt.want {
|
||||||
|
t.Fatalf("containsTraversal(%q) = %v, want %v", tt.path, got, tt.want)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
Loading…
Add table
Reference in a new issue