diff --git a/forge-cli/build/dockerfile_stage.go b/forge-cli/build/dockerfile_stage.go index be96d8e..a208082 100644 --- a/forge-cli/build/dockerfile_stage.go +++ b/forge-cli/build/dockerfile_stage.go @@ -7,6 +7,7 @@ import ( "io" "os" "path/filepath" + "sort" "text/template" "github.com/initializ/forge/forge-cli/templates" @@ -227,11 +228,10 @@ func (s *DockerfileStage) injectLocalBins(bc *pipeline.BuildContext) { } } -// copyLocalBins copies local binary files into .local-bins/ in the build output directory. -// It collects binaries from both forge.yaml config (BinOverrides with LocalPath) and -// CLI flags (bc.LocalBins). -func (s *DockerfileStage) copyLocalBins(bc *pipeline.BuildContext) error { - // Collect local bins from config +// localBins returns the union of local binary overrides from forge.yaml +// config (BinOverrides with LocalPath) and CLI flags (bc.LocalBins), which +// may have additional entries not yet in config. +func localBins(bc *pipeline.BuildContext) map[string]string { bins := make(map[string]string) if bc.Config != nil { for name, override := range bc.Config.Package.BinOverrides { @@ -240,11 +240,15 @@ func (s *DockerfileStage) copyLocalBins(bc *pipeline.BuildContext) error { } } } - // CLI flags (bc.LocalBins) may have additional entries not yet in config for name, path := range bc.LocalBins { bins[name] = path } + return bins +} +// copyLocalBins copies local binary files into .local-bins/ in the build output directory. +func (s *DockerfileStage) copyLocalBins(bc *pipeline.BuildContext) error { + bins := localBins(bc) if len(bins) == 0 { return nil } @@ -338,10 +342,25 @@ compiled/ # Recursive — the Dockerfile shouldn't be baked into its own image. Dockerfile .dockerignore -# Build-only local binary stash; binaries land in /usr/local/bin via -# the bin stage already. +# Build-only local binary stash. Local bin overrides are re-included +# below (last match wins) — the generated Dockerfile COPYs each one +# from .local-bins/ directly, so excluding them breaks the build. .local-bins/ ` + // Re-include local bin overrides: for each one the generated Dockerfile + // carries a `COPY .local-bins/ /usr/local/bin/` that reads + // straight from the build context, which the blanket .local-bins/ + // exclusion above would otherwise break. + if bins := localBins(bc); len(bins) > 0 { + names := make([]string, 0, len(bins)) + for name := range bins { + names = append(names, name) + } + sort.Strings(names) + for _, name := range names { + dockerignoreContent += fmt.Sprintf("!.local-bins/%s\n", name) + } + } ignorePath := filepath.Join(bc.Opts.OutputDir, ".dockerignore") if err := os.WriteFile(ignorePath, []byte(dockerignoreContent), 0644); err != nil { return fmt.Errorf("writing .dockerignore: %w", err) diff --git a/forge-cli/build/dockerfile_stage_issue147_test.go b/forge-cli/build/dockerfile_stage_issue147_test.go index ff14c14..7dcc166 100644 --- a/forge-cli/build/dockerfile_stage_issue147_test.go +++ b/forge-cli/build/dockerfile_stage_issue147_test.go @@ -156,3 +156,77 @@ func TestDockerfile_NoLongerCopiesDeadCompiledArtifacts(t *testing.T) { } } } + +// TestDockerignore_ReincludesLocalBinOverrides pins the --local-bin fix: +// the generated Dockerfile COPYs each local bin override straight from +// .local-bins/ in the build context, so the blanket .local-bins/ exclusion +// must be followed by a !.local-bins/ negation per override +// (last match wins) or the docker build fails with "not found". +func TestDockerignore_ReincludesLocalBinOverrides(t *testing.T) { + tmpDir := t.TempDir() + outDir := filepath.Join(tmpDir, "output") + if err := os.MkdirAll(outDir, 0755); err != nil { + t.Fatal(err) + } + + bc := pipeline.NewBuildContext(pipeline.PipelineOptions{WorkDir: tmpDir, OutputDir: outDir}) + bc.Config = &types.ForgeConfig{AgentID: "test", Version: "1.0.0"} + bc.Config.Package.BinOverrides = map[string]types.BinOverride{ + "jq": {LocalPath: "/usr/bin/jq"}, + } + bc.LocalBins = map[string]string{"forge": "/usr/local/bin/forge"} + bc.Spec = &agentspec.AgentSpec{AgentID: "test"} + + s := &DockerfileStage{} + if err := s.writeDockerignore(bc); err != nil { + t.Fatalf("writeDockerignore: %v", err) + } + + data, err := os.ReadFile(filepath.Join(outDir, ".dockerignore")) + if err != nil { + t.Fatalf("reading .dockerignore: %v", err) + } + got := string(data) + + // The blanket exclusion stays (keeps unrelated stash content out)… + if !strings.Contains(got, ".local-bins/\n") { + t.Errorf(".dockerignore lost the .local-bins/ exclusion. Full content:\n%s", got) + } + // …and every override is re-included after it, config and CLI alike. + for _, name := range []string{"forge", "jq"} { + neg := "!.local-bins/" + name + "\n" + if !strings.Contains(got, neg) { + t.Errorf(".dockerignore is missing negation %q. Full content:\n%s", neg, got) + } + if strings.Index(got, neg) < strings.Index(got, ".local-bins/\n") { + t.Errorf("negation %q must come after the .local-bins/ exclusion. Full content:\n%s", neg, got) + } + } +} + +// TestDockerignore_NoLocalBins_KeepsPlainExclusion guards the default path: +// without overrides no negations are emitted. +func TestDockerignore_NoLocalBins_KeepsPlainExclusion(t *testing.T) { + tmpDir := t.TempDir() + outDir := filepath.Join(tmpDir, "output") + if err := os.MkdirAll(outDir, 0755); err != nil { + t.Fatal(err) + } + + bc := pipeline.NewBuildContext(pipeline.PipelineOptions{WorkDir: tmpDir, OutputDir: outDir}) + bc.Config = &types.ForgeConfig{AgentID: "test", Version: "1.0.0"} + bc.Spec = &agentspec.AgentSpec{AgentID: "test"} + + s := &DockerfileStage{} + if err := s.writeDockerignore(bc); err != nil { + t.Fatalf("writeDockerignore: %v", err) + } + + data, err := os.ReadFile(filepath.Join(outDir, ".dockerignore")) + if err != nil { + t.Fatalf("reading .dockerignore: %v", err) + } + if strings.Contains(string(data), "!.local-bins/") { + t.Errorf(".dockerignore must not contain negations without local bin overrides. Full content:\n%s", string(data)) + } +} diff --git a/forge-cli/cmd/build.go b/forge-cli/cmd/build.go index 91d8638..bb71984 100644 --- a/forge-cli/cmd/build.go +++ b/forge-cli/cmd/build.go @@ -153,6 +153,9 @@ func parseLocalBins(args []string) (map[string]string, error) { return nil, fmt.Errorf("invalid --local-bin format %q: expected name=/path/to/file", arg) } name, path := parts[0], parts[1] + if !validate.ValidBinOverrideName(name) { + return nil, fmt.Errorf("invalid --local-bin name %q: must be a plain binary basename matching ^[a-zA-Z0-9_.-]{1,64}$ (and not \".\" or \"..\")", name) + } absPath, err := filepath.Abs(path) if err != nil { return nil, fmt.Errorf("resolving path for --local-bin %s: %w", name, err) diff --git a/forge-cli/cmd/build_test.go b/forge-cli/cmd/build_test.go index 6f710b3..5f1708e 100644 --- a/forge-cli/cmd/build_test.go +++ b/forge-cli/cmd/build_test.go @@ -107,3 +107,24 @@ entrypoint: "" t.Fatal("expected error for invalid config") } } + +// TestParseLocalBins_NameValidation pins the --local-bin name check +// (PR #374 follow-up): the name flows verbatim into the .local-bins/ +// path, the Dockerfile COPY, and the .dockerignore negation. +func TestParseLocalBins_NameValidation(t *testing.T) { + tmp := t.TempDir() + binPath := filepath.Join(tmp, "somebin") + if err := os.WriteFile(binPath, []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatal(err) + } + + if _, err := parseLocalBins([]string{"forge=" + binPath}); err != nil { + t.Errorf("valid name rejected: %v", err) + } + + for _, name := range []string{"../escape", "..", ".", "a/b", "bad\nname", "*", "!x"} { + if _, err := parseLocalBins([]string{name + "=" + binPath}); err == nil { + t.Errorf("name %q should be rejected", name) + } + } +} diff --git a/forge-core/validate/forge_config.go b/forge-core/validate/forge_config.go index 6615b01..9d4b5d0 100644 --- a/forge-core/validate/forge_config.go +++ b/forge-core/validate/forge_config.go @@ -3,6 +3,7 @@ package validate import ( "fmt" "regexp" + "sort" "strings" "github.com/initializ/forge/forge-core/llm" @@ -35,6 +36,22 @@ func isGatewayScheme(s string) bool { return s == llm.AuthSchemeAPIKeyHeader || s == llm.AuthSchemeAPIKeyHeaderOnly } +// binOverrideNamePattern is the accepted charset for a bin-override name +// (forge.yaml bin_overrides. / --local-bin name=…). The name is a +// binary basename that reaches three sinks verbatim — the .local-bins/ +// filesystem path, the Dockerfile COPY line, and the .dockerignore +// re-include negation — so anything beyond a plain basename (path +// separators, whitespace, glob metacharacters) is an injection vector. +var binOverrideNamePattern = regexp.MustCompile(`^[a-zA-Z0-9_.-]{1,64}$`) + +// ValidBinOverrideName reports whether name is a safe bin-override name. +// Pure-dot names are rejected explicitly: "." and ".." pass the charset +// pattern but are path navigation, not basenames — ".." is exactly the +// traversal the pattern exists to stop. +func ValidBinOverrideName(name string) bool { + return binOverrideNamePattern.MatchString(name) && strings.Trim(name, ".") != "" +} + var ( agentIDPattern = regexp.MustCompile(`^[a-z0-9-]+$`) semverPattern = regexp.MustCompile(`^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$`) @@ -137,6 +154,20 @@ func ValidateForgeConfig(cfg *types.ForgeConfig) *ValidationResult { r.Warnings = append(r.Warnings, "egress mode 'dev-open' is not recommended for production") } + // Validate bin-override names (package.bin_overrides). The name flows + // verbatim into the .local-bins/ path, the Dockerfile COPY, and the + // .dockerignore negation — reject anything that isn't a plain basename. + binNames := make([]string, 0, len(cfg.Package.BinOverrides)) + for name := range cfg.Package.BinOverrides { + binNames = append(binNames, name) + } + sort.Strings(binNames) + for _, name := range binNames { + if !ValidBinOverrideName(name) { + r.Errors = append(r.Errors, fmt.Sprintf("package.bin_overrides name %q must be a plain binary basename matching ^[a-zA-Z0-9_.-]{1,64}$ (and not %q or %q)", name, ".", "..")) + } + } + // Validate secrets config for _, p := range cfg.Secrets.Providers { if !knownSecretProviders[p] { diff --git a/forge-core/validate/forge_config_test.go b/forge-core/validate/forge_config_test.go index 2b664c9..5b3b083 100644 --- a/forge-core/validate/forge_config_test.go +++ b/forge-core/validate/forge_config_test.go @@ -225,3 +225,37 @@ func TestValidateForgeConfig_OrgIDOnOpenAI(t *testing.T) { } } } + +// TestValidateForgeConfig_BinOverrideNames pins the bin-override name +// validation (PR #374 follow-up): the name flows verbatim into the +// .local-bins/ filesystem path, the Dockerfile COPY line, and the +// .dockerignore negation, so only plain basenames may pass. +func TestValidateForgeConfig_BinOverrideNames(t *testing.T) { + valid := []string{"forge", "jq", "kubectl", "my-tool_2", "tool.v2", "a"} + for _, name := range valid { + cfg := validConfig() + cfg.Package.BinOverrides = map[string]types.BinOverride{name: {LocalPath: "/x"}} + if r := ValidateForgeConfig(cfg); hasSubstr(r.Errors, "bin_overrides") { + t.Errorf("name %q should be accepted, got errors: %v", name, r.Errors) + } + } + + invalid := []string{ + "../escape", // path traversal into the fs sink + "..", // pure-dot traversal — passes the charset alone + ".", // pure-dot + "a/b", // path separator + "bad\nname", // newline → .dockerignore line injection + "!important", // dockerignore negation metacharacter + "*", // glob re-includes the whole stash + "name with space", // whitespace + strings.Repeat("a", 65), // over length cap + } + for _, name := range invalid { + cfg := validConfig() + cfg.Package.BinOverrides = map[string]types.BinOverride{name: {LocalPath: "/x"}} + if r := ValidateForgeConfig(cfg); !hasSubstr(r.Errors, "bin_overrides") { + t.Errorf("name %q should be rejected, got errors: %v", name, r.Errors) + } + } +}