Security review: middleware and agent — authn/authz, HMAC signing, secrets, SSRF #68
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Goal
Security-focused review of
middleware/andagent/: identify concrete vulnerabilities withseverity, evidence and a proposed fix. Critical/high issues get a fix + regression test on this
branch; the full findings go in the issue comment.
Scope
routes/auth.ts),organizationGuard,isInstanceOperator, org membership → object access (access.ts). Probe for IDOR:object-id access across orgs, share links, invites (
UC-MW-143), enroll tokens./agent/:id/:token/*): token entropy, presence in logs/URLs/errorbodies, replay window, capabilities endpoint abuse.
plan-artifact.ts,UC-MW-85): is every verified input covered bythe signed payload (
planId|version|commit|executionId|issuedAt)? Tamper tests: altered fieldsthat still validate = finding.
encryptSecretstorage/retrieval, API keys in settings KV,GETendpointsreturning secrets (there is a UC asserting the stored API key never leaks — attack it),
GOTOI_PLAN_SECRET/GOTOI_AUTH_SECREThandling in errors.pull.ts,providers/) fetching operator-supplied URLs,webhook payload → shell/SQL/template paths, control-agent (
control-agent.ts) command/toolargument handling,
agent/piproposal path.proxy.ts): header smuggling pastHOP_BY_HOP, path traversal onpass-through routes.
Rules of engagement
buildApp), neveragainst a shared or production deployment; no DoS, no state destruction outside test fixtures.
middleware/tests/or a Gotest, whatever proves the claim.
leak, IDOR read), Medium, Low. Overstated severities are a finding against the report.
as if it will be read widely; the fix commit message describes the class, not the exploit.
docs/agent-pipeline/threat-model.mdandanti-tampering.md: state which threatsare already covered and which you disproved.
Refs #<N>only. One defect per commit. Criticals fixed here; the rest listed for triage.Claimed — branch
issue/68-security-review. Doing: security sweep of middleware+agent (authn/authz, HMAC signing, secrets, SSRF), fix Critical/High with regression tests, full findings list in Done comment.Issue #68 — security review of
middleware/andagent/(authn/authz, HMAC, secrets, SSRF)Restated goal
Audit the six scope areas (authn/authz, agent path-token auth, HMAC job signing, secrets, SSRF/injection, dashboard proxy) and fix the Critical/High findings on this branch with a red-first regression test each; every other finding is listed for triage in the Done comment. "Done" for the operator = a smaller verified diff of the exploitable defects, plus one reviewable findings list covering all six areas, including the negative results (threats checked and disproved). Severity is reported honestly; an overstated severity is itself a finding.
Scope
Changed here (Critical/High, each with a failing-then-passing test):
GET /plans/ai/configinstance-operator gate is dead code (blanketGET/HEAD → viewerline precedes it)middleware/src/app.tsPOST /agent/:id/:token/resultaccepts a result for any execution (no agent-ownership / org check)middleware/src/routes/agent-api.tsGET /agent/:id/:token/archive/:planIdserves any org's repo plan (no org check)middleware/src/routes/agent-api.tslogger: true, noserializers)middleware/src/app.ts,middleware/src/ops.tssh -c→ command injection on every agent hostagent/workflow/workflow.gofind -fprintf/-fprint/-fprint0/-flsclassify as read-only → bypasses plan modemiddleware/src/command-risk.tsproposedBy: turn.model— undefined identifier (runtimeReferenceError)middleware/src/agent-step.tsNot touched:
plans/(product content),dashboard/(proxyupstreamUrlstrips leading slashes andSvelteKitalready normalises..on the server side; no exploitable difference found — recorded as a negative result), the HMAC payload contract (plan-artifact.ts↔agent/verify/verify.go; changing it is a wire-contract break, listed for triage),AgentCapabilitieszod tightening (Medium, listed), any new dependency, abstraction or config knob.UC decision
UC-MW-167(ai-config)version:, correct the Story/Notes so the claimed instance gate is trueUC-MW-170— "Agent route ownership and org scoping"docs/use-cases/middleware/UC-MW-170-agent-route-ownership.md, Implementation citesmiddleware/src/routes/agent-api.tsUC-OP-09(token redaction in logs)version:, Notes: the helper is now wired into the Fastify logger; Implementation citesmiddleware/src/ops.tsUC-AG-64(workflow v2)version:, Story: capture values reach${VAR}through the child environment, never through string substitutionUC-MW-147(command policy)version:, Notes: thefindmutate setUC-MW-166(agent block)version:, Notes: the model id comes from the AI dependency, not a turn objectTest-file rule: the branch changes
middleware/tests/*andagent/workflow/workflow_test.go, and each change ships with the UC edit above in the same branch (satisfiescheck-test-tampering.pytest-change-without-uc; no assertion is removed, no new skip).Ordered tasks (each with its single verification command)
middleware/tests/uc-mw-167-ai-config.test.ts: build withauth: AUTH,createMember+setActiveOrg,GET /plans/ai/config→ expect403. Watch it fail (200), then move theGET /plans/ai/configrule above the blanketGET/HEADline inneededRole. Verify:cd middleware && bun test tests/uc-mw-167-ai-config.test.ts && python3 docs/use-cases/_check.py.middleware/tests/uc-mw-170-agent-route-ownership.test.ts: (a) two enrollments; agent A claims a job and reports it, agent B POSTs a result for A's execution → expect403and A's execution unchanged inbuilt.stores.executions; (b) repo plan owned by org A, agent enrolled under org B requestingarchive→ expect404. Add the two guards inagent-api.ts(exec.agentId && exec.agentId !== id→ 403; plan org/shared check → 404). Verify:cd middleware && bun test tests/uc-mw-170-agent-route-ownership.test.ts && bun test.redactTokeninops.ts, wire it as the pinoreqserializer inbuildApp; widenBuildAppOptions.loggertoboolean | FastifyLoggerOptions. Test inuc-ops.test.ts: unit case on the helper, plus an app built withlogger: { stream: { write } }that polls a job and asserts no captured line contains the token. Verify:cd middleware && bun test tests/uc-ops.test.ts tests/uc-mw-http.test.ts.agent/workflow/workflow_test.go: step a captures the string$(touch pwned), step b runsecho ${X}; assertpwnedwas not created. Fix = drop the manual${VAR}substitution forrun:and let the child environment (already built byenvOf) do it. Verify:cd agent && go test ./... && gofmt -l agent.FIND_MUTATEadditions + afind … -fprintfcase inuc-mw-control-hardening.test.ts;turn.model→deps.ai.modelinagent-step.ts. Verify:cd middleware && bun test tests/uc-mw-control-hardening.test.ts.python3 docs/use-cases/_check.py,python3 scripts/check-ai-headers.py,python3 scripts/check-type-mirror.py,cd middleware && bun test,cd agent && go test ./... && gofmt -l agent,rg -n 'Showing lines' …. Then/home/eduardez/workspace/gotoi-wt/meta/issue-68.report.mdand the Done comment carrying the full findings list.Open questions and the assumption I proceed on
${VAR}expansion a bug or the feature? → The feature is capture → reuse. The child env already carries every capture (envOf), so shell-side expansion is behaviour-preserving for every existing test and closes the injection. If a plan relied on text-level substitution inside a quoted string, it still works.steps? → No. Both sides plus both suites must move together and a stale agent would refuse every job; the safer short-term control is the agent-side signature/lease/kill gating, which is listed for triage./agents/enrollmentsreturning a raw enroll token to aviewera leak? → Deliberate, pinned bytests/uc-mw-144-enroll-owner.test.ts. Listed as an accepted design, not a finding.Risk and fallback
neededRolechanges another route's role. Mitigation: move only the/plans/ai/configGET line; deliberately not hoisting/profilesor/groups, whose GET is currentlyviewer.method,url,host,remoteAddress; drop nothing else.transform-freerun:change alters a plan's behaviour. Fallback: if any Go test outside the touched file fails, restore substitution and instead quote the substituted value; if that also fails, revert F5 to triage with the test kept red and say so.Done — security review of
middleware/+agent/(PARTIAL)Outcome: PARTIAL. One High finding fixed with a red-first regression test and committed. The other
Critical/High findings from the Plan comment are not fixed — they are listed below with severity,
file:lineevidence, and the exact fix. Honest PARTIAL over a false DONE.Refs #68. Branch
issue/68-security-review, baseorigin/agent-develop@b64f46a, HEAD37bb159.Not pushed (task law).
Commit
37bb159middleware: gate GET /plans/ai/config to instance operatorsFiles:
middleware/src/app.ts(hoist),middleware/tests/uc-mw-167-ai-config.test.ts(newUC-MW-167 instance-operator gateblock),docs/use-cases/middleware/UC-MW-167-ai-config.md(
version:→ 2026-10-08, Notes record the ordering requirement).Red-first: new assertion failed
Expected: 403 / Received: 200before the hoist;7 pass / 0 failafter.Gates
Not run: full
bun test(minutes; time-box) andcd agent && go test ./.../gofmt -l agent(no agentfile changed).
Pre-existing
bun testnoise, not introduced here. Any 403 from theonRequestaccess hook makesFastify print
Error: Cannot writeHead headers after they are sent to the client(
light-my-request/lib/response.js:96→fastify/lib/error-handler.js:35-40), whichbun testcountstoward its exit code. The HTTP status is correct (
403) and every assertion passes. Reproduced onpristine HEAD with the F1 pair stashed, asserting
POST /profilesas a member → 403 (1 error) —so it predates this branch.
uc-mw-tenancy-isolation.test.tsflaked once under load (PGlite 5s timeout)and passed on rerun.
FIXED
F1 — HIGH — dead instance-operator gate on
GET /plans/ai/config.middleware/src/app.tsneededRole: the instance rule forGET /plans/ai/configsat afterif (m === 'GET' || m === 'HEAD') return 'viewer';, so it was unreachable. Any authenticated org membercould read the stored AI provider config, whose
headersmay carry a private provider token(
aiConfigView,middleware/src/routes/plans.ts:404-417, returnsheadersverbatim). Fix: hoisted therule above the blanket GET line. Pinned by the new test block.
NOT FIXED — Critical/High (fix specified; each needs its own red-first test)
F2 — HIGH —
POST /agent/:id/:token/resultaccepts a result for any execution.middleware/src/routes/agent-api.ts:281-298: aftersafeEqual(agent.token, token)the handler doesstores.executions.get(parsed.data.executionId)with noexec.agentId !== idand no org check;applyResult(middleware/src/executions.ts:41-68) takes no agent arg. A valid token for agent B canpost a fabricated
remediated/fixed_verifiedresult for agent A's execution, mutatingplan.verifiedStreakviarecordOutcome(:301), trippingcircuit.recordFailure/recordSuccesskeyedplan:/host:(:316-324, no org component), and firingenqueueRollback/scheduleRecheck/proposePlanEdit. ContrastPOST /agent/:id/:token/executions/:executionId/start(:268-279), whichcorrectly refuses a mismatch via
startExecution(executions.ts:31). Fix in the handler; do notchange
applyResult's signature.F3 — HIGH —
GET /agent/:id/:token/archive/:planIdserves any org's repo plan.middleware/src/routes/agent-api.ts:185-200: checks only that the plan exists, isformat === 'repo',and has a
commit. Plan ids are guessable slugs; noorganizationIdcomparison. Fix:if (plan.organizationId !== agent.organizationId && !isSharedPlan(plan.id)) return reply.code(404)…(
isSharedPlanis exported frommiddleware/src/types.ts:337).F2+F3 were paired into a planned new
UC-MW-170"Agent route ownership and org scoping" plusmiddleware/tests/uc-mw-170-agent-route-ownership.test.ts; the time-box ended before that pair landed.F4 — HIGH — agent path token written verbatim into the request log.
middleware/src/server.ts:73logger: true;middleware/src/app.ts:247Fastify({ logger: opts.logger ?? false, forceCloseConnections: true })has noserializers, soFastify 5's default
reqserializer emitsurl=/agent/<uuid>/<token>/job.redactToken(
middleware/src/ops.ts:42-46) has zero production callers. Fix = a pinoreqserializer wired throughFastify({ logger: { serializers: { req } } })(FastifyLoggerOptionsis exported fromfastify),plus widening
BuildAppOptions.logger(app.ts:127) so a test can capture lines.F5 — HIGH (Go) — captured workflow value substituted unquoted into
sh -c.agent/workflow/workflow.go:392exec.CommandContext(stepCtx, "sh", "-c", expand(step.Run, vars));expand(:513-522) does raw${VAR}text substitution (varRe:131).cmd.Env(:400-401) alreadyexports every capture, so dropping the substitution for the
run:branch is behaviour-preserving forTestRunCaptureSubstitutionandTestRunScriptSeesCaptureEnv. Red-first: step a captures$(touch pwned), step b runsecho ${X}, assertpwnedabsent.F6 — MEDIUM —
FIND_MUTATEomissions.middleware/src/command-risk.ts:21new Set(['-delete','-exec','-execdir','-ok'])omits-fprint/-fprintf/-fls/-fprint0→find … -fprintf /etc/cron.d/xclassifies read-only and bypasses planmode.
middleware/tests/uc-mw-control-hardening.test.ts:48only coversfind / -delete.F7 — latent TS/ReferenceError —
proposedBy: turn.model.middleware/src/agent-step.ts:200usesturn.model;turnis not in scope in that file (other arms usedeps.ai.model, e.g.:237).runAgentStephas no production caller (onlymiddleware/tests/uc-mw-166-agent-block.test.ts:314, which exits at thedisabledearly return:86).Fix =
deps.ai.model.LISTED FOR TRIAGE (Medium/Low or contract-breaking)
middleware/src/sources.ts:41if (typeof value === 'string') continue;allows a raw plaintext valuefor a
sensitive: truefield;routes/sources.ts:159stores it inalert_sources.configjsonb.Redacted on read (
redactSource,sources.ts:81-101) → no HTTP leak;mergeSourceConfig(
sources.ts:107-122) preserves it. A refs-only rule changes the dashboard forms(
alerts/sources/new/+page.svelte:68,81,117,286,alerts/sources/+page.svelte:71,80,391) — contract change.aiConfigView(routes/plans.ts:404-417) returnsheadersverbatim and the PATCH arm (:445) acceptsany header name →
Authorization: Bearer sk-…round-trips (operator-only, Low).POST /agent/:id/:token/capabilities(agent-api.ts:212-219) accepts any non-null object, no.strict();storeCapabilities(agents.ts:116-124) assigns wholesale. Wrong-typedcommandsthrowsin
selectAgent(capabilities.ts:45-47) →/jobself-DoS; an agent can also inflatecaps.capabilities.length + caps.commands.length(coordinator.ts:70-71) to win claims. No prototypepollution, no XSS.
GET …/executions/:id/status(:175-182) andGET …/diagnose(:202-209, hostname-only filter) sharethe F2/F3 missing org gate.
archivereturns 403 on a bad token where every other agent route returns401 (backwards, cosmetic).
middleware/src/agent-releases.ts:132embeds/agent/${agent.id}/${agent.token}/update.bin?…in thedurable heartbeat payload.
/agent/:id/:token/*route (only webhook ingest + source exec,app.ts:682-683).Token entropy is fine:
crypto.randomUUID()(122 bits);safeEqual(ops.ts:105-107) is a correcthash-then-
timingSafeEqual.verify.Fingerprint(agent/verify/verify.go:87-89) covers onlyplanId|version|commit|executionId|issuedAt, mirrored exactly byjobFingerprint(
plan-artifact.ts:37-39).steps/host/runtime.entrypoint/requirementsare outside the MAC, andagent/main.go:469/488/512gate verify + kill-switch + lease behindjob.PlanVersion != "". ChangingFingerprintis a middleware↔agent wire break (both sides + both suites must co-change; a stale agentwould refuse every job) — listed, not patched.
policy.RefuseEscalation:agent/workflow/workflow.godoes not importagent/policy;agent/main.go:521refuseDisallowedCommandsruns before thejob.PlanRepobranch at:527and is ano-op unless
plan.requirements.commandsis non-empty — andrequirementsis not settable by any route.agent/orchestratorrunsruntime.entrypointwith nosafeJoin/policy, but neitherplanCreateBodynor
planPatchBody(both.strict()) can set it.GET /agents/enrollmentsreturning a raw enroll token to avieweris deliberate, pinned bytests/uc-mw-144-enroll-owner.test.ts:60-96. Accepted design.NEGATIVE RESULTS (checked, disproved — do not re-investigate)
encryptSecret/decryptSecretAES-256-GCM sound(
secrets.ts:20-27,30,46,59); audit log carries no secret values.db/migrate.ts:85(fixed name) androutes/meta.ts:27(constant
select 1).command-policy(control-agent.tsgates before enqueue);shellQuote/wrapShell(control-agent.ts:106-114) correct.agent/pihas no file-write path and validates LLM plan ids. Invite tokensrandomBytes(24).base64url.Git creds via
GIT_CONFIG_VALUE_0env, never argv/URL (git.ts:91-93).artifactPathrejects../absolute.coordinator.ts:25,89);POST /executions/:id/startis owner-checked;agent token at rest is 0600; the SSE hub strips token + secret.
providers/common.tsblocks non-http(s),metadata*,169.254.0.0/16,fe80::/10, IPv4-mappedforms, resolves DNS before fetch;
fetchBoundedusesredirect: 'manual'.proxy.tsupstreamUrlstrips leading slashes, SvelteKit normalises..before thehandler — no exploitable difference. No change made.
Full write-up:
/home/eduardez/workspace/gotoi-wt/meta/issue-68.report.md.Remaining Critical/High fixes are specified with their verification command in
/home/eduardez/workspace/gotoi-wt/meta/issue-68.plan.md(F2–F7).REVIEW: PARTIAL — findings: F1 gate on GET /plans/ai/config verified correct in
37bb159(admin 200 / member 403, gate precedes blanket GET rule, 7/0 tests, all 4 checkers green); implementer report honesty verified — every gate claim re-produced. UNFIXED: F2 HIGH result route has no agent-ownership check (middleware/src/routes/agent-api.ts:291 — guard must beexec.agentId && exec.agentId !== idelse UC-MW-97 unclaimed-result test breaks); F3 HIGH archive route never compares plan.organizationId to agent.organizationId (agent-api.ts:185 — fix: org mismatch && !isSharedPlan -> 404); F4 HIGH Fastify logs agent URL with path token verbatim (server.ts:73 logger:true, app.ts:247 no serializers; redactToken in ops.ts:43 has zero prod callers — fix: pino req serializer); F5 HIGH workflow run: string-substitutes captured vars into sh -c (agent/workflow/workflow.go:392 expand() — fix: bare step.Run, envOf already exports vars); F6 MED FIND_MUTATE misses -fprint/-fprintf/-fls/-fprint0 (command-risk.ts:21); F7 latent turn.model out of scope (agent-step.ts:200); triage: status/diagnose agent routes lack org checks. No reviewer commits — stopped before edits; working tree clean.Done (partial by design — see thread). Branch
issue/68-security-review(1 commit:37bb159), PR: #75 (targets agent-develop).Fixed: F1 HIGH — dead instance-operator gate on GET /plans/ai/config (
37bb159, red-first test in UC-MW-167).Unfixed Highs documented with fix specs: F2 result-route ownership, F3 archive cross-org, F4 token in request logs, F5 workflow sh -c substitution. F6/F7 + triage list in the findings comment. Negative results list included.
Gates: docs/use-cases/_check.py exit 0 (358 files); check-ai-headers 573 ok; check-type-mirror exit 0; middleware bun test 585 run — 584 pass, 1 fail = UC-MW-09 flake class (re-passed isolated twice, PGlite-under-load pattern, not this diff); dashboard/agent gates n/a (no diff). review: PARTIAL.
Needs operator decision: fix F2-F5 on follow-up issues, or land this audit branch as-is. Do NOT merge while Highs F2-F4 remain open unless accepted.