Require a valid job token on the older runner artifact endpoints (#40)
PUT and GET /runner/jobs/:jobId/artifacts/:name only parsed the token's shape (djt_<repo>_<job>_...) before touching R2. A PUT stored the body before the job was authenticated, and a GET never authenticated at all, so anyone who knew a repository and job id could plant or read a run's artifacts. Both now have the repository verify the token first, the way the cache endpoints already do. Claude-Session: https://claude.ai/code/session_016yhgAu6oKfqes812Yd4qjo Co-authored-by: Claude <noreply@anthropic.com>
Canberk Demirci authored · GitHub committed signed1 parent 8793844commit 79c6c38bbcd6727abc76be5d916d2e659f9f6bf2Browse files
4 files changed+49−10
Files (4)
modifiedapps/diffinery/src/http/runner.ts+5−3View file
| @@ -13,6 +13,7 @@ | ||
| 13 | 13 | /** Endpoints the runner agent inside a job container calls. Auth: the job token. */ |
| 14 | 14 | export const runner = new Hono<{ Bindings: Env; Variables: AppVars }>(); |
| 15 | 15 | |
| 16 | +/** The job token's shape only: which repository to ask. Every handler must still have the repository verify the token. */ | |
| 16 | 17 | function jobAuth(req: Request, jobId: string): { token: string; repoId: string } { |
| 17 | 18 | const token = credentialFrom(req); |
| 18 | 19 | if (!token?.startsWith("djt_")) throw new HttpError(401, "job token required"); |
| @@ -109,7 +110,8 @@ | ||
| 109 | 110 | const jobId = c.req.param("jobId"); |
| 110 | 111 | const { token, repoId } = jobAuth(c.req.raw, jobId); |
| 111 | 112 | const stub = repoStub(c.env, repoId); |
| 112 | - const keys = await stub.artifactKeysFor(jobId, c.req.param("name")); | |
| 113 | + // Authenticates the job before anything is written: jobAuth only reads the token's shape. | |
| 114 | + const keys = await stub.artifactKeysFor(jobId, token, c.req.param("name")); | |
| 113 | 115 | // What is left of the repository's artifact quota, counting live artifacts from both upload paths. |
| 114 | 116 | const room = await stub.artifactRoom(); |
| 115 | 117 | if (room === 0) throw new HttpError(413, "the repository's artifact storage quota is used up; delete artifacts or wait for them to expire"); |
| @@ -120,8 +122,8 @@ | ||
| 120 | 122 | |
| 121 | 123 | runner.get("/runner/jobs/:jobId/artifacts/:name", async (c) => { |
| 122 | 124 | const jobId = c.req.param("jobId"); |
| 123 | - const { repoId } = jobAuth(c.req.raw, jobId); | |
| 124 | - const obj = await getFirst(c.env.BUCKET, await repoStub(c.env, repoId).artifactKeysFor(jobId, c.req.param("name"))); | |
| 125 | + const { token, repoId } = jobAuth(c.req.raw, jobId); | |
| 126 | + const obj = await getFirst(c.env.BUCKET, await repoStub(c.env, repoId).artifactKeysFor(jobId, token, c.req.param("name"))); | |
| 125 | 127 | if (!obj) throw new HttpError(404, "artifact not found"); |
| 126 | 128 | return new Response(obj.body, { headers: { "content-type": "application/gzip" } }); |
| 127 | 129 | }); |
modifiedapps/diffinery/src/repo/RepoDO.ts+2−2View file
| @@ -844,8 +844,8 @@ | ||
| 844 | 844 | cacheReadKeys(...a: Parameters<Actions["cacheReadKeys"]>) { |
| 845 | 845 | return this.actions.cacheReadKeys(...a); |
| 846 | 846 | } |
| 847 | - artifactKeysFor(jobId: string, name: string) { | |
| 848 | - return this.actions.artifactR2Keys(this.actions.runIdForJob(jobId), name); | |
| 847 | + artifactKeysFor(...a: Parameters<Actions["artifactKeysFor"]>) { | |
| 848 | + return this.actions.artifactKeysFor(...a); | |
| 849 | 849 | } |
| 850 | 850 | artifactQuota() { |
| 851 | 851 | return this.runtime.artifactQuota; |
modifiedapps/diffinery/src/repo/actions.ts+6−5View file
| @@ -1465,6 +1465,12 @@ | ||
| 1465 | 1465 | return { r2Key: this.artifactR2Keys(job.run_id, name)[0]! }; |
| 1466 | 1466 | } |
| 1467 | 1467 | |
| 1468 | + /** Where job `jobId` (authenticated by its token) reads or writes artifact `name` of its run. */ | |
| 1469 | + async artifactKeysFor(jobId: string, token: string, name: string): Promise<string[]> { | |
| 1470 | + const job = await this.authJob(jobId, token); | |
| 1471 | + return this.artifactR2Keys(job.run_id, name); | |
| 1472 | + } | |
| 1473 | + | |
| 1468 | 1474 | /** Where an older runner's artifact is stored: the current key, then the old layout's. */ |
| 1469 | 1475 | artifactR2Keys(runId: string, name: string): string[] { |
| 1470 | 1476 | return legacyArtifactKeys(this.repo.meta.repoId, runId, name); |
| @@ -1492,11 +1498,6 @@ | ||
| 1492 | 1498 | return [...current, ...legacy.filter((l) => !current.some((c) => c.name === l.name))].sort((a, b) => a.name.localeCompare(b.name)); |
| 1493 | 1499 | } |
| 1494 | 1500 | |
| 1495 | - /** Runs of the same commit; lets a job download artifacts uploaded by earlier jobs. */ | |
| 1496 | - runIdForJob(jobId: string): string { | |
| 1497 | - return this.jobRow(jobId).run_id; | |
| 1498 | - } | |
| 1499 | - | |
| 1500 | 1501 | // ---------------------------------------------------------------- sandboxes for agents |
| 1501 | 1502 | |
| 1502 | 1503 | async createSandbox(caller: Caller, ref?: string): Promise<{ id: string; ref: string }> { |
modifiedapps/diffinery/test/e2e/e2e.test.mjs+36−0View file
| @@ -1123,6 +1123,42 @@ | ||
| 1123 | 1123 | await job.done(); |
| 1124 | 1124 | }); |
| 1125 | 1125 | |
| 1126 | +test("an older runner's artifact API needs the job's own token", async () => { | |
| 1127 | + const owner = (await ok("GET", "/user")).login; | |
| 1128 | + await ok("POST", "/user/repos", { name: "legacy-artifacts" }); | |
| 1129 | + const src = join(work, "legacy-artifacts"); | |
| 1130 | + mkdirSync(join(src, ".github/workflows"), { recursive: true }); | |
| 1131 | + writeFileSync(join(src, ".github/workflows/w.yml"), "on: push\njobs:\n j:\n runs-on: [self-hosted, linux]\n steps: [{ run: echo }]\n"); | |
| 1132 | + git(src, ["init", "-q", "-b", "main"]); | |
| 1133 | + git(src, ["add", "-A"]); | |
| 1134 | + git(src, ["commit", "-q", "-m", "legacy"]); | |
| 1135 | + git(src, ["remote", "add", "origin", `${base}/${owner}/legacy-artifacts.git`]); | |
| 1136 | + git(src, ["push", "-q", "origin", "main"]); | |
| 1137 | + const claim = await until("a job to claim", async () => { | |
| 1138 | + const r = await api("POST", `/repos/${owner}/legacy-artifacts/actions/runners/claim`, { name: "old-runner", labels: ["linux"] }); | |
| 1139 | + return r.status === 200 ? r.json : null; | |
| 1140 | + }); | |
| 1141 | + const at = `${base}/runner/jobs/${claim.job_id}/artifacts/out`; | |
| 1142 | + const real = { authorization: `Bearer ${claim.token}` }; | |
| 1143 | + // Same shape (djt_<repo>_<job>_...) as the real token, but not it. | |
| 1144 | + const forged = { authorization: `Bearer ${claim.token.split("_").slice(0, 3).join("_")}_${"0".repeat(48)}` }; | |
| 1145 | + assert.equal((await fetch(`${base}/runner/jobs/${claim.job_id}`, { headers: real })).status, 200); | |
| 1146 | + const put = async (h, body) => (await fetch(at, { method: "PUT", headers: { ...h, "content-length": String(body.length) }, body })).status; | |
| 1147 | + const get = async (h) => { | |
| 1148 | + const r = await fetch(at, { headers: h }); | |
| 1149 | + return r.status === 200 ? await r.text() : r.status; | |
| 1150 | + }; | |
| 1151 | + assert.equal(await put(real, "real bytes"), 200); | |
| 1152 | + assert.equal(await put(forged, "forged bytes"), 401); | |
| 1153 | + assert.equal(await get(forged), 401); | |
| 1154 | + assert.equal(await get({}), 401); | |
| 1155 | + // The forged upload never reached storage. | |
| 1156 | + assert.equal(await get(real), "real bytes"); | |
| 1157 | + assert.equal((await fetch(`${base}/runner/jobs/${claim.job_id}/complete`, { method: "POST", headers: { ...real, "content-type": "application/json" }, body: JSON.stringify({ conclusion: "success", outputs: {} }) })).status, 200); | |
| 1158 | + // A finished job's token no longer reads or writes. | |
| 1159 | + assert.equal(await get(real), 401); | |
| 1160 | +}); | |
| 1161 | + | |
| 1126 | 1162 | const hasDocker = spawnSync("docker", ["info"], { stdio: "ignore", timeout: 15_000 }).status === 0; |
| 1127 | 1163 | |
| 1128 | 1164 | test("containers: a job container and service containers on a self-hosted runner", { skip: !hasDocker && "Docker is not available" }, async () => { |