GITGUD

neiam neiam /gitgud

Git Gud

public · Issues · Pulls · Labels · Forks · Compare · Actions success · Badges · Packages

⭐ 1

Log in to mark this repository.

10.1 KiB · text History 876aa0b

Plan: artifacts and cache that stay in their own run and repository

Status: ready to execute. Written to be carried out start to finish by one agent without further questions. Independent of the other CI plans, but read step 3 against workflow-runner-context.md: that plan adds run_id to the job context, which is what the artifact URLs are built from.

Why

Two security holes and one data-corruption bug, all in the services a running job calls back into.

  1. Artifacts trust the URL, not the token. GitGudWeb.ActionsArtifactController takes run_id from the path (/api/actions/_apis/pipelines/workflows/:run_id/artifacts…) and never compares it with the JWT’s rid, which GitGudWeb.Plugs.RunnerJwtAuth has already put in conn.assigns.runner_run_id. create/2 writes into whatever run the path names; list/2 lists any run; download/2 ignores run_id completely and serves any artifact id. So any job, in any repository, holding its own valid job token, can read every artifact on the instance (ids are sequential) and plant artifacts in other repositories’ runs.
  2. Cache uploads aren’t checked against the repository either. GitGudWeb.ActionsCacheController.upload_chunk/2 and finalize/2 load the entry by id (ActionsCache.get!/1) with no check that entry.repository_id is the token’s pid. A job can append to another repository’s reserved-but-unfinished cache entry (cache poisoning).
  3. Cache chunks are concatenated in arrival order. ActionsCache.append/2 reads the whole stored object and writes it back with the new body appended; the Content-Range header the client sends is ignored. actions/cache uploads in 32 MB chunks, four at a time (uploadConcurrency), so any cache over 32 MB is stored with its chunks in whatever order the requests finished — and two concurrent appends can also lose a chunk (read-modify-write race). The cache “saves”, the next job restores a corrupt tarball. It is also O(n²) in memory for a large cache.

