From 4dbf335d7fd10a0b83fb352dbd65cda221a5208a Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Wed, 15 Apr 2026 00:18:09 -0700 Subject: [PATCH] =?UTF-8?q?fix(security):=20#103=20=E2=80=94=20path-saniti?= =?UTF-8?q?ze=20+=20admin-gate=20POST=20/org/import?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #103 (HIGH). Three attack surfaces on the import endpoint — body.Dir, workspace.Template, workspace.FilesDir — were concatenated via filepath.Join without validation, letting an unauthenticated caller probe arbitrary filesystem paths with "../../../etc". Two layers of defense: 1. resolveInsideRoot() rejects absolute paths and any relative path whose lexically cleaned join escapes the provided root (Abs + HasPrefix + separator guard). 6 tests cover happy path, traversal attempts, absolute path, empty input, prefix-sibling escape, and deep subpath resolution. 2. Route now runs behind middleware.AdminAuth so an unauthenticated attacker can't reach the handler at all once a token exists. Co-Authored-By: Claude Opus 4.6 (1M context) --- platform/internal/handlers/org.go | 61 ++++++++++-- platform/internal/handlers/org_path_test.go | 100 ++++++++++++++++++++ platform/internal/router/router.go | 6 +- 3 files changed, 159 insertions(+), 8 deletions(-) create mode 100644 platform/internal/handlers/org_path_test.go diff --git a/platform/internal/handlers/org.go b/platform/internal/handlers/org.go index 41ca62d1..d6d3a94e 100644 --- a/platform/internal/handlers/org.go +++ b/platform/internal/handlers/org.go @@ -196,7 +196,16 @@ func (h *OrgHandler) Import(c *gin.Context) { var orgBaseDir string // base directory for files_dir resolution if body.Dir != "" { - orgBaseDir = filepath.Join(h.orgDir, body.Dir) + // Reject traversal attempts — `dir` must resolve inside h.orgDir. + // Without this, `dir: "../../../etc"` gets joined into h.orgDir and + // filepath.Join's lexical cleanup resolves it outside the root, + // letting an unauthenticated caller probe arbitrary filesystem paths. + resolved, err := resolveInsideRoot(h.orgDir, body.Dir) + if err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("invalid dir: %v", err)}) + return + } + orgBaseDir = resolved orgFile := filepath.Join(orgBaseDir, "org.yaml") data, err := os.ReadFile(orgFile) if err != nil { @@ -349,9 +358,12 @@ func (h *OrgHandler) createWorkspaceTree(ws OrgWorkspace, parentID *string, defa } templatePath := "" if ws.Template != "" { - tp := filepath.Join(h.configsDir, ws.Template) - if _, err := os.Stat(tp); err == nil { - templatePath = tp + // `template` comes from the uploaded YAML — treat as untrusted. + // Only accept paths that stay inside h.configsDir. + if tp, err := resolveInsideRoot(h.configsDir, ws.Template); err == nil { + if _, statErr := os.Stat(tp); statErr == nil { + templatePath = tp + } } } if templatePath == "" { @@ -367,9 +379,12 @@ func (h *OrgHandler) createWorkspaceTree(ws OrgWorkspace, parentID *string, defa // Copy files_dir contents on top (system-prompt.md, CLAUDE.md, skills/, etc.) // Uses templatePath for CopyTemplateToContainer — runs AFTER configFiles are written if ws.FilesDir != "" && orgBaseDir != "" { - filesPath := filepath.Join(orgBaseDir, ws.FilesDir) - if info, err := os.Stat(filesPath); err == nil && info.IsDir() { - templatePath = filesPath + // `files_dir` also comes from untrusted YAML. Join inside orgBaseDir + // (already validated above) and reject anything that escapes. + if filesPath, err := resolveInsideRoot(orgBaseDir, ws.FilesDir); err == nil { + if info, statErr := os.Stat(filesPath); statErr == nil && info.IsDir() { + templatePath = filesPath + } } } @@ -815,3 +830,35 @@ func mergePlugins(defaultPlugins, wsPlugins []string) []string { } return out } + +// resolveInsideRoot joins `userPath` onto `root` and ensures the lexically +// cleaned result stays inside root. Rejects absolute paths outright and +// anything containing ".." that would escape the root. +// +// Both arguments are resolved to absolute paths via filepath.Abs before the +// prefix check so a root passed as a relative path still works correctly. +// Follows Go's standard pattern for SSRF-class path sanitization; using +// strings.HasPrefix on an absolute-path pair plus the separator guard rejects +// sibling directories that share a prefix (e.g. "/foo" vs "/foobar"). +func resolveInsideRoot(root, userPath string) (string, error) { + if userPath == "" { + return "", fmt.Errorf("path is empty") + } + if filepath.IsAbs(userPath) { + return "", fmt.Errorf("absolute paths are not allowed") + } + absRoot, err := filepath.Abs(root) + if err != nil { + return "", fmt.Errorf("root abs: %w", err) + } + joined := filepath.Join(absRoot, userPath) + absJoined, err := filepath.Abs(joined) + if err != nil { + return "", fmt.Errorf("joined abs: %w", err) + } + // Allow exact-root match (rare but valid) and any descendant. + if absJoined != absRoot && !strings.HasPrefix(absJoined, absRoot+string(filepath.Separator)) { + return "", fmt.Errorf("path escapes root") + } + return absJoined, nil +} diff --git a/platform/internal/handlers/org_path_test.go b/platform/internal/handlers/org_path_test.go new file mode 100644 index 00000000..2ec707ff --- /dev/null +++ b/platform/internal/handlers/org_path_test.go @@ -0,0 +1,100 @@ +package handlers + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// Exercises resolveInsideRoot — the SSRF-class path sanitizer used by +// POST /org/import for `dir` / `template` / `files_dir`. Issue #103. +// The helper is the single chokepoint preventing `../../../etc` escape, +// so it earns a dedicated test file. + +func TestResolveInsideRoot_HappyPath(t *testing.T) { + tmp := t.TempDir() + sub := filepath.Join(tmp, "molecule-dev") + if err := os.Mkdir(sub, 0o755); err != nil { + t.Fatal(err) + } + got, err := resolveInsideRoot(tmp, "molecule-dev") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // Compare via Abs to tolerate macOS /private symlink normalization. + wantAbs, _ := filepath.Abs(sub) + if got != wantAbs { + t.Errorf("got %q, want %q", got, wantAbs) + } +} + +func TestResolveInsideRoot_RejectsTraversal(t *testing.T) { + tmp := t.TempDir() + cases := []string{ + "../etc", + "../../etc/passwd", + "molecule-dev/../../..", + "../../../../../../../../../etc", + } + for _, tc := range cases { + t.Run(tc, func(t *testing.T) { + if _, err := resolveInsideRoot(tmp, tc); err == nil { + t.Errorf("expected error for %q, got nil", tc) + } + }) + } +} + +func TestResolveInsideRoot_RejectsAbsolute(t *testing.T) { + tmp := t.TempDir() + if _, err := resolveInsideRoot(tmp, "/etc/passwd"); err == nil { + t.Error("absolute path must be rejected") + } +} + +func TestResolveInsideRoot_RejectsEmpty(t *testing.T) { + tmp := t.TempDir() + if _, err := resolveInsideRoot(tmp, ""); err == nil { + t.Error("empty path must be rejected") + } +} + +// A path whose Abs shares a prefix with root but is NOT inside root must be +// rejected. Catches the classic string-prefix bug where "/foo" matches +// "/foobar". +func TestResolveInsideRoot_RejectsPrefixSibling(t *testing.T) { + tmp := t.TempDir() + sibling := tmp + "-sibling" + if err := os.MkdirAll(sibling, 0o755); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(sibling) }) + + // Use a relative path that lexically resolves to the sibling directory. + up := "../" + filepath.Base(sibling) + if _, err := resolveInsideRoot(tmp, up); err == nil { + t.Errorf("sibling-prefix path %q must be rejected", up) + } +} + +func TestResolveInsideRoot_DeepSubpath(t *testing.T) { + tmp := t.TempDir() + deep := filepath.Join(tmp, "a", "b", "c") + if err := os.MkdirAll(deep, 0o755); err != nil { + t.Fatal(err) + } + got, err := resolveInsideRoot(tmp, "a/b/c") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + wantAbs, _ := filepath.Abs(deep) + if got != wantAbs { + t.Errorf("got %q want %q", got, wantAbs) + } + // Sanity: result is a strict descendant of root. + rootAbs, _ := filepath.Abs(tmp) + if !strings.HasPrefix(got, rootAbs+string(filepath.Separator)) { + t.Errorf("result %q is not inside %q", got, rootAbs) + } +} diff --git a/platform/internal/router/router.go b/platform/internal/router/router.go index b8b9730b..f3740adf 100644 --- a/platform/internal/router/router.go +++ b/platform/internal/router/router.go @@ -291,7 +291,11 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi orgDir := findOrgDir(configsDir) orgh := handlers.NewOrgHandler(wh, broadcaster, prov, channelMgr, configsDir, orgDir) r.GET("/org/templates", orgh.ListTemplates) - r.POST("/org/import", orgh.Import) + // /org/import can create arbitrary workspaces from an uploaded YAML — it + // must be an admin-gated route. The handler also path-sanitizes + // `dir`/`template`/`files_dir` via resolveInsideRoot, but defence-in- + // depth keeps the route behind AdminAuth regardless. + r.POST("/org/import", middleware.AdminAuth(db.DB), orgh.Import) // Channels (social integrations — Telegram, Slack, Discord, etc.) chh := handlers.NewChannelHandler(channelMgr)