fix(gitlab): harden API redirects and CSRF compatibility - #46
Open
jonschumaker wants to merge 3 commits into
Open
fix(gitlab): harden API redirects and CSRF compatibility#46jonschumaker wants to merge 3 commits into
jonschumaker wants to merge 3 commits into
Conversation
…ect paths Two narrow hardening fixes on top of the GitLab provider work. gitlab_api/gitlab_api_capture passed --location while supplying the API token through `header = "PRIVATE-TOKEN: ..."`. curl strips only the Authorization header on a cross-host redirect, so a 3xx from a self-managed instance would resend the token to whatever host it named. These are plain /api/v4 GETs against the configured base URL and have no reason to follow a redirect off it, so drop -L. GITLAB_PROJECTS was iterated as `for project in $GITLAB_PROJECTS`, which is subject to pathname expansion as well as word splitting. A plausible entry like `group/*` expands against the process CWD, so the advisory queue/stats/public-visibility scans would query whatever directory names happened to match. Split the list with `read -a` instead and emit only well-formed namespace/project paths. The field is telemetry-only and does not define runner scope, so a malformed entry is skipped rather than failing validation and blocking a fleet. gitlab_stats_refresh reads its project list on fd 3 because its per-project status scan is itself a `while read` on stdin.
📝 WalkthroughWalkthroughThe change updates platform CSRF validation for legacy states and non-scalar values. It also centralizes GitLab API requests, rejects redirects, validates monitored projects, and updates queue/statistics processing and test coverage. ChangesCSRF validation
GitLab provider hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GitLabProvider
participant gitlab_api_request
participant Curl
GitLabProvider->>gitlab_api_request: Send method, path, body, and headers
gitlab_api_request->>Curl: Execute validated request without redirects
Curl-->>gitlab_api_request: Return status, headers, and body
gitlab_api_request-->>GitLabProvider: Return 2xx response or failure
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/usr/local/emhttp/plugins/ci-runner-farm/include/providers/gitlab.sh`:
- Around line 360-368: Update gitlab_projects_list to split the complete
GITLAB_PROJECTS value on default-IFS whitespace without truncating at the first
newline or allowing pathname expansion, while preserving validation and output
behavior. Add a regression case in tests/gitlab-policy.sh covering multiple
newline-delimited project entries and verifying that each valid project is
emitted.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a5b41d23-7014-48ee-97de-d51c2732ebb9
📒 Files selected for processing (5)
src/usr/local/emhttp/plugins/ci-runner-farm/include/exec.phpsrc/usr/local/emhttp/plugins/ci-runner-farm/include/providers/gitlab.shtests/exec-csrf.shtests/gitlab-policy.shtests/provider-mocks.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PRIVATE-TOKENfrom reaching redirect targets, even when a user curl configuration enables redirect followingGITLAB_PROJECTSvalue without pathname expansion and safely retain valid dot/underscore-prefixed GitLab pathsThis carries forward Eli Bosley's closed fork PR #3 with authorship preserved, plus the follow-up fixes and regression coverage found during validation.
Root cause
Dropping curl's location flag blocks the normal redirect path, but curl's fail mode still treats 3xx responses as successful and a user-level
.curlrccan turn location following back on. That could both expose the custom GitLab token and turn a redirect body into false queue/stat data.The endpoint's platform CSRF path also assumed that Unraid's preloader always retained
$csrf_token. Unraid 6.12–7.2 validates and consumes the request token without creating that local variable; Unraid 7.3 retains it.Behavior
-qfirst, never follows redirects, and accepts only status codes in the 2xx classValidation
bash tests/run-linux-checks.sh