Skip to content

Feat/add gitlab mr approval controls - #423

Open
Joseph94m wants to merge 3 commits into
mainfrom
feat/add-gitlab-mr-approval-controls
Open

Feat/add gitlab mr approval controls#423
Joseph94m wants to merge 3 commits into
mainfrom
feat/add-gitlab-mr-approval-controls

Conversation

@Joseph94m

@Joseph94m Joseph94m commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

@Joseph94m
Joseph94m force-pushed the feat/add-gitlab-mr-approval-controls branch from d96ac25 to f33e099 Compare August 20, 2026 07:18
@Joseph94m
Joseph94m marked this pull request as ready for review August 20, 2026 07:52
Comment thread control/task.go
Comment thread control/task.go
Comment thread policies/mr_approval_rules_min_approvals.rego
Comment thread policies/mr_approval_rules_min_approvals.rego
@Joseph94m
Joseph94m force-pushed the feat/add-gitlab-mr-approval-controls branch from f33e099 to f85cb93 Compare August 20, 2026 10:56
Comment thread gitlab/dataCollectionGitlabProtection.go
Comment thread cmd/init.go
Comment thread policies/mr_approval_rules_min_approvals.rego
@Joseph94m
Joseph94m force-pushed the feat/add-gitlab-mr-approval-controls branch 2 times, most recently from 90ac8ab to 0688320 Compare August 20, 2026 11:32
Comment thread gitlab/rest.go
Comment thread control/task.go
@Joseph94m
Joseph94m force-pushed the feat/add-gitlab-mr-approval-controls branch from 0688320 to 8b7a80a Compare August 20, 2026 11:46
Comment thread gitlab/dataCollectionGitlabProtection.go
Comment thread policies/mr_approval_rules_cover_all_branches.rego
@Joseph94m
Joseph94m force-pushed the feat/add-gitlab-mr-approval-controls branch from 8b7a80a to 02c5edf Compare August 20, 2026 12:01
@thomasboni

thomasboni commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@Joseph94m Reviewed + tested e2e. Also checked legacy fidelity line-by-line against the backend2 controls. All of this by Claude. I've verified its results.

Blocker

  1. An approval rule with an empty name is invisible to ISSUE-502. MRApprovalRule.Name is serialized with json:"name,omitempty" (internal/ir/pipeline.go), and mr_approval_rules_min_approvals.rego references rule.name bare in the message sprintf and the ruleName data field. When the key is absent the whole deny body is undefined, so no finding is emitted. Reproduced end to end: a rule {id: 99, name: "", rule_type: "any_approver", approvals_required: 0, applies_to_all_protected_branches: true} with minimumRequiredApprovals: 2 yields status: passed, zero issues. The same rule with a name is flagged. GitLab API-created and auto-created rules can carry empty names, so this is a realistic silent false negative on exactly the violation class the control exists for. Fix: object.get(rule, "name", "") in the rego (and/or drop the omitempty), plus the unnamed-rule case in policies/rules_test.go. Identity keys on approvalRuleId, so the fix re-keys nothing.
  2. gitlab/dataCollectionGitlabProtection.go classifies 403/404 by substring on the error string, and this PR makes that drive MRApprovalRulesKnown and a user-visible status. The error string embeds the request URL, so a project whose ID contains "403"/"404" returning a 500 gets misclassified as premium-missing (safe direction, Known=false, but a hard failure is silently downgraded). Prefer the typed response status code.
  3. The config key renames the platform's minimumRequiredApprovalAllProtectedBranches to minimumRequiredApprovals and the mapping is recorded nowhere (FINGERPRINT.md covers the identity correction only). Write it down for the platform migration.
  4. Same terminal-rendering gap as Feat/add gitlab variables controls #422: on a 403 the JSON says error but the report shows both controls as a bare green check under "Passed Controls". Needs the caveat stat line (one shared fix for both PRs).
  5. There is still one not-resolved Claude code comment review in the PR

Nits

  • enabled: true without minimumRequiredApprovals is a silent no-op (rego defaults the minimum to 0). Matches legacy and is documented, but a config-load warning would save users from a control that can never fire.
  • Collector/IR comments say non-premium yields 403/404 while the rego/defaultConfig comments say Free returns 200-empty; both paths are handled, harmonize the wording.
  • Docs say "401/403"; a genuine 401 actually aborts the whole protection collection (outcome still safe via nil ProtectionData).
  • 404 branch of the tolerance is untested (403 is covered, same path, cheap to add).
  • Init wizard: MRApprovalMinCount is unvalidated free text; "0" produces an enabled no-op, non-numeric silently falls back.

@getplumber getplumber deleted a comment from github-actions Bot Aug 21, 2026
@Joseph94m
Joseph94m force-pushed the feat/add-gitlab-mr-approval-controls branch from ab36c42 to f15f4b2 Compare August 21, 2026 15:34
@Joseph94m

Copy link
Copy Markdown
Collaborator Author

@thomasboni should be good !

Comment thread gitlab/dataCollectionGitlabProtection.go
…ing caveat, key-rename doc, 403/404 comment wording
Comment thread cmd/render_details.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants