Skip to content

fix(gitlab): stop leaking the API token on redirect and globbing project paths - #3

Closed
elibosley wants to merge 1 commit into
jonschumaker:feat/gitlab-providerfrom
unraid:review/gitlab-provider-hardening
Closed

fix(gitlab): stop leaking the API token on redirect and globbing project paths#3
elibosley wants to merge 1 commit into
jonschumaker:feat/gitlab-providerfrom
unraid:review/gitlab-provider-hardening

Conversation

@elibosley

Copy link
Copy Markdown

Review follow-ups for unraid#44 — two mechanical fixes I could make safely without touching the design. Targets feat/gitlab-provider so they land in the same PR. Everything else from the review is left for you, notes at the bottom.

1. API token could be resent to a redirect target

gitlab_api / gitlab_api_capture used curl -fsSL while supplying the token through header = "PRIVATE-TOKEN: ...". curl strips only Authorization on a cross-host redirect — custom headers are resent — so a 3xx from a self-managed instance would hand the token to whatever host it named. These are plain /api/v4 GETs against the configured base URL, so I just dropped -L.

2. GITLAB_PROJECTS was glob-expanded

for project in $GITLAB_PROJECTS is subject to pathname expansion as well as word splitting. group/* is a plausible thing to type into a field labelled "Monitored projects", and it would expand against the process CWD — the advisory queue/stats/public-visibility scans would then query whatever directory names happened to match.

Added gitlab_projects_list, which splits with read -a (no globbing) and emits only well-formed namespace/project paths. Malformed entries are dropped rather than failing gitlab_validate_settings: the field is telemetry-only and doesn't define runner scope, so it shouldn't be able to block a fleet from starting.

gitlab_stats_refresh reads the list on fd 3 because its per-project status scan is itself a while read on stdin.

Tests

Extended tests/gitlab-policy.sh to assert the list drops glob/relative/bare entries and does not expand against the CWD.

gitlab-policy, provider-contract, config-parity and exec-csrf pass locally. I could not run tests/run-linux-checks.sh (no Docker on this machine); tests/provider-mocks.sh fails on macOS for me at the glrt- shape check because BSD grep caps interval repetition at 255 and the pattern is {10,507} — that reproduces identically on your branch without my changes, so it's just a host limitation of running the suite outside the Linux container.

Not addressed here — needs your call

  • The CSRF rewrite in exec.php is the one I'd most want a second pair of eyes on. The platform path requires $csrf_token to still be set while $_POST['csrf_token'] and $_SERVER['HTTP_X_CSRF_TOKEN'] are both unset. If a supported Unraid version also unsets $csrf_token, the standalone fallback then reads an already-removed $_POST key and every action — including all the existing GitHub ones — 403s. tests/exec-csrf.sh fabricates that state in PHP so it can't confirm it. Worth a manual smoke test on each Unraid version in support, not just the one dev box.
  • Every GitLab job container gets a Docker socket via the executor volumes entry, and gitlab_validate_settings requires DIND=true or SHARE_DOCKER_SOCK=true, so there's no configuration where job code lacks Docker access. It's disclosed clearly in the UI and the PR body, and the manager does need a daemon — but the job container doesn't, so a toggle would give operators a materially safer option. Your design call, not something I wanted to change unilaterally.
  • fleet.lock is now held across a full image-update rollover (imageupdate_tickimageupdate_rolloverdrain_and_recreate per slot). With IMAGE_DRAIN_TIMEOUT=3600 and GitLab's shutdown_timeout=7200 that can be hours during which UI Start/Stop/Scale/Recycle fail on the 20s flock -w. The lock is what makes the roll safe so I'd keep it, but the UI probably wants to distinguish "rollover in progress" from a generic busy error.

…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.
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.

1 participant