And two smaller ones the same work should fix:

  1. The README says “GitHub Actions artifacts API v4”. The code implements a simplified v1 “pipelines” API only. actions/upload-artifact@v4 and download-artifact@v4 (what GitHub’s docs and every new workflow use) speak a different, Twirp-based protocol (twirp/github.actions.results.api.v1.ArtifactService/*, located via ACTIONS_RESULTS_URL) and fail. That failure is loud, so this plan only corrects the README and records v4 as a follow-up (see “Out”).
  2. A PR run’s cache falls back only to the default branch (ActionsCacheController.refs_for/1); GitHub falls back to the PR’s base branch.

Semantics to match

  • An artifact belongs to one workflow run. A job may create, list and download artifacts of its own run only (that is how upload-artifact@v3/download-artifact@v3 share files between jobs of a run). Anything else is 404 (not 403: don’t confirm the id exists).
  • A cache entry belongs to one repository. Only a job of that repository may upload to, finalize, or read it. (Read was already scoped: ActionsCache.lookup/5 filters on repository_id.)
  • Cache upload chunks carry Content-Range: bytes <start>-<end>/*; the stored object is the chunks placed at their offsets. Finalize gives the total size.
  • Cache restore order for a PR run: current ref, then the PR’s base branch, then the default branch.

Where things are
Routes lib/git_gud_web/router.ex, scope "/api/actions/_apis" with pipe_through :runner_jwt
JWT plug lib/git_gud_web/plugs/runner_jwt_auth.ex — assigns :runner_jwt_claims, :runner_job_id, :runner_run_id
Artifacts lib/git_gud_web/controllers/actions_artifact_controller.ex (create/2, upload/2, finalize/2, list/2, download/2); schema lib/git_gud/workflows/workflow_artifact.ex
Cache lib/git_gud_web/controllers/actions_cache_controller.ex (get_cache/2, reserve_cache/2, upload_chunk/2, finalize/2, refs_for/1); lib/git_gud/workflows/actions_cache.ex (reserve/4, append/2, finalize/2); schema lib/git_gud/workflows/cache_entry.ex
Storage GitGud.Storage (put/4, get/2, presign_url/4) — check what else the behaviour offers (multipart? append?) before choosing step 4’s design
Tests test/git_gud_web/controllers/actions_endpoints_test.exs, test/git_gud/workflows/actions_cache_test.exs, test/git_gud/workflows/runner_jwt_test.exs (minting a token for a job)

Steps

1. Artifacts: the token’s run, not the path’s

In ActionsArtifactController, add a private own_run(conn, run_id_param) that returns {:ok, rid} when String.to_integer(run_id_param) == conn.assigns.runner_run_id, else :error; an unparsable id is :error. Every action starts with it and answers 404 on :error.

  • create/2, list/2, finalize/2: as above.
  • upload/2: already compares the artifact’s run with the path; also require the path’s run to be the token’s.
  • download/2: load the artifact and require artifact.workflow_run_id == conn.assigns.runner_run_id, else 404. Use Repo.get/2, not get!/2 (a missing id is 404, not a 500).
  • upload/2 and upload_chunk/2 read bodies with fixed caps (1_000_000_000, 200_000_000). Leave the values; just make the oversize case answer 413 instead of crashing (Plug.Conn.read_body/2 returns {:more, …} past :length).

Also: if run_id in the path is 0/empty because the job had no github.run_id (see the context plan), the old behaviour wrote into run 1 of some other repository. After this step it is a 404 — loud, and fixed for real by the context plan’s run_id.

2. Cache: the token’s repository

In ActionsCacheController.upload_chunk/2 and finalize/2, load with ActionsCache.get/1 (add it: Repo.get(CacheEntry, id)) and require entry.repository_id == jwt_repo_id(conn); otherwise 404. Also refuse (409) an upload to an entry whose archived_at is set — a finalized entry is immutable.

3. Cache: chunks at their offsets

Replace read-append-write with offset-placed chunks:

  • Parse Content-Range: bytes S-E/* in upload_chunk/2 (missing or malformed → 400). Check E - S + 1 == byte_size(body).
  • Store each chunk as its own object, "#{entry.storage_key}.part.#{S}", with Storage.put/4. Concurrent chunks then never touch the same object.
  • finalize/2: list the parts for the entry (keep the offsets in a new table cache_entry_parts (cache_entry_id, offset, size, storage_key) written by upload_chunk/2 — simpler and more portable than listing the bucket; mix ecto.gen.migration add_cache_entry_parts), check they tile 0..size-1 with no gaps or overlaps (else 400 and leave the entry unarchived), then assemble them in offset order into entry.storage_key and delete the parts. Assemble by streaming if GitGud.Storage can (look at the behaviour; Garage is S3, so a multipart upload or server-side compose may exist); otherwise concatenate in memory — still correct, and the 200 MB-per-request cap bounds each read.
  • ActionsCache.append/2 is then unused: remove it.
  • The CacheEviction worker (lib/git_gud/workflows/workers/cache_eviction.ex) must also delete orphaned parts of entries that were never finalized: check what it does with unarchived entries today and extend it.

4. Cache: a PR falls back to its base branch

In refs_for/1, when the run’s trigger == "pull_request" and it has pr_base_ref (column added by the pull_request plan), the fallback list is [run.head_ref, "refs/heads/" <> run.pr_base_ref, default_ref]. ActionsCache.lookup/5 takes a single default_ref; change it to take a list of fallback refs ([ref | fallbacks]) and keep its exact-then-prefix order across them. Saving still goes to the run’s own ref.

5. README

In README’s CI list, replace “GitHub Actions artifacts API v4” with what is true: the v1 pipelines artifact API (actions/upload-artifact@v3 / download-artifact@v3), scoped to the run; @v4 not yet supported. State the cache’s restore order.

Tests

actions_endpoints_test.exs (mint tokens with RunnerJwt.mint/2 for jobs in two different repositories’ runs):

  • job A creates an artifact in its run; job B (other repo) gets 404 on create/list/upload/download against A’s run and on download of A’s artifact id through B’s own run path;
  • job A lists and downloads its own artifact (still works);
  • a second job of the same run can list and download it (cross-job sharing still works).

actions_cache_test.exs / controller test:

  • B’s token gets 404 on upload_chunk/finalize of A’s entry;
  • three chunks uploaded out of order (offsets 2N, 0, N) then finalize → the restored bytes equal the original;
  • a gap (chunks 0 and 2N only) → finalize 400, entry not archived;
  • upload after finalize → 409;
  • PR run: an entry saved on the base branch (not the default branch) is found by a PR run’s lookup; an entry on an unrelated branch is not.

Out

  • Artifacts v4 (ArtifactService Twirp: CreateArtifact, FinalizeArtifact, ListArtifacts, GetSignedArtifactURL, blob upload to a signed URL, ACTIONS_RESULTS_URL in the job env). Worth its own plan once this one lands; Forgejo implements it in routers/api/actions/artifactsv4.go, which is the reference.
  • Cache service v2 (actions/cache@v4 with ACTIONS_CACHE_SERVICE_V2). First check whether jobs reach this cache at all: forgejo-runner ships its own cache server and may point ACTIONS_CACHE_URL at it regardless of the ACTIONS_CACHE_URL key gitgud puts in the task context (runner_controller.ex, build_context_struct/4). Record what you find in the README.

Done when

  • mix test passes; no new compile warnings (the repo has pre-existing ones: compare the warning list before and after, mix compile --force). mix format only the files you touched — a repo-wide mix format rewrites ~40 unrelated files.
  • One commit, in the repository’s message style, naming the cross-repo artifact access as the reason.
  • Not pushed. Pushing gitgud deploys the forge.