From 02c5edf2cf31f5019d846dd6ad6f794cf7862e92 Mon Sep 17 00:00:00 2001 From: Joseph Moukarzel Date: Mon, 17 Aug 2026 19:23:10 +0200 Subject: [PATCH 1/2] feat(controls): Add MR approval rules controls --- .plumber.yaml | 19 +++ README.md | 1 + cmd/analyze_shared.go | 1 + cmd/init.go | 63 +++++++- cmd/init_mr_approval_test.go | 59 +++++++ cmd/legacy_json.go | 58 +++++++ cmd/legacy_json_gitlab_test.go | 119 ++++++++++++++ cmd/render_details.go | 22 +++ configuration/plumberconfig.go | 54 +++++++ configuration/plumberconfig_test.go | 2 + configuration/registry.go | 32 ++-- configuration/v1_to_v2.go | 4 + control/catalog.go | 16 ++ control/codes.go | 22 +++ control/mr_approval_config_contract_test.go | 75 +++++++++ control/status.go | 14 ++ control/status_test.go | 6 + control/task.go | 91 +++++++++-- control/task_approval_caveat_test.go | 34 ++++ control/task_mr_approval_gate_test.go | 133 ++++++++++++++++ control/types.go | 9 ++ defaultConfig/.plumber.yaml | 38 +++++ docs/FINGERPRINT.md | 10 ++ finding/identity/declarations.go | 4 + finding/identity/identity_test.go | 2 + gitlab/dataCollectionGitlabProtection.go | 22 ++- ...dataCollectionGitlabProtection_run_test.go | 91 +++++++++++ gitlab/gitlab_ir.go | 31 ++++ gitlab/gitlab_ir_test.go | 40 +++++ gitlab/rest.go | 28 +++- gitlab/rest_approval_rules_test.go | 90 +++++++++++ internal/ir/pipeline.go | 42 +++++ .../mr_approval_rules_cover_all_branches.rego | 57 +++++++ policies/mr_approval_rules_min_approvals.rego | 64 ++++++++ policies/rules_test.go | 147 ++++++++++++++++++ 35 files changed, 1455 insertions(+), 45 deletions(-) create mode 100644 cmd/init_mr_approval_test.go create mode 100644 cmd/legacy_json_gitlab_test.go create mode 100644 control/mr_approval_config_contract_test.go create mode 100644 control/task_approval_caveat_test.go create mode 100644 control/task_mr_approval_gate_test.go create mode 100644 gitlab/dataCollectionGitlabProtection_run_test.go create mode 100644 gitlab/rest_approval_rules_test.go create mode 100644 policies/mr_approval_rules_cover_all_branches.rego create mode 100644 policies/mr_approval_rules_min_approvals.rego diff --git a/.plumber.yaml b/.plumber.yaml index 54d4d379..54837a95 100644 --- a/.plumber.yaml +++ b/.plumber.yaml @@ -178,6 +178,25 @@ 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 + # =========================================== # 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..75b7b6ba 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) - 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..8af6da26 100644 --- a/cmd/analyze_shared.go +++ b/cmd/analyze_shared.go @@ -104,6 +104,7 @@ 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) printSectionHeader("Summary") fmt.Println() diff --git a/cmd/init.go b/cmd/init.go index 3743aacf..90c9676a 100644 --- a/cmd/init.go +++ b/cmd/init.go @@ -27,7 +27,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 +195,12 @@ type initWizardState struct { BranchMinMergeAccessLevel string BranchMinPushAccessLevel string + // mergeRequestApprovalRulesMustRequireMinimumApprovals / + // ...MustCoverAllProtectedBranches (GitLab-only, catAccess). + MRApprovalMinEnabled bool + MRApprovalMinCount string + MRApprovalCoverAllEnabled bool + // pipelineMustNotEnableDebugTrace DebugForbiddenVariablesMultiline string @@ -490,6 +496,28 @@ 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 hasProvider(st, "github") { if err := survey.AskOne(&survey.Confirm{ @@ -773,6 +801,30 @@ 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 +} + // defaultForbiddenTags is the CSV prompt default for forbidden image tags, // sourced from the GitLab containerImageMustNotUseForbiddenTags default. func defaultForbiddenTags() string { @@ -1009,6 +1061,15 @@ 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 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..2b9248ac --- /dev/null +++ b/cmd/init_mr_approval_test.go @@ -0,0 +1,59 @@ +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) + } + }) +} diff --git a/cmd/legacy_json.go b/cmd/legacy_json.go index 0489987f..30a9de51 100644 --- a/cmd/legacy_json.go +++ b/cmd/legacy_json.go @@ -98,10 +98,32 @@ 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, + } + } } 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." + +// 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 +166,46 @@ 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) } 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, + } +} + // 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..8e3d6077 --- /dev/null +++ b/cmd/legacy_json_gitlab_test.go @@ -0,0 +1,119 @@ +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") + } +} + +// 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") + } +} diff --git a/cmd/render_details.go b/cmd/render_details.go index 6c7e48a6..48f362a9 100644 --- a/cmd/render_details.go +++ b/cmd/render_details.go @@ -76,6 +76,20 @@ 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) +} + // 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 +866,14 @@ 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 "branchMustBeProtected": total, toProtect, protected, unprotected := _branchProtectionCounts(result, pc) nonCompliant := 0 diff --git a/configuration/plumberconfig.go b/configuration/plumberconfig.go index bbaaaefe..4eec52a0 100644 --- a/configuration/plumberconfig.go +++ b/configuration/plumberconfig.go @@ -36,6 +36,12 @@ var validControlSchema = map[string][]string{ "allowForcePush", "codeOwnerApprovalRequired", "minMergeAccessLevel", "minPushAccessLevel", }, + "mergeRequestApprovalRulesMustRequireMinimumApprovals": { + "enabled", "minimumRequiredApprovals", + }, + "mergeRequestApprovalRulesMustCoverAllProtectedBranches": { + "enabled", + }, "pipelineMustNotIncludeHardcodedJobs": { "enabled", }, @@ -258,6 +264,18 @@ 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"` + // PipelineMustNotIncludeHardcodedJobs control configuration PipelineMustNotIncludeHardcodedJobs *HardcodedJobsControlConfig `yaml:"pipelineMustNotIncludeHardcodedJobs,omitempty"` @@ -602,6 +620,28 @@ 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 +} + type BranchProtectionControlConfig struct { // Enabled controls whether this check runs Enabled *bool `yaml:"enabled,omitempty"` @@ -1131,6 +1171,20 @@ 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 +} + // 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..2b9ec0d7 100644 --- a/configuration/plumberconfig_test.go +++ b/configuration/plumberconfig_test.go @@ -367,6 +367,8 @@ func TestValidControlNames(t *testing.T) { "githubActionMustComeFromAuthorizedSources", "includesMustBeUpToDate", "includesMustNotUseForbiddenVersions", + "mergeRequestApprovalRulesMustCoverAllProtectedBranches", + "mergeRequestApprovalRulesMustRequireMinimumApprovals", "pipelineMustIncludeComponent", "pipelineMustIncludeTemplate", "pipelineMustNotEnableDebugTrace", diff --git a/configuration/registry.go b/configuration/registry.go index 888043f1..e750065c 100644 --- a/configuration/registry.go +++ b/configuration/registry.go @@ -30,21 +30,23 @@ 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}}, + "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..165d4f70 100644 --- a/configuration/v1_to_v2.go +++ b/configuration/v1_to_v2.go @@ -71,6 +71,8 @@ func controlsConfigIsZero(c ControlsConfig) bool { return c.ContainerImageMustNotUseForbiddenTags == nil && c.ContainerImageMustComeFromAuthorizedSources == nil && c.BranchMustBeProtected == nil && + c.MergeRequestApprovalRulesMustRequireMinimumApprovals == nil && + c.MergeRequestApprovalRulesMustCoverAllProtectedBranches == nil && c.PipelineMustNotIncludeHardcodedJobs == nil && c.IncludesMustBeUpToDate == nil && c.IncludesMustNotUseForbiddenVersions == nil && @@ -96,6 +98,8 @@ 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.PipelineMustNotIncludeHardcodedJobs == b.PipelineMustNotIncludeHardcodedJobs && a.IncludesMustBeUpToDate == b.IncludesMustBeUpToDate && a.IncludesMustNotUseForbiddenVersions == b.IncludesMustNotUseForbiddenVersions && diff --git a/control/catalog.go b/control/catalog.go index 67d8c6fe..7cf8b678 100644 --- a/control/catalog.go +++ b/control/catalog.go @@ -52,6 +52,16 @@ 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: "Pipeline must not include hardcoded jobs", ControlName: "pipelineMustNotIncludeHardcodedJobs", @@ -315,6 +325,12 @@ 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.PipelineMustNotIncludeHardcodedJobs; cfg == nil || !cfg.IsEnabled() { out["pipelineMustNotIncludeHardcodedJobs"] = true } diff --git a/control/codes.go b/control/codes.go index d3dabe56..b649bd1e 100644 --- a/control/codes.go +++ b/control/codes.go @@ -178,6 +178,10 @@ 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-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-803: Job runs with overly broad permissions (write-all) @@ -550,6 +554,24 @@ 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", + }, + 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, diff --git a/control/mr_approval_config_contract_test.go b/control/mr_approval_config_contract_test.go new file mode 100644 index 00000000..d61bcc85 --- /dev/null +++ b/control/mr_approval_config_contract_test.go @@ -0,0 +1,75 @@ +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) + } +} diff --git a/control/status.go b/control/status.go index bcf0b01e..ad87bd7e 100644 --- a/control/status.go +++ b/control/status.go @@ -88,6 +88,20 @@ 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 result.CiMissing || !result.CiValid { return StatusError } diff --git a/control/status_test.go b/control/status_test.go index d223e824..036b6e5e 100644 --- a/control/status_test.go +++ b/control/status_test.go @@ -9,6 +9,7 @@ import ( func TestStatusFor(t *testing.T) { content := ControlEntry{ControlName: "actionsMustBePinnedByCommitSha"} branch := ControlEntry{ControlName: "branchMustBeProtected"} + approval := ControlEntry{ControlName: "mergeRequestApprovalRulesMustRequireMinimumApprovals"} healthy := &AnalysisResult{CiValid: true} cases := []struct { @@ -33,6 +34,11 @@ 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}, {"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..1041700a 100644 --- a/control/task.go +++ b/control/task.go @@ -33,6 +33,53 @@ const opaEvaluateTimeout = 2 * time.Minute // the catalog in catalog.go. const controlBranchMustBeProtected = "branchMustBeProtected" const controlMutableRemoteExec = "actionsMustNotExecuteMutableRemoteCode" +const controlMRApprovalRulesMinApprovals = "mergeRequestApprovalRulesMustRequireMinimumApprovals" +const controlMRApprovalRulesCoverAllBranches = "mergeRequestApprovalRulesMustCoverAllProtectedBranches" + +// 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) +} + +// protectionDataNeeded reports whether any control needs the GitLab protection +// collection this run: branchMustBeProtected, or either approval-rule 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) +} // shouldScanMutableExec reports whether the collector should fetch and // scan action source for actionsMustNotExecuteMutableRemoteCode @@ -280,6 +327,14 @@ 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.IncludesMustNotUseForbiddenVersions; c != nil { defaultForbidden := false if c.DefaultBranchIsForbiddenVersion != nil { @@ -626,23 +681,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 +706,10 @@ 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) 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..5d6eaf74 --- /dev/null +++ b/control/task_mr_approval_gate_test.go @@ -0,0 +1,133 @@ +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") + } + }) +} + +// 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") + } +} diff --git a/control/types.go b/control/types.go index 898f7a4d..84e3f7df 100644 --- a/control/types.go +++ b/control/types.go @@ -79,6 +79,15 @@ 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:"-"` + // 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..e177ccac 100644 --- a/defaultConfig/.plumber.yaml +++ b/defaultConfig/.plumber.yaml @@ -244,6 +244,44 @@ 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 + # =========================================== # 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..701ed202 100644 --- a/docs/FINGERPRINT.md +++ b/docs/FINGERPRINT.md @@ -377,6 +377,16 @@ 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). - **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..22ef8f50 100644 --- a/finding/identity/declarations.go +++ b/finding/identity/declarations.go @@ -155,6 +155,10 @@ 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"}, + // 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"}, // Workflow has no explicit name: one finding per workflow file, keyed on the file (benched, not yet live: declaration provisional, revisit on unbench). diff --git a/finding/identity/identity_test.go b/finding/identity/identity_test.go index 76ccdc35..24ce9a54 100644 --- a/finding/identity/identity_test.go +++ b/finding/identity/identity_test.go @@ -359,6 +359,8 @@ func TestDeclarations_EveryCodeFingerprintIsPinned(t *testing.T) { "ISSUE-420": "ac3048041dde8446", "ISSUE-421": "7573d13ae392133e", "ISSUE-501": "e36cebae06c15f85", + "ISSUE-502": "b51fcaa43b9409cf", + "ISSUE-504": "b698c0c9440ef0f5", "ISSUE-505": "4e929715c61fcba6", "ISSUE-601": "9c1ecbe668ad9a36", "ISSUE-701": "87a2f87a752971bd", diff --git a/gitlab/dataCollectionGitlabProtection.go b/gitlab/dataCollectionGitlabProtection.go index d838a366..77153f76 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) 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..7064f7fa 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,7 @@ 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) if origin != nil && origin.MergedConf != nil { if globals := extractGitLabVariables(origin.MergedConf.GlobalVariables); len(globals) > 0 { pipeline.GlobalVariables = globals @@ -61,6 +63,35 @@ 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 +} + // 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..be4f0f77 100644 --- a/gitlab/gitlab_ir_test.go +++ b/gitlab/gitlab_ir_test.go @@ -4,8 +4,48 @@ 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) + } +} + 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..f5e4e2e3 100644 --- a/internal/ir/pipeline.go +++ b/internal/ir/pipeline.go @@ -66,6 +66,23 @@ 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"` + // 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 +417,28 @@ 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"` +} 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/rules_test.go b/policies/rules_test.go index 4dfdcd8b..ec549ea3 100644 --- a/policies/rules_test.go +++ b/policies/rules_test.go @@ -1148,6 +1148,153 @@ func TestIssue501_BranchUnprotected(t *testing.T) { } } +// countCode counts findings carrying the given code. +func countCode(findings []opaengine.Finding, code string) int { + n := 0 + for _, f := range findings { + if f.Code == code { + n++ + } + } + return n +} + +// TestIssue502_MRApprovalRulesMinApprovals flags merge-request approval rules +// that cover all protected branches yet require fewer approvals than the +// configured minimum. Rules scoped to specific branches are out of scope, and +// the rule abstains when the approvals listing was unreadable +// (MRApprovalRulesKnown=false), so a token that cannot read approvals reports +// not-evaluable rather than a false pass. Identity keys on the stable rule ID. +func TestIssue502_MRApprovalRulesMinApprovals(t *testing.T) { + engine := opaengine.New() + if err := engine.LoadFromFSFiltered(policies.FS, nil); err != nil { + t.Fatalf("load embedded policies: %v", err) + } + cfg := map[string]any{ + "mergeRequestApprovalRulesMustRequireMinimumApprovals": map[string]any{ + "minimumRequiredApprovals": 2, + }, + } + + // Positive: two all-branches rules below the minimum (the explicit flag, + // and the zero-scope form), one compliant all-branches rule, and one + // scoped rule below the minimum that is out of scope. + pipeline := &ir.NormalizedPipeline{ + Provider: ir.ProviderGitLab, + MRApprovalRulesKnown: true, + MRApprovalRules: []ir.MRApprovalRule{ + {ID: "10", Name: "All-1", ApprovalsRequired: 1, AppliesToAllProtectedBranches: true}, + {ID: "20", Name: "All-zero-scope", ApprovalsRequired: 0, ProtectedBranchCount: 0}, + {ID: "30", Name: "Compliant", ApprovalsRequired: 3, AppliesToAllProtectedBranches: true}, + {ID: "40", Name: "Scoped", ApprovalsRequired: 0, ProtectedBranchCount: 2}, + }, + } + findings, err := engine.Evaluate(context.Background(), pipeline, cfg) + if err != nil { + t.Fatalf("evaluate: %v", err) + } + if got := countCode(findings, "ISSUE-502"); got != 2 { + t.Fatalf("expected 2 ISSUE-502 findings, got %d", got) + } + assertSubjectKey(t, findings, "ISSUE-502", "approvalRuleId", []string{"10", "20"}) + + // Negative: every all-branches rule meets the minimum -> no findings. + clean := &ir.NormalizedPipeline{ + Provider: ir.ProviderGitLab, + MRApprovalRulesKnown: true, + MRApprovalRules: []ir.MRApprovalRule{ + {ID: "10", Name: "All", ApprovalsRequired: 2, AppliesToAllProtectedBranches: true}, + }, + } + if f, _ := engine.Evaluate(context.Background(), clean, cfg); countCode(f, "ISSUE-502") != 0 { + t.Fatalf("expected 0 ISSUE-502 findings when every covering rule meets the minimum") + } + + // Abstain: listing unreadable (Known=false) -> not-evaluable, so no + // findings even though a covering rule is below the minimum. + unknown := &ir.NormalizedPipeline{ + Provider: ir.ProviderGitLab, + MRApprovalRulesKnown: false, + MRApprovalRules: []ir.MRApprovalRule{ + {ID: "10", Name: "All", ApprovalsRequired: 0, AppliesToAllProtectedBranches: true}, + }, + } + if f, _ := engine.Evaluate(context.Background(), unknown, cfg); countCode(f, "ISSUE-502") != 0 { + t.Fatalf("expected 0 ISSUE-502 findings when the approvals listing is unreadable") + } +} + +// TestIssue504_MRApprovalRulesCoverAllBranches flags a project where no +// approval rule applies to all protected branches (a singleton finding). It +// fires on zero rules too (no coverage at all), matching the legacy control, +// and abstains when the listing was unreadable. +func TestIssue504_MRApprovalRulesCoverAllBranches(t *testing.T) { + engine := opaengine.New() + if err := engine.LoadFromFSFiltered(policies.FS, nil); err != nil { + t.Fatalf("load embedded policies: %v", err) + } + + // Positive: rules exist but none covers all protected branches. + pipeline := &ir.NormalizedPipeline{ + Provider: ir.ProviderGitLab, + MRApprovalRulesKnown: true, + MRApprovalRules: []ir.MRApprovalRule{ + {ID: "10", Name: "Scoped-a", ApprovalsRequired: 2, ProtectedBranchCount: 1}, + {ID: "20", Name: "Scoped-b", ApprovalsRequired: 2, ProtectedBranchCount: 3}, + }, + } + findings, err := engine.Evaluate(context.Background(), pipeline, nil) + if err != nil { + t.Fatalf("evaluate: %v", err) + } + if got := countCode(findings, "ISSUE-504"); got != 1 { + t.Fatalf("expected 1 ISSUE-504 finding, got %d", got) + } + + // Positive: zero rules is also a finding — no rule covers all protected + // branches, so the gate is absent. + empty := &ir.NormalizedPipeline{Provider: ir.ProviderGitLab, MRApprovalRulesKnown: true} + if f, _ := engine.Evaluate(context.Background(), empty, nil); countCode(f, "ISSUE-504") != 1 { + t.Fatalf("expected 1 ISSUE-504 finding when no approval rule is defined") + } + + // Negative: at least one rule covers all protected branches -> no finding. + covered := &ir.NormalizedPipeline{ + Provider: ir.ProviderGitLab, + MRApprovalRulesKnown: true, + MRApprovalRules: []ir.MRApprovalRule{ + {ID: "10", Name: "All", ApprovalsRequired: 2, AppliesToAllProtectedBranches: true}, + {ID: "20", Name: "Scoped", ApprovalsRequired: 2, ProtectedBranchCount: 1}, + }, + } + if f, _ := engine.Evaluate(context.Background(), covered, nil); countCode(f, "ISSUE-504") != 0 { + t.Fatalf("expected 0 ISSUE-504 findings when a rule covers all protected branches") + } + + // Positive: an "All branches" rule (no branch scope — flag off, zero + // protected-branch count) is deliberately NOT counted as covering all + // protected branches. 504 targets the explicit "all protected branches" + // flag only, matching the legacy platform; blanket all-branches coverage is + // a separate concern. A project whose only rule is "All branches" still + // fires 504. (Contrast ISSUE-502, which does accept a zero-scope rule.) + allBranchesOnly := &ir.NormalizedPipeline{ + Provider: ir.ProviderGitLab, + MRApprovalRulesKnown: true, + MRApprovalRules: []ir.MRApprovalRule{ + {ID: "30", Name: "All branches", ApprovalsRequired: 2, AppliesToAllProtectedBranches: false, ProtectedBranchCount: 0}, + }, + } + if f, _ := engine.Evaluate(context.Background(), allBranchesOnly, nil); countCode(f, "ISSUE-504") != 1 { + t.Fatalf("expected 1 ISSUE-504 finding: an \"All branches\" rule is not the explicit all-protected-branches flag (legacy-faithful)") + } + + // Abstain: listing unreadable -> not-evaluable. + unknown := &ir.NormalizedPipeline{Provider: ir.ProviderGitLab, MRApprovalRulesKnown: false} + if f, _ := engine.Evaluate(context.Background(), unknown, nil); countCode(f, "ISSUE-504") != 0 { + t.Fatalf("expected 0 ISSUE-504 findings when the approvals listing is unreadable") + } +} + // TestIssue505_BranchNonCompliant flags protected branches whose // settings fail to meet the declared minimum bar. func TestIssue505_BranchNonCompliant(t *testing.T) { From e590d3ee5f3142d9ee907f4d61ae944f1f3248d7 Mon Sep 17 00:00:00 2001 From: Joseph Moukarzel Date: Fri, 21 Aug 2026 17:49:16 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(controls):=20finish=20MR-approval-rules?= =?UTF-8?q?=20review=20nits=20=E2=80=94=20unreadable-listing=20caveat,=20k?= =?UTF-8?q?ey-rename=20doc,=20403/404=20comment=20wording?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- cmd/render_details.go | 23 +++++++++++++++++++++++ configuration/plumberconfig.go | 6 ++++++ control/status.go | 6 +++--- defaultConfig/.plumber.yaml | 4 ++-- 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/cmd/render_details.go b/cmd/render_details.go index 48f362a9..8052c0c9 100644 --- a/cmd/render_details.go +++ b/cmd/render_details.go @@ -206,6 +206,23 @@ func caveatStatLines(g findingGroup) []statLine { return out } +// approvalRulesUnreadableCaveat returns a single ⚠ caveat stat line when the +// approval-rules listing could not be read authoritatively (nil ProtectionData, +// or MRApprovalRulesKnown false from a 401/403). Both approval-rule stat +// builders use it so an unreadable listing shows the caveat instead of a bare +// green "passed" — the JSON status is already error via StatusFor, this closes +// the terminal gap. Returns nil when the listing was read (the zero-rules-on- +// Free case is handled separately by renderApprovalRulesTierCaveat). +func approvalRulesUnreadableCaveat(result *control.AnalysisResult) []statLine { + if result != nil && result.ProtectionData != nil && result.ProtectionData.MRApprovalRulesKnown { + return nil + } + return []statLine{{ + Label: statCaveatPrefix + " Approval rules not evaluated", + Value: "the approvals API could not be read — token lacks permission, or it requires GitLab Premium/Ultimate", + }} +} + // renderSkippedControlsSummary prints the "Skipped Controls" section: a // top-level section header followed by each skipped control with its // skip reason. @@ -867,10 +884,16 @@ func buildGitLabControlStats(controlName string, result *control.AnalysisResult, {Label: "Mutable Remote Exec Found", Value: fmt.Sprintf("%d", findingsCount)}, } case "mergeRequestApprovalRulesMustRequireMinimumApprovals": + if lines := approvalRulesUnreadableCaveat(result); lines != nil { + return lines + } return []statLine{ {Label: "Rules Below Minimum", Value: fmt.Sprintf("%d", findingsCount)}, } case "mergeRequestApprovalRulesMustCoverAllProtectedBranches": + if lines := approvalRulesUnreadableCaveat(result); lines != nil { + return lines + } return []statLine{ {Label: "All-Branches Rule Missing", Value: fmt.Sprintf("%d", findingsCount)}, } diff --git a/configuration/plumberconfig.go b/configuration/plumberconfig.go index 4eec52a0..28b530df 100644 --- a/configuration/plumberconfig.go +++ b/configuration/plumberconfig.go @@ -630,6 +630,12 @@ type MRApprovalRulesMinApprovalsControlConfig struct { // 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). + // + // Platform migration: this key was named minimumRequiredApprovalAllProtectedBranches + // on the backend2 platform. It is deliberately shortened to + // minimumRequiredApprovals in the CLI (the "all protected branches" scope is + // already implied by the control). A platform-config importer must map the + // old key to this one. MinimumRequiredApprovals *int `yaml:"minimumRequiredApprovals,omitempty"` } diff --git a/control/status.go b/control/status.go index ad87bd7e..fba54e62 100644 --- a/control/status.go +++ b/control/status.go @@ -94,9 +94,9 @@ func StatusFor(e ControlEntry, result *AnalysisResult, findingCount int) string // 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. + // never ran, or a genuine 401 aborted it) or MRApprovalRulesKnown=false + // (a 403/404 the collector tolerated) 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 } diff --git a/defaultConfig/.plumber.yaml b/defaultConfig/.plumber.yaml index e177ccac..0a6bedd8 100644 --- a/defaultConfig/.plumber.yaml +++ b/defaultConfig/.plumber.yaml @@ -255,7 +255,7 @@ gitlab: # 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 are available. A genuine 403/404 (a token that cannot read # approval rules) reports not-evaluable, not a false pass. Ships disabled: # enable it and set your minimum. mergeRequestApprovalRulesMustRequireMinimumApprovals: @@ -276,7 +276,7 @@ gitlab: # 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 + # where approval rules are available. A genuine 403/404 (a token without # scope) reports not-evaluable. Ships disabled. mergeRequestApprovalRulesMustCoverAllProtectedBranches: # Set to true to enable this control