diff --git a/.plumber.yaml b/.plumber.yaml index 54d4d379..35dfebb5 100644 --- a/.plumber.yaml +++ b/.plumber.yaml @@ -178,6 +178,60 @@ gitlab: # Minimum access level required to push (0=No one, 30=Developer, 40=Maintainer) minPushAccessLevel: 40 # =========================================== + # MR approval rules must require a minimum number of approvals + # =========================================== + # Flags approval rules covering all protected branches that require fewer + # approvals than the minimum below. GitLab Premium/Ultimate feature; on + # Free the approvals API returns no rules, so it passes vacuously. A + # genuine 401/403 reports not-evaluable. + mergeRequestApprovalRulesMustRequireMinimumApprovals: + enabled: true + minimumRequiredApprovals: 1 + # =========================================== + # MR approval rules must cover all protected branches + # =========================================== + # Flags a project where no approval rule applies to all protected branches + # (explicit "all protected branches" target only). GitLab Premium/Ultimate + # feature; on Free the approvals API returns empty, so this fires. A genuine + # 401/403 reports not-evaluable. + mergeRequestApprovalRulesMustCoverAllProtectedBranches: + enabled: true + # =========================================== + # MR approval settings must be compliant + # =========================================== + # Checks the project's MR approval settings against the expectations + # below (unset/false = not checked; behaviorWhenCommitIsAdded is a + # minimum on keep_approvals < remove_approvals_by_code_owners < + # remove_all_approvals). REQUIRES GITLAB PREMIUM OR ULTIMATE: on Free + # the settings do not exist and the API answers with defaults, so this + # fires. A genuine 401/403 reports not-evaluable. + mergeRequestApprovalSettingsMustBeCompliant: + enabled: true + preventApprovalByAuthor: true + preventApprovalsByCommitters: true + preventEditingApprovalRulesInMR: true + requireReAuthToApprove: false + behaviorWhenCommitIsAdded: remove_all_approvals + # =========================================== + # MR settings must be compliant + # =========================================== + # Requires the project's merge-request/merge settings (Settings > Merge + # requests) to match the values below exactly. Every field is optional: + # remove any you don't want enforced. mergeMethod is one of merge, ff, + # rebase_merge; squashOption is one of never, always, default_on, + # default_off. mergePipelinesEnabled and mergeTrainsEnabled are GitLab + # Premium/Ultimate (always false on Free, so drop them there). + mergeRequestSettingsMustBeCompliant: + enabled: false + mergeMethod: ff # linear history, no merge commits + squashOption: default_on # squash to one commit by default + mergePipelinesEnabled: true # run the pipeline on the merged result + mergeTrainsEnabled: false + allowMergeOnSkippedPipeline: false # never merge when CI was skipped + resolveOutdatedDiffDiscussions: true # auto-resolve stale review threads + printingMergeRequestLinkEnabled: true + removeSourceBranchAfterMerge: true # clean up the branch after merge + # =========================================== # Pipeline must not include hardcoded jobs # =========================================== # Detects CI/CD jobs that are defined directly in the .gitlab-ci.yml file diff --git a/README.md b/README.md index d6f95cce..0ba27e54 100644 --- a/README.md +++ b/README.md @@ -313,6 +313,7 @@ Plumber ships controls for: - container image pinning and authorized sources - branch protection +- GitLab merge request approval rules (minimum approvals, coverage of all protected branches) and approval settings (author/committer approval, per-MR overrides, re-authentication, approval reset) - unverified script execution (`curl | bash`, `base64 -d | bash`, etc.) - Docker-in-Docker - weakened security jobs diff --git a/cmd/analyze_shared.go b/cmd/analyze_shared.go index a9c885b7..ec4b9cf7 100644 --- a/cmd/analyze_shared.go +++ b/cmd/analyze_shared.go @@ -104,6 +104,9 @@ func outputTextWithProvider(p provider.Provider, result *control.AnalysisResult, // real) and drop the green stat blocks (#220). renderFindingGroups(filterGroupsForDegraded(groups, result.DataCollectionDegraded)) renderWarnings(result.Warnings) + renderApprovalRulesTierCaveat(result) + renderMRApprovalSettingsTierCaveat(result) + renderMRSettingsPremiumCaveat(result) printSectionHeader("Summary") fmt.Println() diff --git a/cmd/init.go b/cmd/init.go index 3743aacf..481d1a21 100644 --- a/cmd/init.go +++ b/cmd/init.go @@ -13,6 +13,7 @@ import ( "github.com/AlecAivazis/survey/v2" "github.com/getplumber/plumber/configuration" defaultconfig "github.com/getplumber/plumber/defaultConfig" + "github.com/getplumber/plumber/internal/ir" "github.com/sirupsen/logrus" "github.com/spf13/cobra" "golang.org/x/term" @@ -27,7 +28,7 @@ const ( catImages = "Container image security (tags, trusted registries)" catComposition = "Pipeline composition (includes, scripts, security jobs, DinD)" - catAccess = "Access control (branch protection)" + catAccess = "Access control (branch protection, MR approvals)" catVariables = "Variable security (debug trace, unsafe expansion)" // GitLab-applicable composition checks (existing). @@ -195,6 +196,20 @@ type initWizardState struct { BranchMinMergeAccessLevel string BranchMinPushAccessLevel string + // mergeRequestApprovalRulesMustRequireMinimumApprovals / + // ...MustCoverAllProtectedBranches (GitLab-only, catAccess). + MRApprovalMinEnabled bool + MRApprovalMinCount string + MRApprovalCoverAllEnabled bool + + // mergeRequestApprovalSettingsMustBeCompliant (GitLab-only, catAccess). + MRApprovalSettingsEnabled bool + MRApprovalSettingsPreventAuthor bool + MRApprovalSettingsPreventCommitters bool + MRApprovalSettingsPreventEditing bool + MRApprovalSettingsRequireReAuth bool + MRApprovalSettingsBehavior string + // pipelineMustNotEnableDebugTrace DebugForbiddenVariablesMultiline string @@ -490,6 +505,70 @@ func (st *initWizardState) askAccessQuestions() error { }, &st.BranchMinPushAccessLevel); err != nil { return err } + if err := survey.AskOne(&survey.Confirm{ + Message: "Flag MR approval rules that require fewer than a minimum number of approvals? (GitLab)", + Help: "Checks approval rules covering all protected branches against a minimum. Requires a token that can read approval rules (a premium feature). Ships off by default.", + Default: defaultMRApprovalMinEnabled(), + }, &st.MRApprovalMinEnabled); err != nil { + return err + } + if st.MRApprovalMinEnabled { + if err := survey.AskOne(&survey.Input{ + Message: "Minimum approvals a rule covering all protected branches must require (GitLab)", + Default: fmt.Sprintf("%d", defaultMRApprovalMinCount()), + }, &st.MRApprovalMinCount); err != nil { + return err + } + } + if err := survey.AskOne(&survey.Confirm{ + Message: "Flag projects where no MR approval rule covers all protected branches? (GitLab)", + Help: "A protected branch with no covering approval rule can be merged with no required approval. Ships off by default.", + Default: defaultMRApprovalCoverAllEnabled(), + }, &st.MRApprovalCoverAllEnabled); err != nil { + return err + } + if err := survey.AskOne(&survey.Confirm{ + Message: "Check MR approval settings against expectations? (GitLab)", + Help: "Compares the project's approval settings (author/committer approval, per-MR overrides, re-auth, approval reset) to the expectations you pick next. Ships off by default.", + Default: defaultMRApprovalSettingsEnabled(), + }, &st.MRApprovalSettingsEnabled); err != nil { + return err + } + if st.MRApprovalSettingsEnabled { + if err := survey.AskOne(&survey.Confirm{ + Message: "Expect that MR authors cannot approve their own merge requests? (GitLab)", + Default: defaultMRApprovalSettingsBool(func(c *configuration.MRApprovalSettingsControlConfig) *bool { return c.PreventApprovalByAuthor }), + }, &st.MRApprovalSettingsPreventAuthor); err != nil { + return err + } + if err := survey.AskOne(&survey.Confirm{ + Message: "Expect that users who committed to an MR cannot approve it? (GitLab)", + Default: defaultMRApprovalSettingsBool(func(c *configuration.MRApprovalSettingsControlConfig) *bool { return c.PreventApprovalsByCommitters }), + }, &st.MRApprovalSettingsPreventCommitters); err != nil { + return err + } + if err := survey.AskOne(&survey.Confirm{ + Message: "Expect that approval rules cannot be edited per merge request? (GitLab)", + Default: defaultMRApprovalSettingsBool(func(c *configuration.MRApprovalSettingsControlConfig) *bool { return c.PreventEditingApprovalRulesInMR }), + }, &st.MRApprovalSettingsPreventEditing); err != nil { + return err + } + if err := survey.AskOne(&survey.Confirm{ + Message: "Expect re-authentication to approve? (GitLab)", + Help: "Strict: every approval re-prompts credentials. Answering no leaves this setting unchecked.", + Default: defaultMRApprovalSettingsBool(func(c *configuration.MRApprovalSettingsControlConfig) *bool { return c.RequireReAuthToApprove }), + }, &st.MRApprovalSettingsRequireReAuth); err != nil { + return err + } + if err := survey.AskOne(&survey.Select{ + Message: "Minimum required behavior when a commit is added to an open MR (GitLab)", + Help: "A project below the chosen rung is flagged: keep_approvals (weakest, effectively unchecked) < remove_approvals_by_code_owners < remove_all_approvals.", + Options: mrApprovalBehaviorOptions(), + Default: defaultMRApprovalSettingsBehavior(), + }, &st.MRApprovalSettingsBehavior); err != nil { + return err + } + } } if hasProvider(st, "github") { if err := survey.AskOne(&survey.Confirm{ @@ -773,6 +852,70 @@ var embeddedDefault = sync.OnceValue(func() *configuration.PlumberConfig { func defaultGitLabControls() configuration.ControlsConfig { return embeddedDefault().GitLab.Controls } func defaultGitHubControls() configuration.ControlsConfig { return embeddedDefault().GitHub.Controls } +// defaultMRApproval* source the wizard's Confirm/Input defaults for the +// merge-request approval-rule controls from the shipped default, so the +// prompts and the zero-config baseline cannot drift. +func defaultMRApprovalMinEnabled() bool { + if c := defaultGitLabControls().MergeRequestApprovalRulesMustRequireMinimumApprovals; c != nil { + return c.IsEnabled() + } + return false +} + +func defaultMRApprovalMinCount() int { + if c := defaultGitLabControls().MergeRequestApprovalRulesMustRequireMinimumApprovals; c != nil && c.MinimumRequiredApprovals != nil { + return *c.MinimumRequiredApprovals + } + return 1 +} + +func defaultMRApprovalCoverAllEnabled() bool { + if c := defaultGitLabControls().MergeRequestApprovalRulesMustCoverAllProtectedBranches; c != nil { + return c.IsEnabled() + } + return false +} + +// defaultMRApprovalSettings* source the wizard's prompt defaults for the +// merge-request approval-settings control from the shipped default, so the +// prompts and the zero-config baseline cannot drift. +func defaultMRApprovalSettingsEnabled() bool { + if c := defaultGitLabControls().MergeRequestApprovalSettingsMustBeCompliant; c != nil { + return c.IsEnabled() + } + return false +} + +// defaultMRApprovalSettingsBool reads one optional boolean expectation from +// the shipped default via the field selector; an unset field defaults the +// prompt to false ("not checked"). +func defaultMRApprovalSettingsBool(field func(*configuration.MRApprovalSettingsControlConfig) *bool) bool { + if c := defaultGitLabControls().MergeRequestApprovalSettingsMustBeCompliant; c != nil { + if v := field(c); v != nil { + return *v + } + } + return false +} + +func defaultMRApprovalSettingsBehavior() string { + if c := defaultGitLabControls().MergeRequestApprovalSettingsMustBeCompliant; c != nil && c.BehaviorWhenCommitIsAdded != nil { + return *c.BehaviorWhenCommitIsAdded + } + return ir.MRApprovalBehaviorKeepApprovals +} + +// mrApprovalBehaviorOptions is the behavior ladder in strictness order, from +// the IR constants the projection emits (the same values config validation +// accepts). +func mrApprovalBehaviorOptions() []string { + return []string{ + ir.MRApprovalBehaviorKeepApprovals, + ir.MRApprovalBehaviorRemoveCodeOwnerApprovals, + ir.MRApprovalBehaviorRemoveAllApprovals, + } +} + // defaultForbiddenTags is the CSV prompt default for forbidden image tags, // sourced from the GitLab containerImageMustNotUseForbiddenTags default. func defaultForbiddenTags() string { @@ -1009,6 +1152,32 @@ func (st *initWizardState) applyAccessControls(gl, gh *configuration.ProviderCon MinMergeAccessLevel: intPtrInit(parseIntInit(st.BranchMinMergeAccessLevel, 30)), MinPushAccessLevel: intPtrInit(parseIntInit(st.BranchMinPushAccessLevel, 40)), } + if st.MRApprovalMinEnabled { + gl.Controls.MergeRequestApprovalRulesMustRequireMinimumApprovals = &configuration.MRApprovalRulesMinApprovalsControlConfig{ + Enabled: boolPtrInit(true), + MinimumRequiredApprovals: intPtrInit(parseIntInit(st.MRApprovalMinCount, defaultMRApprovalMinCount())), + } + } + if st.MRApprovalCoverAllEnabled { + gl.Controls.MergeRequestApprovalRulesMustCoverAllProtectedBranches = &configuration.EnabledOnlyControlConfig{Enabled: boolPtrInit(true)} + } + if st.MRApprovalSettingsEnabled { + // Every expectation is emitted, answered value included: a false + // boolean means "not checked" (same as unset) but keeps the full + // configuration surface visible in the generated file. + behavior := st.MRApprovalSettingsBehavior + if behavior == "" { + behavior = defaultMRApprovalSettingsBehavior() + } + gl.Controls.MergeRequestApprovalSettingsMustBeCompliant = &configuration.MRApprovalSettingsControlConfig{ + Enabled: boolPtrInit(true), + PreventApprovalByAuthor: boolPtrInit(st.MRApprovalSettingsPreventAuthor), + PreventApprovalsByCommitters: boolPtrInit(st.MRApprovalSettingsPreventCommitters), + PreventEditingApprovalRulesInMR: boolPtrInit(st.MRApprovalSettingsPreventEditing), + RequireReAuthToApprove: boolPtrInit(st.MRApprovalSettingsRequireReAuth), + BehaviorWhenCommitIsAdded: &behavior, + } + } } if gh != nil { // GitHub branch-protection ignores access-level fields. diff --git a/cmd/init_mr_approval_test.go b/cmd/init_mr_approval_test.go new file mode 100644 index 00000000..fbf40491 --- /dev/null +++ b/cmd/init_mr_approval_test.go @@ -0,0 +1,119 @@ +package cmd + +import ( + "testing" + + "github.com/getplumber/plumber/configuration" +) + +// applyAccessControls maps the three MR-approval wizard answers onto GitLab +// config. A field mix-up (wrong parse target, inverted Enabled, or one control +// writing the other's field) would silently emit a wrong .plumber.yaml from +// `plumber config init` with no drift guard, since both controls ship disabled. +// This pins the mapping. +func TestApplyAccessControls_MRApprovalMapping(t *testing.T) { + t.Run("min-approvals: enabled with the parsed count", func(t *testing.T) { + gl := &configuration.ProviderConfig{} + (&initWizardState{MRApprovalMinEnabled: true, MRApprovalMinCount: "3"}).applyAccessControls(gl, nil) + c := gl.Controls.MergeRequestApprovalRulesMustRequireMinimumApprovals + if c == nil || !c.IsEnabled() { + t.Fatal("min-approvals control should be enabled") + } + if c.MinimumRequiredApprovals == nil || *c.MinimumRequiredApprovals != 3 { + t.Fatalf("MinimumRequiredApprovals = %v, want 3", c.MinimumRequiredApprovals) + } + if gl.Controls.MergeRequestApprovalRulesMustCoverAllProtectedBranches != nil { + t.Fatal("cover-all must stay unset when only min-approvals was chosen (field mix-up)") + } + }) + t.Run("cover-all: enabled, min-approvals untouched", func(t *testing.T) { + gl := &configuration.ProviderConfig{} + (&initWizardState{MRApprovalCoverAllEnabled: true}).applyAccessControls(gl, nil) + if c := gl.Controls.MergeRequestApprovalRulesMustCoverAllProtectedBranches; c == nil || !c.IsEnabled() { + t.Fatal("cover-all control should be enabled") + } + if gl.Controls.MergeRequestApprovalRulesMustRequireMinimumApprovals != nil { + t.Fatal("min-approvals must stay unset when only cover-all was chosen (field mix-up)") + } + }) + t.Run("neither approval control set when neither chosen", func(t *testing.T) { + gl := &configuration.ProviderConfig{} + (&initWizardState{}).applyAccessControls(gl, nil) + if gl.Controls.MergeRequestApprovalRulesMustRequireMinimumApprovals != nil || gl.Controls.MergeRequestApprovalRulesMustCoverAllProtectedBranches != nil { + t.Fatal("no approval controls should be set when neither was chosen") + } + }) + // The wizard defaults must come from the embedded shipped default (both ship + // disabled), so the prompt defaults and the zero-config baseline cannot drift. + t.Run("defaults sourced from the embedded shipped default", func(t *testing.T) { + if defaultMRApprovalMinEnabled() { + t.Error("defaultMRApprovalMinEnabled should be false (ships disabled)") + } + if defaultMRApprovalCoverAllEnabled() { + t.Error("defaultMRApprovalCoverAllEnabled should be false (ships disabled)") + } + if got := defaultMRApprovalMinCount(); got < 1 { + t.Errorf("defaultMRApprovalMinCount = %d, want >= 1 (the shipped minimum)", got) + } + }) +} + +// applyAccessControls maps the approval-settings wizard answers onto GitLab +// config: enabled emits the FULL surface (all five expectations, answered +// values included — a false boolean documents "not checked"), disabled leaves +// the control unset. +func TestApplyAccessControls_MRApprovalSettingsMapping(t *testing.T) { + t.Run("enabled: full surface with the answered values", func(t *testing.T) { + gl := &configuration.ProviderConfig{} + (&initWizardState{ + MRApprovalSettingsEnabled: true, + MRApprovalSettingsPreventAuthor: true, + MRApprovalSettingsPreventCommitters: true, + MRApprovalSettingsRequireReAuth: false, + MRApprovalSettingsBehavior: "remove_all_approvals", + }).applyAccessControls(gl, nil) + c := gl.Controls.MergeRequestApprovalSettingsMustBeCompliant + if c == nil || !c.IsEnabled() { + t.Fatal("approval-settings control should be enabled") + } + if c.PreventApprovalByAuthor == nil || !*c.PreventApprovalByAuthor { + t.Errorf("PreventApprovalByAuthor = %v, want true", c.PreventApprovalByAuthor) + } + if c.PreventApprovalsByCommitters == nil || !*c.PreventApprovalsByCommitters { + t.Errorf("PreventApprovalsByCommitters = %v, want true", c.PreventApprovalsByCommitters) + } + if c.RequireReAuthToApprove == nil || *c.RequireReAuthToApprove { + t.Errorf("RequireReAuthToApprove = %v, want an explicit false (documented surface)", c.RequireReAuthToApprove) + } + if c.BehaviorWhenCommitIsAdded == nil || *c.BehaviorWhenCommitIsAdded != "remove_all_approvals" { + t.Errorf("BehaviorWhenCommitIsAdded = %v, want remove_all_approvals", c.BehaviorWhenCommitIsAdded) + } + // Whatever the wizard emits must pass the same validation the loaded + // file goes through — a wizard writing an invalid file is the exact + // drift the embedded-default sourcing exists to prevent. + if err := (&configuration.PlumberConfig{Version: "2.0", GitLab: gl}).Validate(); err != nil { + t.Errorf("wizard-emitted config failed validation: %v", err) + } + }) + t.Run("disabled: control stays unset", func(t *testing.T) { + gl := &configuration.ProviderConfig{} + (&initWizardState{}).applyAccessControls(gl, nil) + if gl.Controls.MergeRequestApprovalSettingsMustBeCompliant != nil { + t.Fatal("approval-settings control must stay unset when not chosen") + } + }) + t.Run("behavior options match what config validation accepts", func(t *testing.T) { + for _, opt := range mrApprovalBehaviorOptions() { + cfg := &configuration.PlumberConfig{Version: "2.0", GitLab: &configuration.ProviderConfig{ + Controls: configuration.ControlsConfig{ + MergeRequestApprovalSettingsMustBeCompliant: &configuration.MRApprovalSettingsControlConfig{ + BehaviorWhenCommitIsAdded: &opt, + }, + }, + }} + if err := cfg.Validate(); err != nil { + t.Errorf("wizard option %q rejected by config validation: %v", opt, err) + } + } + }) +} diff --git a/cmd/legacy_json.go b/cmd/legacy_json.go index 0489987f..3aba11a5 100644 --- a/cmd/legacy_json.go +++ b/cmd/legacy_json.go @@ -98,10 +98,67 @@ func _withControlMeta(block any, e control.ControlEntry, result *control.Analysi if m, ok := block.(map[string]any); ok { m["controlName"] = e.ControlName m["status"] = control.StatusFor(e, result, findingCount) + if result != nil && result.ApprovalRulesTierCaveat && isApprovalRuleControl(e.ControlName) { + // Structured so a consumer can key on it: the approvals API returned + // no rules, which on GitLab Free means the feature is unavailable + // (the API 200-empties) rather than a real misconfiguration. + m["tierCaveat"] = map[string]any{ + "reason": "no-approval-rules-returned", + "requiresTier": "premium_or_ultimate", + "message": approvalRulesTierCaveatMessage, + } + } + if result != nil && result.MRApprovalSettingsTierCaveat && e.ControlName == "mergeRequestApprovalSettingsMustBeCompliant" { + // Same tier-ambiguity as the rules caveat, one step blinder: the + // settings API 200-defaults every protection off on GitLab Free, so a + // no-protection read can't be told apart from a genuinely unlocked paid + // project. Keyed so a consumer can suppress or annotate ISSUE-503. + m["tierCaveat"] = map[string]any{ + "reason": "no-approval-protections", + "requiresTier": "premium_or_ultimate", + "message": mrApprovalSettingsTierCaveatMessage, + } + } + if result != nil && len(result.MRSettingsPremiumCaveatFields) > 0 && e.ControlName == "mergeRequestSettingsMustBeCompliant" { + // One or more configured MR-setting expectations require a Premium/ + // Ultimate feature the project cannot turn on (merge trains, merged- + // results pipelines). Keyed with the offending fields so a consumer can + // annotate ISSUE-506 or advise disabling just those expectations. + m["tierCaveat"] = map[string]any{ + "reason": "premium-settings-expected-on", + "requiresTier": "premium_or_ultimate", + "fields": result.MRSettingsPremiumCaveatFields, + "message": mrSettingsTierCaveatMessage, + } + } } return block } +// approvalRulesTierCaveatMessage explains the Premium/Ultimate requirement for +// the MR approval-rule controls when the approvals API returned no rules. +// Shared by the terminal caveat (render_details.go) and the JSON tierCaveat. +const approvalRulesTierCaveatMessage = "MR approval rules are a GitLab Premium/Ultimate feature. Disable these controls if you don't have GitLab Premium or Ultimate." + +// mrApprovalSettingsTierCaveatMessage explains the Premium/Ultimate requirement +// for the MR approval-settings control (ISSUE-503) when the project has no +// approval protection in effect. Shared by the terminal caveat +// (render_details.go) and the JSON tierCaveat. +const mrApprovalSettingsTierCaveatMessage = "MR approval settings are a GitLab Premium/Ultimate feature and this project has no approval protections in effect. If you're not on GitLab Premium or Ultimate you can't set them — disable this control." + +// mrSettingsTierCaveatMessage explains that one or more configured MR-setting +// expectations (see the tierCaveat "fields") require a GitLab Premium/Ultimate +// feature the project cannot turn on. Shared by the terminal caveat +// (render_details.go) and the JSON tierCaveat. +const mrSettingsTierCaveatMessage = "One or more expected MR settings are GitLab Premium/Ultimate features that read as off (see fields). The API gives no tier signal, so this may be a Free project that cannot enable them or a paid project that left them off. If this project is not on GitLab Premium/Ultimate, set those expectations to false or remove them from mergeRequestSettingsMustBeCompliant; if it is, enable the feature to satisfy the check." + +// isApprovalRuleControl reports whether a control name is one of the two +// GitLab MR approval-rule controls the tier caveat applies to. +func isApprovalRuleControl(name string) bool { + return name == "mergeRequestApprovalRulesMustRequireMinimumApprovals" || + name == "mergeRequestApprovalRulesMustCoverAllProtectedBranches" +} + // buildLegacyResult routes a control entry to its legacy JSON // builder and returns the (jsonKey, block) pair. func buildLegacyResult(e control.ControlEntry, result *control.AnalysisResult, pc *configuration.PlumberConfig, findings []opaengine.Finding) (string, any) { @@ -144,10 +201,85 @@ func buildLegacyResult(e control.ControlEntry, result *control.AnalysisResult, p return "jobVariablesOverrideResult", buildJobVariablesOverrideBlock(common, result, findings) case "pipelineMustNotUseDockerInDocker": return "dockerInDockerResult", buildDockerInDockerBlock(common, result, findings) + case "mergeRequestApprovalRulesMustRequireMinimumApprovals": + return "mrApprovalRulesMinApprovalsResult", buildMRApprovalRulesMinApprovalsBlock(common, findings) + case "mergeRequestApprovalRulesMustCoverAllProtectedBranches": + return "mrApprovalRulesCoverAllBranchesResult", buildMRApprovalRulesCoverAllBranchesBlock(common, findings) + case "mergeRequestApprovalSettingsMustBeCompliant": + return "mrApprovalSettingsResult", buildMRApprovalSettingsBlock(common, findings) + case "mergeRequestSettingsMustBeCompliant": + return "mrSettingsResult", buildMRSettingsBlock(common, findings) } return "", nil } +// buildMRApprovalRulesMinApprovalsBlock and +// buildMRApprovalRulesCoverAllBranchesBlock emit the legacy JSON blocks for the +// merge-request approval-rule controls. The findings are settings-level (no +// file/job), so each issue carries the approval-rule identity (approvalRuleId) +// plus the ruleName / approvalsRequired / minApprovalsRequired data that +// projectFindings preserves from f.Data. +func buildMRApprovalRulesMinApprovalsBlock(c legacyCommon, findings []opaengine.Finding) map[string]any { + return map[string]any{ + "issues": projectFindings(findings, "job"), + "metrics": map[string]any{ + "rulesBelowMinimum": len(findings), + }, + "version": "0.1.0", + "ciValid": c.CiValid, + "ciMissing": c.CiMissing, + "skipped": c.Skipped, + } +} + +func buildMRApprovalRulesCoverAllBranchesBlock(c legacyCommon, findings []opaengine.Finding) map[string]any { + return map[string]any{ + "issues": projectFindings(findings, "job"), + "metrics": map[string]any{ + "allProtectedBranchesRuleMissing": len(findings), + }, + "version": "0.1.0", + "ciValid": c.CiValid, + "ciMissing": c.CiMissing, + "skipped": c.Skipped, + } +} + +// buildMRApprovalSettingsBlock emits the legacy JSON block for the +// merge-request approval-settings control (ISSUE-503). The finding is a +// settings-level singleton (no file/job); its deviatingSettings list and the +// project's behaviorWhenCommitIsAdded ride in the issue's data, preserved by +// projectFindings. +func buildMRApprovalSettingsBlock(c legacyCommon, findings []opaengine.Finding) map[string]any { + return map[string]any{ + "issues": projectFindings(findings, "job"), + "metrics": map[string]any{ + "hasNonCompliantSettings": len(findings), + }, + "version": "0.1.0", + "ciValid": c.CiValid, + "ciMissing": c.CiMissing, + "skipped": c.Skipped, + } +} + +// buildMRSettingsBlock emits the legacy JSON block for the merge-request +// settings control (ISSUE-506). The finding is a settings-level singleton (no +// file/job); its deviatingSettings list rides in the issue's data, preserved by +// projectFindings. +func buildMRSettingsBlock(c legacyCommon, findings []opaengine.Finding) map[string]any { + return map[string]any{ + "issues": projectFindings(findings, "job"), + "metrics": map[string]any{ + "hasNonCompliantSettings": len(findings), + }, + "version": "0.1.0", + "ciValid": c.CiValid, + "ciMissing": c.CiMissing, + "skipped": c.Skipped, + } +} + // legacyCommon carries the bookkeeping fields shared by every // `*Result` block: ciValid, ciMissing, skipped. type legacyCommon struct { diff --git a/cmd/legacy_json_gitlab_test.go b/cmd/legacy_json_gitlab_test.go new file mode 100644 index 00000000..0177a422 --- /dev/null +++ b/cmd/legacy_json_gitlab_test.go @@ -0,0 +1,240 @@ +package cmd + +import ( + "testing" + + "github.com/getplumber/plumber/control" + opaengine "github.com/getplumber/plumber/internal/engine/opa" +) + +// TestMRApprovalRulesJSONBlocks locks the results.json detail blocks for the +// two GitLab merge-request approval-rule controls (ISSUE-502/504). Before the +// per-control wiring, buildLegacyResult would return ("", nil) and the blocks +// would be dropped entirely — a dashboard would see the score deficit in +// plumberScore.codeLosses with no per-control detail. Mirrors the GitHub +// TestPullRequestTargetHeadCheckoutJSONBlock, the guard for the same bug class. +func TestMRApprovalRulesJSONBlocks(t *testing.T) { + result := &control.AnalysisResult{CiValid: true} + + // ISSUE-502: minimum-approvals — one failing rule, keyed on the stable ID. + minEntry := control.ControlEntry{ + DisplayName: "MR approval rules must require a minimum number of approvals", + ControlName: "mergeRequestApprovalRulesMustRequireMinimumApprovals", + } + minFindings := []opaengine.Finding{{ + Code: "ISSUE-502", + Data: map[string]any{ + "approvalRuleId": "42", + "ruleName": "Security", + "approvalsRequired": 1, + "minApprovalsRequired": 2, + }, + }} + + name, block := buildLegacyResult(minEntry, result, nil, minFindings) + if name != "mrApprovalRulesMinApprovalsResult" { + t.Fatalf("502 block name = %q, want mrApprovalRulesMinApprovalsResult (dispatch dropped the block)", name) + } + m, ok := block.(map[string]any) + if !ok { + t.Fatalf("502 block is %T, want map[string]any", block) + } + issues, ok := m["issues"].([]map[string]any) + if !ok || len(issues) != 1 { + t.Fatalf("502 issues = %v, want exactly 1 entry", m["issues"]) + } + if issues[0]["code"] != "ISSUE-502" { + t.Errorf("502 issue code = %v, want ISSUE-502", issues[0]["code"]) + } + // The rule identity (approvalRuleId) must survive into the issue block, so a + // platform can group the finding across runs; the renameable name is data. + if issues[0]["approvalRuleId"] != "42" { + t.Errorf("502 issue must carry approvalRuleId=42, got %v", issues[0]["approvalRuleId"]) + } + if metrics, ok := m["metrics"].(map[string]any); !ok || metrics["rulesBelowMinimum"] != 1 { + t.Errorf("502 metrics.rulesBelowMinimum = %v, want 1", m["metrics"]) + } + + // ISSUE-504: cover-all — singleton finding. + coverEntry := control.ControlEntry{ + DisplayName: "MR approval rules must cover all protected branches", + ControlName: "mergeRequestApprovalRulesMustCoverAllProtectedBranches", + } + coverFindings := []opaengine.Finding{{ + Code: "ISSUE-504", + Data: map[string]any{"totalRules": 2}, + }} + + name, block = buildLegacyResult(coverEntry, result, nil, coverFindings) + if name != "mrApprovalRulesCoverAllBranchesResult" { + t.Fatalf("504 block name = %q, want mrApprovalRulesCoverAllBranchesResult (dispatch dropped the block)", name) + } + m, ok = block.(map[string]any) + if !ok { + t.Fatalf("504 block is %T, want map[string]any", block) + } + if issues, ok := m["issues"].([]map[string]any); !ok || len(issues) != 1 || issues[0]["code"] != "ISSUE-504" { + t.Fatalf("504 issues = %v, want exactly 1 ISSUE-504 entry", m["issues"]) + } + if metrics, ok := m["metrics"].(map[string]any); !ok || metrics["allProtectedBranchesRuleMissing"] != 1 { + t.Errorf("504 metrics.allProtectedBranchesRuleMissing = %v, want 1", m["metrics"]) + } + + // A clean run (no findings) still returns a block, with an empty issues list + // — the control is present and evaluated, not absent. + if _, clean := buildLegacyResult(minEntry, result, nil, nil); clean == nil { + t.Errorf("502 clean run returned a nil block; the control must still appear") + } +} + +// TestMRApprovalSettingsJSONBlock locks the results.json detail block for the +// GitLab merge-request approval-settings control (ISSUE-503): the dispatch +// must route it (an unrouted control's block is dropped entirely) and the +// deviation detail must survive into the issue entry. +func TestMRApprovalSettingsJSONBlock(t *testing.T) { + result := &control.AnalysisResult{CiValid: true} + entry := control.ControlEntry{ + DisplayName: "MR approval settings must be compliant", + ControlName: "mergeRequestApprovalSettingsMustBeCompliant", + } + findings := []opaengine.Finding{{ + Code: "ISSUE-503", + Data: map[string]any{ + "deviatingSettings": []any{"preventApprovalByAuthor", "requireReAuthToApprove"}, + "behaviorWhenCommitIsAdded": "keep_approvals", + }, + }} + + name, block := buildLegacyResult(entry, result, nil, findings) + if name != "mrApprovalSettingsResult" { + t.Fatalf("503 block name = %q, want mrApprovalSettingsResult (dispatch dropped the block)", name) + } + m, ok := block.(map[string]any) + if !ok { + t.Fatalf("503 block is %T, want map[string]any", block) + } + issues, ok := m["issues"].([]map[string]any) + if !ok || len(issues) != 1 { + t.Fatalf("503 issues = %v, want exactly 1 entry (singleton)", m["issues"]) + } + if issues[0]["code"] != "ISSUE-503" { + t.Errorf("503 issue code = %v, want ISSUE-503", issues[0]["code"]) + } + // The deviation list is the actionable payload: a consumer must see WHICH + // settings fell short, not just that something did. + devs, ok := issues[0]["deviatingSettings"].([]any) + if !ok || len(devs) != 2 { + t.Errorf("503 issue must carry the deviatingSettings list, got %v", issues[0]["deviatingSettings"]) + } + if metrics, ok := m["metrics"].(map[string]any); !ok || metrics["hasNonCompliantSettings"] != 1 { + t.Errorf("503 metrics.hasNonCompliantSettings = %v, want 1", m["metrics"]) + } + + // A clean run still returns a block: present and evaluated, not absent. + if _, clean := buildLegacyResult(entry, result, nil, nil); clean == nil { + t.Errorf("503 clean run returned a nil block; the control must still appear") + } +} + +// TestMRApprovalRulesTierCaveatJSON pins the structured Premium/Ultimate caveat +// stamped onto the approval-rule blocks when the run flagged the ambiguous +// zero-rules case (GitLab Free returns an empty list). It must attach only to +// the two approval-rule controls, and only when the run set the flag. +func TestMRApprovalRulesTierCaveatJSON(t *testing.T) { + flagged := &control.AnalysisResult{CiValid: true, ApprovalRulesTierCaveat: true} + approval := control.ControlEntry{ControlName: "mergeRequestApprovalRulesMustCoverAllProtectedBranches"} + + block := _withControlMeta(map[string]any{"issues": []map[string]any{}}, approval, flagged, 0) + m := block.(map[string]any) + tc, ok := m["tierCaveat"].(map[string]any) + if !ok { + t.Fatalf("expected a tierCaveat on the approval-rule block, got %v", m) + } + if tc["reason"] != "no-approval-rules-returned" || tc["requiresTier"] != "premium_or_ultimate" { + t.Errorf("tierCaveat shape mismatch: %v", tc) + } + + // A non-approval control must NOT get the caveat, even when the flag is set. + other := _withControlMeta(map[string]any{}, control.ControlEntry{ControlName: "branchMustBeProtected"}, flagged, 0) + if _, present := other.(map[string]any)["tierCaveat"]; present { + t.Errorf("tierCaveat leaked onto a non-approval control") + } + + // Flag not set (rules were present, or a premium project): no caveat. + clean := &control.AnalysisResult{CiValid: true} + if _, present := _withControlMeta(map[string]any{}, approval, clean, 0).(map[string]any)["tierCaveat"]; present { + t.Errorf("tierCaveat present when the run did not flag it") + } +} + +// TestMRApprovalSettingsTierCaveatJSON pins the structured Premium/Ultimate +// caveat stamped onto the ISSUE-503 block when the run flagged the all-settings- +// false case (the GitLab Free signature). It must attach only to the +// approval-settings control, and only when the run set the flag. +func TestMRApprovalSettingsTierCaveatJSON(t *testing.T) { + flagged := &control.AnalysisResult{CiValid: true, MRApprovalSettingsTierCaveat: true} + settings := control.ControlEntry{ControlName: "mergeRequestApprovalSettingsMustBeCompliant"} + + block := _withControlMeta(map[string]any{"issues": []map[string]any{}}, settings, flagged, 1) + m := block.(map[string]any) + tc, ok := m["tierCaveat"].(map[string]any) + if !ok { + t.Fatalf("expected a tierCaveat on the approval-settings block, got %v", m) + } + if tc["reason"] != "no-approval-protections" || tc["requiresTier"] != "premium_or_ultimate" { + t.Errorf("tierCaveat shape mismatch: %v", tc) + } + + // The rules flag must NOT stamp the settings caveat, and vice versa. + rulesFlagged := &control.AnalysisResult{CiValid: true, ApprovalRulesTierCaveat: true} + if _, present := _withControlMeta(map[string]any{}, settings, rulesFlagged, 1).(map[string]any)["tierCaveat"]; present { + t.Errorf("rules caveat flag must not stamp the settings block") + } + + // A non-settings control must NOT get the caveat, even when the flag is set. + other := _withControlMeta(map[string]any{}, control.ControlEntry{ControlName: "branchMustBeProtected"}, flagged, 0) + if _, present := other.(map[string]any)["tierCaveat"]; present { + t.Errorf("tierCaveat leaked onto a non-settings control") + } + + // Flag not set (a paid project with a real misconfiguration): no caveat. + clean := &control.AnalysisResult{CiValid: true} + if _, present := _withControlMeta(map[string]any{}, settings, clean, 1).(map[string]any)["tierCaveat"]; present { + t.Errorf("tierCaveat present when the run did not flag it") + } +} + +// TestMRSettingsPremiumCaveatJSON pins the ISSUE-506 tierCaveat: when the run +// flagged Premium expectations the project can't satisfy, the block carries the +// offending field list. It attaches only to the MR-settings control and only +// when the field list is non-empty. +func TestMRSettingsPremiumCaveatJSON(t *testing.T) { + flagged := &control.AnalysisResult{CiValid: true, MRSettingsPremiumCaveatFields: []string{"mergePipelinesEnabled"}} + settings := control.ControlEntry{ControlName: "mergeRequestSettingsMustBeCompliant"} + + block := _withControlMeta(map[string]any{"issues": []map[string]any{}}, settings, flagged, 1) + m := block.(map[string]any) + tc, ok := m["tierCaveat"].(map[string]any) + if !ok { + t.Fatalf("expected a tierCaveat on the MR-settings block, got %v", m) + } + if tc["reason"] != "premium-settings-expected-on" || tc["requiresTier"] != "premium_or_ultimate" { + t.Errorf("tierCaveat shape mismatch: %v", tc) + } + fields, _ := tc["fields"].([]string) + if len(fields) != 1 || fields[0] != "mergePipelinesEnabled" { + t.Errorf("tierCaveat must carry the offending fields, got %v", tc["fields"]) + } + + // A non-MR-settings control must NOT get the caveat, even when the flag is set. + other := _withControlMeta(map[string]any{}, control.ControlEntry{ControlName: "branchMustBeProtected"}, flagged, 0) + if _, present := other.(map[string]any)["tierCaveat"]; present { + t.Errorf("tierCaveat leaked onto a non-MR-settings control") + } + + // Empty field list (a Free-satisfiable config): no caveat. + clean := &control.AnalysisResult{CiValid: true} + if _, present := _withControlMeta(map[string]any{}, settings, clean, 1).(map[string]any)["tierCaveat"]; present { + t.Errorf("tierCaveat present when no premium field was flagged") + } +} diff --git a/cmd/render_details.go b/cmd/render_details.go index 6c7e48a6..c33d36a7 100644 --- a/cmd/render_details.go +++ b/cmd/render_details.go @@ -76,6 +76,49 @@ func renderWarnings(warnings []string) { fmt.Printf(" %s↳ set PLUMBER_METADATA_TOKEN (a token with public-repo read) to resolve blocked action versions — see the README.%s\n", colorYellow, colorReset) } +// renderApprovalRulesTierCaveat prints a caveat when an MR approval-rule +// control ran against a project that returned zero approval rules: the feature +// requires GitLab Premium/Ultimate, and on Free the API returns an empty list, +// so the result may not reflect a real misconfiguration (see +// AnalysisResult.ApprovalRulesTierCaveat). No-op otherwise. +func renderApprovalRulesTierCaveat(result *control.AnalysisResult) { + if result == nil || !result.ApprovalRulesTierCaveat { + return + } + fmt.Println() + fmt.Printf(" %s⚠ MR approval rules are a GitLab Premium/Ultimate feature.%s\n", colorYellow, colorReset) + fmt.Printf(" %s•%s Disable these controls if you don't have GitLab Premium or Ultimate.\n", colorYellow, colorReset) +} + +// renderMRApprovalSettingsTierCaveat prints a caveat when the MR +// approval-settings control ran against a project with no approval protection in +// effect: the feature requires GitLab Premium/Ultimate, and on Free the API +// 200-defaults every protection off, so the operator cannot lock any of them +// down (see AnalysisResult.MRApprovalSettingsTierCaveat). No-op otherwise. +func renderMRApprovalSettingsTierCaveat(result *control.AnalysisResult) { + if result == nil || !result.MRApprovalSettingsTierCaveat { + return + } + fmt.Println() + fmt.Printf(" %s⚠ MR approval settings are a GitLab Premium/Ultimate feature.%s\n", colorYellow, colorReset) + fmt.Printf(" %s•%s No approval protections are in effect — if you're not on GitLab Premium or Ultimate you can't set them, so disable this control.\n", colorYellow, colorReset) +} + +// renderMRSettingsPremiumCaveat prints a caveat when ISSUE-506 requires +// Premium/Ultimate MR settings the project cannot turn on (merge trains, +// merged-results pipelines). It names the specific expectations so the operator +// can set them false or remove them instead of hitting an unfixable failure +// (see AnalysisResult.MRSettingsPremiumCaveatFields). No-op otherwise. +func renderMRSettingsPremiumCaveat(result *control.AnalysisResult) { + if result == nil || len(result.MRSettingsPremiumCaveatFields) == 0 { + return + } + fields := strings.Join(result.MRSettingsPremiumCaveatFields, ", ") + fmt.Println() + fmt.Printf(" %s⚠ These expected MR settings are GitLab Premium/Ultimate features and read as off: %s%s\n", colorYellow, fields, colorReset) + fmt.Printf(" %s•%s If this project isn't on GitLab Premium/Ultimate it can't enable them, so set those expectations to false (or remove them) in mergeRequestSettingsMustBeCompliant. If it is, turn the feature on to satisfy the check.\n", colorYellow, colorReset) +} + // renderDegradedCaveat prints an up-front warning that the run scored // against incomplete data because one or more collection/enrichment // steps failed (#220). Without it a partial GitHub run looks identical @@ -852,6 +895,22 @@ func buildGitLabControlStats(controlName string, result *control.AnalysisResult, {Label: statActionRefsChecked, Value: fmt.Sprintf("%d", actionRefs)}, {Label: "Mutable Remote Exec Found", Value: fmt.Sprintf("%d", findingsCount)}, } + case "mergeRequestApprovalRulesMustRequireMinimumApprovals": + return []statLine{ + {Label: "Rules Below Minimum", Value: fmt.Sprintf("%d", findingsCount)}, + } + case "mergeRequestApprovalRulesMustCoverAllProtectedBranches": + return []statLine{ + {Label: "All-Branches Rule Missing", Value: fmt.Sprintf("%d", findingsCount)}, + } + case "mergeRequestApprovalSettingsMustBeCompliant": + return []statLine{ + {Label: "Non-Compliant Approval Settings", Value: fmt.Sprintf("%d", findingsCount)}, + } + case "mergeRequestSettingsMustBeCompliant": + return []statLine{ + {Label: "Non-Compliant MR Settings", Value: fmt.Sprintf("%d", findingsCount)}, + } case "branchMustBeProtected": total, toProtect, protected, unprotected := _branchProtectionCounts(result, pc) nonCompliant := 0 diff --git a/configuration/plumberconfig.go b/configuration/plumberconfig.go index bbaaaefe..82352931 100644 --- a/configuration/plumberconfig.go +++ b/configuration/plumberconfig.go @@ -4,7 +4,9 @@ import ( "errors" "fmt" "os" + "slices" "sort" + "strings" defaultconfig "github.com/getplumber/plumber/defaultConfig" "github.com/getplumber/plumber/utils" @@ -36,6 +38,23 @@ var validControlSchema = map[string][]string{ "allowForcePush", "codeOwnerApprovalRequired", "minMergeAccessLevel", "minPushAccessLevel", }, + "mergeRequestApprovalRulesMustRequireMinimumApprovals": { + "enabled", "minimumRequiredApprovals", + }, + "mergeRequestApprovalRulesMustCoverAllProtectedBranches": { + "enabled", + }, + "mergeRequestApprovalSettingsMustBeCompliant": { + "enabled", "preventApprovalByAuthor", "preventApprovalsByCommitters", + "preventEditingApprovalRulesInMR", "requireReAuthToApprove", + "behaviorWhenCommitIsAdded", + }, + "mergeRequestSettingsMustBeCompliant": { + "enabled", "mergeMethod", "squashOption", "mergePipelinesEnabled", + "mergeTrainsEnabled", "allowMergeOnSkippedPipeline", + "resolveOutdatedDiffDiscussions", "printingMergeRequestLinkEnabled", + "removeSourceBranchAfterMerge", + }, "pipelineMustNotIncludeHardcodedJobs": { "enabled", }, @@ -258,6 +277,29 @@ type ControlsConfig struct { // BranchMustBeProtected control configuration BranchMustBeProtected *BranchProtectionControlConfig `yaml:"branchMustBeProtected,omitempty"` + // MergeRequestApprovalRulesMustRequireMinimumApprovals control + // configuration (GitLab only). Flags approval rules covering all + // protected branches that require fewer approvals than the configured + // minimum (ISSUE-502). + MergeRequestApprovalRulesMustRequireMinimumApprovals *MRApprovalRulesMinApprovalsControlConfig `yaml:"mergeRequestApprovalRulesMustRequireMinimumApprovals,omitempty"` + + // MergeRequestApprovalRulesMustCoverAllProtectedBranches control + // configuration (GitLab only). Flags a project where no approval rule + // applies to all protected branches (ISSUE-504). Config-free beyond + // `enabled`. + MergeRequestApprovalRulesMustCoverAllProtectedBranches *EnabledOnlyControlConfig `yaml:"mergeRequestApprovalRulesMustCoverAllProtectedBranches,omitempty"` + + // MergeRequestApprovalSettingsMustBeCompliant control configuration + // (GitLab only). Checks the project's merge-request approval settings + // against per-setting optional expectations (ISSUE-503). + MergeRequestApprovalSettingsMustBeCompliant *MRApprovalSettingsControlConfig `yaml:"mergeRequestApprovalSettingsMustBeCompliant,omitempty"` + + // MergeRequestSettingsMustBeCompliant control configuration (GitLab only). + // Checks the project's merge-request/merge settings (merge method, squash, + // merge trains, source-branch removal, etc.) against per-setting optional + // expectations for exact equality (ISSUE-506). + MergeRequestSettingsMustBeCompliant *MRSettingsControlConfig `yaml:"mergeRequestSettingsMustBeCompliant,omitempty"` + // PipelineMustNotIncludeHardcodedJobs control configuration PipelineMustNotIncludeHardcodedJobs *HardcodedJobsControlConfig `yaml:"pipelineMustNotIncludeHardcodedJobs,omitempty"` @@ -602,6 +644,174 @@ type ImageAuthorizedSourcesControlConfig struct { } // BranchProtectionControlConfig configuration for the branch protection control +// MRApprovalRulesMinApprovalsControlConfig configures the GitLab +// merge-request approval-rules minimum-approvals check (ISSUE-502). +// GitLab-only. +type MRApprovalRulesMinApprovalsControlConfig struct { + // Enabled controls whether this check runs. + Enabled *bool `yaml:"enabled,omitempty"` + + // MinimumRequiredApprovals is the fewest approvals a rule covering all + // protected branches must require; a covering rule below it is flagged. + // When unset (nil) the control asserts nothing (treated as 0). + MinimumRequiredApprovals *int `yaml:"minimumRequiredApprovals,omitempty"` +} + +// IsEnabled reports whether the control is enabled. Returns false when the +// wrapper or the field is nil — same convention as every other IsEnabled(). +func (c *MRApprovalRulesMinApprovalsControlConfig) IsEnabled() bool { + if c == nil || c.Enabled == nil { + return false + } + return *c.Enabled +} + +// MRApprovalSettingsControlConfig configures the GitLab merge-request +// approval-settings check (ISSUE-503). GitLab-only. Every expectation is +// optional: an unset field is not checked. The booleans check only when set +// true — an explicit false is the same as unset, matching the legacy +// platform's conf semantics (there is no "expect the unsafe setting" mode). +type MRApprovalSettingsControlConfig struct { + // Enabled controls whether this check runs. + Enabled *bool `yaml:"enabled,omitempty"` + + // PreventApprovalByAuthor, when true, expects that MR authors cannot + // approve their own merge requests. + PreventApprovalByAuthor *bool `yaml:"preventApprovalByAuthor,omitempty"` + + // PreventApprovalsByCommitters, when true, expects that users who + // committed to an MR cannot approve it. + PreventApprovalsByCommitters *bool `yaml:"preventApprovalsByCommitters,omitempty"` + + // PreventEditingApprovalRulesInMR, when true, expects that approval + // rules cannot be overridden per merge request. + PreventEditingApprovalRulesInMR *bool `yaml:"preventEditingApprovalRulesInMR,omitempty"` + + // RequireReAuthToApprove, when true, expects that approving requires + // re-authentication. + RequireReAuthToApprove *bool `yaml:"requireReAuthToApprove,omitempty"` + + // BehaviorWhenCommitIsAdded is the MINIMUM required strictness for what + // happens to existing approvals when a commit is added to an open MR: + // "keep_approvals" < "remove_approvals_by_code_owners" < + // "remove_all_approvals". A project below the configured rung is + // flagged; any other value fails config validation. + BehaviorWhenCommitIsAdded *string `yaml:"behaviorWhenCommitIsAdded,omitempty"` +} + +// IsEnabled reports whether the control is enabled. Returns false when the +// wrapper or the field is nil — same convention as every other IsEnabled(). +func (c *MRApprovalSettingsControlConfig) IsEnabled() bool { + if c == nil || c.Enabled == nil { + return false + } + return *c.Enabled +} + +// mrApprovalBehaviorValues are the accepted behaviorWhenCommitIsAdded +// expectations, in strictness order. Kept in the configuration package (the +// validation site) and mirrored by the ir.MRApprovalBehavior* constants the +// projection emits; TestMRApprovalBehaviorValuesMatchIR pins the two lists +// together. +var mrApprovalBehaviorValues = []string{ + "keep_approvals", + "remove_approvals_by_code_owners", + "remove_all_approvals", +} + +// validateBehaviorWhenCommitIsAdded rejects a behaviorWhenCommitIsAdded +// expectation outside the known ladder AT CONFIG LOAD. Load-bearing: the Rego +// rule looks the value up in its rank map and an unknown string would make +// that lookup undefined, silently disabling the check — a typo like +// "keep-approvals" would read as "not checked" forever. +func (c *MRApprovalSettingsControlConfig) validateBehaviorWhenCommitIsAdded() error { + if c == nil || c.BehaviorWhenCommitIsAdded == nil { + return nil + } + if slices.Contains(mrApprovalBehaviorValues, *c.BehaviorWhenCommitIsAdded) { + return nil + } + return fmt.Errorf("mergeRequestApprovalSettingsMustBeCompliant.behaviorWhenCommitIsAdded: unknown value %q (expected one of %s)", + *c.BehaviorWhenCommitIsAdded, strings.Join(mrApprovalBehaviorValues, ", ")) +} + +// MRSettingsControlConfig configures the GitLab merge-request/merge-settings +// check (ISSUE-506). GitLab-only. Every expectation is optional: an unset field +// is not checked, and each set field is compared against the project's actual +// value for EXACT equality (the legacy platform compared all fields, but always +// with a fully populated policy — optional here keeps a hand-authored YAML from +// flagging on a field the operator never set). +type MRSettingsControlConfig struct { + // Enabled controls whether this check runs. + Enabled *bool `yaml:"enabled,omitempty"` + + // MergeMethod is the expected merge method: "merge", "ff", or + // "rebase_merge". Any other value fails config validation. + MergeMethod *string `yaml:"mergeMethod,omitempty"` + + // SquashOption is the expected squash policy: "never", "always", + // "default_on", or "default_off". Any other value fails config validation. + SquashOption *string `yaml:"squashOption,omitempty"` + + // MergePipelinesEnabled is the expected merged-results-pipelines setting. + MergePipelinesEnabled *bool `yaml:"mergePipelinesEnabled,omitempty"` + + // MergeTrainsEnabled is the expected merge-trains setting. + MergeTrainsEnabled *bool `yaml:"mergeTrainsEnabled,omitempty"` + + // AllowMergeOnSkippedPipeline is the expected "allow merge when the + // pipeline is skipped" setting. + AllowMergeOnSkippedPipeline *bool `yaml:"allowMergeOnSkippedPipeline,omitempty"` + + // ResolveOutdatedDiffDiscussions is the expected auto-resolve-outdated- + // discussions setting. + ResolveOutdatedDiffDiscussions *bool `yaml:"resolveOutdatedDiffDiscussions,omitempty"` + + // PrintingMergeRequestLinkEnabled is the expected print-MR-link-on-push + // setting. + PrintingMergeRequestLinkEnabled *bool `yaml:"printingMergeRequestLinkEnabled,omitempty"` + + // RemoveSourceBranchAfterMerge is the expected delete-source-branch-after- + // merge default. + RemoveSourceBranchAfterMerge *bool `yaml:"removeSourceBranchAfterMerge,omitempty"` +} + +// IsEnabled reports whether the control is enabled. Returns false when the +// wrapper or the field is nil — same convention as every other IsEnabled(). +func (c *MRSettingsControlConfig) IsEnabled() bool { + if c == nil || c.Enabled == nil { + return false + } + return *c.Enabled +} + +// mrSettingsMergeMethodValues are the accepted mergeMethod expectations, and +// mrSettingsSquashOptionValues the accepted squashOption expectations. Kept at +// the validation site; an unknown value would make the Rego equality check +// compare against a string GitLab never returns (always deviating), so it is +// rejected at config load instead. +var ( + mrSettingsMergeMethodValues = []string{"merge", "ff", "rebase_merge"} + mrSettingsSquashOptionValues = []string{"never", "always", "default_on", "default_off"} +) + +// validateEnums rejects a mergeMethod or squashOption expectation outside the +// known set AT CONFIG LOAD. +func (c *MRSettingsControlConfig) validateEnums() error { + if c == nil { + return nil + } + if c.MergeMethod != nil && !slices.Contains(mrSettingsMergeMethodValues, *c.MergeMethod) { + return fmt.Errorf("mergeRequestSettingsMustBeCompliant.mergeMethod: unknown value %q (expected one of %s)", + *c.MergeMethod, strings.Join(mrSettingsMergeMethodValues, ", ")) + } + if c.SquashOption != nil && !slices.Contains(mrSettingsSquashOptionValues, *c.SquashOption) { + return fmt.Errorf("mergeRequestSettingsMustBeCompliant.squashOption: unknown value %q (expected one of %s)", + *c.SquashOption, strings.Join(mrSettingsSquashOptionValues, ", ")) + } + return nil +} + type BranchProtectionControlConfig struct { // Enabled controls whether this check runs Enabled *bool `yaml:"enabled,omitempty"` @@ -1069,6 +1279,12 @@ func validateControlsConfig(c *ControlsConfig) error { return err } } + if err := c.MergeRequestApprovalSettingsMustBeCompliant.validateBehaviorWhenCommitIsAdded(); err != nil { + return err + } + if err := c.MergeRequestSettingsMustBeCompliant.validateEnums(); err != nil { + return err + } return nil } @@ -1131,6 +1347,34 @@ func (c *PlumberConfig) GetBranchMustBeProtectedConfig() *BranchProtectionContro return c.ControlsFor("gitlab").BranchMustBeProtected } +func (c *PlumberConfig) GetMergeRequestApprovalRulesMustRequireMinimumApprovalsConfig() *MRApprovalRulesMinApprovalsControlConfig { + if c == nil { + return nil + } + return c.ControlsFor("gitlab").MergeRequestApprovalRulesMustRequireMinimumApprovals +} + +func (c *PlumberConfig) GetMergeRequestApprovalRulesMustCoverAllProtectedBranchesConfig() *EnabledOnlyControlConfig { + if c == nil { + return nil + } + return c.ControlsFor("gitlab").MergeRequestApprovalRulesMustCoverAllProtectedBranches +} + +func (c *PlumberConfig) GetMergeRequestApprovalSettingsMustBeCompliantConfig() *MRApprovalSettingsControlConfig { + if c == nil { + return nil + } + return c.ControlsFor("gitlab").MergeRequestApprovalSettingsMustBeCompliant +} + +func (c *PlumberConfig) GetMergeRequestSettingsMustBeCompliantConfig() *MRSettingsControlConfig { + if c == nil { + return nil + } + return c.ControlsFor("gitlab").MergeRequestSettingsMustBeCompliant +} + // IsEnabled returns whether the control is enabled // Returns false if not properly configured func (c *BranchProtectionControlConfig) IsEnabled() bool { diff --git a/configuration/plumberconfig_test.go b/configuration/plumberconfig_test.go index 42700797..10ef3307 100644 --- a/configuration/plumberconfig_test.go +++ b/configuration/plumberconfig_test.go @@ -5,6 +5,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/getplumber/plumber/internal/ir" ) // A missing config file must wrap ErrConfigNotFound so callers can detect the @@ -367,6 +369,10 @@ func TestValidControlNames(t *testing.T) { "githubActionMustComeFromAuthorizedSources", "includesMustBeUpToDate", "includesMustNotUseForbiddenVersions", + "mergeRequestApprovalRulesMustCoverAllProtectedBranches", + "mergeRequestApprovalRulesMustRequireMinimumApprovals", + "mergeRequestApprovalSettingsMustBeCompliant", + "mergeRequestSettingsMustBeCompliant", "pipelineMustIncludeComponent", "pipelineMustIncludeTemplate", "pipelineMustNotEnableDebugTrace", @@ -399,6 +405,96 @@ func TestValidControlNames(t *testing.T) { } } +// behaviorWhenCommitIsAdded is validated AT CONFIG LOAD because the Rego rule +// looks the value up in a rank map: an unknown string would make the lookup +// undefined and silently disable the check, so a typo must fail loudly here +// instead. +func TestMRApprovalSettingsBehaviorValidation(t *testing.T) { + valid := "remove_all_approvals" + invalid := "keep-approvals" // dash, not underscore: the plausible typo + enabled := true + + mk := func(behavior *string) *PlumberConfig { + return &PlumberConfig{ + Version: "2.0", + GitLab: &ProviderConfig{Controls: ControlsConfig{ + MergeRequestApprovalSettingsMustBeCompliant: &MRApprovalSettingsControlConfig{ + Enabled: &enabled, + BehaviorWhenCommitIsAdded: behavior, + }, + }}, + } + } + + if err := mk(nil).Validate(); err != nil { + t.Fatalf("unset behavior expectation must validate, got %v", err) + } + if err := mk(&valid).Validate(); err != nil { + t.Fatalf("valid behavior expectation must validate, got %v", err) + } + err := mk(&invalid).Validate() + if err == nil { + t.Fatal("an unknown behaviorWhenCommitIsAdded value must fail config validation, not silently disable the check") + } + if !strings.Contains(err.Error(), invalid) || !strings.Contains(err.Error(), "remove_approvals_by_code_owners") { + t.Fatalf("the error must name the bad value and the accepted ladder, got %v", err) + } +} + +// mergeMethod and squashOption are validated AT CONFIG LOAD because the Rego +// rule compares them for exact equality against GitLab's value: an unknown +// expectation would never equal a real setting and would flag every project, so +// a typo must fail loudly here instead. +func TestMRSettingsEnumValidation(t *testing.T) { + enabled := true + strPtr := func(s string) *string { return &s } + mk := func(mutate func(*MRSettingsControlConfig)) *PlumberConfig { + c := &MRSettingsControlConfig{Enabled: &enabled} + mutate(c) + return &PlumberConfig{Version: "2.0", GitLab: &ProviderConfig{Controls: ControlsConfig{ + MergeRequestSettingsMustBeCompliant: c, + }}} + } + + if err := mk(func(c *MRSettingsControlConfig) {}).Validate(); err != nil { + t.Fatalf("unset enum expectations must validate, got %v", err) + } + if err := mk(func(c *MRSettingsControlConfig) { + c.MergeMethod = strPtr("ff") + c.SquashOption = strPtr("default_on") + }).Validate(); err != nil { + t.Fatalf("valid enum expectations must validate, got %v", err) + } + err := mk(func(c *MRSettingsControlConfig) { c.MergeMethod = strPtr("squash") }).Validate() + if err == nil || !strings.Contains(err.Error(), "squash") || !strings.Contains(err.Error(), "rebase_merge") { + t.Fatalf("an unknown mergeMethod must fail validation naming the bad value and the accepted set, got %v", err) + } + err = mk(func(c *MRSettingsControlConfig) { c.SquashOption = strPtr("sometimes") }).Validate() + if err == nil || !strings.Contains(err.Error(), "sometimes") || !strings.Contains(err.Error(), "default_off") { + t.Fatalf("an unknown squashOption must fail validation naming the bad value and the accepted set, got %v", err) + } +} + +// The configuration package validates behaviorWhenCommitIsAdded against its +// own list while the projection emits the ir.MRApprovalBehavior* constants; +// this pins the two so they cannot drift (a value the projection emits but +// validation rejects would make a correct config impossible to write). +func TestMRApprovalBehaviorValuesMatchIR(t *testing.T) { + want := []string{ + ir.MRApprovalBehaviorKeepApprovals, + ir.MRApprovalBehaviorRemoveCodeOwnerApprovals, + ir.MRApprovalBehaviorRemoveAllApprovals, + } + if len(mrApprovalBehaviorValues) != len(want) { + t.Fatalf("mrApprovalBehaviorValues has %d entries, want %d", len(mrApprovalBehaviorValues), len(want)) + } + for i, v := range want { + if mrApprovalBehaviorValues[i] != v { + t.Fatalf("mrApprovalBehaviorValues[%d] = %q, want the IR constant %q (strictness order matters)", i, mrApprovalBehaviorValues[i], v) + } + } +} + func TestIsIncludePlumberDefaultsDefaultsTrue(t *testing.T) { var action *ActionAuthorizedSourcesControlConfig if !action.IsIncludePlumberDefaults() { diff --git a/configuration/registry.go b/configuration/registry.go index 888043f1..f215044b 100644 --- a/configuration/registry.go +++ b/configuration/registry.go @@ -30,21 +30,25 @@ const ( // GitHub-only control gets {ProviderGitHub}. var controlsMeta = map[string]ControlMeta{ // Cross-provider (same control name + rego logic, provider-specific values). - "branchMustBeProtected": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "containerImageMustComeFromAuthorizedSources": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "containerImageMustNotUseForbiddenTags": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "externalRefsMustNotCollide": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "includesMustBeUpToDate": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "includesMustNotUseForbiddenVersions": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "pipelineMustIncludeComponent": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "pipelineMustIncludeTemplate": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "pipelineMustNotEnableDebugTrace": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "pipelineMustNotExecuteUnverifiedScripts": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "pipelineMustNotIncludeHardcodedJobs": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "pipelineMustNotOverrideJobVariables": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "pipelineMustNotUseDockerInDocker": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "pipelineMustNotUseUnsafeVariableExpansion": {Providers: []string{ProviderGitLab, ProviderGitHub}}, - "securityJobsMustNotBeWeakened": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "branchMustBeProtected": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "mergeRequestApprovalRulesMustRequireMinimumApprovals": {Providers: []string{ProviderGitLab}}, + "mergeRequestApprovalRulesMustCoverAllProtectedBranches": {Providers: []string{ProviderGitLab}}, + "mergeRequestApprovalSettingsMustBeCompliant": {Providers: []string{ProviderGitLab}}, + "mergeRequestSettingsMustBeCompliant": {Providers: []string{ProviderGitLab}}, + "containerImageMustComeFromAuthorizedSources": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "containerImageMustNotUseForbiddenTags": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "externalRefsMustNotCollide": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "includesMustBeUpToDate": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "includesMustNotUseForbiddenVersions": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "pipelineMustIncludeComponent": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "pipelineMustIncludeTemplate": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "pipelineMustNotEnableDebugTrace": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "pipelineMustNotExecuteUnverifiedScripts": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "pipelineMustNotIncludeHardcodedJobs": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "pipelineMustNotOverrideJobVariables": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "pipelineMustNotUseDockerInDocker": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "pipelineMustNotUseUnsafeVariableExpansion": {Providers: []string{ProviderGitLab, ProviderGitHub}}, + "securityJobsMustNotBeWeakened": {Providers: []string{ProviderGitLab, ProviderGitHub}}, // GitHub-only. "actionPinCommentsMustMatchSha": {Providers: []string{ProviderGitHub}}, diff --git a/configuration/v1_to_v2.go b/configuration/v1_to_v2.go index 737b6be1..ef1fe893 100644 --- a/configuration/v1_to_v2.go +++ b/configuration/v1_to_v2.go @@ -71,6 +71,10 @@ func controlsConfigIsZero(c ControlsConfig) bool { return c.ContainerImageMustNotUseForbiddenTags == nil && c.ContainerImageMustComeFromAuthorizedSources == nil && c.BranchMustBeProtected == nil && + c.MergeRequestApprovalRulesMustRequireMinimumApprovals == nil && + c.MergeRequestApprovalRulesMustCoverAllProtectedBranches == nil && + c.MergeRequestApprovalSettingsMustBeCompliant == nil && + c.MergeRequestSettingsMustBeCompliant == nil && c.PipelineMustNotIncludeHardcodedJobs == nil && c.IncludesMustBeUpToDate == nil && c.IncludesMustNotUseForbiddenVersions == nil && @@ -96,6 +100,10 @@ func controlsConfigEqual(a, b ControlsConfig) bool { return a.ContainerImageMustNotUseForbiddenTags == b.ContainerImageMustNotUseForbiddenTags && a.ContainerImageMustComeFromAuthorizedSources == b.ContainerImageMustComeFromAuthorizedSources && a.BranchMustBeProtected == b.BranchMustBeProtected && + a.MergeRequestApprovalRulesMustRequireMinimumApprovals == b.MergeRequestApprovalRulesMustRequireMinimumApprovals && + a.MergeRequestApprovalRulesMustCoverAllProtectedBranches == b.MergeRequestApprovalRulesMustCoverAllProtectedBranches && + a.MergeRequestApprovalSettingsMustBeCompliant == b.MergeRequestApprovalSettingsMustBeCompliant && + a.MergeRequestSettingsMustBeCompliant == b.MergeRequestSettingsMustBeCompliant && a.PipelineMustNotIncludeHardcodedJobs == b.PipelineMustNotIncludeHardcodedJobs && a.IncludesMustBeUpToDate == b.IncludesMustBeUpToDate && a.IncludesMustNotUseForbiddenVersions == b.IncludesMustNotUseForbiddenVersions && diff --git a/control/catalog.go b/control/catalog.go index 67d8c6fe..e4ba2efb 100644 --- a/control/catalog.go +++ b/control/catalog.go @@ -52,6 +52,26 @@ func GitLabControls(pc *configuration.PlumberConfig) []ControlEntry { ControlName: "branchMustBeProtected", Skipped: c.BranchMustBeProtected == nil || !c.BranchMustBeProtected.IsEnabled(), }) + entries = append(entries, ControlEntry{ + DisplayName: "MR approval rules must require a minimum number of approvals", + ControlName: "mergeRequestApprovalRulesMustRequireMinimumApprovals", + Skipped: c.MergeRequestApprovalRulesMustRequireMinimumApprovals == nil || !c.MergeRequestApprovalRulesMustRequireMinimumApprovals.IsEnabled(), + }) + entries = append(entries, ControlEntry{ + DisplayName: "MR approval rules must cover all protected branches", + ControlName: "mergeRequestApprovalRulesMustCoverAllProtectedBranches", + Skipped: c.MergeRequestApprovalRulesMustCoverAllProtectedBranches == nil || !c.MergeRequestApprovalRulesMustCoverAllProtectedBranches.IsEnabled(), + }) + entries = append(entries, ControlEntry{ + DisplayName: "MR approval settings must be compliant", + ControlName: "mergeRequestApprovalSettingsMustBeCompliant", + Skipped: c.MergeRequestApprovalSettingsMustBeCompliant == nil || !c.MergeRequestApprovalSettingsMustBeCompliant.IsEnabled(), + }) + entries = append(entries, ControlEntry{ + DisplayName: "MR settings must be compliant", + ControlName: "mergeRequestSettingsMustBeCompliant", + Skipped: c.MergeRequestSettingsMustBeCompliant == nil || !c.MergeRequestSettingsMustBeCompliant.IsEnabled(), + }) entries = append(entries, ControlEntry{ DisplayName: "Pipeline must not include hardcoded jobs", ControlName: "pipelineMustNotIncludeHardcodedJobs", @@ -315,6 +335,18 @@ func DisabledControlNames(c *configuration.ControlsConfig) map[string]bool { if cfg := c.BranchMustBeProtected; cfg == nil || !cfg.IsEnabled() { out["branchMustBeProtected"] = true } + if cfg := c.MergeRequestApprovalRulesMustRequireMinimumApprovals; cfg == nil || !cfg.IsEnabled() { + out["mergeRequestApprovalRulesMustRequireMinimumApprovals"] = true + } + if cfg := c.MergeRequestApprovalRulesMustCoverAllProtectedBranches; cfg == nil || !cfg.IsEnabled() { + out["mergeRequestApprovalRulesMustCoverAllProtectedBranches"] = true + } + if cfg := c.MergeRequestApprovalSettingsMustBeCompliant; cfg == nil || !cfg.IsEnabled() { + out["mergeRequestApprovalSettingsMustBeCompliant"] = true + } + if cfg := c.MergeRequestSettingsMustBeCompliant; cfg == nil || !cfg.IsEnabled() { + out["mergeRequestSettingsMustBeCompliant"] = true + } if cfg := c.PipelineMustNotIncludeHardcodedJobs; cfg == nil || !cfg.IsEnabled() { out["pipelineMustNotIncludeHardcodedJobs"] = true } diff --git a/control/codes.go b/control/codes.go index d3dabe56..d88d729a 100644 --- a/control/codes.go +++ b/control/codes.go @@ -178,8 +178,16 @@ const ( const ( // ISSUE-501: Branch is not protected CodeBranchUnprotected ErrorCode = "ISSUE-501" + // ISSUE-502: A merge-request approval rule covering all protected branches requires fewer approvals than the configured minimum + CodeMRApprovalRulesBelowMinimum ErrorCode = "ISSUE-502" + // ISSUE-503: The project's merge-request approval settings do not meet the configured expectations + CodeMRApprovalSettingsNonCompliant ErrorCode = "ISSUE-503" + // ISSUE-504: No merge-request approval rule applies to all protected branches + CodeMRApprovalRulesAllBranchesMissing ErrorCode = "ISSUE-504" // ISSUE-505: Branch has non-compliant protection settings CodeBranchNonCompliant ErrorCode = "ISSUE-505" + // ISSUE-506: The project's merge-request/merge settings do not meet the configured expectations + CodeMRSettingsNonCompliant ErrorCode = "ISSUE-506" // ISSUE-803: Job runs with overly broad permissions (write-all) CodeExcessivePermissions ErrorCode = "ISSUE-803" ) @@ -550,6 +558,33 @@ var errorCodeRegistry = map[ErrorCode]ErrorCodeInfo{ DocURL: docsBaseURL + string(CodeBranchUnprotected), ControlName: "branchMustBeProtected", }, + CodeMRApprovalRulesBelowMinimum: { + Code: CodeMRApprovalRulesBelowMinimum, + Severity: SeverityHigh, + Title: "Merge request approval rule requires too few approvals", + Description: "A merge request approval rule that covers all protected branches requires fewer approvals than the configured minimum, so a protected branch can be merged with too little review.", + Remediation: "Raise the rule's required approvals in Settings > Merge requests > Approval rules to at least your configured minimum, or narrow the rule's scope if it is not meant to cover all protected branches.", + DocURL: docsBaseURL + string(CodeMRApprovalRulesBelowMinimum), + ControlName: "mergeRequestApprovalRulesMustRequireMinimumApprovals", + }, + CodeMRApprovalSettingsNonCompliant: { + Code: CodeMRApprovalSettingsNonCompliant, + Severity: SeverityHigh, + Title: "MR approval settings are not compliant", + Description: "The project's merge request approval settings fall short of the configured expectations (authors or committers can approve, rules can be overridden per MR, no re-authentication, or approvals survive new commits), so approvals can be weakened at the project level regardless of any approval rule.", + Remediation: "Tighten the project's approval settings in Settings > Merge requests: prevent approval by the author and by committers, prevent editing approval rules in merge requests, require re-authentication to approve, and remove approvals when new commits are added, as your policy requires.", + DocURL: docsBaseURL + string(CodeMRApprovalSettingsNonCompliant), + ControlName: "mergeRequestApprovalSettingsMustBeCompliant", + }, + CodeMRApprovalRulesAllBranchesMissing: { + Code: CodeMRApprovalRulesAllBranchesMissing, + Severity: SeverityHigh, + Title: "No approval rule covers all protected branches", + Description: "No merge request approval rule applies to all protected branches, so a protected branch can exist with no required approval and be merged without review.", + Remediation: "Add a merge request approval rule that applies to all protected branches in Settings > Merge requests > Approval rules.", + DocURL: docsBaseURL + string(CodeMRApprovalRulesAllBranchesMissing), + ControlName: "mergeRequestApprovalRulesMustCoverAllProtectedBranches", + }, CodeBranchNonCompliant: { Code: CodeBranchNonCompliant, Severity: SeverityHigh, @@ -559,6 +594,15 @@ var errorCodeRegistry = map[ErrorCode]ErrorCodeInfo{ DocURL: docsBaseURL + string(CodeBranchNonCompliant), ControlName: "branchMustBeProtected", }, + CodeMRSettingsNonCompliant: { + Code: CodeMRSettingsNonCompliant, + Severity: SeverityMedium, + Title: "MR settings are not compliant", + Description: "The project's merge-request/merge settings (merge method, squash policy, merge trains, source-branch removal, and related options) do not match the configured expectations, so merges can happen in a way the policy does not allow.", + Remediation: "Align the project's merge-request settings in Settings > Merge requests with your policy: set the required merge method and squash option, and toggle the merge-train, skipped-pipeline, discussion-resolution, and source-branch-removal options to the expected values.", + DocURL: docsBaseURL + string(CodeMRSettingsNonCompliant), + ControlName: "mergeRequestSettingsMustBeCompliant", + }, CodeTemplateInjection: { Code: CodeTemplateInjection, Severity: SeverityCritical, diff --git a/control/mr_approval_config_contract_test.go b/control/mr_approval_config_contract_test.go new file mode 100644 index 00000000..2077adca --- /dev/null +++ b/control/mr_approval_config_contract_test.go @@ -0,0 +1,225 @@ +package control + +import ( + "context" + "testing" + + "github.com/getplumber/plumber/configuration" + opaengine "github.com/getplumber/plumber/internal/engine/opa" + "github.com/getplumber/plumber/internal/ir" + "github.com/getplumber/plumber/policies" +) + +// TestMRApprovalMinApprovalsConfigContract pins the struct -> map -> rego chain +// for ISSUE-502: buildEngineConfig (task.go) emits +// cfg["mergeRequestApprovalRulesMustRequireMinimumApprovals"]["minimumRequiredApprovals"], +// and the rego reads exactly those keys. Every rego-only test hand-builds +// input.config, so a rename on either side would silently make the rego fall +// back to a minimum of 0 — ISSUE-502 would never fire (a false pass) with all +// other tests still green. Mirrors TestCachePoisoningConfigContract. +func TestMRApprovalMinApprovalsConfigContract(t *testing.T) { + intPtr := func(i int) *int { return &i } + boolPtr := func(b bool) *bool { return &b } + + engine := opaengine.New() + if err := engine.LoadFromFSFiltered(policies.FS, nil); err != nil { + t.Fatalf("load policies: %v", err) + } + // A rule covering all protected branches, requiring only 1 approval. + pipeline := &ir.NormalizedPipeline{ + Provider: ir.ProviderGitLab, + MRApprovalRulesKnown: true, + MRApprovalRules: []ir.MRApprovalRule{ + {ID: "10", Name: "weak", ApprovalsRequired: 1, AppliesToAllProtectedBranches: true}, + }, + } + count502 := func(engineCfg map[string]any) int { + findings, err := engine.Evaluate(context.Background(), pipeline, engineCfg) + if err != nil { + t.Fatalf("evaluate: %v", err) + } + n := 0 + for _, f := range findings { + if f.Code == "ISSUE-502" { + n++ + } + } + return n + } + + // Minimum 2, via the REAL buildEngineConfig projection: the 1-approval rule + // must fire exactly one ISSUE-502. A key rename on either side breaks this. + cfg2 := buildEngineConfig(&configuration.ControlsConfig{ + MergeRequestApprovalRulesMustRequireMinimumApprovals: &configuration.MRApprovalRulesMinApprovalsControlConfig{ + Enabled: boolPtr(true), + MinimumRequiredApprovals: intPtr(2), + }, + }) + if _, ok := cfg2["mergeRequestApprovalRulesMustRequireMinimumApprovals"]; !ok { + t.Fatal("buildEngineConfig did not project a mergeRequestApprovalRulesMustRequireMinimumApprovals block") + } + if n := count502(cfg2); n != 1 { + t.Fatalf("min=2 through the real config projection: expected 1 ISSUE-502, got %d — the struct->map->rego key contract is broken", n) + } + + // Minimum unset -> the rego's object.get defaults to 0 -> nothing is below 0, + // so no finding. Pins the documented "treated as 0" behaviour end to end. + cfgNil := buildEngineConfig(&configuration.ControlsConfig{ + MergeRequestApprovalRulesMustRequireMinimumApprovals: &configuration.MRApprovalRulesMinApprovalsControlConfig{ + Enabled: boolPtr(true), + }, + }) + if n := count502(cfgNil); n != 0 { + t.Fatalf("min unset (treated as 0): expected 0 ISSUE-502, got %d", n) + } +} + +// TestMRApprovalSettingsConfigContract pins the same struct -> map -> rego +// chain for ISSUE-503. The stakes are higher here than for 502: every +// expectation is optional and the rego treats an absent key as "not checked", +// so a key rename on either side would not fail anything — it would read as +// "operator chose not to check this" and the control would silently assert +// nothing, forever, with every rego-only test (which hand-builds +// input.config) still green. +func TestMRApprovalSettingsConfigContract(t *testing.T) { + boolPtr := func(b bool) *bool { return &b } + strPtr := func(s string) *string { return &s } + + engine := opaengine.New() + if err := engine.LoadFromFSFiltered(policies.FS, nil); err != nil { + t.Fatalf("load policies: %v", err) + } + // A fully unlocked project: every expectation that reaches the rego + // deviates, so the deviation list mirrors exactly which config keys + // survived the projection. + pipeline := &ir.NormalizedPipeline{ + Provider: ir.ProviderGitLab, + MRApprovalSettings: &ir.MRApprovalSettings{ + BehaviorWhenCommitIsAdded: ir.MRApprovalBehaviorKeepApprovals, + }, + } + deviations503 := func(engineCfg map[string]any) []any { + findings, err := engine.Evaluate(context.Background(), pipeline, engineCfg) + if err != nil { + t.Fatalf("evaluate: %v", err) + } + for _, f := range findings { + if f.Code == "ISSUE-503" { + devs, _ := f.Data["deviatingSettings"].([]any) + return devs + } + } + return nil + } + + // All five expectations set, via the REAL buildEngineConfig projection: + // all five must come back as deviations. Any dropped or renamed key + // shrinks this list. + cfgAll := buildEngineConfig(&configuration.ControlsConfig{ + MergeRequestApprovalSettingsMustBeCompliant: &configuration.MRApprovalSettingsControlConfig{ + Enabled: boolPtr(true), + PreventApprovalByAuthor: boolPtr(true), + PreventApprovalsByCommitters: boolPtr(true), + PreventEditingApprovalRulesInMR: boolPtr(true), + RequireReAuthToApprove: boolPtr(true), + BehaviorWhenCommitIsAdded: strPtr(ir.MRApprovalBehaviorRemoveAllApprovals), + }, + }) + if _, ok := cfgAll["mergeRequestApprovalSettingsMustBeCompliant"]; !ok { + t.Fatal("buildEngineConfig did not project a mergeRequestApprovalSettingsMustBeCompliant block") + } + if devs := deviations503(cfgAll); len(devs) != 5 { + t.Fatalf("all 5 expectations through the real config projection: expected 5 deviations, got %v — the struct->map->rego key contract is broken", devs) + } + + // Only one expectation set -> exactly that one deviation: unset fields + // must NOT be projected (the rego reads an absent key as "not checked"). + cfgOne := buildEngineConfig(&configuration.ControlsConfig{ + MergeRequestApprovalSettingsMustBeCompliant: &configuration.MRApprovalSettingsControlConfig{ + Enabled: boolPtr(true), + PreventApprovalByAuthor: boolPtr(true), + }, + }) + devs := deviations503(cfgOne) + if len(devs) != 1 || devs[0] != "preventApprovalByAuthor" { + t.Fatalf("one expectation set: expected exactly [preventApprovalByAuthor], got %v", devs) + } +} + +// TestMRSettingsConfigContract pins the struct -> map -> rego chain for +// ISSUE-506. Every expectation is optional and the rego reads an absent key as +// "not checked", so a key rename on either side would silently make the control +// assert nothing forever with the rego-only tests (which hand-build +// input.config) still green. +func TestMRSettingsConfigContract(t *testing.T) { + boolPtr := func(b bool) *bool { return &b } + strPtr := func(s string) *string { return &s } + + engine := opaengine.New() + if err := engine.LoadFromFSFiltered(policies.FS, nil); err != nil { + t.Fatalf("load policies: %v", err) + } + // A project whose every setting differs from the configured expectation, so + // the deviation list mirrors exactly which config keys survived projection. + pipeline := &ir.NormalizedPipeline{ + Provider: ir.ProviderGitLab, + MRSettings: &ir.MRSettings{ + MergeMethod: "merge", + SquashOption: "never", + MergePipelinesEnabled: false, + MergeTrainsEnabled: false, + AllowMergeOnSkippedPipeline: false, + ResolveOutdatedDiffDiscussions: false, + PrintingMergeRequestLinkEnabled: false, + RemoveSourceBranchAfterMerge: false, + }, + } + deviations506 := func(engineCfg map[string]any) []any { + findings, err := engine.Evaluate(context.Background(), pipeline, engineCfg) + if err != nil { + t.Fatalf("evaluate: %v", err) + } + for _, f := range findings { + if f.Code == "ISSUE-506" { + devs, _ := f.Data["deviatingSettings"].([]any) + return devs + } + } + return nil + } + + // All eight expectations set (each the opposite of the pipeline's value), + // via the REAL buildEngineConfig projection: all eight must deviate. + cfgAll := buildEngineConfig(&configuration.ControlsConfig{ + MergeRequestSettingsMustBeCompliant: &configuration.MRSettingsControlConfig{ + Enabled: boolPtr(true), + MergeMethod: strPtr("ff"), + SquashOption: strPtr("always"), + MergePipelinesEnabled: boolPtr(true), + MergeTrainsEnabled: boolPtr(true), + AllowMergeOnSkippedPipeline: boolPtr(true), + ResolveOutdatedDiffDiscussions: boolPtr(true), + PrintingMergeRequestLinkEnabled: boolPtr(true), + RemoveSourceBranchAfterMerge: boolPtr(true), + }, + }) + if _, ok := cfgAll["mergeRequestSettingsMustBeCompliant"]; !ok { + t.Fatal("buildEngineConfig did not project a mergeRequestSettingsMustBeCompliant block") + } + if devs := deviations506(cfgAll); len(devs) != 8 { + t.Fatalf("all 8 expectations through the real config projection: expected 8 deviations, got %v — the struct->map->rego key contract is broken", devs) + } + + // Only one expectation set -> exactly that one deviation: unset fields must + // NOT be projected (the rego reads an absent key as "not checked"). + cfgOne := buildEngineConfig(&configuration.ControlsConfig{ + MergeRequestSettingsMustBeCompliant: &configuration.MRSettingsControlConfig{ + Enabled: boolPtr(true), + MergeMethod: strPtr("ff"), + }, + }) + devs := deviations506(cfgOne) + if len(devs) != 1 || devs[0] != "mergeMethod" { + t.Fatalf("one expectation set: expected exactly [mergeMethod], got %v", devs) + } +} diff --git a/control/status.go b/control/status.go index bcf0b01e..7d2ae6f5 100644 --- a/control/status.go +++ b/control/status.go @@ -88,6 +88,32 @@ func StatusFor(e ControlEntry, result *AnalysisResult, findingCount int) string } return StatusPassed } + if e.ControlName == "mergeRequestApprovalRulesMustRequireMinimumApprovals" || e.ControlName == "mergeRequestApprovalRulesMustCoverAllProtectedBranches" { + // Approval-rule controls evaluate the project's merge-request approval + // rules, not the CI file, so the CiMissing / CiValid check below does + // not apply. The rules are authoritative only when the protection + // collection ran and the approvals listing was read (ProtectionData + // set, MRApprovalRulesKnown=true). A nil ProtectionData (the collection + // never ran) or MRApprovalRulesKnown=false (a 401/403 from a + // non-premium GitLab, or a token without scope) means the control never + // truly evaluated: an empty findings list here must not read as a pass. + if result.ProtectionData == nil || !result.ProtectionData.MRApprovalRulesKnown { + return StatusError + } + return StatusPassed + } + if e.ControlName == "mergeRequestApprovalSettingsMustBeCompliant" { + // Same settings-level reasoning as the approval-rule controls above, + // with the settings' own Known signal: the fetch leaves + // MRApprovalSettings nil on a 401/403 (a token without scope), and a + // nil ProtectionData means the collection never ran — either way the + // control never truly evaluated, so an empty findings list must not + // read as a pass. + if result.ProtectionData == nil || result.ProtectionData.MRApprovalSettings == nil { + return StatusError + } + return StatusPassed + } if result.CiMissing || !result.CiValid { return StatusError } diff --git a/control/status_test.go b/control/status_test.go index d223e824..3b72011d 100644 --- a/control/status_test.go +++ b/control/status_test.go @@ -4,11 +4,14 @@ import ( "testing" "github.com/getplumber/plumber/gitlab" + glab "gitlab.com/gitlab-org/api/client-go" ) func TestStatusFor(t *testing.T) { content := ControlEntry{ControlName: "actionsMustBePinnedByCommitSha"} branch := ControlEntry{ControlName: "branchMustBeProtected"} + approval := ControlEntry{ControlName: "mergeRequestApprovalRulesMustRequireMinimumApprovals"} + approvalSettings := ControlEntry{ControlName: "mergeRequestApprovalSettingsMustBeCompliant"} healthy := &AnalysisResult{CiValid: true} cases := []struct { @@ -33,6 +36,16 @@ func TestStatusFor(t *testing.T) { {"branch control errors on partial protection details", branch, &AnalysisResult{CiValid: true, GitHubStats: &GitHubAnalysisStats{BranchesProtectionDetailsUnknown: 2}}, 0, StatusError}, {"branch control on github ignores content-only degradation", branch, &AnalysisResult{CiValid: true, GitHubStats: &GitHubAnalysisStats{}, DegradedReasons: []string{"2 workflow file(s) could not be fetched and were skipped"}}, 0, StatusPassed}, {"branch control on gitlab ignores content-only degradation when its own collection ran", branch, &AnalysisResult{CiValid: true, ProtectionData: &gitlab.GitlabProtectionAnalysisData{}, DegradedReasons: []string{"2 include(s) could not be resolved; their jobs were not analysed"}}, 0, StatusPassed}, + {"approval-rule control passes when the approvals listing was read and clean", approval, &AnalysisResult{CiValid: true, ProtectionData: &gitlab.GitlabProtectionAnalysisData{MRApprovalRulesKnown: true}}, 0, StatusPassed}, + {"approval-rule control ignores missing CI config (settings-independent) when the listing was read", approval, &AnalysisResult{CiMissing: true, ProtectionData: &gitlab.GitlabProtectionAnalysisData{MRApprovalRulesKnown: true}}, 0, StatusPassed}, + {"approval-rule control errors when the protection collection never ran", approval, &AnalysisResult{CiValid: true}, 0, StatusError}, + {"approval-rule control errors on an unreadable approvals listing (401/403, Known=false): empty findings are not a pass", approval, &AnalysisResult{CiValid: true, ProtectionData: &gitlab.GitlabProtectionAnalysisData{MRApprovalRulesKnown: false}}, 0, StatusError}, + {"approval-rule control with findings is failed regardless of CI/collection state", approval, &AnalysisResult{CiValid: true, ProtectionData: &gitlab.GitlabProtectionAnalysisData{MRApprovalRulesKnown: true}}, 3, StatusFailed}, + {"approval-settings control passes when the settings were read and clean", approvalSettings, &AnalysisResult{CiValid: true, ProtectionData: &gitlab.GitlabProtectionAnalysisData{MRApprovalSettings: &glab.ProjectApprovals{}}}, 0, StatusPassed}, + {"approval-settings control ignores missing CI config when the settings were read", approvalSettings, &AnalysisResult{CiMissing: true, ProtectionData: &gitlab.GitlabProtectionAnalysisData{MRApprovalSettings: &glab.ProjectApprovals{}}}, 0, StatusPassed}, + {"approval-settings control errors when the protection collection never ran", approvalSettings, &AnalysisResult{CiValid: true}, 0, StatusError}, + {"approval-settings control errors on unreadable settings (401/403, nil): empty findings are not a pass", approvalSettings, &AnalysisResult{CiValid: true, ProtectionData: &gitlab.GitlabProtectionAnalysisData{}}, 0, StatusError}, + {"approval-settings control with findings is failed", approvalSettings, &AnalysisResult{CiValid: true, ProtectionData: &gitlab.GitlabProtectionAnalysisData{MRApprovalSettings: &glab.ProjectApprovals{}}}, 1, StatusFailed}, {"nil result defaults to passed (hand-built test fixtures)", content, nil, 0, StatusPassed}, } for _, tc := range cases { diff --git a/control/task.go b/control/task.go index e1035120..6e420187 100644 --- a/control/task.go +++ b/control/task.go @@ -33,6 +33,136 @@ const opaEvaluateTimeout = 2 * time.Minute // the catalog in catalog.go. const controlBranchMustBeProtected = "branchMustBeProtected" const controlMutableRemoteExec = "actionsMustNotExecuteMutableRemoteCode" +const controlMRApprovalRulesMinApprovals = "mergeRequestApprovalRulesMustRequireMinimumApprovals" +const controlMRApprovalRulesCoverAllBranches = "mergeRequestApprovalRulesMustCoverAllProtectedBranches" +const controlMRApprovalSettings = "mergeRequestApprovalSettingsMustBeCompliant" +const controlMRSettings = "mergeRequestSettingsMustBeCompliant" + +// mrApprovalRuleControlEnabled reports whether either merge-request +// approval-rule control (ISSUE-502/504) is active for this run. Both read the +// approval rules the GitLab protection collection fetches, so that collection +// must run when either is enabled even if branchMustBeProtected is not. +func mrApprovalRuleControlEnabled(conf *configuration.Configuration) bool { + if c := conf.PlumberConfig.GetMergeRequestApprovalRulesMustRequireMinimumApprovalsConfig(); c != nil && c.IsEnabled() && shouldRunControl(controlMRApprovalRulesMinApprovals, conf) { + return true + } + if c := conf.PlumberConfig.GetMergeRequestApprovalRulesMustCoverAllProtectedBranchesConfig(); c != nil && c.IsEnabled() && shouldRunControl(controlMRApprovalRulesCoverAllBranches, conf) { + return true + } + return false +} + +// approvalRulesReturnedNone reports whether the protection collection ran and +// the GitLab approvals API returned zero rules — the ambiguous case where the +// project is either on GitLab Free (feature unavailable, the API 200-empties) +// or on Premium/Ultimate with no rules configured. The renderers surface a +// Premium/Ultimate caveat for it via AnalysisResult.ApprovalRulesTierCaveat. +func approvalRulesReturnedNone(protectionData *gitlab.GitlabProtectionAnalysisData) bool { + return protectionData != nil && protectionData.MRApprovalRulesKnown && + len(protectionData.MRApprovalRules) == 0 +} + +// approvalRulesTierCaveatApplies reports whether to surface the Premium/Ultimate +// caveat: an approval-rule control ran (mrApprovalRuleControlEnabled) AND the +// approvals API returned zero rules (approvalRulesReturnedNone). The enabled +// guard is load-bearing — a branch-protection-only run on a zero-rules project +// satisfies approvalRulesReturnedNone but must NOT show the caveat. +func approvalRulesTierCaveatApplies(conf *configuration.Configuration, protectionData *gitlab.GitlabProtectionAnalysisData) bool { + return mrApprovalRuleControlEnabled(conf) && approvalRulesReturnedNone(protectionData) +} + +// mrApprovalSettingsHasNoProtections reports whether the protection collection +// read the project's approval settings and NONE of them are locked down — the +// fully-unlocked state a GitLab Free project returns, where the feature does not +// exist and the approvals API 200-defaults every protection off. Unlike the +// approval RULES case (a 200-empty list), the settings API gives no other tier +// signal, so "no protection in effect" is the only heuristic available. +// +// The author field has INVERTED polarity: MergeRequestsAuthorApproval == true +// means authors CAN approve (no protection), so unlocked wants it true while the +// other five flags must be false. The check is deliberately conservative: any +// single protection active proves the project CAN lock a setting down (so it is +// on a paid tier) and suppresses the caveat. +func mrApprovalSettingsHasNoProtections(protectionData *gitlab.GitlabProtectionAnalysisData) bool { + if protectionData == nil || protectionData.MRApprovalSettings == nil { + return false + } + s := protectionData.MRApprovalSettings + return s.MergeRequestsAuthorApproval && // authors CAN approve == not locked down + !s.MergeRequestsDisableCommittersApproval && + !s.DisableOverridingApproversPerMergeRequest && + !s.RequirePasswordToApprove && + !s.ResetApprovalsOnPush && + !s.SelectiveCodeOwnerRemovals +} + +// mrApprovalSettingsTierCaveatApplies reports whether to surface the +// Premium/Ultimate caveat for the approval-settings control (ISSUE-503): the +// control ran (mrApprovalSettingsControlEnabled) AND the project has no approval +// protection in effect (mrApprovalSettingsHasNoProtections), the tell-tale +// GitLab-Free signature. The enabled guard is load-bearing the same way it is +// for the rules caveat — a run that never turned this control on must not show +// the caveat. +func mrApprovalSettingsTierCaveatApplies(conf *configuration.Configuration, protectionData *gitlab.GitlabProtectionAnalysisData) bool { + return mrApprovalSettingsControlEnabled(conf) && mrApprovalSettingsHasNoProtections(protectionData) +} + +// mrApprovalSettingsControlEnabled reports whether the merge-request +// approval-settings control (ISSUE-503) is active for this run. It reads the +// approval settings the GitLab protection collection fetches, so that +// collection must run when it is enabled even if no other protection control +// is. +func mrApprovalSettingsControlEnabled(conf *configuration.Configuration) bool { + c := conf.PlumberConfig.GetMergeRequestApprovalSettingsMustBeCompliantConfig() + return c != nil && c.IsEnabled() && shouldRunControl(controlMRApprovalSettings, conf) +} + +// mrSettingsControlEnabled reports whether the merge-request settings control +// (ISSUE-506) is active for this run. It reads the project MR settings the +// GitLab protection collection fetches, so that collection must run when it is +// enabled even if no other protection control is. +func mrSettingsControlEnabled(conf *configuration.Configuration) bool { + c := conf.PlumberConfig.GetMergeRequestSettingsMustBeCompliantConfig() + return c != nil && c.IsEnabled() && shouldRunControl(controlMRSettings, conf) +} + +// mrSettingsPremiumFieldsNeedingUpgrade returns the Premium/Ultimate MR-setting +// expectations that read as OFF while the config expects them ON (merge trains, +// merged-results pipelines). These features require a paid tier and the project +// payload carries no tier signal, so an OFF read is ambiguous: a Free project +// that cannot enable them, or a paid project that simply left them off. An +// expectation of false is satisfiable on any tier and never appears here. The +// renderers turn this into a CONDITIONAL caveat next to ISSUE-506 (disable the +// expectation on Free; enable the feature on a paid tier) rather than asserting +// which tier the project is on, since the API cannot tell us. +func mrSettingsPremiumFieldsNeedingUpgrade(conf *configuration.Configuration, protectionData *gitlab.GitlabProtectionAnalysisData) []string { + if !mrSettingsControlEnabled(conf) || protectionData == nil || protectionData.MRSettings == nil { + return nil + } + c := conf.PlumberConfig.GetMergeRequestSettingsMustBeCompliantConfig() + s := protectionData.MRSettings + var fields []string + if c.MergePipelinesEnabled != nil && *c.MergePipelinesEnabled && !s.MergePipelinesEnabled { + fields = append(fields, "mergePipelinesEnabled") + } + if c.MergeTrainsEnabled != nil && *c.MergeTrainsEnabled && !s.MergeTrainsEnabled { + fields = append(fields, "mergeTrainsEnabled") + } + return fields +} + +// protectionDataNeeded reports whether any control needs the GitLab protection +// collection this run: branchMustBeProtected, either approval-rule control, the +// approval-settings control, or the MR-settings control (they all read the one +// GitlabProtectionAnalysisData). +func protectionDataNeeded(conf *configuration.Configuration) bool { + if shouldRunControl(controlBranchMustBeProtected, conf) { + if cfg := conf.PlumberConfig.GetBranchMustBeProtectedConfig(); cfg != nil && cfg.IsEnabled() { + return true + } + } + return mrApprovalRuleControlEnabled(conf) || mrApprovalSettingsControlEnabled(conf) || mrSettingsControlEnabled(conf) +} // shouldScanMutableExec reports whether the collector should fetch and // scan action source for actionsMustNotExecuteMutableRemoteCode @@ -280,6 +410,69 @@ func buildEngineConfig(controls *configuration.ControlsConfig) map[string]any { cfg["branchMustBeProtected"] = entry } + if c := controls.MergeRequestApprovalRulesMustRequireMinimumApprovals; c != nil { + entry := map[string]any{} + if c.MinimumRequiredApprovals != nil { + entry["minimumRequiredApprovals"] = *c.MinimumRequiredApprovals + } + cfg["mergeRequestApprovalRulesMustRequireMinimumApprovals"] = entry + } + + if c := controls.MergeRequestApprovalSettingsMustBeCompliant; c != nil { + // Only SET expectations reach the engine: the Rego rule treats an + // absent key as "not checked", which is what makes every expectation + // optional. + entry := map[string]any{} + if c.PreventApprovalByAuthor != nil { + entry["preventApprovalByAuthor"] = *c.PreventApprovalByAuthor + } + if c.PreventApprovalsByCommitters != nil { + entry["preventApprovalsByCommitters"] = *c.PreventApprovalsByCommitters + } + if c.PreventEditingApprovalRulesInMR != nil { + entry["preventEditingApprovalRulesInMR"] = *c.PreventEditingApprovalRulesInMR + } + if c.RequireReAuthToApprove != nil { + entry["requireReAuthToApprove"] = *c.RequireReAuthToApprove + } + if c.BehaviorWhenCommitIsAdded != nil { + entry["behaviorWhenCommitIsAdded"] = *c.BehaviorWhenCommitIsAdded + } + cfg["mergeRequestApprovalSettingsMustBeCompliant"] = entry + } + + if c := controls.MergeRequestSettingsMustBeCompliant; c != nil { + // Only SET expectations reach the engine: the Rego rule treats an + // absent key as "not checked", which is what makes every expectation + // optional (unlike the legacy platform's unconditional equality). + entry := map[string]any{} + if c.MergeMethod != nil { + entry["mergeMethod"] = *c.MergeMethod + } + if c.SquashOption != nil { + entry["squashOption"] = *c.SquashOption + } + if c.MergePipelinesEnabled != nil { + entry["mergePipelinesEnabled"] = *c.MergePipelinesEnabled + } + if c.MergeTrainsEnabled != nil { + entry["mergeTrainsEnabled"] = *c.MergeTrainsEnabled + } + if c.AllowMergeOnSkippedPipeline != nil { + entry["allowMergeOnSkippedPipeline"] = *c.AllowMergeOnSkippedPipeline + } + if c.ResolveOutdatedDiffDiscussions != nil { + entry["resolveOutdatedDiffDiscussions"] = *c.ResolveOutdatedDiffDiscussions + } + if c.PrintingMergeRequestLinkEnabled != nil { + entry["printingMergeRequestLinkEnabled"] = *c.PrintingMergeRequestLinkEnabled + } + if c.RemoveSourceBranchAfterMerge != nil { + entry["removeSourceBranchAfterMerge"] = *c.RemoveSourceBranchAfterMerge + } + cfg["mergeRequestSettingsMustBeCompliant"] = entry + } + if c := controls.IncludesMustNotUseForbiddenVersions; c != nil { defaultForbidden := false if c.DefaultBranchIsForbiddenVersion != nil { @@ -626,23 +819,23 @@ func RunAnalysis(conf *configuration.Configuration) (*AnalysisResult, error) { // corresponding control — the Rego policy needs the protection // settings to check every branch against the declared bar. var protectionData *gitlab.GitlabProtectionAnalysisData - if shouldRunControl(controlBranchMustBeProtected, conf) { - if cfg := conf.PlumberConfig.GetBranchMustBeProtectedConfig(); cfg != nil && cfg.IsEnabled() { - reportProgress(conf, 9, analysisStepCount, "Checking branch protection") - protectionDC := &gitlab.GitlabProtectionDataCollection{} - pData, _, pErr := protectionDC.Run(projectInfo, conf.GitlabToken, conf) - if pErr != nil { - // A network failure here leaves branchMustBeProtected with zero - // branches → a vacuous 100% green. Flag degraded so that control's - // pass is not trusted (mirrors the GitHub branch path, #220). A - // non-network failure stays a soft warn as before. - if isNetworkError(pErr) { - markDegraded(result, degradedReasonBranchProtectionPrefix+" (network or timeout)") - } - l.WithError(pErr).Warn("Protection data collection failed; branch policies will see no branches") - } else { - protectionData = pData + if protectionDataNeeded(conf) { + reportProgress(conf, 9, analysisStepCount, "Checking branch protection") + protectionDC := &gitlab.GitlabProtectionDataCollection{} + pData, _, pErr := protectionDC.Run(projectInfo, conf.GitlabToken, conf) + if pErr != nil { + // A network failure here leaves branchMustBeProtected with zero + // branches → a vacuous 100% green. Flag degraded so that control's + // pass is not trusted (mirrors the GitHub branch path, #220). A + // non-network failure stays a soft warn as before. The approval-rule + // controls need no degraded flag here: a nil protectionData makes + // them report not-evaluable via StatusFor. + if isNetworkError(pErr) { + markDegraded(result, degradedReasonBranchProtectionPrefix+" (network or timeout)") } + l.WithError(pErr).Warn("Protection data collection failed; branch and approval-rule policies will see no data") + } else { + protectionData = pData } } @@ -651,6 +844,19 @@ func RunAnalysis(conf *configuration.Configuration) (*AnalysisResult, error) { // docs/REFACTOR_MULTI_PROVIDER.md §8 Phase A). result.Findings = runRegoEngine(l, conf, project, pipelineOriginData, pipelineImageData, protectionData) result.ProtectionData = protectionData + // An approval-rule control that ran but saw zero rules is the ambiguous + // GitLab-Free-vs-premium-with-no-rules case (the approvals API 200-empties + // on Free). Flag it so the renderers can surface a Premium/Ultimate caveat. + result.ApprovalRulesTierCaveat = approvalRulesTierCaveatApplies(conf, protectionData) + // The approval-settings API gives no tier signal at all (Free 200-defaults + // every protection off), so "no protection in effect" is the only heuristic; + // surface the same Premium/Ultimate caveat next to ISSUE-503. + result.MRApprovalSettingsTierCaveat = mrApprovalSettingsTierCaveatApplies(conf, protectionData) + // ISSUE-506 can require Premium/Ultimate MR settings (merge trains, merged- + // results pipelines) the project cannot turn on without a paid tier; flag + // those so the renderers advise disabling the specific expectation rather + // than presenting an unfixable failure. + result.MRSettingsPremiumCaveatFields = mrSettingsPremiumFieldsNeedingUpgrade(conf, protectionData) reportProgress(conf, analysisStepCount, analysisStepCount, "Analysis complete") diff --git a/control/task_approval_caveat_test.go b/control/task_approval_caveat_test.go new file mode 100644 index 00000000..cd185dc4 --- /dev/null +++ b/control/task_approval_caveat_test.go @@ -0,0 +1,34 @@ +package control + +import ( + "testing" + + "github.com/getplumber/plumber/gitlab" + glab "gitlab.com/gitlab-org/api/client-go" +) + +// TestApprovalRulesReturnedNone covers the tier-caveat trigger's data +// condition. The caveat fires only when the approvals API was read +// authoritatively (Known=true) and returned zero rules — the ambiguous +// GitLab-Free-vs-Premium-with-no-rules case. nil data or an unreadable listing +// (Known=false) is a collection failure, not "zero rules", and must not fire; +// a listing that returned rules is a clearly-Premium project, also no caveat. +func TestApprovalRulesReturnedNone(t *testing.T) { + cases := []struct { + name string + data *gitlab.GitlabProtectionAnalysisData + want bool + }{ + {"nil protection (collection never ran)", nil, false}, + {"unreadable listing (401/403, Known=false)", &gitlab.GitlabProtectionAnalysisData{MRApprovalRulesKnown: false}, false}, + {"known and zero rules (the caveat case)", &gitlab.GitlabProtectionAnalysisData{MRApprovalRulesKnown: true}, true}, + {"known with rules present", &gitlab.GitlabProtectionAnalysisData{MRApprovalRulesKnown: true, MRApprovalRules: []*glab.ProjectApprovalRule{{ID: 1}}}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := approvalRulesReturnedNone(tc.data); got != tc.want { + t.Errorf("approvalRulesReturnedNone = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/control/task_mr_approval_gate_test.go b/control/task_mr_approval_gate_test.go new file mode 100644 index 00000000..c2f4cfbe --- /dev/null +++ b/control/task_mr_approval_gate_test.go @@ -0,0 +1,291 @@ +package control + +import ( + "testing" + + "github.com/getplumber/plumber/configuration" + "github.com/getplumber/plumber/gitlab" + glab "gitlab.com/gitlab-org/api/client-go" +) + +func boolPtrT(b bool) *bool { return &b } + +// mrApprovalRuleControlEnabled decides whether the GitLab protection collection +// runs for an approval-only configuration. If it wrongly returns false when an +// approval control is enabled, protectionData stays nil and both controls +// silently report not-evaluable — the feature quietly stops working. Mirrors +// TestShouldScanMutableExec / TestCicdVariableControlEnabled. +func TestMrApprovalRuleControlEnabled(t *testing.T) { + minOn := &configuration.MRApprovalRulesMinApprovalsControlConfig{Enabled: boolPtrT(true)} + minOff := &configuration.MRApprovalRulesMinApprovalsControlConfig{Enabled: boolPtrT(false)} + coverOn := &configuration.EnabledOnlyControlConfig{Enabled: boolPtrT(true)} + coverOff := &configuration.EnabledOnlyControlConfig{Enabled: boolPtrT(false)} + + cfgWith := func(min *configuration.MRApprovalRulesMinApprovalsControlConfig, cover *configuration.EnabledOnlyControlConfig) *configuration.Configuration { + return &configuration.Configuration{PlumberConfig: &configuration.PlumberConfig{ + GitLab: &configuration.ProviderConfig{Controls: configuration.ControlsConfig{ + MergeRequestApprovalRulesMustRequireMinimumApprovals: min, + MergeRequestApprovalRulesMustCoverAllProtectedBranches: cover, + }}, + }} + } + + t.Run("nil PlumberConfig -> false", func(t *testing.T) { + if mrApprovalRuleControlEnabled(&configuration.Configuration{}) { + t.Fatal("expected false when PlumberConfig is nil") + } + }) + t.Run("both absent -> false", func(t *testing.T) { + if mrApprovalRuleControlEnabled(cfgWith(nil, nil)) { + t.Fatal("expected false when neither control is configured") + } + }) + t.Run("both disabled -> false", func(t *testing.T) { + if mrApprovalRuleControlEnabled(cfgWith(minOff, coverOff)) { + t.Fatal("expected false when both controls are disabled") + } + }) + t.Run("min-approvals only -> true", func(t *testing.T) { + if !mrApprovalRuleControlEnabled(cfgWith(minOn, nil)) { + t.Fatal("expected true when the min-approvals control is enabled") + } + }) + t.Run("cover-all only -> true", func(t *testing.T) { + if !mrApprovalRuleControlEnabled(cfgWith(nil, coverOn)) { + t.Fatal("expected true when the cover-all control is enabled") + } + }) + t.Run("--skip-controls excludes both -> false", func(t *testing.T) { + conf := cfgWith(minOn, coverOn) + conf.SkipControlsFilter = []string{controlMRApprovalRulesMinApprovals, controlMRApprovalRulesCoverAllBranches} + if mrApprovalRuleControlEnabled(conf) { + t.Fatal("expected false when both controls are in --skip-controls") + } + }) + t.Run("--controls omitting both -> false", func(t *testing.T) { + conf := cfgWith(minOn, coverOn) + conf.ControlsFilter = []string{"branchMustBeProtected"} + if mrApprovalRuleControlEnabled(conf) { + t.Fatal("expected false when --controls omits both approval controls") + } + }) +} + +// protectionDataNeeded is the crux of the PR: an approval-only run (with +// branchMustBeProtected disabled) must still fetch protection data. +func TestProtectionDataNeeded(t *testing.T) { + branchOn := &configuration.BranchProtectionControlConfig{Enabled: boolPtrT(true)} + approvalOn := &configuration.MRApprovalRulesMinApprovalsControlConfig{Enabled: boolPtrT(true)} + + base := func(branch *configuration.BranchProtectionControlConfig, min *configuration.MRApprovalRulesMinApprovalsControlConfig) *configuration.Configuration { + return &configuration.Configuration{PlumberConfig: &configuration.PlumberConfig{ + GitLab: &configuration.ProviderConfig{Controls: configuration.ControlsConfig{ + BranchMustBeProtected: branch, + MergeRequestApprovalRulesMustRequireMinimumApprovals: min, + }}, + }} + } + + t.Run("nothing enabled -> false", func(t *testing.T) { + if protectionDataNeeded(base(nil, nil)) { + t.Fatal("expected false when neither branch nor approval controls need protection") + } + }) + t.Run("branch protection enabled -> true", func(t *testing.T) { + if !protectionDataNeeded(base(branchOn, nil)) { + t.Fatal("expected true when branchMustBeProtected is enabled") + } + }) + t.Run("approval-only (branch disabled) still needs protection -> true", func(t *testing.T) { + if !protectionDataNeeded(base(nil, approvalOn)) { + t.Fatal("expected true when only an approval-rule control is enabled") + } + }) + t.Run("approval-settings-only still needs protection -> true", func(t *testing.T) { + conf := &configuration.Configuration{PlumberConfig: &configuration.PlumberConfig{ + GitLab: &configuration.ProviderConfig{Controls: configuration.ControlsConfig{ + MergeRequestApprovalSettingsMustBeCompliant: &configuration.MRApprovalSettingsControlConfig{Enabled: boolPtrT(true)}, + }}, + }} + if !protectionDataNeeded(conf) { + t.Fatal("expected true when only the approval-settings control is enabled") + } + }) +} + +// mrApprovalSettingsControlEnabled gates the protection collection for a +// settings-only run the same way mrApprovalRuleControlEnabled does for the +// rule controls: wrongly false means protectionData stays nil and ISSUE-503 +// silently reports not-evaluable on every run. +func TestMrApprovalSettingsControlEnabled(t *testing.T) { + cfgWith := func(c *configuration.MRApprovalSettingsControlConfig) *configuration.Configuration { + return &configuration.Configuration{PlumberConfig: &configuration.PlumberConfig{ + GitLab: &configuration.ProviderConfig{Controls: configuration.ControlsConfig{ + MergeRequestApprovalSettingsMustBeCompliant: c, + }}, + }} + } + + if mrApprovalSettingsControlEnabled(&configuration.Configuration{}) { + t.Fatal("expected false when PlumberConfig is nil") + } + if mrApprovalSettingsControlEnabled(cfgWith(nil)) { + t.Fatal("expected false when the control is not configured") + } + if mrApprovalSettingsControlEnabled(cfgWith(&configuration.MRApprovalSettingsControlConfig{Enabled: boolPtrT(false)})) { + t.Fatal("expected false when the control is disabled") + } + if !mrApprovalSettingsControlEnabled(cfgWith(&configuration.MRApprovalSettingsControlConfig{Enabled: boolPtrT(true)})) { + t.Fatal("expected true when the control is enabled") + } + skipped := cfgWith(&configuration.MRApprovalSettingsControlConfig{Enabled: boolPtrT(true)}) + skipped.SkipControlsFilter = []string{controlMRApprovalSettings} + if mrApprovalSettingsControlEnabled(skipped) { + t.Fatal("expected false when the control is in --skip-controls") + } + filtered := cfgWith(&configuration.MRApprovalSettingsControlConfig{Enabled: boolPtrT(true)}) + filtered.ControlsFilter = []string{"branchMustBeProtected"} + if mrApprovalSettingsControlEnabled(filtered) { + t.Fatal("expected false when --controls omits the control") + } +} + +// mrSettingsControlEnabled gates the protection collection for an MR-settings- +// only run the same way its siblings do: wrongly false means protectionData +// stays nil and ISSUE-506 silently reports not-evaluable on every run. +func TestMrSettingsControlEnabled(t *testing.T) { + cfgWith := func(c *configuration.MRSettingsControlConfig) *configuration.Configuration { + return &configuration.Configuration{PlumberConfig: &configuration.PlumberConfig{ + GitLab: &configuration.ProviderConfig{Controls: configuration.ControlsConfig{ + MergeRequestSettingsMustBeCompliant: c, + }}, + }} + } + + if mrSettingsControlEnabled(&configuration.Configuration{}) { + t.Fatal("expected false when PlumberConfig is nil") + } + if mrSettingsControlEnabled(cfgWith(nil)) { + t.Fatal("expected false when the control is not configured") + } + if mrSettingsControlEnabled(cfgWith(&configuration.MRSettingsControlConfig{Enabled: boolPtrT(false)})) { + t.Fatal("expected false when the control is disabled") + } + if !mrSettingsControlEnabled(cfgWith(&configuration.MRSettingsControlConfig{Enabled: boolPtrT(true)})) { + t.Fatal("expected true when the control is enabled") + } + skipped := cfgWith(&configuration.MRSettingsControlConfig{Enabled: boolPtrT(true)}) + skipped.SkipControlsFilter = []string{controlMRSettings} + if mrSettingsControlEnabled(skipped) { + t.Fatal("expected false when the control is in --skip-controls") + } +} + +// mrSettingsPremiumFieldsNeedingUpgrade flags only the Premium expectations the +// project cannot satisfy: feature expected ON but read back OFF. Expecting OFF, +// or a project that has the feature ON, is not a tier problem. +func TestMRSettingsPremiumFieldsNeedingUpgrade(t *testing.T) { + cfgWith := func(c *configuration.MRSettingsControlConfig) *configuration.Configuration { + return &configuration.Configuration{PlumberConfig: &configuration.PlumberConfig{ + GitLab: &configuration.ProviderConfig{Controls: configuration.ControlsConfig{ + MergeRequestSettingsMustBeCompliant: c, + }}, + }} + } + expectPremiumOn := &configuration.MRSettingsControlConfig{ + Enabled: boolPtrT(true), + MergePipelinesEnabled: boolPtrT(true), + MergeTrainsEnabled: boolPtrT(true), + } + freeProject := &gitlab.GitlabProtectionAnalysisData{MRSettings: &glab.Project{}} // both off + premiumProject := &gitlab.GitlabProtectionAnalysisData{MRSettings: &glab.Project{MergePipelinesEnabled: true, MergeTrainsEnabled: true}} + + if got := mrSettingsPremiumFieldsNeedingUpgrade(cfgWith(&configuration.MRSettingsControlConfig{Enabled: boolPtrT(false)}), freeProject); got != nil { + t.Fatalf("disabled control must yield no caveat fields, got %v", got) + } + if got := mrSettingsPremiumFieldsNeedingUpgrade(cfgWith(expectPremiumOn), freeProject); len(got) != 2 { + t.Fatalf("both premium fields expected-on-but-off must be flagged, got %v", got) + } + if got := mrSettingsPremiumFieldsNeedingUpgrade(cfgWith(expectPremiumOn), premiumProject); got != nil { + t.Fatalf("a project with the premium features ON needs no caveat, got %v", got) + } + expectOff := &configuration.MRSettingsControlConfig{ + Enabled: boolPtrT(true), + MergePipelinesEnabled: boolPtrT(false), + MergeTrainsEnabled: boolPtrT(false), + } + if got := mrSettingsPremiumFieldsNeedingUpgrade(cfgWith(expectOff), freeProject); got != nil { + t.Fatalf("expecting the features off is satisfiable on any tier, got %v", got) + } + if got := mrSettingsPremiumFieldsNeedingUpgrade(cfgWith(expectPremiumOn), &gitlab.GitlabProtectionAnalysisData{}); got != nil { + t.Fatalf("nil settings must yield no caveat fields, got %v", got) + } +} + +// approvalRulesTierCaveatApplies composes the enabled gate with the zero-rules +// signal. The enabled guard is the load-bearing half: a branch-protection-only +// run on a zero-rules project satisfies approvalRulesReturnedNone but must NOT +// surface the Premium/Ultimate caveat. +func TestApprovalRulesTierCaveatApplies(t *testing.T) { + withApproval := &configuration.Configuration{PlumberConfig: &configuration.PlumberConfig{ + GitLab: &configuration.ProviderConfig{Controls: configuration.ControlsConfig{ + MergeRequestApprovalRulesMustRequireMinimumApprovals: &configuration.MRApprovalRulesMinApprovalsControlConfig{Enabled: boolPtrT(true)}, + }}, + }} + branchOnly := &configuration.Configuration{PlumberConfig: &configuration.PlumberConfig{ + GitLab: &configuration.ProviderConfig{Controls: configuration.ControlsConfig{ + BranchMustBeProtected: &configuration.BranchProtectionControlConfig{Enabled: boolPtrT(true)}, + }}, + }} + zeroRules := &gitlab.GitlabProtectionAnalysisData{MRApprovalRulesKnown: true} + withRules := &gitlab.GitlabProtectionAnalysisData{MRApprovalRulesKnown: true, MRApprovalRules: []*glab.ProjectApprovalRule{{ID: 1}}} + + if approvalRulesTierCaveatApplies(branchOnly, zeroRules) { + t.Fatal("caveat must NOT fire when the approval controls are disabled (branch-only run, zero rules)") + } + if !approvalRulesTierCaveatApplies(withApproval, zeroRules) { + t.Fatal("caveat must fire when an approval control is enabled and zero rules were returned") + } + if approvalRulesTierCaveatApplies(withApproval, withRules) { + t.Fatal("caveat must NOT fire when approval rules are present") + } +} + +// mrApprovalSettingsTierCaveatApplies composes the enabled gate with the +// all-settings-false signal. The enabled guard is load-bearing the same way it +// is for the rules caveat, and any single locked setting proves the project is +// on a paid tier and must suppress the caveat. +func TestMRApprovalSettingsTierCaveatApplies(t *testing.T) { + withControl := func(enabled bool) *configuration.Configuration { + return &configuration.Configuration{PlumberConfig: &configuration.PlumberConfig{ + GitLab: &configuration.ProviderConfig{Controls: configuration.ControlsConfig{ + MergeRequestApprovalSettingsMustBeCompliant: &configuration.MRApprovalSettingsControlConfig{Enabled: boolPtrT(enabled)}, + }}, + }} + } + // Fully unlocked (the GitLab-Free signature): authors CAN approve (the author + // flag is inverted — true means no protection) and every other protection is + // off. Equivalent to the r2devops/jobs read. + unlocked := &gitlab.GitlabProtectionAnalysisData{MRApprovalSettings: &glab.ProjectApprovals{MergeRequestsAuthorApproval: true}} + // Author approval PREVENTED (author flag false) is a protection in place, so + // even a zero-value read is a configured paid tier, not the Free signature. + authorLocked := &gitlab.GitlabProtectionAnalysisData{MRApprovalSettings: &glab.ProjectApprovals{}} + // Any other single protection active likewise proves a paid tier. + overrideLocked := &gitlab.GitlabProtectionAnalysisData{MRApprovalSettings: &glab.ProjectApprovals{MergeRequestsAuthorApproval: true, DisableOverridingApproversPerMergeRequest: true}} + + if mrApprovalSettingsTierCaveatApplies(withControl(false), unlocked) { + t.Fatal("caveat must NOT fire when the settings control is disabled") + } + if !mrApprovalSettingsTierCaveatApplies(withControl(true), unlocked) { + t.Fatal("caveat must fire when the control is enabled and the project has no protections") + } + if mrApprovalSettingsTierCaveatApplies(withControl(true), authorLocked) { + t.Fatal("caveat must NOT fire when author approval is prevented (a protection proves a paid tier)") + } + if mrApprovalSettingsTierCaveatApplies(withControl(true), overrideLocked) { + t.Fatal("caveat must NOT fire when a setting is locked (proves a paid tier)") + } + if mrApprovalSettingsTierCaveatApplies(withControl(true), &gitlab.GitlabProtectionAnalysisData{}) { + t.Fatal("caveat must NOT fire when the settings were not read (nil settings)") + } +} diff --git a/control/types.go b/control/types.go index 898f7a4d..6236a0d9 100644 --- a/control/types.go +++ b/control/types.go @@ -79,6 +79,34 @@ type AnalysisResult struct { // 3) so a degraded check is visible instead of silently passing. Warnings []string `json:"warnings,omitempty"` + // ApprovalRulesTierCaveat is set when an MR approval-rule control ran but + // the GitLab approvals API returned zero rules — the ambiguous case where + // the project is either on GitLab Free (feature unavailable, API returns an + // empty list) or on Premium/Ultimate with no rules configured. The API + // gives no tier signal to tell them apart, so renderers surface a + // Premium/Ultimate caveat next to ISSUE-502/504 rather than presenting the + // result as authoritative. + ApprovalRulesTierCaveat bool `json:"-"` + + // MRApprovalSettingsTierCaveat is set when the MR approval-settings control + // ran and the project has no approval protection in effect — the GitLab-Free + // signature (the feature does not exist there and the API 200-defaults every + // protection off, which the operator cannot change). The settings API gives + // no other tier signal, so renderers surface a Premium/Ultimate caveat next + // to ISSUE-503 advising the operator to disable the control if they are on + // Free. Any single protection being active proves a paid tier and clears it. + MRApprovalSettingsTierCaveat bool `json:"-"` + + // MRSettingsPremiumCaveatFields lists the Premium/Ultimate MR-setting + // expectations (mergePipelinesEnabled, mergeTrainsEnabled) that read as OFF + // while the config expects them ON. These features require a paid tier, and + // the project payload gives no tier signal, so an OFF read is ambiguous: a + // Free project that cannot enable them, or a paid project that left them off + // (a real misconfiguration). Non-empty => renderers surface a CONDITIONAL + // caveat next to ISSUE-506 (disable the expectation if on Free; enable the + // feature if on a paid tier) rather than asserting the tier either way. + MRSettingsPremiumCaveatFields []string `json:"-"` + // DataCollectionDegraded is set when a collection or enrichment step // failed mid-run, so the analysis ran on incomplete data: a GitLab // merged-CI fetch that timed out (empty pipeline), or a GitHub run diff --git a/defaultConfig/.plumber.yaml b/defaultConfig/.plumber.yaml index 9c5fcb4d..baa3b967 100644 --- a/defaultConfig/.plumber.yaml +++ b/defaultConfig/.plumber.yaml @@ -244,6 +244,96 @@ gitlab: # Minimum access level required to push (0=No one, 30=Developer, 40=Maintainer) minPushAccessLevel: 40 # =========================================== + # MR approval rules must require a minimum number of approvals + # =========================================== + # Flags GitLab merge-request approval rules that cover all protected + # branches yet require fewer approvals than the minimum below, so a + # protected branch can be merged with too little review. Rules scoped to + # specific branches are out of scope; a rule targeting "All branches" + # counts as covering all protected branches. + # + # Merge request approval rules are a GitLab Premium/Ultimate feature. On + # GitLab Free the approvals API returns no rules (there are none), so this + # control has nothing to flag and passes vacuously — enable it only where + # approval rules are available. A genuine 401/403 (a token that cannot read + # approval rules) reports not-evaluable, not a false pass. Ships disabled: + # enable it and set your minimum. + mergeRequestApprovalRulesMustRequireMinimumApprovals: + # Set to true to enable this control + enabled: false + # The fewest approvals a rule covering all protected branches must + # require. A covering rule below this is flagged. + minimumRequiredApprovals: 1 + # =========================================== + # MR approval rules must cover all protected branches + # =========================================== + # Flags a project where no merge-request approval rule applies to all + # protected branches, so a protected branch can be merged with no required + # approval at all. Counts a rule only when it carries GitLab's explicit + # "all protected branches" target, matching the platform; a broader + # "All branches" rule is a separate concern and is not counted here. + # + # Merge request approval rules are a GitLab Premium/Ultimate feature. On + # GitLab Free the approvals API returns an empty list (not an error), so a + # project there reads as zero rules and this control FIRES — enable it only + # where approval rules are available. A genuine 401/403 (a token without + # scope) reports not-evaluable. Ships disabled. + mergeRequestApprovalRulesMustCoverAllProtectedBranches: + # Set to true to enable this control + enabled: false + # =========================================== + # MR approval settings must be compliant + # =========================================== + # Checks the project's merge-request approval settings against the + # expectations below. Each expectation is optional: a setting left unset + # (or false) is not checked, matching the platform this control migrates + # from — there is no "expect the unsafe setting" mode. + # + # REQUIRES GITLAB PREMIUM OR ULTIMATE. Merge request approval settings do + # not exist on GitLab Free, and the API gives no tier signal: on Free it + # answers 200 with defaults rather than an error, so a Free project reads + # as not locked down and this control FIRES on every run. That is why it + # ships disabled — enable it only on Premium/Ultimate projects. A genuine + # 401/403 (a token that cannot read the settings) is distinguishable and + # reports not-evaluable, not a false pass. + mergeRequestApprovalSettingsMustBeCompliant: + # Set to true to enable this control + enabled: false + # Expect that MR authors cannot approve their own merge requests. + preventApprovalByAuthor: true + # Expect that users who committed to an MR cannot approve it. + preventApprovalsByCommitters: true + # Expect that approval rules cannot be edited per merge request. + preventEditingApprovalRulesInMR: true + # Expect re-authentication (password/SAML) to approve. Strict — every + # approval re-prompts credentials; false leaves it unchecked. + requireReAuthToApprove: false + # Minimum strictness for what happens to existing approvals when a + # commit is added to an open MR, on the ladder + # keep_approvals < remove_approvals_by_code_owners < remove_all_approvals. + # remove_all_approvals is GitLab's own default for new projects, so this + # flags only projects that loosened it. + behaviorWhenCommitIsAdded: remove_all_approvals + # =========================================== + # MR settings must be compliant + # =========================================== + # Requires the project's merge-request/merge settings (Settings > Merge + # requests) to match the values below exactly. Every field is optional: + # remove any you don't want enforced. mergeMethod is one of merge, ff, + # rebase_merge; squashOption is one of never, always, default_on, + # default_off. mergePipelinesEnabled and mergeTrainsEnabled are GitLab + # Premium/Ultimate (always false on Free, so drop them there). + mergeRequestSettingsMustBeCompliant: + enabled: false + mergeMethod: ff # linear history, no merge commits + squashOption: default_on # squash to one commit by default + mergePipelinesEnabled: true # run the pipeline on the merged result + mergeTrainsEnabled: false + allowMergeOnSkippedPipeline: false # never merge when CI was skipped + resolveOutdatedDiffDiscussions: true # auto-resolve stale review threads + printingMergeRequestLinkEnabled: true + removeSourceBranchAfterMerge: true # clean up the branch after merge + # =========================================== # Pipeline must not include hardcoded jobs # =========================================== # Detects CI/CD jobs defined directly in .gitlab-ci.yml instead of being diff --git a/docs/FINGERPRINT.md b/docs/FINGERPRINT.md index 1c32884c..cb5b258f 100644 --- a/docs/FINGERPRINT.md +++ b/docs/FINGERPRINT.md @@ -377,6 +377,20 @@ an edited file as new findings. by no code, so rewording a rule cannot re-key a registered finding; prose identity survives only in the backstop for an undeclared code, which the parity test makes unreachable (see The message fallback above). +- **A rule keys on a stable coordinate, not a renameable or mutable label.** + ISSUE-502 (a merge-request approval rule requiring too few approvals) keys on + the approval rule's GitLab **ID** (`approvalRuleId`), not its user-facing + name: renaming the rule leaves the fingerprint unchanged, and only deleting + and recreating it (a new ID) re-keys it. This corrects the legacy platform, + which keyed the same control on the renameable rule name. The container-image + controls follow the same discipline — ISSUE-101/103 key on the tagless image + repository (`imageRepo`), not the mutable tag — so a routine tag or name + change never re-keys a finding. ISSUE-504, a per-project singleton, keys on + `code` alone (the platform's identity was likewise empty). ISSUE-503 (MR + approval settings not compliant) is the same per-project singleton: the + deviating-settings list is data, not identity, so a project drifting from + three deviations to one keeps the same finding rather than spawning a new + one per combination. - **A declared field holding a non-string is skipped, not coerced**, and renders as an empty pair, the same as an absent key. A JSON round trip turns a numeric `tag: 7` into a float64, so this is reachable from real payload. diff --git a/finding/identity/declarations.go b/finding/identity/declarations.go index a03bf30b..acc4942b 100644 --- a/finding/identity/declarations.go +++ b/finding/identity/declarations.go @@ -155,8 +155,16 @@ var declarations = map[string][]string{ "ISSUE-421": {"file", "job", "uses", "step"}, // Branch protection missing: keyed on the branch name. "ISSUE-501": {"file", "job", "branchName"}, + // MR approval rule below the configured minimum: keyed on the rule's stable GitLab ID. The renameable rule name is data only; keying on the ID keeps a rename from re-keying the finding, per the #370 volatile-field discipline (the platform IdOnly used the renameable name — corrected here). + "ISSUE-502": {"approvalRuleId"}, + // MR approval settings not compliant: singleton finding (one per project); the platform IdOnly was empty, so the identity is the code alone. Deliberate consequence: changing WHICH settings deviate does not re-key the finding. + "ISSUE-503": {}, + // No approval rule covers all protected branches: singleton finding (one per project); the platform IdOnly was empty, so the identity is the code alone. + "ISSUE-504": {}, // Branch protection not compliant: keyed on the branch name. "ISSUE-505": {"file", "job", "branchName"}, + // MR settings not compliant: singleton finding (one per project); the platform IdOnly was empty, so the identity is the code alone. Deliberate consequence: changing WHICH settings deviate does not re-key the finding. + "ISSUE-506": {}, // Workflow has no explicit name: one finding per workflow file, keyed on the file (benched, not yet live: declaration provisional, revisit on unbench). "ISSUE-601": {"file"}, // Action not pinned by commit SHA: keyed on the action ref (uses); step separates a reused action. diff --git a/finding/identity/identity_test.go b/finding/identity/identity_test.go index 76ccdc35..9505e352 100644 --- a/finding/identity/identity_test.go +++ b/finding/identity/identity_test.go @@ -359,7 +359,11 @@ func TestDeclarations_EveryCodeFingerprintIsPinned(t *testing.T) { "ISSUE-420": "ac3048041dde8446", "ISSUE-421": "7573d13ae392133e", "ISSUE-501": "e36cebae06c15f85", + "ISSUE-502": "b51fcaa43b9409cf", + "ISSUE-503": "86ae932a6e3d76ca", + "ISSUE-504": "b698c0c9440ef0f5", "ISSUE-505": "4e929715c61fcba6", + "ISSUE-506": "aa6378b9df051002", "ISSUE-601": "9c1ecbe668ad9a36", "ISSUE-701": "87a2f87a752971bd", "ISSUE-702": "875ec32b1513e8a8", diff --git a/gitlab/dataCollectionGitlabProtection.go b/gitlab/dataCollectionGitlabProtection.go index d838a366..94e61a83 100644 --- a/gitlab/dataCollectionGitlabProtection.go +++ b/gitlab/dataCollectionGitlabProtection.go @@ -55,12 +55,18 @@ type GitlabProtectionDataBranch struct { // GitlabProtectionAnalysisData holds all the data needed by protection controls type GitlabProtectionAnalysisData struct { - Branches []string `json:"branches"` - BranchProtections []BranchProtection `json:"branchProtections"` - MRApprovalRules []*glab.ProjectApprovalRule `json:"mrApprovalRules"` - MRApprovalSettings *glab.ProjectApprovals `json:"mrApprovalSettings"` - MRSettings *glab.Project `json:"mrSettings"` - ProjectMembers []GitlabMemberInfo `json:"projectMembers"` + Branches []string `json:"branches"` + BranchProtections []BranchProtection `json:"branchProtections"` + MRApprovalRules []*glab.ProjectApprovalRule `json:"mrApprovalRules"` + // MRApprovalRulesKnown records whether the approval-rules listing was + // read authoritatively. It stays false on a 403/404 (non-premium + // GitLab, or a token without scope), so the approval-rule controls + // (ISSUE-502/504) report not-evaluable rather than a false pass: an + // unreadable listing must not make a project look compliant. + MRApprovalRulesKnown bool `json:"mrApprovalRulesKnown"` + MRApprovalSettings *glab.ProjectApprovals `json:"mrApprovalSettings"` + MRSettings *glab.Project `json:"mrSettings"` + ProjectMembers []GitlabMemberInfo `json:"projectMembers"` } // Run fetches all GitLab protection data needed by the controls @@ -99,9 +105,11 @@ func (dc *GitlabProtectionDataCollection) Run( return nil, metrics, err } l.WithError(err).Warn("MR approval rules not available (may require premium)") - // If 403/404 error, MRApprovalRules will be nil which controls can handle + // If 403/404 error, MRApprovalRules stays nil and MRApprovalRulesKnown + // stays false, so ISSUE-502/504 report not-evaluable, not a false pass. } else { returnedData.MRApprovalRules = approvalRules + returnedData.MRApprovalRulesKnown = true } // Get project MR approval settings (may fail with 403/404 on non-premium GitLab) @@ -113,7 +121,8 @@ func (dc *GitlabProtectionDataCollection) Run( return nil, metrics, err } l.WithError(err).Warn("MR approval settings not available (may require premium)") - // If 403/404 error, MRApprovalSettings will be nil which controls can handle + // If 403/404 error, MRApprovalSettings stays nil, so ISSUE-503 reports + // not-evaluable, not a false pass (the nil pointer is the Known signal). } else { returnedData.MRApprovalSettings = approvalSettings } diff --git a/gitlab/dataCollectionGitlabProtection_run_test.go b/gitlab/dataCollectionGitlabProtection_run_test.go new file mode 100644 index 00000000..d0b2bed0 --- /dev/null +++ b/gitlab/dataCollectionGitlabProtection_run_test.go @@ -0,0 +1,91 @@ +package gitlab + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/getplumber/plumber/configuration" +) + +// protectionRunServer stands up a mux covering every endpoint +// GitlabProtectionDataCollection.Run touches, with the approval_rules endpoint's +// HTTP status parameterised so each test can drive the MRApprovalRulesKnown +// mapping without the other fetches aborting the run. +func protectionRunServer(approvalRulesStatus int, approvalRulesBody string) *httptest.Server { + mux := http.NewServeMux() + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + p := r.URL.Path + switch { + case strings.HasSuffix(p, "/approval_rules"): + if approvalRulesStatus != http.StatusOK { + w.WriteHeader(approvalRulesStatus) + return + } + _, _ = w.Write([]byte(approvalRulesBody)) + case strings.HasSuffix(p, "/approvals"): + _, _ = w.Write([]byte(`{}`)) + case strings.HasSuffix(p, "/repository/branches"): + _, _ = w.Write([]byte(`[{"name":"main"}]`)) + case strings.HasSuffix(p, "/protected_branches"): + _, _ = w.Write([]byte(`[]`)) + case strings.HasSuffix(p, "/members/all"): + _, _ = w.Write([]byte(`[]`)) + default: // GET /projects/:id — the project payload + _, _ = w.Write([]byte(`{"id":42,"name":"proj"}`)) + } + }) + return httptest.NewServer(mux) +} + +// TestProtectionRun_ApprovalRulesKnownMapping pins the crux of the +// not-evaluable-vs-false-pass design: Run records MRApprovalRulesKnown=true only +// on a real success, false on a 403/404 (premium-gated, continue), and aborts +// the whole collection on any other error. None of the individual pieces +// (FetchProjectMRApprovalRules, buildApprovalRules, StatusFor) exercise this glue. +func TestProtectionRun_ApprovalRulesKnownMapping(t *testing.T) { + proj := &ProjectInfo{ID: 42, Path: "group/project"} + dc := &GitlabProtectionDataCollection{} + + t.Run("success -> Known=true with the rules", func(t *testing.T) { + srv := protectionRunServer(http.StatusOK, `[{"id":7,"name":"r","approvals_required":1,"applies_to_all_protected_branches":true}]`) + defer srv.Close() + conf := &configuration.Configuration{GitlabURL: srv.URL, HTTPClientTimeout: 30 * time.Second} + data, _, err := dc.Run(proj, "tok", conf) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !data.MRApprovalRulesKnown { + t.Fatal("a successful approval-rules fetch must set MRApprovalRulesKnown=true") + } + if len(data.MRApprovalRules) != 1 { + t.Fatalf("expected 1 rule, got %d", len(data.MRApprovalRules)) + } + }) + + t.Run("403 (non-premium) -> Known=false, run continues", func(t *testing.T) { + srv := protectionRunServer(http.StatusForbidden, "") + defer srv.Close() + conf := &configuration.Configuration{GitlabURL: srv.URL, HTTPClientTimeout: 30 * time.Second} + data, _, err := dc.Run(proj, "tok", conf) + if err != nil { + t.Fatalf("a 403 on approval_rules must not abort the run: %v", err) + } + if data.MRApprovalRulesKnown { + t.Fatal("a 403 approval-rules fetch must leave MRApprovalRulesKnown=false (not-evaluable), not a false pass") + } + }) + + t.Run("other error (non-403/404) -> aborts the whole collection", func(t *testing.T) { + // 400 rather than 500: a 5xx would trigger the client's retry/backoff + // (slow); any non-403/404 status exercises the same abort branch in Run. + srv := protectionRunServer(http.StatusBadRequest, "") + defer srv.Close() + conf := &configuration.Configuration{GitlabURL: srv.URL, HTTPClientTimeout: 30 * time.Second} + if _, _, err := dc.Run(proj, "tok", conf); err == nil { + t.Fatal("a non-403/404 error on approval_rules must abort the collection (return an error)") + } + }) +} diff --git a/gitlab/gitlab_ir.go b/gitlab/gitlab_ir.go index 4c5d8f7f..84c973fb 100644 --- a/gitlab/gitlab_ir.go +++ b/gitlab/gitlab_ir.go @@ -5,6 +5,7 @@ import ( "fmt" "regexp" "sort" + "strconv" "strings" "gopkg.in/yaml.v2" @@ -47,6 +48,9 @@ func ToNormalizedPipeline( pipeline.Includes = buildIncludes(origin, ciConfigPath) pipeline.Jobs = buildJobs(origin, imagesByJob, ciConfigPath, pipeline.Includes) pipeline.Branches = buildBranches(protection) + pipeline.MRApprovalRules, pipeline.MRApprovalRulesKnown = buildApprovalRules(protection) + pipeline.MRApprovalSettings = buildApprovalSettings(protection) + pipeline.MRSettings = buildMRSettings(protection) if origin != nil && origin.MergedConf != nil { if globals := extractGitLabVariables(origin.MergedConf.GlobalVariables); len(globals) > 0 { pipeline.GlobalVariables = globals @@ -61,6 +65,92 @@ func ToNormalizedPipeline( return pipeline } +// buildApprovalRules projects the collected merge-request approval rules +// onto the IR. It reads the same protection collection as buildBranches and +// carries only what the approval-rule controls check: the rule's stable ID +// (stringified — the ISSUE-502 identity subject), its name (messages only), +// how many approvals it requires, and its protected-branch coverage. The +// second return is MRApprovalRulesKnown: false when the listing was +// unreadable (nil protection, or a 403/404 the collector recorded as +// MRApprovalRulesKnown=false), so a control keyed on these rules reports +// not-evaluable rather than a false pass. +func buildApprovalRules(protection *GitlabProtectionAnalysisData) ([]ir.MRApprovalRule, bool) { + if protection == nil || !protection.MRApprovalRulesKnown { + return nil, false + } + out := make([]ir.MRApprovalRule, 0, len(protection.MRApprovalRules)) + for _, r := range protection.MRApprovalRules { + if r == nil { + continue + } + out = append(out, ir.MRApprovalRule{ + ID: strconv.FormatInt(r.ID, 10), + Name: r.Name, + ApprovalsRequired: int(r.ApprovalsRequired), + AppliesToAllProtectedBranches: r.AppliesToAllProtectedBranches, + ProtectedBranchCount: len(r.ProtectedBranches), + }) + } + return out, true +} + +// buildApprovalSettings projects the collected merge-request approval +// settings onto the IR in positive-security form: every boolean reads true +// when the safer choice is in effect, so the polarity GitLab's API uses +// (merge_requests_author_approval is "author CAN approve") is normalized +// here, once, and the Rego rule compares like-for-like. The two reset flags +// collapse into the behaviorWhenCommitIsAdded ladder exactly the way the +// legacy platform derived it (controlGitlabProtectionMRApprovalSettings.go): +// keep when neither flag is set, remove-all when only reset_approvals_on_push +// is, remove-code-owner-approvals whenever selective_code_owner_removals is. +// Returns nil when the settings were not read (nil protection, or the +// 401/403 the collector recorded as a nil MRApprovalSettings), so ISSUE-503 +// abstains and reports not-evaluable rather than a false pass. +func buildApprovalSettings(protection *GitlabProtectionAnalysisData) *ir.MRApprovalSettings { + if protection == nil || protection.MRApprovalSettings == nil { + return nil + } + s := protection.MRApprovalSettings + behavior := ir.MRApprovalBehaviorKeepApprovals + switch { + case s.ResetApprovalsOnPush && !s.SelectiveCodeOwnerRemovals: + behavior = ir.MRApprovalBehaviorRemoveAllApprovals + case s.SelectiveCodeOwnerRemovals: + behavior = ir.MRApprovalBehaviorRemoveCodeOwnerApprovals + } + return &ir.MRApprovalSettings{ + PreventApprovalByAuthor: !s.MergeRequestsAuthorApproval, + PreventApprovalsByCommitters: s.MergeRequestsDisableCommittersApproval, + PreventEditingApprovalRulesInMR: s.DisableOverridingApproversPerMergeRequest, + RequireReAuthToApprove: s.RequirePasswordToApprove, + BehaviorWhenCommitIsAdded: behavior, + } +} + +// buildMRSettings projects the project-level merge-request/merge settings from +// the collected GitLab project payload onto the IR in raw form, so the +// mergeRequestSettingsMustBeCompliant control (ISSUE-506) can compare each field +// against the configured expectation for exact equality. The enum values +// (MergeMethod, SquashOption) are GitLab typed strings, carried as plain +// strings. Returns nil when the project payload was not read, so the control +// abstains and reports not-evaluable rather than a false pass. +func buildMRSettings(protection *GitlabProtectionAnalysisData) *ir.MRSettings { + if protection == nil || protection.MRSettings == nil { + return nil + } + p := protection.MRSettings + return &ir.MRSettings{ + MergeMethod: string(p.MergeMethod), + SquashOption: string(p.SquashOption), + MergePipelinesEnabled: p.MergePipelinesEnabled, + MergeTrainsEnabled: p.MergeTrainsEnabled, + AllowMergeOnSkippedPipeline: p.AllowMergeOnSkippedPipeline, + ResolveOutdatedDiffDiscussions: p.ResolveOutdatedDiffDiscussions, + PrintingMergeRequestLinkEnabled: p.PrintingMergeRequestLinkEnabled, + RemoveSourceBranchAfterMerge: p.RemoveSourceBranchAfterMerge, + } +} + // buildBranches flattens the GitLab protection API response into // ir.Branch entries. Each repository branch is matched against the // declared protection patterns; when a pattern matches, its settings diff --git a/gitlab/gitlab_ir_test.go b/gitlab/gitlab_ir_test.go index e018aa51..516a2bc3 100644 --- a/gitlab/gitlab_ir_test.go +++ b/gitlab/gitlab_ir_test.go @@ -4,8 +4,110 @@ import ( "testing" "github.com/getplumber/plumber/internal/ir" + glab "gitlab.com/gitlab-org/api/client-go" ) +// TestBuildApprovalRules covers the approval-rules projection: an unreadable +// listing stays known=false (so ISSUE-502/504 report not-evaluable), and a +// known listing projects the stable ID (stringified), the renameable name, +// the approvals count, and protected-branch coverage. +func TestBuildApprovalRules(t *testing.T) { + // nil protection -> not known, no rules. + if got, known := buildApprovalRules(nil); got != nil || known { + t.Fatalf("nil protection: got %v known %v, want nil/false", got, known) + } + + // An unreadable listing (a 403 the collector recorded) stays known=false + // even if rules are somehow present. + if _, known := buildApprovalRules(&GitlabProtectionAnalysisData{MRApprovalRulesKnown: false}); known { + t.Fatal("unreadable approval-rules listing must report known=false") + } + + data := &GitlabProtectionAnalysisData{ + MRApprovalRulesKnown: true, + MRApprovalRules: []*glab.ProjectApprovalRule{ + {ID: 42, Name: "Security", ApprovalsRequired: 1, AppliesToAllProtectedBranches: true}, + {ID: 7, Name: "Scoped", ApprovalsRequired: 2, ProtectedBranches: []*glab.ProtectedBranch{{Name: "main"}}}, + nil, + }, + } + got, known := buildApprovalRules(data) + if !known { + t.Fatal("known listing must report known=true") + } + if len(got) != 2 { + t.Fatalf("want 2 projected rules (nil entry skipped), got %d", len(got)) + } + if r := got[0]; r.ID != "42" || r.Name != "Security" || r.ApprovalsRequired != 1 || !r.AppliesToAllProtectedBranches || r.ProtectedBranchCount != 0 { + t.Fatalf("rule 0 projection mismatch: %+v", r) + } + if r := got[1]; r.ID != "7" || r.ApprovalsRequired != 2 || r.AppliesToAllProtectedBranches || r.ProtectedBranchCount != 1 { + t.Fatalf("rule 1 projection mismatch: %+v", r) + } +} + +// TestBuildApprovalSettings covers the approval-settings projection: unread +// settings project to nil (so ISSUE-503 reports not-evaluable), booleans are +// normalized to positive-security form (author-approval polarity inverted), +// and the two GitLab reset flags collapse onto the behaviorWhenCommitIsAdded +// ladder the way the legacy platform derived it. +func TestBuildApprovalSettings(t *testing.T) { + // nil protection, or settings the collector could not read -> nil. + if got := buildApprovalSettings(nil); got != nil { + t.Fatalf("nil protection: got %+v, want nil", got) + } + if got := buildApprovalSettings(&GitlabProtectionAnalysisData{}); got != nil { + t.Fatalf("unread settings (403/404): got %+v, want nil", got) + } + + // Polarity: author approval ALLOWED and committers/editing/re-auth all + // off must project to all-false prevent* fields. + weak := buildApprovalSettings(&GitlabProtectionAnalysisData{ + MRApprovalSettings: &glab.ProjectApprovals{ + MergeRequestsAuthorApproval: true, + }, + }) + if weak == nil || weak.PreventApprovalByAuthor || weak.PreventApprovalsByCommitters || + weak.PreventEditingApprovalRulesInMR || weak.RequireReAuthToApprove { + t.Fatalf("weak settings projection mismatch: %+v", weak) + } + if weak.BehaviorWhenCommitIsAdded != ir.MRApprovalBehaviorKeepApprovals { + t.Fatalf("neither reset flag set must project keep_approvals, got %q", weak.BehaviorWhenCommitIsAdded) + } + + // Polarity: everything locked down projects to all-true prevent* fields. + strict := buildApprovalSettings(&GitlabProtectionAnalysisData{ + MRApprovalSettings: &glab.ProjectApprovals{ + MergeRequestsAuthorApproval: false, + MergeRequestsDisableCommittersApproval: true, + DisableOverridingApproversPerMergeRequest: true, + RequirePasswordToApprove: true, + ResetApprovalsOnPush: true, + }, + }) + if strict == nil || !strict.PreventApprovalByAuthor || !strict.PreventApprovalsByCommitters || + !strict.PreventEditingApprovalRulesInMR || !strict.RequireReAuthToApprove { + t.Fatalf("strict settings projection mismatch: %+v", strict) + } + if strict.BehaviorWhenCommitIsAdded != ir.MRApprovalBehaviorRemoveAllApprovals { + t.Fatalf("reset_approvals_on_push alone must project remove_all_approvals, got %q", strict.BehaviorWhenCommitIsAdded) + } + + // selective_code_owner_removals wins the ladder's middle rung whenever it + // is set, even alongside reset_approvals_on_push (legacy derivation). + for _, reset := range []bool{false, true} { + selective := buildApprovalSettings(&GitlabProtectionAnalysisData{ + MRApprovalSettings: &glab.ProjectApprovals{ + ResetApprovalsOnPush: reset, + SelectiveCodeOwnerRemovals: true, + }, + }) + if selective.BehaviorWhenCommitIsAdded != ir.MRApprovalBehaviorRemoveCodeOwnerApprovals { + t.Fatalf("selective_code_owner_removals (reset=%v) must project remove_approvals_by_code_owners, got %q", reset, selective.BehaviorWhenCommitIsAdded) + } + } +} + func TestToNormalizedPipeline_Empty(t *testing.T) { pipeline := ToNormalizedPipeline("group/project", "main", "", nil, nil, nil) if pipeline.Provider != ir.ProviderGitLab { diff --git a/gitlab/rest.go b/gitlab/rest.go index a5a7180b..7bad6818 100644 --- a/gitlab/rest.go +++ b/gitlab/rest.go @@ -206,14 +206,30 @@ func FetchProjectMRApprovalRules(projectID int, token string, APIURL string, con return nil, err } - rules, _, err := glab.Projects.GetProjectApprovalRules(projectID, nil) - if err != nil { - l.WithError(err).Warn("Failed to fetch MR approval rules") - return nil, err + // Paginate: GET /projects/:id/approval_rules defaults to per_page 20, so a + // project with more rules than one page would otherwise be silently + // truncated — and the caller marks the listing authoritative + // (MRApprovalRulesKnown=true), which would turn a missed weak rule into a + // false pass for ISSUE-502. Mirrors FetchProjectMembers / FetchProjectBranchData. + var allRules []*gitlab.ProjectApprovalRule + options := &gitlab.GetProjectApprovalRulesListsOptions{ + ListOptions: gitlab.ListOptions{PerPage: 100}, + } + for page := int64(1); ; page++ { + options.Page = page + rules, resp, err := glab.Projects.GetProjectApprovalRules(projectID, options) + if err != nil { + l.WithError(err).Warn("Failed to fetch MR approval rules") + return nil, err + } + allRules = append(allRules, rules...) + if resp == nil || resp.NextPage == 0 { + break + } } - l.WithField("ruleCount", len(rules)).Debug("Fetched MR approval rules") - return rules, nil + l.WithField("ruleCount", len(allRules)).Debug("Fetched MR approval rules") + return allRules, nil } // FetchProjectMRApprovalSettings retrieves MR approval settings for a project diff --git a/gitlab/rest_approval_rules_test.go b/gitlab/rest_approval_rules_test.go new file mode 100644 index 00000000..cf2151a0 --- /dev/null +++ b/gitlab/rest_approval_rules_test.go @@ -0,0 +1,90 @@ +package gitlab + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/getplumber/plumber/configuration" +) + +// TestFetchProjectMRApprovalRules_Paginates guards the truncation-to-false-pass +// regression: the approval_rules endpoint paginates (default per_page 20), and +// the caller marks the listing authoritative, so a rule on a later page must +// still be returned. The page-2 rule here requires only 1 approval — exactly the +// weak rule a truncated fetch would drop and turn into a false pass for ISSUE-502. +func TestFetchProjectMRApprovalRules_Paginates(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v4/projects/42/approval_rules", func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Query().Get("page") { + case "", "1": + w.Header().Set("X-Next-Page", "2") + _ = json.NewEncoder(w).Encode([]map[string]any{ + {"id": 1, "name": "page1-rule", "approvals_required": 2, "applies_to_all_protected_branches": true}, + }) + case "2": + // Final page: no X-Next-Page -> NextPage 0 -> loop terminates. + _ = json.NewEncoder(w).Encode([]map[string]any{ + {"id": 2, "name": "page2-rule", "approvals_required": 1, "applies_to_all_protected_branches": true}, + }) + default: + t.Errorf("unexpected page %q", r.URL.Query().Get("page")) + } + }) + srv := httptest.NewServer(mux) + defer srv.Close() + conf := &configuration.Configuration{HTTPClientTimeout: 30 * time.Second} + + rules, err := FetchProjectMRApprovalRules(42, "glpat-test", srv.URL, conf) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + names := map[string]bool{} + for _, r := range rules { + names[r.Name] = true + } + if len(rules) != 2 || !names["page1-rule"] || !names["page2-rule"] { + t.Fatalf("expected the union of both pages, got %d rules: %v", len(rules), names) + } +} + +// TestFetchProjectMRApprovalRules_SinglePageTerminates: with no X-Next-Page the +// loop must stop after one request (no infinite loop / no extra call). +func TestFetchProjectMRApprovalRules_SinglePageTerminates(t *testing.T) { + calls := 0 + mux := http.NewServeMux() + mux.HandleFunc("/api/v4/projects/42/approval_rules", func(w http.ResponseWriter, _ *http.Request) { + calls++ + _ = json.NewEncoder(w).Encode([]map[string]any{{"id": 1, "name": "only", "approvals_required": 2}}) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + conf := &configuration.Configuration{HTTPClientTimeout: 30 * time.Second} + + rules, err := FetchProjectMRApprovalRules(42, "t", srv.URL, conf) + if err != nil || len(rules) != 1 { + t.Fatalf("got (%d rules, %v), want (1, nil)", len(rules), err) + } + if calls != 1 { + t.Errorf("expected the loop to terminate after 1 page, made %d calls", calls) + } +} + +// TestFetchProjectMRApprovalRules_ErrorPropagates: a mid-fetch API error must +// return a non-nil error so the caller leaves MRApprovalRulesKnown=false +// (not-evaluable) rather than treating a partial/empty list as authoritative. +func TestFetchProjectMRApprovalRules_ErrorPropagates(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v4/projects/42/approval_rules", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + conf := &configuration.Configuration{HTTPClientTimeout: 30 * time.Second} + + if _, err := FetchProjectMRApprovalRules(42, "t", srv.URL, conf); err == nil { + t.Fatal("expected a non-nil error so the caller leaves MRApprovalRulesKnown=false") + } +} diff --git a/internal/ir/pipeline.go b/internal/ir/pipeline.go index 46fb91a7..f8b00a06 100644 --- a/internal/ir/pipeline.go +++ b/internal/ir/pipeline.go @@ -66,6 +66,41 @@ type NormalizedPipeline struct { // (root, .github/, or docs/). Empty when the file is absent. SecurityPolicyPath string `json:"securityPolicyPath,omitempty"` + // MRApprovalRules are the project's merge-request approval rules + // (GitLab: Settings > Merge requests > Approval rules), each with the + // number of approvals it requires and its protected-branch coverage. + // Projected from the same protection collection as Branches; empty on + // providers without approval rules. Read by the + // mergeRequestApprovalRulesMust* controls (ISSUE-502/504). + MRApprovalRules []MRApprovalRule `json:"mrApprovalRules,omitempty"` + + // MRApprovalRulesKnown is true when the approval-rules listing was + // fetched authoritatively (an empty MRApprovalRules then means "no + // rules", a real state the rules reason about). It is false when the + // listing could not be read — a 401/403 on the approvals API (commonly + // a non-premium GitLab, or a token without scope) — so a control keyed + // on these rules reports not-evaluable rather than a false pass. + // Mirrors Branch.ProtectionDetailsKnown. + MRApprovalRulesKnown bool `json:"mrApprovalRulesKnown,omitempty"` + + // MRApprovalSettings are the project's merge-request approval settings + // (GitLab: Settings > Merge requests), normalized to positive-security + // form. nil when the settings could not be read (a 401/403 from a token + // without scope), so the mergeRequestApprovalSettingsMustBeCompliant + // control (ISSUE-503) abstains and reports not-evaluable rather than a + // false pass. Projected from the same protection collection as + // MRApprovalRules. + MRApprovalSettings *MRApprovalSettings `json:"mrApprovalSettings,omitempty"` + + // MRSettings are the project's merge-request/merge settings (GitLab: + // Settings > Merge requests — merge method, squash, merge trains, etc.), + // carried in raw form so the mergeRequestSettingsMustBeCompliant control + // (ISSUE-506) can compare each against the configured expectation. nil when + // the project payload could not be read, so the control abstains and reports + // not-evaluable rather than a false pass. Projected from the same protection + // collection as MRApprovalSettings. + MRSettings *MRSettings `json:"mrSettings,omitempty"` + // Dockerfiles lists every Dockerfile the collector scanned at the // repo root and under common build directories, with each FROM // base-image extracted so policies can check pinning state. @@ -400,3 +435,97 @@ type Branch struct { MinMergeAccessLevel int `json:"minMergeAccessLevel,omitempty"` ProtectionDetailsKnown bool `json:"protectionDetailsKnown,omitempty"` } + +// MRApprovalRule is one merge-request approval rule (GitLab: Settings > +// Merge requests > Approval rules). It carries the rule's stable identity +// and the fields the approval-rule controls check: how many approvals it +// requires and whether it covers all protected branches. +type MRApprovalRule struct { + // ID is GitLab's approval-rule ID, stringified. It is stable for the + // rule's lifetime (it churns only on delete-and-recreate, which is a + // new rule), so ISSUE-502 keys its finding identity on ID rather than + // on the renameable Name, per the #370 volatile-field discipline. + ID string `json:"id"` + // Name is the human label. It renders in findings but is deliberately + // NOT an identity field: renaming a rule must not re-key its finding. + Name string `json:"name,omitempty"` + // ApprovalsRequired is how many approvals the rule mandates. + ApprovalsRequired int `json:"approvalsRequired"` + // AppliesToAllProtectedBranches is GitLab's explicit "all protected + // branches" flag. ISSUE-504 requires at least one rule to carry it. + AppliesToAllProtectedBranches bool `json:"appliesToAllProtectedBranches,omitempty"` + // ProtectedBranchCount is how many specific protected branches the rule + // is scoped to. Zero means it is scoped to none in particular, which + // GitLab treats as covering all branches; ISSUE-502 checks a rule that + // covers all protected branches (the explicit flag OR a zero count). + ProtectedBranchCount int `json:"protectedBranchCount"` +} + +// BehaviorWhenCommitIsAdded values, ordered by strictness: what happens to +// existing approvals when a new commit lands on an open merge request. +// ISSUE-503 treats the configured expectation as a minimum on this ladder +// (keep < remove code-owner approvals < remove all), matching the legacy +// platform's ordinal comparison. +const ( + MRApprovalBehaviorKeepApprovals = "keep_approvals" + MRApprovalBehaviorRemoveCodeOwnerApprovals = "remove_approvals_by_code_owners" + MRApprovalBehaviorRemoveAllApprovals = "remove_all_approvals" +) + +// MRApprovalSettings are the project-level merge-request approval settings +// (GitLab: Settings > Merge requests), normalized to positive-security form: +// every boolean is true when the safer choice is in effect, regardless of the +// polarity GitLab's API uses (merge_requests_author_approval, for example, is +// inverted at projection). Read by the +// mergeRequestApprovalSettingsMustBeCompliant control (ISSUE-503). +type MRApprovalSettings struct { + // PreventApprovalByAuthor is true when MR authors cannot approve their + // own merge requests (GitLab: !merge_requests_author_approval). + PreventApprovalByAuthor bool `json:"preventApprovalByAuthor"` + // PreventApprovalsByCommitters is true when users who committed to an MR + // cannot approve it (GitLab: merge_requests_disable_committers_approval). + PreventApprovalsByCommitters bool `json:"preventApprovalsByCommitters"` + // PreventEditingApprovalRulesInMR is true when approval rules cannot be + // overridden per merge request + // (GitLab: disable_overriding_approvers_per_merge_request). + PreventEditingApprovalRulesInMR bool `json:"preventEditingApprovalRulesInMR"` + // RequireReAuthToApprove is true when approving requires + // re-authentication (GitLab: require_password_to_approve). + RequireReAuthToApprove bool `json:"requireReAuthToApprove"` + // BehaviorWhenCommitIsAdded is one of the MRApprovalBehavior* values, + // derived from GitLab's reset_approvals_on_push and + // selective_code_owner_removals flags. + BehaviorWhenCommitIsAdded string `json:"behaviorWhenCommitIsAdded"` +} + +// MRSettings are the project-level merge-request/merge settings, carried in raw +// form (not positive-security) so the mergeRequestSettingsMustBeCompliant +// control (ISSUE-506) can compare each field against the operator's configured +// expectation for exact equality. Field semantics mirror GitLab's project +// payload (Settings > Merge requests). +type MRSettings struct { + // MergeMethod is the project's merge method: "merge", "ff", or + // "rebase_merge" (GitLab: merge_method). + MergeMethod string `json:"mergeMethod"` + // SquashOption is the project's squash policy: "never", "always", + // "default_on", or "default_off" (GitLab: squash_option). + SquashOption string `json:"squashOption"` + // MergePipelinesEnabled is true when merged results pipelines are enabled + // (GitLab: merge_pipelines_enabled; Premium/Ultimate). + MergePipelinesEnabled bool `json:"mergePipelinesEnabled"` + // MergeTrainsEnabled is true when merge trains are enabled + // (GitLab: merge_trains_enabled; Premium/Ultimate). + MergeTrainsEnabled bool `json:"mergeTrainsEnabled"` + // AllowMergeOnSkippedPipeline is true when an MR can be merged while its + // pipeline is skipped (GitLab: allow_merge_on_skipped_pipeline). + AllowMergeOnSkippedPipeline bool `json:"allowMergeOnSkippedPipeline"` + // ResolveOutdatedDiffDiscussions is true when outdated diff discussions are + // auto-resolved on push (GitLab: resolve_outdated_diff_discussions). + ResolveOutdatedDiffDiscussions bool `json:"resolveOutdatedDiffDiscussions"` + // PrintingMergeRequestLinkEnabled is true when the MR-creation link is + // printed on push (GitLab: printing_merge_request_link_enabled). + PrintingMergeRequestLinkEnabled bool `json:"printingMergeRequestLinkEnabled"` + // RemoveSourceBranchAfterMerge is true when the source branch is deleted by + // default after merge (GitLab: remove_source_branch_after_merge). + RemoveSourceBranchAfterMerge bool `json:"removeSourceBranchAfterMerge"` +} diff --git a/policies/mr_approval_rules_cover_all_branches.rego b/policies/mr_approval_rules_cover_all_branches.rego new file mode 100644 index 00000000..c2307c93 --- /dev/null +++ b/policies/mr_approval_rules_cover_all_branches.rego @@ -0,0 +1,57 @@ +# mr-approval-rules-cover-all-branches — flag a project where no merge-request +# approval rule targets all protected branches. When every rule is scoped to +# specific branches, a protected branch can exist that no rule covers, so it +# can be merged with no required approval at all. GitLab-only singleton finding +# (one per project); the legacy platform's identity was empty, so the identity +# here is the code alone. +# +# A rule counts only when it carries GitLab's explicit +# `applies_to_all_protected_branches` flag, matching the legacy platform. A +# broader rule targeting "All branches" (no branch scope) is deliberately NOT +# counted here: this control is specifically about the "all protected branches" +# target, and blanket all-branches coverage is a separate concern. This is why +# the minimum-approvals sibling (ISSUE-502), which also accepts a zero-scope +# rule, uses a wider predicate than this one. +# +# The flag is also the ONLY signal used — the rule's scoped branch names are +# never unioned against the project's protected branches. So a project whose +# protected branches are each covered by branch-scoped rules (most commonly a +# single-protected-branch repo with a rule scoped to that one branch) still +# fires, because no rule carries the explicit flag. This is a deliberate +# legacy-faithful limitation, the same one ISSUE-502 carries for scoped rules +# (see mr_approval_rules_min_approvals.rego): the legacy control +# (controlGitlabProtectionMRApprovalRulesAllProtectedBranchesMissing.go) checked +# only the flag, and closing it would need the rule's branch names on the IR +# (only protectedBranchCount is projected today) to compare against +# input.pipeline.branches — a change away from the platform, not a bug fix. +# +# Reads input.pipeline.mrApprovalRules, projected from the protection +# collection. input.pipeline.mrApprovalRulesKnown is false when the approvals +# API could not be read (a 401/403 from a token without scope); the rule +# abstains then, so the control reports not-evaluable, not a pass. Merge +# request approval rules are a GitLab Premium/Ultimate feature; on GitLab Free +# the API returns an empty list (not an error), so a project there reads as +# zero rules and this control fires — enable it only where approval rules are +# available. A project with zero rules IS a finding: no rule covers all +# protected branches, so the gate is absent. +package mr_approval_rules_cover_all_branches + +import rego.v1 + +deny contains finding if { + input.pipeline.provider == "gitlab" + input.pipeline.mrApprovalRulesKnown + rules := object.get(input.pipeline, "mrApprovalRules", []) + not _has_all_protected_branches_rule(rules) + finding := { + "code": "ISSUE-504", + "severity": "high", + "message": sprintf("no merge request approval rule applies to all protected branches (%d rule(s) defined) — a protected branch can be merged with no required approval", [count(rules)]), + "totalRules": count(rules), + } +} + +_has_all_protected_branches_rule(rules) if { + some rule in rules + rule.appliesToAllProtectedBranches +} diff --git a/policies/mr_approval_rules_min_approvals.rego b/policies/mr_approval_rules_min_approvals.rego new file mode 100644 index 00000000..56dfae71 --- /dev/null +++ b/policies/mr_approval_rules_min_approvals.rego @@ -0,0 +1,64 @@ +# mr-approval-rules-min-approvals — flag merge-request approval rules that +# require fewer approvals than the configured minimum. GitLab lets a rule +# covering all protected branches require zero (or too few) approvals, which +# quietly weakens the review gate on exactly the branches that ship to +# production. Only rules that cover ALL protected branches are checked: the +# explicit `applies_to_all_protected_branches` flag, or a rule scoped to no +# specific branch (GitLab treats that as covering every branch). A rule scoped +# to one feature branch is out of scope for this control, matching the legacy +# platform semantics. +# +# Three deliberate limitations, kept to match the legacy platform exactly — we +# are migrating it, not improving it (see +# jobs/control/controlGitlabProtectionMRApprovalRulesBelowMinApprovalRequired.go): +# - Per rule, not aggregate: each covering rule below the minimum is flagged +# on its own; a stricter covering rule does NOT suppress a weaker one. Two +# covering rules requiring 1 and 2 both surface when the minimum is 2, even +# though GitLab requires an MR to satisfy the stricter rule anyway. +# - Coverage is decided by the flag or a zero branch scope, never by comparing +# a named branch list against the project's protected branches. A rule that +# enumerates every protected branch by name (protectedBranchCount > 0) is +# treated as out of scope. +# - Every approval rule type is checked (regular, any_approver, code_owner, +# report_approver); the type is not projected onto the IR and not filtered. +# A non-review rule — e.g. a scan-result-policy report_approver rule or the +# built-in Coverage-Check — that covers all protected branches with a low bar +# is flagged the same as a human-review rule. +# +# GitLab-only: reads input.pipeline.mrApprovalRules, projected from the +# protection collection (gitlab/gitlab_ir.go::buildApprovalRules). +# input.pipeline.mrApprovalRulesKnown is false when the approvals API could +# not be read (a 401/403 from a non-premium GitLab, or a token without scope); +# the rule abstains then, so the control reports not-evaluable, not a pass. +# Identity keys on the rule's stable ID (approvalRuleId), never the renameable +# name, per the #370 volatile-field discipline. +package mr_approval_rules_min_approvals + +import rego.v1 + +deny contains finding if { + input.pipeline.provider == "gitlab" + input.pipeline.mrApprovalRulesKnown + some rule in object.get(input.pipeline, "mrApprovalRules", []) + _covers_all_protected_branches(rule) + cfg := object.get(input.config, "mergeRequestApprovalRulesMustRequireMinimumApprovals", {}) + minimum := object.get(cfg, "minimumRequiredApprovals", 0) + rule.approvalsRequired < minimum + finding := { + "code": "ISSUE-502", + "severity": "high", + "message": sprintf("merge request approval rule %q requires %d approval(s), below the configured minimum of %d — a protected branch can be merged with too little review", [rule.name, rule.approvalsRequired, minimum]), + "approvalRuleId": rule.id, + "ruleName": rule.name, + "approvalsRequired": rule.approvalsRequired, + "minApprovalsRequired": minimum, + } +} + +# A rule covers all protected branches when GitLab's explicit flag is set, or +# when it is scoped to no specific protected branch (protectedBranchCount == +# 0), which GitLab treats as applying to every branch. Mirrors the legacy +# control's `AppliesToAllProtectedBranches || len(ProtectedBranches) == 0`. +_covers_all_protected_branches(rule) if rule.appliesToAllProtectedBranches + +_covers_all_protected_branches(rule) if rule.protectedBranchCount == 0 diff --git a/policies/mr_approval_settings_compliant.rego b/policies/mr_approval_settings_compliant.rego new file mode 100644 index 00000000..96cb8108 --- /dev/null +++ b/policies/mr_approval_settings_compliant.rego @@ -0,0 +1,105 @@ +# mr-approval-settings-compliant — flag a project whose merge-request approval +# settings fall short of the configured expectations. GitLab lets approvals be +# weakened at the project level (authors approving their own MRs, committers +# approving code they wrote, per-MR rule overrides, no re-auth, approvals kept +# when new commits land), which quietly undermines every approval rule sitting +# on top. GitLab-only singleton finding (one per project); the legacy +# platform's identity was empty, so the identity here is the code alone — a +# deliberate consequence is that changing WHICH settings deviate does not +# re-key the finding. +# +# Every expectation is optional (unset = not checked). The four booleans check +# only when configured true, matching the legacy platform's conf semantics +# (controlGitlabProtectionMRApprovalSettings.go: `if p.PreventX && !actualX`), +# so an explicit `false` is the same as unset — there is no "expect the unsafe +# setting" mode. behaviorWhenCommitIsAdded is a minimum on the strictness +# ladder keep_approvals < remove_approvals_by_code_owners < +# remove_all_approvals, again the platform's ordinal comparison. A value +# outside the ladder is rejected at config load (plumberconfig validation), +# so an unknown string never silently disables the check here. +# +# Reads input.pipeline.mrApprovalSettings, projected in positive-security form +# from the protection collection (gitlab/gitlab_ir.go::buildApprovalSettings). +# The projection is nil — absent here — when the settings API could not be +# read (a 401/403 from a token without scope); the rule abstains then, so the +# control reports not-evaluable, not a pass. Merge-request approval settings +# require GitLab Premium or Ultimate; they do not exist on Free, where the API +# answers 200 with defaults instead of an error. That leaves no tier signal to +# branch on, so a Free project reads as unlocked-down and fires — which is why +# the control ships disabled and documents the tier requirement rather than +# guessing (the same call as #412 for the unmasked-variables control). +package mr_approval_settings_compliant + +import rego.v1 + +_behavior_rank := { + "keep_approvals": 1, + "remove_approvals_by_code_owners": 2, + "remove_all_approvals": 3, +} + +# The four boolean expectations share one shape: configured true + actual +# false = deviation. Listed once so the deviation set and the finding message +# cannot drift apart. +_boolean_settings := [ + "preventApprovalByAuthor", + "preventApprovalsByCommitters", + "preventEditingApprovalRulesInMR", + "requireReAuthToApprove", +] + +deny contains finding if { + input.pipeline.provider == "gitlab" + settings := input.pipeline.mrApprovalSettings + cfg := object.get(input.config, "mergeRequestApprovalSettingsMustBeCompliant", {}) + deviations := _deviations(settings, cfg) + count(deviations) > 0 + clauses := [_deviation_clause(name, settings, cfg) | some name in deviations] + finding := { + "code": "ISSUE-503", + "severity": "high", + "message": sprintf( + "merge request approval settings can be weakened at the project level, overriding any approval rule: %s", + [concat("; ", clauses)], + ), + "deviatingSettings": deviations, + "behaviorWhenCommitIsAdded": settings.behaviorWhenCommitIsAdded, + } +} + +# _deviation_clause renders one deviation as a human-readable current-vs-expected +# clause, so the finding reads as "what is wrong and what was expected" rather +# than a bare list of config keys. The four booleans only ever reach here in +# their unsafe state (configured true, projected false), so each clause is +# fixed; behaviorWhenCommitIsAdded carries the project's actual rung and the +# configured minimum off the strictness ladder. +_deviation_clause("preventApprovalByAuthor", _, _) := "authors can approve their own merge requests (should be prevented)" + +_deviation_clause("preventApprovalsByCommitters", _, _) := "users who added commits can approve (should be prevented)" + +_deviation_clause("preventEditingApprovalRulesInMR", _, _) := "approval rules can be overridden per merge request (should be locked)" + +_deviation_clause("requireReAuthToApprove", _, _) := "approving does not require re-authentication (should be required)" + +_deviation_clause("behaviorWhenCommitIsAdded", settings, cfg) := sprintf( + "approvals are %q when a commit is added (should be at least %q)", + [settings.behaviorWhenCommitIsAdded, object.get(cfg, "behaviorWhenCommitIsAdded", "")], +) + +# _deviations returns the sorted list of expectation names the project fails: +# the boolean expectations configured true whose projected setting is false, +# plus behaviorWhenCommitIsAdded when the project sits below the configured +# minimum on the strictness ladder. +_deviations(settings, cfg) := sort(array.concat( + [name | + some name in _boolean_settings + object.get(cfg, name, false) == true + object.get(settings, name, false) == false + ], + [name | + name := "behaviorWhenCommitIsAdded" + expected := object.get(cfg, name, "") + expected != "" + _behavior_rank[expected] > _behavior_rank[settings.behaviorWhenCommitIsAdded] + ], +)) diff --git a/policies/mr_settings_compliant.rego b/policies/mr_settings_compliant.rego new file mode 100644 index 00000000..df6051f3 --- /dev/null +++ b/policies/mr_settings_compliant.rego @@ -0,0 +1,77 @@ +# mr-settings-compliant — flag a project whose merge-request/merge settings do +# not match the operator's configured expectations (GitLab: Settings > Merge +# requests — merge method, squash policy, merge trains, source-branch removal, +# and related options). GitLab-only singleton finding (one per project); the +# legacy platform's identity was empty, so the identity here is the code alone — +# changing WHICH settings deviate does not re-key the finding. +# +# Every expectation is optional (unset = not checked) and each set expectation +# is compared for EXACT equality against the project's actual value. This +# differs deliberately from the legacy platform, which compared all eight fields +# unconditionally with the policy's zero value when a field was unset; the +# platform always supplied a fully populated policy, so optional-here is +# equivalent in practice and keeps a hand-authored YAML from flagging on a field +# the operator never set. Two settings are enums validated at config load +# (mergeMethod, squashOption), so an unknown expectation never reaches here. +# +# The config key allowMergeOnSkippedPipeline maps to GitLab's +# allow_merge_on_skipped_pipeline; the legacy platform called it +# mergeTrainsSkipTrainAllowed, a misnomer corrected in this migration. +# +# Reads input.pipeline.mrSettings, projected in raw form from the protection +# collection (gitlab/gitlab_ir.go::buildMRSettings). The projection is nil — +# absent here — when the project payload could not be read; the rule abstains +# then, so the control reports not-evaluable, not a pass. Merge trains and +# merged-results pipelines are GitLab Premium/Ultimate (false on Free); the +# other settings exist on every tier, so no tier caveat applies to this control. +package mr_settings_compliant + +import rego.v1 + +# The settings the control can check, mapping each key to a human-readable +# label used in the finding message. Listed once so the deviation set and the +# message cannot drift apart. +_labels := { + "mergeMethod": "merge method", + "squashOption": "squash option", + "mergePipelinesEnabled": "merged results pipelines", + "mergeTrainsEnabled": "merge trains", + "allowMergeOnSkippedPipeline": "allow merge on skipped pipeline", + "resolveOutdatedDiffDiscussions": "resolve outdated diff discussions", + "printingMergeRequestLinkEnabled": "print merge request link on push", + "removeSourceBranchAfterMerge": "remove source branch after merge", +} + +deny contains finding if { + input.pipeline.provider == "gitlab" + settings := input.pipeline.mrSettings + cfg := object.get(input.config, "mergeRequestSettingsMustBeCompliant", {}) + deviations := _deviations(settings, cfg) + count(deviations) > 0 + clauses := [_clause(name, settings, cfg) | some name in deviations] + finding := { + "code": "ISSUE-506", + "severity": "medium", + "message": sprintf( + "merge request settings do not match the configured expectations: %s", + [concat("; ", clauses)], + ), + "deviatingSettings": deviations, + } +} + +# _deviations returns the sorted list of setting names whose configured +# expectation does not equal the project's actual value. A setting absent from +# the config is not checked: its cfg lookup is undefined and the row drops. +_deviations(settings, cfg) := sort([name | + some name, actual in settings + expected := cfg[name] + expected != actual +]) + +# _clause renders one deviation as a human-readable "