Skip to content

Feat/add gitlab variables controls - #422

Open
Joseph94m wants to merge 2 commits into
mainfrom
feat/add-gitlab-variables-controls
Open

Feat/add gitlab variables controls#422
Joseph94m wants to merge 2 commits into
mainfrom
feat/add-gitlab-variables-controls

Conversation

@Joseph94m

@Joseph94m Joseph94m commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

@Joseph94m
Joseph94m force-pushed the feat/add-gitlab-variables-controls branch from a7c469b to 8ff046b Compare August 20, 2026 07:17
@Joseph94m
Joseph94m marked this pull request as ready for review August 20, 2026 07:52
Comment thread gitlab/dataCollectionGitlabVariables.go
Comment thread control/task.go
Comment thread gitlab/dataCollectionGitlabVariables.go Outdated
Comment thread cmd/init.go
Comment thread policies/cicd_variables_must_be_masked.rego
Comment thread gitlab/dataCollectionGitlabVariables.go
@Joseph94m
Joseph94m force-pushed the feat/add-gitlab-variables-controls branch from 8ff046b to 4c9e23c Compare August 20, 2026 08:17
Comment thread gitlab/dataCollectionGitlabVariables.go Outdated
Comment thread control/task.go Outdated
Comment thread gitlab/dataCollectionGitlabPipelineImage.go
Comment thread gitlab/request.go
Comment thread gitlab/dataCollectionGitlabVariables.go
@Joseph94m
Joseph94m force-pushed the feat/add-gitlab-variables-controls branch from 4c9e23c to 3aac977 Compare August 20, 2026 08:42
Comment thread cmd/legacy_json.go
Comment thread policies/cicd_variables_must_be_protected.rego
@Joseph94m
Joseph94m force-pushed the feat/add-gitlab-variables-controls branch from 3aac977 to bfdd4eb Compare August 20, 2026 09:04
@thomasboni

thomasboni commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@Joseph94m Reviewed + tested e2e by Claude, then I verified its results:

What works

  • Exact expected findings on a 6-variable matrix (unprotected, unmasked, short-value, file-type, env-scoped, hidden). File-type exclusion from the masked rule and the short-unmaskable-value decision are both implemented and documented.
  • GraphQL cursor pagination: a variable on page 3 is flagged.
  • No value leak: sentinel values planted in the API responses appear nowhere in JSON, SARIF, stdout or stderr. ir.SettingsVariable has no Value field, VariablesData is json:"-".
  • 401/403 and project: null correctly yield status: error in JSON, never a false pass. The pointer-struct change killing the old "null decodes as empty list" silent pass is right.
  • Identity fields exactly variableName/variableType/environment, fingerprints pinned. Default-disabled but visible as "disabled in configuration". Gate exits 1 on findings.

Blockers

  1. False "pass" in the terminal report when the listing is unreadable. On a 401/403/null-project run the JSON says error, but the human report lists both controls under "Passed Controls" as a bare green check, with score 100/100 and Status: PASSED. That is exactly the success-on-incomplete-data footgun the renderPassedControlsSummary comment warns about, and branchMustBeProtected already solves it with a warning caveat stat line. The new stat builders in cmd/render_details.go (buildGitLabControlStats, the two new cases) should emit that caveat when result.VariablesData == nil || !result.VariablesData.Known.
  2. MR comment shows the control failed with zero detail lines. control/mrcomment.go writeIssueDetails iterates a hand-written order list of control names; neither cicdVariablesMustBeProtected nor cicdVariablesMustBeMasked was added, so ISSUE-201/202 findings are silently dropped from the MR comment body.
  3. A transient network failure on the variables fetch degrades the run (exit 3, correct) but flips every unrelated CI-file control to error (reproduced e2e: image collection succeeded, then a connection reset on the variables call turned dockerInDockerResult, imageAuthorizedSourcesResult, etc. into error). Branch protection has a degraded-reason carve-out for exactly this (degradedReasonBranchProtectionPrefix in control/degraded.go); the variables reason needs the same classifier.

Nits (should be done too)

  • Root .plumber.yaml: both new blocks say "Ships disabled" directly above enabled: true (the root file enabling them is fine, the copied narrative is not).
  • Missing # === separator line above the "CI/CD variables must be masked" title in both .plumber.yaml files.
  • control/task.go: "Checking CI/CD variables" reuses progress step 9, same as branch protection.
  • Stats show only the findings count; "Variables Checked: N" would give a denominator like the comparable controls.
  • The GraphQL query still fetches value on the settings-control path (needed only by image resolution). Never persisted (verified), but blanking Value in CollectGitlabVariables or using a value-free query would harden it.
  • Test gaps: no multi-page cursor test, and no task-level test pinning that a 401 does not mark the run degraded.

@Joseph94m
Joseph94m force-pushed the feat/add-gitlab-variables-controls branch from bfdd4eb to 334d820 Compare August 21, 2026 14:40
Comment thread control/task.go
Comment thread cmd/render_details.go
Comment thread gitlab/dataCollectionGitlabPipelineImage.go
Comment thread gitlab/dataCollectionGitlabVariables.go
@Joseph94m

Copy link
Copy Markdown
Collaborator Author

@thomasboni should be better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate platform control: CI/CD variables must be protected (ISSUE-201) Migrate platform control: CI/CD variables must be masked (ISSUE-202)

2 participants