From 541be597f47eff1bedb9c31383c8f6ba1cf868bd Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Mon, 31 Aug 2026 08:10:13 +0500 Subject: [PATCH 1/3] fix(cache): authorize a warm claim by its run, not by its job name A promoted warm container keeps its provider instance name while the one-job JIT runner takes GitHub's runner name, so the broker proves the pair through the queue journal before handing over cache credentials. It did that with BindRunning, which must resolve exactly one intent because it writes to it, and which separates a run's sibling jobs by comparing the journal's display name against GITHUB_JOB. Those two agree for a bare job and for a reusable-workflow prefix, and disagree for every matrix job: `Analyze (python)` is not `analyze`, and `Backend pytest shard 4/4` is nothing like its id. This fleet is mostly matrix jobs. Measured over one live hour: 7 warm correlations bound, 19 refused. The claim is nonfatal, so those jobs still ran -- uncached, which is the slow path warm capacity exists to remove. Authorization does not need to identify the job. The credential is scoped to a repository, so the question is whether this scale set is serving that repository's run, and (scale set, owner, repository, workflow run id) is a tighter answer than any job-name comparison. AuthorizeRunning asks exactly that and writes nothing; the exact intent is still bound by BindRunning and its asynchronous retry once GitHub reports the job started. An intent admitted before JobAvailable carries no run id yet, and BindRunning already treats that as compatible, so authorization does too -- refusing it would deny warm capacity the head-of-queue case it is for. The refusal was also the only path in this handler that logged no identity at all: the one failure costing every warm runner its cache was the one that could not be read from its own log. It now carries instance, runner, repository, run, job and pool, and the bind error. A foreign repository is still refused and its one-job token is still not consumed; there is a test for that, and one that reproduces the matrix shape from the live journal. --- internal/cachebroker/cachebroker_test.go | 139 +++++++++++++++++++++- internal/cachebroker/handler.go | 48 +++++--- internal/queueintent/correlation_linux.go | 62 ++++++++++ 3 files changed, 231 insertions(+), 18 deletions(-) diff --git a/internal/cachebroker/cachebroker_test.go b/internal/cachebroker/cachebroker_test.go index d18c47fc..ec3c2bb4 100644 --- a/internal/cachebroker/cachebroker_test.go +++ b/internal/cachebroker/cachebroker_test.go @@ -378,7 +378,7 @@ func TestWarmClaimBindsProviderInstanceToExactRuntimeRunner(t *testing.T) { if journal.Claims["warm-standard-example"].ClaimedRepository != "example-org/example-repo" { t.Fatalf("warm claim was not bound to the exact repository: %+v", journal.Claims["warm-standard-example"]) } - if !strings.Contains(logs.String(), `"msg":"warm runner correlation bound"`) || + if !strings.Contains(logs.String(), `"msg":"warm runner correlation authorized"`) || !strings.Contains(logs.String(), `"runner.name":"runner-runtime"`) { t.Fatalf("warm correlation evidence is missing: %s", logs.String()) } @@ -484,3 +484,140 @@ func TestJobCorrelationIsAllOrNothingAndBounded(t *testing.T) { t.Fatal("oversized job identity accepted") } } + +// A matrix job's display name is what GitHub puts in the scale-set message -- +// `Analyze (python)` -- while GITHUB_JOB is the workflow's job id, `analyze`. +// Requiring those to match refused the cache to every matrix job on a warm +// runner: 19 refusals against 7 binds in one live hour. Authorization asks the +// tighter question instead, whether this scale set is serving that repository's +// exact workflow run. +func TestWarmClaimIsAuthorizedForAMatrixJobDisplayName(t *testing.T) { + now := time.Date(2026, 8, 31, 3, 0, 0, 0, time.UTC) + directory := t.TempDir() + store := Store{Path: filepath.Join(directory, "claims.json"), LockPath: filepath.Join(directory, "claims.lock")} + token := bytes.Repeat([]byte{9}, ClaimTokenBytes) + if err := store.Add(context.Background(), "warm-standard-matrix", "example-standard", "correlation-only", token); err != nil { + t.Fatal(err) + } + queuePath := filepath.Join(directory, "queue-intents.json") + queueLock := filepath.Join(directory, "queue-intents.lock") + intents := map[string]queueintent.Intent{} + for index, language := range []string{"python", "go", "rust"} { + key := "github-scale-set-job:v2:42:job-matrix-" + language + intents[key] = queueintent.Intent{ + Key: key, ScaleSetID: 42, JobID: "job-matrix-" + language, RunnerRequestID: int64(700 + index), + ScaleSetName: "example-standard", JobDisplayName: "Analyze (" + language + ")", Owner: "example-org", + Repository: "example-org/example-repo", WorkflowRef: "unavailable-before-job-available", EventName: "push", + QueueTime: now.Add(-time.Minute), State: queueintent.StateAssigned, Priority: 1, WorkflowRunID: 456, + StateEnteredAt: now.Add(-30 * time.Second), UpdatedAt: now, ExpiresAt: now.Add(time.Hour), + } + } + queue := queueintent.Journal{ + SchemaVersion: queueintent.SchemaVersion, Generation: 11, UpdatedAt: now, Intents: intents, + Repositories: map[string]queueintent.RepositoryState{}, TerminalJobs: map[string]time.Time{}, + } + raw, err := json.Marshal(queue) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(queuePath, append(raw, '\n'), 0o600); err != nil { + t.Fatal(err) + } + config := Config{ + SchemaVersion: 1, ListenAddress: "192.0.2.2:9444", Endpoint: "https://192.0.2.1:9002", + Region: "us-east-1", Bucket: "github-actions-cache", CAFile: "/tmp/ca", + JournalFile: store.Path, JournalLock: store.LockPath, QueueJournalFile: queuePath, QueueJournalLock: queueLock, + } + body, _ := json.Marshal(ClaimRequest{ + InstanceName: "warm-standard-matrix", RunnerName: "runner-runtime-matrix", Repository: "example-org/example-repo", + RepositoryID: 123, WorkflowRunID: 456, RunAttempt: 1, JobName: "analyze", + WorkflowRef: "example-org/example-repo/.github/workflows/codeql.yml@refs/heads/main", + CommitSHA: strings.Repeat("b", 40), Token: base64.RawURLEncoding.EncodeToString(token), + }) + correlator := &queueintent.Correlator{ + Path: queuePath, LockPath: queueLock, Now: func() time.Time { return now.Add(time.Second) }, Attempts: 1, + } + var logs bytes.Buffer + response := httptest.NewRecorder() + Handler{Config: config, Store: store, QueueCorrelator: correlator, Logger: slog.New(slog.NewJSONHandler(&logs, nil))}. + ServeHTTP(response, httptest.NewRequest(http.MethodPost, "https://x"+ClaimPath, bytes.NewReader(body))) + if response.Code != http.StatusNoContent { + t.Fatalf("a matrix job was refused its cache: status=%d body=%s logs=%s", response.Code, response.Body.String(), logs.String()) + } + journal, err := store.Read(context.Background()) + if err != nil { + t.Fatal(err) + } + if journal.Claims["warm-standard-matrix"].ClaimedRepository != "example-org/example-repo" { + t.Fatalf("warm claim was not bound to the exact repository: %+v", journal.Claims["warm-standard-matrix"]) + } + // Three sibling intents share this run, so no single one can be resolved and + // the journal must be left for the asynchronous binder rather than guessed at. + if !strings.Contains(logs.String(), `"msg":"warm runner correlation authorized"`) || + !strings.Contains(logs.String(), `"msg":"queue running correlation deferred"`) { + t.Fatalf("authorization evidence is missing: %s", logs.String()) + } +} + +// Authorization is not a weakening: a warm runner presenting a repository this +// scale set holds no active work for is still refused, and its one-job token is +// not consumed. +func TestWarmClaimForAnotherRepositoryIsStillRefused(t *testing.T) { + now := time.Date(2026, 8, 31, 3, 0, 0, 0, time.UTC) + directory := t.TempDir() + store := Store{Path: filepath.Join(directory, "claims.json"), LockPath: filepath.Join(directory, "claims.lock")} + token := bytes.Repeat([]byte{10}, ClaimTokenBytes) + if err := store.Add(context.Background(), "warm-standard-foreign", "example-standard", "correlation-only", token); err != nil { + t.Fatal(err) + } + queuePath := filepath.Join(directory, "queue-intents.json") + queueLock := filepath.Join(directory, "queue-intents.lock") + key := "github-scale-set-job:v2:42:job-owned" + queue := queueintent.Journal{ + SchemaVersion: queueintent.SchemaVersion, Generation: 3, UpdatedAt: now, + Intents: map[string]queueintent.Intent{key: { + Key: key, ScaleSetID: 42, JobID: "job-owned", RunnerRequestID: 71, + ScaleSetName: "example-standard", JobDisplayName: "Analyze (python)", Owner: "example-org", + Repository: "example-org/example-repo", WorkflowRef: "unavailable-before-job-available", EventName: "push", + QueueTime: now.Add(-time.Minute), State: queueintent.StateAssigned, Priority: 1, WorkflowRunID: 456, + StateEnteredAt: now.Add(-30 * time.Second), UpdatedAt: now, ExpiresAt: now.Add(time.Hour), + }}, Repositories: map[string]queueintent.RepositoryState{}, TerminalJobs: map[string]time.Time{}, + } + raw, err := json.Marshal(queue) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(queuePath, append(raw, '\n'), 0o600); err != nil { + t.Fatal(err) + } + body, _ := json.Marshal(ClaimRequest{ + InstanceName: "warm-standard-foreign", RunnerName: "runner-runtime-foreign", Repository: "other-org/other-repo", + RepositoryID: 321, WorkflowRunID: 456, RunAttempt: 1, JobName: "analyze", + WorkflowRef: "other-org/other-repo/.github/workflows/codeql.yml@refs/heads/main", + CommitSHA: strings.Repeat("c", 40), Token: base64.RawURLEncoding.EncodeToString(token), + }) + correlator := &queueintent.Correlator{ + Path: queuePath, LockPath: queueLock, Now: func() time.Time { return now.Add(time.Second) }, Attempts: 1, + } + var logs bytes.Buffer + response := httptest.NewRecorder() + Handler{Store: store, QueueCorrelator: correlator, Logger: slog.New(slog.NewJSONHandler(&logs, nil))}. + ServeHTTP(response, httptest.NewRequest(http.MethodPost, "https://x"+ClaimPath, bytes.NewReader(body))) + if response.Code != http.StatusForbidden { + t.Fatalf("a foreign repository was authorized: status=%d body=%s", response.Code, response.Body.String()) + } + journal, err := store.Read(context.Background()) + if err != nil { + t.Fatal(err) + } + if journal.Claims["warm-standard-foreign"].ClaimedRepository != "" { + t.Fatal("a refused warm claim consumed the one-job token") + } + // The refusal used to log nothing at all, which made the one failure that + // costs every warm runner its cache the only one that could not be read. + if !strings.Contains(logs.String(), `"msg":"warm runner correlation refused"`) || + !strings.Contains(logs.String(), `"github.repository":"other-org/other-repo"`) || + !strings.Contains(logs.String(), `"github.job_name":"analyze"`) { + t.Fatalf("refusal is not diagnosable from its own log: %s", logs.String()) + } +} diff --git a/internal/cachebroker/handler.go b/internal/cachebroker/handler.go index b9b09e59..3da5d165 100644 --- a/internal/cachebroker/handler.go +++ b/internal/cachebroker/handler.go @@ -142,7 +142,6 @@ func (h Handler) ServeHTTP(writer http.ResponseWriter, request *http.Request) { deny(logger, writer, request, http.StatusForbidden, "claim refused") return } - correlationBound := false var correlation queueintent.RunningCorrelation if claimRequest.WorkflowRunID > 0 { correlation = queueintent.RunningCorrelation{ @@ -153,32 +152,47 @@ func (h Handler) ServeHTTP(writer http.ResponseWriter, request *http.Request) { } // A promoted warm container intentionally keeps its provider instance name // while the one-job JIT runner receives GitHub's runner name. The claim token - // proves the former; an exact active queue correlation proves that the latter - // is the runner executing this repository job. Never weaken this to accepting - // two arbitrary names or to a timing/job-name heuristic. + // proves the former; an active queue intent for this exact repository and + // workflow run proves that the latter is the runner executing this + // repository's job. Never weaken this to accepting two arbitrary names or to + // a timing heuristic. + // + // It used to require BindRunning, which must resolve one intent because it + // writes to it, and which picks between a run's sibling jobs by comparing + // the journal's display name against GITHUB_JOB. Those disagree for every + // matrix job, so a correct warm runner was refused its cache and built from + // cold: 19 refusals against 7 binds in an hour on the live fleet. The + // credential is repository-scoped, so the run is the right key and the job + // name was never load-bearing for authorization. The exact intent is still + // bound below, and retried asynchronously once GitHub reports the job + // started. if warmRuntimeIdentity { if claimRequest.WorkflowRunID <= 0 || h.QueueCorrelator == nil { deny(logger, writer, request, http.StatusForbidden, "warm runner correlation required") return } - result, bindErr := h.QueueCorrelator.BindRunning(ctx, correlation) - if bindErr != nil { - logger.WarnContext(ctx, "warm runner correlation refused") + if authorizeErr := h.QueueCorrelator.AuthorizeRunning(ctx, correlation); authorizeErr != nil { + logger.WarnContext(ctx, "warm runner correlation refused", + telemetryattrs.InstanceName, claimRequest.InstanceName, + telemetryattrs.RunnerName, claimRequest.RunnerName, + telemetryattrs.GitHubRepository, claimRequest.Repository, + telemetryattrs.GitHubWorkflowRunID, claimRequest.WorkflowRunID, + telemetryattrs.GitHubJobName, claimRequest.JobName, + "pool", claim.PoolName, + "error", authorizeErr) deny(logger, writer, request, http.StatusForbidden, "warm runner correlation refused") return } - correlationBound = true - logger.InfoContext(ctx, "warm runner correlation bound", - "journal_generation", result.Generation, - "changed", result.Changed) + logger.InfoContext(ctx, "warm runner correlation authorized", + telemetryattrs.InstanceName, claimRequest.InstanceName, + telemetryattrs.RunnerName, claimRequest.RunnerName, + telemetryattrs.GitHubRepository, claimRequest.Repository, + telemetryattrs.GitHubWorkflowRunID, claimRequest.WorkflowRunID, + "pool", claim.PoolName) } if claimRequest.WorkflowRunID > 0 { if h.QueueCorrelator != nil { - var result queueintent.CorrelationResult - var bindErr error - if !correlationBound { - result, bindErr = h.QueueCorrelator.BindRunning(ctx, correlation) - } + result, bindErr := h.QueueCorrelator.BindRunning(ctx, correlation) if bindErr != nil { logger.WarnContext(ctx, "queue running correlation deferred", telemetryattrs.InstanceName, claimRequest.InstanceName, @@ -188,7 +202,7 @@ func (h Handler) ServeHTTP(writer http.ResponseWriter, request *http.Request) { if errors.Is(bindErr, queueintent.ErrRunningCorrelationNotReady) { h.retryQueueCorrelation(correlation, logger) } - } else if !correlationBound { + } else { logger.InfoContext(ctx, "queue running correlation bound", telemetryattrs.InstanceName, claimRequest.InstanceName, telemetryattrs.GitHubRepository, claimRequest.Repository, diff --git a/internal/queueintent/correlation_linux.go b/internal/queueintent/correlation_linux.go index fbe601c5..c7050425 100644 --- a/internal/queueintent/correlation_linux.go +++ b/internal/queueintent/correlation_linux.go @@ -50,6 +50,68 @@ func (c Correlator) Ready(ctx context.Context) error { return err } +// AuthorizeRunning proves that this scale set currently holds active work for +// the exact repository and workflow run the runner presents, without demanding +// the one intent that job will eventually occupy. +// +// BindRunning has to identify a single intent, because it writes to it. To pick +// one out of a run's sibling jobs it compares the journal's job display name -- +// what GitHub puts in the scale-set message -- against GITHUB_JOB, which is the +// job id from the workflow file. Those two agree for a bare job and for a +// reusable-workflow prefix, and disagree for every matrix job: `Analyze +// (python)` is not `analyze`. Measured over one hour on the live fleet, 19 warm +// claims were refused and 7 bound. +// +// Authorization does not need that. The credential a claim yields is scoped to +// a repository, so the question is whether this warm container is serving that +// repository's run -- and (scale set, owner, repository, workflow run id) is a +// strictly tighter answer than any job-name comparison. Siblings of the same +// run are interchangeable for this purpose: they carry the same repository and +// the same trust role. Nothing here mutates the journal; the exact intent is +// still bound by BindRunning and its asynchronous retry once GitHub reports the +// job started. +func (c Correlator) AuthorizeRunning(ctx context.Context, correlation RunningCorrelation) error { + if err := validateRunningCorrelation(correlation); err != nil { + return err + } + if err := ctx.Err(); err != nil { + return err + } + if !filepath.IsAbs(c.Path) { + return errors.New("queue journal must be an absolute path") + } + journal, err := readJournal(c.Path) + if err != nil { + return err + } + now := time.Now().UTC() + if c.Now != nil { + now = c.Now().UTC() + } + owner := strings.SplitN(correlation.Repository, "/", 2)[0] + for _, intent := range journal.Intents { + if !intent.ExpiresAt.After(now) || intent.ScaleSetName != correlation.PoolName { + continue + } + switch intent.State { + case StateAssigned, StateAcquiring, StateAcquired, StateRunning: + default: + continue + } + if intent.OwnerAccount() != owner || (intent.Repository != owner && intent.Repository != correlation.Repository) { + continue + } + // An intent admitted before JobAvailable carries no run id yet, and + // BindRunning already treats that as compatible. Refusing it here would + // deny a warm runner exactly the head-of-queue case warm capacity serves. + if intent.WorkflowRunID != 0 && intent.WorkflowRunID != correlation.WorkflowRunID { + continue + } + return nil + } + return ErrRunningCorrelationNotReady +} + func (c Correlator) BindRunning(ctx context.Context, correlation RunningCorrelation) (CorrelationResult, error) { if err := validateRunningCorrelation(correlation); err != nil { return CorrelationResult{}, err From fcba264fd970dab50088ee66ea5feae6a2cacacb Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Mon, 31 Aug 2026 08:10:47 +0500 Subject: [PATCH 2/3] chore(provider): release v0.1.5-nddev.95 The cache broker takes its version from this manifest -- CONTROLLER_VERSION reads derivative_version and stamps gha-fleet, gha-fleet-observer and gha-cache-broker with it -- so a broker behaviour change that ships under the previous version leaves the live identity unable to say which code is running. That is the divergence this manifest exists to prevent. Two local rebuilds with the workflow's own flags agree. --- config/example-runner-1.yaml | 2 +- config/example-runner-2.yaml | 2 +- config/example-runner-3.yaml | 2 +- config/example-runner-4.yaml | 2 +- config/example-services.yaml | 2 +- config/provider-derivative.yaml | 6 +++--- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/config/example-runner-1.yaml b/config/example-runner-1.yaml index 751251f4..45de816c 100644 --- a/config/example-runner-1.yaml +++ b/config/example-runner-1.yaml @@ -9,7 +9,7 @@ control_plane: manager_version: v0.2.1-nddev.86 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.94 + provider_version: v0.1.5-nddev.95 provider_interface: v0.1.0 worker_kind: incus-container runner: actions/runner diff --git a/config/example-runner-2.yaml b/config/example-runner-2.yaml index 0dfc6385..d6dfb2cc 100644 --- a/config/example-runner-2.yaml +++ b/config/example-runner-2.yaml @@ -9,7 +9,7 @@ control_plane: manager_version: v0.2.1-nddev.86 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.94 + provider_version: v0.1.5-nddev.95 provider_interface: v0.1.0 worker_kind: incus-container runner: actions/runner diff --git a/config/example-runner-3.yaml b/config/example-runner-3.yaml index 32cad5fa..8c5bc4df 100644 --- a/config/example-runner-3.yaml +++ b/config/example-runner-3.yaml @@ -9,7 +9,7 @@ control_plane: manager_version: v0.2.1-nddev.86 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.94 + provider_version: v0.1.5-nddev.95 provider_interface: v0.1.0 worker_kind: incus-container runner: actions/runner diff --git a/config/example-runner-4.yaml b/config/example-runner-4.yaml index a46e4356..599e22f4 100644 --- a/config/example-runner-4.yaml +++ b/config/example-runner-4.yaml @@ -9,7 +9,7 @@ control_plane: manager_version: v0.2.1-nddev.86 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.94 + provider_version: v0.1.5-nddev.95 provider_interface: v0.1.0 worker_kind: incus-container runner: actions/runner diff --git a/config/example-services.yaml b/config/example-services.yaml index a1928cd8..1592509f 100644 --- a/config/example-services.yaml +++ b/config/example-services.yaml @@ -27,7 +27,7 @@ control_plane: manager_version: v0.2.1-nddev.86 scheduling_mode: scale-set provider: incus - provider_version: v0.1.5-nddev.94 + provider_version: v0.1.5-nddev.95 provider_interface: v0.1.0 worker_kind: incus-container runner: actions/runner diff --git a/config/provider-derivative.yaml b/config/provider-derivative.yaml index eab4ed28..3973e7a8 100644 --- a/config/provider-derivative.yaml +++ b/config/provider-derivative.yaml @@ -16,7 +16,7 @@ artifact: garm-provider-incus # state all move together, because all three derive from here. A provider change # that does not bump it ships under the previous version, which is exactly how # runner-1 and runner-2 diverged. -derivative_version: v0.1.5-nddev.94 +derivative_version: v0.1.5-nddev.95 # The external-provider protocol GARM speaks to this binary. It moves on its own # schedule -- a provider release does not imply an interface release -- so it is @@ -37,8 +37,8 @@ runtime: queue_intent_schema_version: 5 build: - source_commit: 41fc16f7bc113f0703c480520665da521d94d578 - binary_sha256: b06a47af5656042b7bfc3491e6a349b83f860f5bd500d1e3647d3958fd0ace3d + source_commit: 541be597f47eff1bedb9c31383c8f6ba1cf868bd + binary_sha256: f7a76eaf5468c67a756a2113285524d8c6a226176d07538a6a9af4a7fab60a63 go_version: go1.26.6 cgo_enabled: false target_os: linux From 913ff5392c9eeaa1bc380c07671bb6ecc09f546b Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Mon, 31 Aug 2026 08:29:18 +0500 Subject: [PATCH 3/3] fix(cache): strip line breaks where the refusal is logged CodeQL flagged the new refusal log as built from user input, and it is right that the guarantee was not visible there. validateJobCorrelation already refuses CR, LF and NUL through boundedText, so nothing reaching this handler can forge a log line today -- but that invariant lives a hundred lines away in a handler that has grown, and neither a reader nor the analyser can see it from the call. logText makes it local. An ordinary value passes through untouched. --- internal/cachebroker/cachebroker_test.go | 10 +++++++ internal/cachebroker/handler.go | 33 +++++++++++++++++------- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/internal/cachebroker/cachebroker_test.go b/internal/cachebroker/cachebroker_test.go index ec3c2bb4..4b901294 100644 --- a/internal/cachebroker/cachebroker_test.go +++ b/internal/cachebroker/cachebroker_test.go @@ -621,3 +621,13 @@ func TestWarmClaimForAnotherRepositoryIsStillRefused(t *testing.T) { t.Fatalf("refusal is not diagnosable from its own log: %s", logs.String()) } } + +func TestLogTextCannotForgeALogLine(t *testing.T) { + forged := "repo\n{\"level\":\"INFO\",\"msg\":\"cache claim delivered\"}" + if got := logText(forged); strings.ContainsAny(got, "\r\n\x00") { + t.Fatalf("logText left a line break in %q", got) + } + if logText("example-org/example-repo") != "example-org/example-repo" { + t.Fatal("logText altered an ordinary value") + } +} diff --git a/internal/cachebroker/handler.go b/internal/cachebroker/handler.go index 3da5d165..db0d7c01 100644 --- a/internal/cachebroker/handler.go +++ b/internal/cachebroker/handler.go @@ -173,22 +173,22 @@ func (h Handler) ServeHTTP(writer http.ResponseWriter, request *http.Request) { } if authorizeErr := h.QueueCorrelator.AuthorizeRunning(ctx, correlation); authorizeErr != nil { logger.WarnContext(ctx, "warm runner correlation refused", - telemetryattrs.InstanceName, claimRequest.InstanceName, - telemetryattrs.RunnerName, claimRequest.RunnerName, - telemetryattrs.GitHubRepository, claimRequest.Repository, + telemetryattrs.InstanceName, logText(claimRequest.InstanceName), + telemetryattrs.RunnerName, logText(claimRequest.RunnerName), + telemetryattrs.GitHubRepository, logText(claimRequest.Repository), telemetryattrs.GitHubWorkflowRunID, claimRequest.WorkflowRunID, - telemetryattrs.GitHubJobName, claimRequest.JobName, - "pool", claim.PoolName, + telemetryattrs.GitHubJobName, logText(claimRequest.JobName), + "pool", logText(claim.PoolName), "error", authorizeErr) deny(logger, writer, request, http.StatusForbidden, "warm runner correlation refused") return } logger.InfoContext(ctx, "warm runner correlation authorized", - telemetryattrs.InstanceName, claimRequest.InstanceName, - telemetryattrs.RunnerName, claimRequest.RunnerName, - telemetryattrs.GitHubRepository, claimRequest.Repository, + telemetryattrs.InstanceName, logText(claimRequest.InstanceName), + telemetryattrs.RunnerName, logText(claimRequest.RunnerName), + telemetryattrs.GitHubRepository, logText(claimRequest.Repository), telemetryattrs.GitHubWorkflowRunID, claimRequest.WorkflowRunID, - "pool", claim.PoolName) + "pool", logText(claim.PoolName)) } if claimRequest.WorkflowRunID > 0 { if h.QueueCorrelator != nil { @@ -323,6 +323,21 @@ func validateJobCorrelation(request ClaimRequest) error { return nil } +// logText makes the CR/LF/NUL guarantee local to the log call. +// +// validateJobCorrelation already refuses those characters through boundedText, +// so nothing reaching here can forge a log line today. That is an invariant +// held a hundred lines away in a handler that has grown, and a reader of the +// log call cannot see it; CodeQL cannot either, and flagged the refusal path +// for exactly that reason. Stripping at the boundary costs nothing and keeps +// the property true if the validator is ever relaxed. +func logText(value string) string { + if !strings.ContainsAny(value, "\r\n\x00") { + return value + } + return strings.NewReplacer("\r", "", "\n", "", "\x00", "").Replace(value) +} + func boundedText(value string) bool { return value != "" && len(value) <= 1024 && strings.TrimSpace(value) == value && !strings.ContainsAny(value, "\r\n\x00")