Repository navigation
fix(#7769): pin GitLab job identity to CI_JOB_TOKEN record - #7817
Conversation
|
🤖 Finished Review · ✅ Success · Started 1:31 AM UTC · Completed 1:55 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $8.68 |
Site previewPreview: https://6206c5ef-site.fullsend-ai.workers.dev Commit: |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Risk Assessment: moderate (2/5) DetailsModerate risk: large security-focused gate-hardening change with elevated churn and regression history is balanced by bot authorship, healthy test coverage, no protected-path edits, opt-in report-only enforcement, and a narrow incremental delta since the prior review. Previous runRisk Assessment: moderate (2/5) DetailsModerate risk: large, security-focused gate-hardening change with elevated churn/regression history is balanced by bot authorship, healthy test coverage, no protected-path edits, and an opt-in (report-only-by-default) enforcement flag; this round's commit is a narrow, well-tested incremental fix (5 files) rather than a fresh large-scope change. Previous run (2)Risk Assessment: moderate (2/5) DetailsModerate risk: large change size and security-focused gate hardening with recent churn and regression history are balanced by bot authorship, healthy test coverage, absence of protected path edits, an opt-in enforcement flag, and zero unresolved issue discussions; score held stable at 2 via re-review anchoring against the prior round. Previous run (3)Risk Assessment: moderate (2/5) DetailsModerate risk: large change size and security-focused gate hardening with recent churn and regression history are balanced by bot authorship, healthy test coverage, absence of protected path edits, an opt-in enforcement flag, and zero unresolved issue discussions; score held stable at 2 via re-review anchoring against the prior round. Previous run (4)Risk Assessment: moderate (2/5) DetailsModerate risk: large change size and security-focused gate hardening with recent churn and regression history are balanced by bot authorship, healthy test coverage, absence of protected path edits, and zero unresolved issue discussions. Previous run (5)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate): re-review anchoring holds across unchanged Tier 1 signals with zero protected path or dependency changes, bot authorship, solid test coverage (0.38), and opt-in rollout safety balancing elevated churn in scaffold scripts. Previous run (6)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate): re-review anchoring holds across the 34-file diff where zero protected path or dependency changes, a strong test ratio (0.38), bot authorship, and opt-in rollout safety keep the composite at moderate despite elevated churn in target scaffold scripts. Previous run (7)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate): anchoring holds across the cumulative 34-file/+3111-231 diff despite a history rewrite precluding clean incremental compare enumeration — zero protected paths or dependency changes, a solid test ratio (0.38), bot authorship, and continued opt-in gating of the newly-added enforcement control keep the composite at moderate. Previous run (8)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) because cumulative diff characteristics remain stable across review rounds and this round's incremental delta is a narrow, rollback-safe UX refinement to report-only status handling under the default-disabled FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION flag, backed by solid test coverage and leaving core identity-pinning logic unchanged. Previous run (9)Risk Assessment: moderate (2/5) DetailsScore reduced from the prior 3 (elevated) to 2 (moderate) because this round's only new commit adds the ci_pipeline_variables_minimum_override_role control behind a new, default-disabled FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION opt-in flag, improving rollback safety for the incremental change; the core identity-pin mechanism reviewed across prior rounds is unchanged. Previous run (10)Risk Assessment: elevated (3/5) DetailsSecurity-critical CI job script identity hardening with large blast radius modifies existing behavior without a feature flag, anchored to prior elevated rating following doc-only prose updates with elevated churn and regression history balanced by bot authorship and substantial test coverage. Previous run (11)Risk Assessment: elevated (3/5) DetailsSecurity-critical CI job script identity hardening with a large blast radius modifies existing behavior without a feature flag, anchored to the prior elevated rating with elevated churn, regression history, and coupling balanced by bot authorship and strong test coverage. Previous run (12)Risk Assessment: elevated (3/5) DetailsSecurity-critical CI job script identity hardening with a large blast radius modifies existing behavior without a feature flag, anchored to the prior elevated rating with elevated churn, regression history, and coupling balanced by bot authorship and strong test coverage. Previous run (13)Risk Assessment: elevated (3/5) DetailsSecurity-critical CI job script identity hardening with a large blast radius modifies existing behavior without a feature flag, anchored to the prior elevated rating with elevated churn, regression history, and coupling balanced by bot authorship and strong test coverage. Previous run (14)Risk Assessment: elevated (3/5) DetailsSecurity-critical CI job script identity hardening with a large blast radius modifies existing behavior without a feature flag, anchored to the prior elevated rating with elevated churn, regression history, and coupling balanced by bot authorship and strong test coverage. Previous run (15)Risk Assessment: elevated (3/5) DetailsSecurity-critical CI job script identity hardening with a large blast radius modifies existing behavior without a feature flag, anchored to the prior elevated rating and balanced by bot authorship, strong test ratio, and issue alignment. Previous run (16)Risk Assessment: elevated (3/5) DetailsSecurity-critical CI hardening modifying existing GitLab CI job scripts without a feature flag, anchored to the prior elevated rating and balanced by bot authorship, strong test coverage, and clear issue alignment. Previous run (17)Risk Assessment: elevated (3/5) DetailsSecurity-critical CI hardening modifying existing GitLab CI job scripts without a feature flag, balanced by strong test coverage, a bot author, and clear issue acceptance criteria. |
ReviewFindingsLow
Prior findings from the previous review round (SHA
A candidate finding about the pinned-ref trusted-config Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Next steps:
Previous run (4)ReviewFindingsMedium
Low
Next steps:
Previous run (5)ReviewFindingsLow
Previous run (6)ReviewFindingsLow
Next steps:
Previous run (7)ReviewFindingsLow
Next steps:
Previous run (8)ReviewFindingsMedium
Low
Next steps:
Previous run (9)ReviewFindingsLow
Next steps:
Previous run (10)ReviewFindingsLow
The two prior-round doc findings — incomplete converge-migration documentation in Previous run (11)ReviewFindingsLow
The medium/low security findings from the prior review round (curl brace-globbing/CURLRC exposure, secrets on curl argv, Next steps:
Previous run (12)ReviewFindingsMedium
Low
Next steps:
|
|
🤖 Finished Fix · ✅ Success · Started 1:57 AM UTC · Completed 2:18 AM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.63 |
…overage Address review feedback on PR #7817: - Route every PAT-bearing curl call in run-agent-job.sh/run-poll-job.sh through fullsend_gate_curl instead of plain curl, so a trigger-supplied HTTP_PROXY/HTTPS_PROXY cannot intercept the role PAT on those calls (only the identity-pin's own JOB-TOKEN calls were previously routed through the proxy-stripping, CA-pinned helper). - Unset fullsend_gate_curl's curl-args temporary on every return, matching the sourced script's convention for its other temporaries. - Document that fullsend_gate_curl is transport-only and reused by callers for PAT-bearing calls after the pin succeeds. - Add pin-ci-job-identity.sh tests asserting fail-closed behavior when the project/branch/pipeline GET routes return a non-200 status. - Add a run-agent-job.sh test for the user-less job-token pipeline payload path, asserting the PAT re-fetch uses the pinned project/ pipeline IDs rather than the overridable CI_PROJECT_ID/CI_PIPELINE_ID. - Move the Run*JobScript_* integration tests into gitlab_job_scripts_test.go to consolidate with the rest of that script-level coverage. - Note the new CI_DEBUG_TRACE deny-before-admit workflow rule in the GitLab manual-uninstall and install-behavior docs, which omitted it. Addresses #7817
🔧 Fix agent — iteration 1 (bot-triggered)Routed every remaining PAT-bearing curl call in the GitLab poll/agent job scripts through the proxy-stripping, CA-pinned fullsend_gate_curl helper; fixed the curl-args temp-var leak in that helper; added fail-closed test coverage for non-200 project/branch/pipeline GET responses and for the user-less pipeline PAT re-fetch path; consolidated the new integration tests into the existing test file; and updated the GitLab uninstall/install docs to mention the new CI_DEBUG_TRACE deny rule. Declined to speculatively rework the identity-pin trust model or broaden the CI_DEBUG_TRACE match without live GitLab/gitlab-runner verification, and declined to edit the PR's closing-keyword wording as out of scope for a branch-code fix. Fixed (7):
Disagreed (3):
Tests: passed CI inspections (14):
Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 2:20 AM UTC · Completed 2:41 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $8.28 |
|
🤖 Finished Fix · ✅ Success · Started 2:43 AM UTC · Completed 3:08 AM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $9.09 |
… PAT calls to job-record project, backfill debug-trace rule on converge Every deny/admit layer for the debug-trace guard matched only the exact string "true", missing the truthy variants gitlab-runner also accepts (Go strconv.ParseBool: "1", "t"/"T", "true"/"TRUE"/"True"). Switch every YAML rule and script-level guard this PR added/touched (.gitlab-ci.yml, fullsend-pipeline.yml, fullsend-agent.yml, fullsend-poll.yml, gitlabci.go's fullsendWorkflowRules/ unmergeWorkflowRules/newGitLabCI, run-agent-job.sh, run-poll-job.sh) to a case-insensitive regex match, and add coverage for non-"true" truthy values. convergeGitLabRootCIFiles only stripped obsolete workflow rules; it never backfilled the CI_DEBUG_TRACE deny rule onto already-enrolled repos that go through converge (not the fresh-install merge path). Add MergeMissingGitLabDebugTraceRule, gated on the same fullsend- ownership check as the obsolete-rule strip, and wire it into convergeGitLabRootCIFiles so enrolled repos pick up the guard. Several PAT-bearing calls in run-agent-job.sh/run-poll-job.sh (resource-group self-heal, Developer-access members lookup, prior-review notes GETs, MR GET, commits GET) still interpolated the overridable CI_PROJECT_ID/CI_MERGE_REQUEST_PROJECT_ID pipeline variables instead of the project id pinned from the CI_JOB_TOKEN job record. Route every one through FULLSEND_PINNED_PROJECT_ID, and fail closed when CI_MERGE_REQUEST_PROJECT_ID disagrees with the pinned project. Addresses #7817
🔧 Fix agent — iteration 2 (bot-triggered)Addressed the review's four findings on PR #7817 (GitLab in-job gate hardening / ADR 0125). Fixed the fail-open CI_DEBUG_TRACE exact-match gap across every YAML rule and shell guard this PR touches by switching to a case-insensitive regex matching gitlab-runner's full truthy set, with new tests for non-"true" truthy values. Fixed the logic-error gap where already-enrolled repos never received the debug-trace rule during converge by adding MergeMissingGitLabDebugTraceRule and wiring it into convergeGitLabRootCIFiles, with updated and new converge tests. Fixed the secret-exposure gap where several PAT-bearing calls used the overridable CI_PROJECT_ID/CI_MERGE_REQUEST_PROJECT_ID instead of the pinned project id, routing them through FULLSEND_PINNED_PROJECT_ID with a fail-closed cross-check and a new full-script regression test. Recorded a disagreement on the low-severity intent-alignment finding (PR closing-keyword mismatch) since it requires a PR-description edit outside this agent's mandate rather than a code change. All new and existing tests pass (go test -race for internal/repos and internal/scaffold), go vet and gofmt are clean, patch coverage on the two modified production Go files (gitlabci.go, converge.go) is at or above the 80% target, and a secret scan found no leaks. Fixed (3):
Disagreed (1):
Tests: passed CI inspections (12):
Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 3:10 AM UTC · Completed 3:33 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $10.71 |
|
🤖 Finished Fix · ✅ Success · Started 3:35 AM UTC · Completed 3:58 AM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $4.93 |
🔧 Fix agent — iteration 3 (bot-triggered)Fixed the medium secret-exposure finding by pinning the fullsend poll/run CLI's --project, --gitlab-url, and --status-repo arguments to the CI_JOB_TOKEN job-record identity instead of overridable CI_PROJECT_PATH/FULLSEND_GITLAB_URL/CI_SERVER_URL pipeline variables, with regression tests proving the pinned identity wins even when those variables point elsewhere. Recorded a disagreement on the low intent-alignment finding: it's valid but its remediation (completing live GitLab verification, or editing the PR's closing keyword) is outside this agent's scope. Also inspected project CI: fixed the codecov/patch failure by adding tests for previously-uncovered branches in MergeMissingGitLabDebugTraceRule and the converge dry-run backfill path; the commit-lint failure (an earlier commit's title exceeds gitlint's 100-char limit) needs a human-requested squash/reword since bot-triggered runs cannot rewrite existing commits. Fixed (1):
Disagreed (1):
Tests: passed CI inspections (10):
Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 1:28 AM UTC · Completed 1:53 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $9.81 |
|
🤖 Finished Fix · ✅ Success · Started 1:55 AM UTC · Completed 2:14 AM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.49 |
🔧 Fix agent — iteration 2 (bot-triggered)Fixed 6 of 9 review findings: allowed a bracketed IPv6-literal CI_API_V4_URL through the identity pin's anti-glob check (with a new real-IPv6-loopback test), stripped SSL_CERT_DIR/SSLKEYLOGFILE alongside the existing CA env vars, routed all post-pin PAT-bearing curls in run-agent-job.sh/run-poll-job.sh through the pinned API root instead of the overridable CI_API_V4_URL, and made three documentation fixes (naming FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION, a comma/semicolon fix, and a CI_DEBUG_TRACE whole-pipeline-skip note). Disagreed with 3 findings: the branches-endpoint JOB-TOKEN-support concern (requires live GitLab verification unavailable in this sandbox, and the reviewer's own confidence is explicitly low), the PR-description 'Closes #7769' wording (forge metadata edit outside fix-agent scope, per the finding's own remediation), and the default-branch-drift-detection enhancement (reviewer explicitly marked 'Not blocking', a feature request not a defect). Full internal/scaffold and internal/repos test suites pass; go vet and gofmt clean; shellcheck was not available in this sandbox (network-restricted, matching the PR's own note), so bash -n syntax checks were used instead. Inspected project CI: the 'behaviour' E2E job fails on 15/33 scenarios unrelated to this PR's GitLab-only diff (PR review/issue dispatch, hooks, pi runtime) and the same job also failed on other unrelated recent PR merges to main, indicating a pre-existing/infra issue rather than something this PR's changes caused. Fixed (6):
Disagreed (3):
Tests: passed CI inspections (11):
Decision points
Next steps:
|
|
🤖 Review · ❌ Terminated · Started 2:16 AM UTC · Ended 2:39 AM UTC Commit: |
|
/fs-code Rebase this PR onto the current main branch and squash the branch into one commit before making further changes. Mold PR #7817 to match the revised least-privilege plan:
Do not modify #7767 or #7768, and do not make this PR depend on #7850/#7851 for the core in-job hardening to pass. Do not overwrite or regress the current PR description; preserve its Developer-level credential scope, #7850/#7851 references, and Closes #7769. |
|
🤖 Finished Review · ✅ Success · Started 2:16 AM UTC · Completed 2:39 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.98 |
|
🤖 Finished Fix · ✅ Success · Started 2:41 AM UTC · Completed 2:48 AM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.25 |
🔧 Fix agent — iteration 3 (bot-triggered)No source changes made this round. Both current findings are process/verification blockers rather than confirmed code defects: the medium finding needs live verification against a real GitLab instance that this sandbox cannot perform, and unilaterally applying any of its code-level remediations would either remove a tested security guarantee or rest on an equally unverified guess; the low finding needs a human edit to the PR description, which is outside this agent's scope (PR mutations belong to the deterministic post-script). Recorded reasoned disagreements for both, consistent with prior rounds. Project CI: all jobs pass except E2E Tests/behaviour, which fails across ~15 scenarios spanning unrelated GitHub-dispatch/harness features this PR's GitLab-only diff never touches; a 3-day scan of E2E Tests runs on main shows frequent failures on unrelated merges, so this is classified as pre-existing flakiness, not caused by this PR. Disagreed (2):
Tests: passed CI inspections (12):
Decision points
Next steps:
|
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
Pin poll and agent jobs to the server-side CI job identity, enforce disjoint source allowlists, and deny CI debug tracing before admission. Converge GitLab pipeline-variable override restrictions and route gate requests through the pinned API URL with trusted CA handling. Addresses #7817 Relates to #7769 Signed-off-by: Greg Allen <gallen@redhat.com> Co-authored-by: Codex <codex@openai.com>
|
🤖 Finished Review · ✅ Success · Started 9:30 PM UTC · Completed 9:56 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $8.90 |
|
🤖 Finished Fix · ✅ Success · Started 9:58 PM UTC · Completed 10:13 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.51 |
Address review findings on PR #7817: - run-agent-job.sh: unset CI_MERGE_REQUEST_IID in the issue-event EVENT_TYPE arm. This job's admit source is api-only, so GitLab never natively populates that variable; newGitLabClientFromEnv routes status-comment API calls to merge_requests/{iid} whenever it is merely non-empty, regardless of FULLSEND_NOTE_TARGET, which could misdirect an issue status comment onto an unrelated merge request. Added a regression test. - pin-ci-job-identity.sh: unset HTTP_PROXY/HTTPS_PROXY/http_proxy/ https_proxy in the parent shell once the pin succeeds, so a trigger-influenced proxy setting left over from job init cannot steer the subsequent fullsend poll/run Go processes' API traffic (fullsend_gate_curl only stripped these for its own curl calls). Added a regression test. - pin-ci-job-identity.sh: documented why the pinned-branch lookup (GET /projects/:id/repository/branches/:branch) is a same-project CI_JOB_TOKEN call rather than a cross-project one, and that GitLab's fine-grained job-token permissions docs separately list this route under read_repository -- addressing the long-recurring "confirm this against GitLab's allowlist" ask; verification against the oldest self-hosted GitLab version fullsend supports remains open. - docs/guides/getting-started/configuring-gitlab.md: document the in-job identity-pin's fail-closed constraints (protected default branch only, disjoint schedule/api pipeline-source allowlist) on the operator-facing getting-started path. Not changed (see structured output for reasoning): the report-only default for the pipeline-variable override-role restriction, and the PR description's issue-closing keyword -- both are flagged in the review as requiring a human decision, not a code change in this diff. Relates to #7769
🔧 Fix agent — iteration 1 (bot-triggered)Fixed two concrete bugs (issue-event note-target misdirection via a stray CI_MERGE_REQUEST_IID, and proxy env leaking into post-pin fullsend CLI subprocesses) with regression tests, added an operator-facing doc section on the identity-pin's runtime constraints, and documented verification research resolving the long-recurring branches-endpoint allowlist concern. Declined two findings that the review's own remediation assigns to a human/PR-tracking decision rather than a code change. Fixed (4):
Disagreed (2):
Tests: passed CI inspections (11):
Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 10:15 PM UTC · Completed 10:34 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $7.35 |
|
🤖 Finished Fix · ✅ Success · Started 10:36 PM UTC · Completed 10:46 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.27 |
…nt dispatch Address review feedback on PR #7817: - In the issue_* EVENT_TYPE arm of run-agent-job.sh, explicitly export FULLSEND_NOTE_TARGET=issues alongside the existing unset of CI_MERGE_REQUEST_IID. newGitLabClientFromEnv selects the merge_requests note target whenever CI_MERGE_REQUEST_IID is non-empty or FULLSEND_NOTE_TARGET is "merge_requests" -- a pre-set FULLSEND_NOTE_TARGET CI/CD variable could otherwise survive into an issue-event dispatch and misdirect the status comment. - In pin-ci-job-identity.sh's post-pin proxy cleanup, also unset ALL_PROXY/all_proxy alongside HTTP_PROXY/HTTPS_PROXY, matching the set fullsend_gate_curl already strips for its own curl calls; git (used for later credential-bearing fetches) honors ALL_PROXY as a fallback. - Extended the existing regression tests (TestRunAgentJobScript_UnsetsMergeRequestIIDForIssueEvent, TestPinCIJobIdentity_UnsetsProxyEnvInParentShellOnSuccess) to cover both cases. The remaining "Closes #7769" PR-description finding requires a human to edit the PR description via the forge UI and is not resolvable by an automated commit; recorded as a disagreement in structured output. Note: pre-commit could not fetch hook repositories (network access is blocked in this sandbox), so pre-commit itself did not run. Ran the network-independent equivalents directly instead: gofmt -l and go vet (clean) for the two changed Go test files, plus manual checks for trailing whitespace, final newline, and merge-conflict markers on all four changed files (clean). The shellcheck hook could not be run directly either, since its shellcheck binary is normally installed via network fetch and is not preinstalled in this sandbox. Addresses #7817
🔧 Fix agent — iteration 2 (bot-triggered)Fixed both code findings from this round's review: pinned FULLSEND_NOTE_TARGET=issues in the issue_* dispatch arm of run-agent-job.sh so a pre-set CI/CD variable cannot misdirect status comments, and extended pin-ci-job-identity.sh's post-pin proxy cleanup to also strip ALL_PROXY/all_proxy. Both existing regression tests were extended to cover the new assertions and pass. The remaining PR-description "Closes #7769" finding is recorded as a disagreement requiring human action, as in prior rounds. Fixed (2):
Disagreed (1):
Tests: passed CI inspections (11):
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 10:48 PM UTC · Completed 11:11 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $10.88 |
Harden the GitLab poll and agent in-job security gates per ADR 0125.
This PR delivers the in-job hardening portion of #7769:
This PR does not implement pipeline-input transport, opaque dispatch records, or dispatcher fast-path wiring. Those are tracked separately:
The final design must keep the Fullsend runtime credential at Developer level. Owner-level credentials are not required for runtime dispatch. GitLab project pipeline-variable restriction support is an administrative/install-time primitive only; this PR must not make an Owner-level runtime credential part of the dispatch design or break the existing path before #7850/#7851 land.
Closes #7769
Verification