From d25b106e4fa548c8250c5edd7b1e8b50895cbc56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=BF=AE=E9=9B=A8?= Date: Thu, 16 Apr 2026 10:53:53 +0800 Subject: [PATCH] fix(plugin): harden plugin system security boundaries - Validate git URL protocol: reject file:// and local paths, only allow https/ssh - Reject symlink entries in ZIP extraction to prevent path traversal - Validate build.output must be relative path within plugin directory - Reject absolute paths in stdio server command declarations - Block dangerous env var names (PATH, LD_PRELOAD, etc.) from plugin config injection - Remove conference from default managed plugins (source not yet available) --- internal/plugin/loader.go | 42 +++++++++++++++++++++++++++++++++++++ internal/plugin/manifest.go | 4 ++++ internal/plugin/updater.go | 5 +++++ pkg/config/constants.go | 4 +--- 4 files changed, 52 insertions(+), 3 deletions(-) diff --git a/internal/plugin/loader.go b/internal/plugin/loader.go index 0d9b8a60..6fc2be81 100644 --- a/internal/plugin/loader.go +++ b/internal/plugin/loader.go @@ -427,9 +427,16 @@ func (l *Loader) InstallFromGit(gitURL string) (*Plugin, error) { // parseGitURL extracts workspace and repo name from a git URL. // Supports: https://github.com/org/repo.git, git@github.com:org/repo.git +// Rejects file:// and other local protocols to prevent reading local files. func parseGitURL(gitURL string) (workspace, repoName string, err error) { gitURL = strings.TrimSpace(gitURL) + // Reject dangerous protocols that could read local files. + lower := strings.ToLower(gitURL) + if strings.HasPrefix(lower, "file://") || strings.HasPrefix(lower, "/") || strings.HasPrefix(lower, ".") { + return "", "", fmt.Errorf("local paths and file:// URLs are not allowed: %q", gitURL) + } + // Handle SSH format: git@github.com:org/repo.git if strings.HasPrefix(gitURL, "git@") { parts := strings.SplitN(gitURL, ":", 2) @@ -450,6 +457,11 @@ func parseGitURL(gitURL string) (workspace, repoName string, err error) { return "", "", fmt.Errorf("cannot parse URL %q: %w", gitURL, err) } + // Only allow https:// and http:// schemes. + if u.Scheme != "https" && u.Scheme != "http" { + return "", "", fmt.Errorf("unsupported URL scheme %q: only https and ssh are allowed", u.Scheme) + } + path := strings.TrimSuffix(strings.Trim(u.Path, "/"), ".git") segments := strings.Split(path, "/") if len(segments) < 2 { @@ -612,6 +624,19 @@ func (l *Loader) ListPluginConfig(pluginName string) map[string]string { // // Environment variables already set by the user take precedence — only // keys not already present in the environment are injected. +// dangerousEnvVars contains environment variable names that must never be +// set from plugin config because they can alter process behavior in +// security-critical ways (library injection, executable search path, etc.). +var dangerousEnvVars = map[string]bool{ + "PATH": true, "HOME": true, "USER": true, "SHELL": true, + "LD_PRELOAD": true, "LD_LIBRARY_PATH": true, + "DYLD_INSERT_LIBRARIES": true, "DYLD_LIBRARY_PATH": true, "DYLD_FRAMEWORK_PATH": true, + "NODE_OPTIONS": true, "PYTHONPATH": true, "RUBYLIB": true, + "GOPATH": true, "GOROOT": true, + "HTTP_PROXY": true, "HTTPS_PROXY": true, "ALL_PROXY": true, "NO_PROXY": true, + "http_proxy": true, "https_proxy": true, "all_proxy": true, "no_proxy": true, +} + func (l *Loader) InjectPluginConfigEnv() { settings := l.loadSettings() if len(settings.PluginConfigs) == 0 { @@ -623,6 +648,12 @@ func (l *Loader) InjectPluginConfigEnv() { if !ok || strVal == "" { continue } + // Block dangerous environment variable names. + if dangerousEnvVars[key] { + slog.Warn("plugin: blocked dangerous env var from config", + "key", key) + continue + } // Do not override existing environment variables. if _, exists := os.LookupEnv(key); exists { continue @@ -761,6 +792,17 @@ func runBuild(pluginDir string, build *BuildConfig) error { return fmt.Errorf("build.command is empty") } + // Validate build.output is a relative path within the plugin directory. + if build.Output != "" { + if filepath.IsAbs(build.Output) { + return fmt.Errorf("build.output must be a relative path, got %q", build.Output) + } + cleanOut := filepath.Clean(build.Output) + if strings.HasPrefix(cleanOut, "..") { + return fmt.Errorf("build.output must not escape plugin directory: %q", build.Output) + } + } + slog.Info("plugin: building", "dir", pluginDir, "command", build.Command) var cmd *exec.Cmd diff --git a/internal/plugin/manifest.go b/internal/plugin/manifest.go index b216e4b9..923e5490 100644 --- a/internal/plugin/manifest.go +++ b/internal/plugin/manifest.go @@ -157,6 +157,10 @@ func validateMCPServer(key string, srv *MCPServer) error { if strings.TrimSpace(srv.Command) == "" { return fmt.Errorf("mcpServers[%q]: stdio requires command", key) } + // Reject absolute paths in command to encourage relative paths within plugin root. + if filepath.IsAbs(srv.Command) { + return fmt.Errorf("mcpServers[%q]: command must be a relative path, got %q", key, srv.Command) + } default: return fmt.Errorf("mcpServers[%q]: unsupported type %q (must be streamable-http or stdio)", key, srv.Type) } diff --git a/internal/plugin/updater.go b/internal/plugin/updater.go index 2392a648..14ddc40f 100644 --- a/internal/plugin/updater.go +++ b/internal/plugin/updater.go @@ -361,6 +361,11 @@ func extractPluginZip(zipPath, destDir string) error { return fmt.Errorf("invalid file path in zip: %s", file.Name) } + // Reject symlinks in ZIP to prevent path traversal attacks. + if file.FileInfo().Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("symlinks are not allowed in plugin zip: %s", file.Name) + } + if file.FileInfo().IsDir() { if err := os.MkdirAll(filePath, 0o755); err != nil { return fmt.Errorf("create directory: %w", err) diff --git a/pkg/config/constants.go b/pkg/config/constants.go index 26eca3ae..3831d8ba 100644 --- a/pkg/config/constants.go +++ b/pkg/config/constants.go @@ -138,6 +138,4 @@ const ( // automatically pulled on first run if not already present locally. // Each entry is the short plugin name (without the workspace prefix); // the full qualified name is OfficialPluginWorkspace + "/" + name. -var DefaultManagedPlugins = []string{ - "conference", -} +var DefaultManagedPlugins = []string{}