diff --git a/internal/config/validate_action_folder_test.go b/internal/config/validate_action_folder_test.go new file mode 100644 index 00000000..d1ef1c74 --- /dev/null +++ b/internal/config/validate_action_folder_test.go @@ -0,0 +1,18 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestValidateActionFolder_RejectsTraversal guards the action_folder sink: +// the generator joins it directly into a filesystem path +// (.github/actions//action.yaml), so it must reject a traversal or +// nested-path shape and accept only a plain folder name. +func TestValidateActionFolder_RejectsTraversal(t *testing.T) { + t.Parallel() + require.NotEmpty(t, validateActionFolder("../../etc/foo")) + require.NotEmpty(t, validateActionFolder("a/b")) + require.Empty(t, validateActionFolder("manage-release")) +} diff --git a/internal/config/validate_action_pins_test.go b/internal/config/validate_action_pins_test.go new file mode 100644 index 00000000..e8c1fdb6 --- /dev/null +++ b/internal/config/validate_action_pins_test.go @@ -0,0 +1,31 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestValidateActionPins_RejectsNewline guards the action_pins override sink: +// actionRef splices the value raw into a generated workflow, so a value +// carrying a newline (or otherwise breaking out of the emitted YAML scalar) +// must be rejected rather than reaching the generator. +func TestValidateActionPins_RejectsNewline(t *testing.T) { + t.Parallel() + cfg := &TrunkConfig{ActionPins: map[string]string{ + "actions/checkout": "v5\n run: curl evil", + }} + errs := validateActionPins(cfg) + require.NotEmpty(t, errs) +} + +// TestValidateActionPins_AllowsShaWithComment confirms the accepted shape: a +// full-length sha plus its trailing "# " comment, the form a +// reconcile adoption writes, still validates clean. +func TestValidateActionPins_AllowsShaWithComment(t *testing.T) { + t.Parallel() + cfg := &TrunkConfig{ActionPins: map[string]string{ + "actions/checkout": "abc123def4567890abc123def4567890abc12345 # v5.1.0", + }} + require.Empty(t, validateActionPins(cfg)) +} diff --git a/internal/config/validate_local_callback_test.go b/internal/config/validate_local_callback_test.go index bcb9afdf..c45e56f1 100644 --- a/internal/config/validate_local_callback_test.go +++ b/internal/config/validate_local_callback_test.go @@ -82,3 +82,43 @@ func TestValidateLocalCallbackWorkflowPath(t *testing.T) { }) } } + +// TestValidateLocalCallbackWorkflowPath_RejectsTraversal guards the callback +// workflow path sink: the .github/workflows/ prefix check alone does not +// reject a value that escapes the directory via a ".." traversal segment +// after that prefix. +func TestValidateLocalCallbackWorkflowPath_RejectsTraversal(t *testing.T) { + t.Parallel() + errs := validateLocalCallbackWorkflowPath("x", ".github/workflows/../../../secret") + if len(errs) == 0 { + t.Fatal("expected an error for a traversal path, got none") + } +} + +// TestValidateLocalCallbackWorkflowPath_RejectsUnsafeCharacters guards both +// accepted branches (bare filename and the .github/workflows/... path) +// against a value carrying a newline or other character that could break out +// of the emitted YAML scalar when the path is spliced raw into a generated +// workflow's uses: line. +func TestValidateLocalCallbackWorkflowPath_RejectsUnsafeCharacters(t *testing.T) { + t.Parallel() + if errs := validateLocalCallbackWorkflowPath("promote_callback", "x\n run: curl evil"); len(errs) == 0 { + t.Fatal("expected an error for a bare filename carrying a newline, got none") + } + if errs := validateLocalCallbackWorkflowPath("promote_callback", ".github/workflows/x.yaml\n run: curl evil"); len(errs) == 0 { + t.Fatal("expected an error for a .github/workflows/... path carrying a newline, got none") + } +} + +// TestValidateLocalCallbackWorkflowPath_RejectsUnsafeCrossRepoRef guards the +// remaining unguarded branch: a cross-repo "@"-containing ref is spliced raw +// into a generated workflow's uses: line just like the local forms, so it +// must reject a value carrying a newline or other character that could break +// out of the emitted YAML scalar too. +func TestValidateLocalCallbackWorkflowPath_RejectsUnsafeCrossRepoRef(t *testing.T) { + t.Parallel() + errs := validateLocalCallbackWorkflowPath("promote_callback", "owner/repo/.github/workflows/w.yaml@main\n run: curl evil") + if len(errs) == 0 { + t.Fatal("expected an error for a cross-repo ref carrying a newline, got none") + } +} diff --git a/internal/config/validate_v1.go b/internal/config/validate_v1.go index 67633929..647fb942 100644 --- a/internal/config/validate_v1.go +++ b/internal/config/validate_v1.go @@ -6,6 +6,7 @@ import ( "sort" "strings" "time" + "unicode" ) // Structural validation for the v1 reserved-shape fields. These rules are the @@ -59,6 +60,32 @@ var jobIDSafeNameRe = regexp.MustCompile(`^[A-Za-z0-9_-]+$`) // commitSHARe matches a full 40-character lowercase-hex Git commit SHA. var commitSHARe = regexp.MustCompile(`^[0-9a-f]{40}$`) +// actionFolderRe matches a safe action_folder name: a plain path component +// with no directory separators or traversal segments, since the generator +// joins it directly into a filesystem path under .github/actions/. +var actionFolderRe = regexp.MustCompile(`^[A-Za-z0-9._-]+$`) + +// validateActionFolder rejects an action_folder value that is empty, contains +// a path separator, contains a ".." traversal segment, or otherwise falls +// outside a safe plain-name charset. The generator joins the value directly +// into .github/actions//action.yaml, so an unsafe value could escape +// the intended actions directory. +func validateActionFolder(folder string) []string { + if folder == "" { + return nil + } + if strings.Contains(folder, "..") || strings.Contains(folder, "/") || !actionFolderRe.MatchString(folder) { + return []string{fmt.Sprintf("action_folder %q must be a plain folder name with no path separators or '..' segments", folder)} + } + return nil +} + +// actionPinValueRe bounds an action_pins override value to a ref plus an +// optional trailing "# " comment. It rejects newlines and any +// character that could break out of the emitted YAML scalar, since actionRef +// splices the value raw into a generated workflow. +var actionPinValueRe = regexp.MustCompile(`^[A-Za-z0-9._+/-]+(?: # [A-Za-z0-9._+-]+)?$`) + // dispatchInputOptionRe matches a choice dispatch-input option that is safe to // emit verbatim as a YAML block-sequence item under workflow_dispatch's // inputs..options. cascade renders each option unquoted, so the accepted @@ -168,24 +195,77 @@ func validateCallbackTimeout(prefix string, isReusableWorkflow bool, timeoutMinu "%s: timeout_minutes is not valid on a reusable-workflow callback; GitHub forbids timeout-minutes on a job that calls a reusable workflow - set timeout-minutes inside your callback workflow instead", prefix)} } +// localCallbackPathRe bounds a bare-filename or .github/workflows/... local +// callback value to characters safe to splice raw into a generated +// workflow's uses: line. It rejects newlines, other control characters, +// quotes, whitespace, and any character that could break out of the emitted +// YAML scalar, while allowing the path characters a callback workflow ref +// legitimately needs. The trailing "$" is Go's default (non-multiline) +// anchor, which matches only the true end of the string, so a trailing +// newline is rejected the same as an embedded one. +var localCallbackPathRe = regexp.MustCompile(`^[A-Za-z0-9._/-]+$`) + +// crossRepoCallbackRe bounds a cross-repo "@"-containing callback ref to a +// path, a single "@", and a ref, each drawn from the same safe path charset +// as localCallbackPathRe. This is the positive-charset counterpart to the +// up-front containsUnsafeChar guard below, applied to the one accepted shape +// that carries an "@". +var crossRepoCallbackRe = regexp.MustCompile(`^[A-Za-z0-9._/-]+@[A-Za-z0-9._/-]+$`) + +// containsUnsafeChar reports whether s contains a newline, carriage return, +// other control character, or whitespace. Every branch of +// validateLocalCallbackWorkflowPath ultimately splices its value raw into a +// generated workflow's uses: line, so this check is applied once, up front, +// rather than duplicated per branch: it keeps guarding every branch even if +// the function grows a new accepted shape later. +func containsUnsafeChar(s string) bool { + for _, r := range s { + if unicode.IsControl(r) || unicode.IsSpace(r) { + return true + } + } + return false +} + // validateLocalCallbackWorkflowPath checks that a local callback workflow path // is either a bare filename, a .github/workflows/... path, or a cross-repo // external ref (containing "@"). Any other form is rejected because GitHub -// requires local reusable workflows to live under .github/workflows/. +// requires local reusable workflows to live under .github/workflows/. Every +// accepted form is charset-validated: the value is spliced raw into a +// generated workflow's uses: line, so it must never carry a newline or other +// character that could break out of the emitted YAML scalar. func validateLocalCallbackWorkflowPath(prefix, workflow string) []string { if workflow == "" { return nil } - // Cross-repo external refs contain "@" - always valid. + if containsUnsafeChar(workflow) { + return []string{fmt.Sprintf("%s: local callback workflow %q contains unsafe characters", prefix, workflow)} + } + // Cross-repo external refs contain "@" - valid only when the whole value + // is a path@ref shape drawn from the safe path charset. if strings.Contains(workflow, "@") { + if !crossRepoCallbackRe.MatchString(workflow) { + return []string{fmt.Sprintf("%s: cross-repo local callback workflow %q must be a path@ref reference", prefix, workflow)} + } return nil } // Bare filename (no "/") - valid; normalizeWorkflowPath will route it. if !strings.Contains(workflow, "/") { + if !localCallbackPathRe.MatchString(workflow) { + return []string{fmt.Sprintf("%s: local callback workflow %q contains unsafe characters", prefix, workflow)} + } return nil } - // .github/workflows/... path - valid. + // .github/workflows/... path - valid only when it stays inside that + // directory and carries no unsafe characters; a ".." traversal segment + // after the prefix must not escape it. if strings.HasPrefix(workflow, ".github/workflows/") || strings.HasPrefix(workflow, "./.github/workflows/") { + if strings.Contains(workflow, "..") { + return []string{fmt.Sprintf("%s: local callback workflow must not contain '..' segments, got %q", prefix, workflow)} + } + if !localCallbackPathRe.MatchString(workflow) { + return []string{fmt.Sprintf("%s: local callback workflow %q contains unsafe characters", prefix, workflow)} + } return nil } return []string{fmt.Sprintf("%s: local callback workflow must be a .github/workflows/... path or a bare filename, got %q", prefix, workflow)} @@ -349,10 +429,28 @@ func validateConfigLevel(cfg *TrunkConfig) []string { errs = append(errs, validateEnvironmentConfig(cfg)...) errs = append(errs, validateTokenSources(cfg)...) errs = append(errs, validateRollback(cfg.Rollback)...) + errs = append(errs, validateActionPins(cfg)...) + errs = append(errs, validateActionFolder(cfg.ActionFolder)...) return errs } +// validateActionPins charset-validates every action_pins override value. Each +// value is spliced raw into a generated workflow's uses: line, so it must be +// bounded to a ref plus an optional trailing "# " comment and must +// never carry a newline or other character that could break out of the +// emitted YAML scalar. +func validateActionPins(cfg *TrunkConfig) []string { + var errs []string + for _, action := range sortedKeys(cfg.ActionPins) { + ref := cfg.ActionPins[action] + if !actionPinValueRe.MatchString(ref) { + errs = append(errs, fmt.Sprintf("action_pins[%q]: invalid ref %q", action, ref)) + } + } + return errs +} + // repositoryDispatchTypeRe matches a repository_dispatch event type that is safe // to emit verbatim under the on.repository_dispatch.types list. GitHub accepts // arbitrary client-chosen event-type strings, but cascade renders them into YAML diff --git a/internal/generate/actions.go b/internal/generate/actions.go index 39842eab..9f3c10f3 100644 --- a/internal/generate/actions.go +++ b/internal/generate/actions.go @@ -4,17 +4,27 @@ import ( "fmt" "os" "path/filepath" + "regexp" "strings" "github.com/stablekernel/cascade/internal/config" ) +// actionFolderRe mirrors the config package's action_folder charset check. +// This is a defense-in-depth guard: config validation already rejects an +// unsafe action_folder, but the generator refuses to join an unsafe value +// into a filesystem path rather than trusting the caller validated it. +var actionFolderRe = regexp.MustCompile(`^[A-Za-z0-9._-]+$`) + // RenderLocalActions returns the composite action file the manifest would // generate, paired with its rendered content, without writing anything to disk. // The path is baseDir/.github/actions//action.yaml where is // cfg.GetActionFolder() (default: "manage-release"). func RenderLocalActions(baseDir string, cfg *config.TrunkConfig) (PlannedFile, error) { actionFolder := cfg.GetActionFolder() + if strings.Contains(actionFolder, "..") || strings.Contains(actionFolder, "/") || !actionFolderRe.MatchString(actionFolder) { + return PlannedFile{}, fmt.Errorf("action_folder %q is not a safe plain folder name", actionFolder) + } actionPath := filepath.Join(baseDir, ".github", "actions", actionFolder, "action.yaml") return PlannedFile{Path: actionPath, Content: generateManageReleaseAction()}, nil } diff --git a/internal/generate/actions_test.go b/internal/generate/actions_test.go new file mode 100644 index 00000000..353ba311 --- /dev/null +++ b/internal/generate/actions_test.go @@ -0,0 +1,37 @@ +package generate + +import ( + "testing" + + "github.com/stablekernel/cascade/internal/config" + "github.com/stretchr/testify/require" +) + +// TestRenderLocalActions_RejectsUnsafeActionFolder is a defense-in-depth +// guard: even though config validation already rejects an unsafe +// action_folder, the generator itself refuses to join an unsafe value into a +// filesystem path rather than trusting the caller validated it. +func TestRenderLocalActions_RejectsUnsafeActionFolder(t *testing.T) { + t.Parallel() + cfg := &config.TrunkConfig{ActionFolder: "../../etc/foo"} + _, err := RenderLocalActions(t.TempDir(), cfg) + require.Error(t, err) +} + +// TestGenerateLocalActions_RejectsUnsafeActionFolder mirrors the above for the +// disk-writing entry point. +func TestGenerateLocalActions_RejectsUnsafeActionFolder(t *testing.T) { + t.Parallel() + cfg := &config.TrunkConfig{ActionFolder: "a/b"} + err := GenerateLocalActions(t.TempDir(), cfg) + require.Error(t, err) +} + +// TestRenderLocalActions_AllowsDefaultActionFolder is the regression lock: a +// safe, default action_folder still renders with no error. +func TestRenderLocalActions_AllowsDefaultActionFolder(t *testing.T) { + t.Parallel() + cfg := &config.TrunkConfig{} + _, err := RenderLocalActions(t.TempDir(), cfg) + require.NoError(t, err) +} diff --git a/internal/pinreconcile/push.go b/internal/pinreconcile/push.go new file mode 100644 index 00000000..88c1f266 --- /dev/null +++ b/internal/pinreconcile/push.go @@ -0,0 +1,16 @@ +// Package pinreconcile implements the pin-reconciliation engine: adopting an +// external action-pin change back into the manifest and regenerating so every +// owned file agrees with it again. +package pinreconcile + +// StagePathspec returns the ONLY pathspec a reconcile commit may stage, so a +// crafted working-tree change cannot ride along (never git add -A). The +// common case is manifest-only (Contents: write); pushWorkflows is set only +// when a regenerate must push workflow files (the Workflows: write path). +func StagePathspec(manifestPath string, pushWorkflows bool) []string { + spec := []string{manifestPath} + if pushWorkflows { + spec = append(spec, ".github/workflows/*.yaml") + } + return spec +} diff --git a/internal/pinreconcile/push_test.go b/internal/pinreconcile/push_test.go new file mode 100644 index 00000000..9b373548 --- /dev/null +++ b/internal/pinreconcile/push_test.go @@ -0,0 +1,22 @@ +package pinreconcile + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestStagePathspec_ManifestOnlyByDefault covers the common case: a reconcile +// commit stages only the manifest, never a wildcard pathspec that could pick +// up an unrelated working-tree change. +func TestStagePathspec_ManifestOnlyByDefault(t *testing.T) { + require.Equal(t, []string{".github/manifest.yaml"}, StagePathspec(".github/manifest.yaml", false)) +} + +// TestStagePathspec_IncludesWorkflowsWhenRegenerating covers the mode where a +// regenerate must itself push workflow files alongside the manifest. +func TestStagePathspec_IncludesWorkflowsWhenRegenerating(t *testing.T) { + require.Equal(t, + []string{".github/manifest.yaml", ".github/workflows/*.yaml"}, + StagePathspec(".github/manifest.yaml", true)) +}