Files
multica/server/internal/handler/agent_env.go
Bohan Jiang 13f74e651a feat(agents): remove custom_env from agent resources, add audited env endpoint (MUL-2600) (#3209)
* feat(agents): remove custom_env from agent resources, add audited env endpoint (MUL-2600)

The agent resource shape (list / get / create / update / archive /
restore responses + WebSocket events) no longer carries `custom_env`
values. Reads/writes of env now flow exclusively through a dedicated
`/api/agents/{id}/env` endpoint that is owner/admin-only, rejects
agent-actor sessions, applies a "****" sentinel preserve guard on
PUT, and writes a persistent audit row per reveal/update.

Why
- `multica agent list --output json` historically returned plaintext
  `custom_env` for owner/admin callers (the redaction gate gave only
  members the masked map). Any agent token running on the workspace
  inherits its owner's role and could read every other agent's
  secrets just by listing.
- Patching list/get redaction alone (PR #3175 direction) left
  symmetric leaks via mutation responses, WS events, the "reveal"
  path itself (no actor-aware auth), and a `****` overwrite footgun
  on UpdateAgent.

What changed
- Backend: drop `custom_env` from AgentResponse; add coarse
  `has_custom_env` + `custom_env_key_count`. Strip env handling from
  UpdateAgent (silently ignored if sent). Keep CreateAgent's
  custom_env acceptance.
- Backend: new GET/PUT `/api/agents/{id}/env` handlers in
  `internal/handler/agent_env.go`:
  - resolveActor → 403 for agent actors (closes the lateral-movement
    path).
  - Owner/admin role gate via existing helper.
  - PUT honours value == "****" as "preserve existing value".
  - Both write to `activity_log` with `agent_env_revealed` /
    `agent_env_updated` actions. Audit details record key names only,
    never values.
- Daemon claim path (`ClaimAgentTask`) unchanged — `TaskAgentData`
  still carries plaintext env for runtime injection.
- SQL: new `UpdateAgentCustomEnv` query; sqlc regenerated (v1.31.1).
- CLI: new `multica agent env get|set` subcommands. `--custom-env*`
  flags removed from `multica agent update`; the no-fields error
  now points to the new path.
- Frontend: drop env fields from `Agent` + `UpdateAgentRequest`; add
  `getAgentEnv` / `updateAgentEnv` client methods; rewrite env-tab
  to show "N variables configured" + explicit "Reveal & edit"
  button, fetching values only on intentional reveal.
- Locales: parity-safe additions to en + zh-Hans.
- Docs: agents-create.{mdx,zh.mdx} reflect the new threat model and
  endpoint.
- Mobile: schema drops `custom_env` / `custom_env_redacted`, adds
  metadata fields.

Tests
- Handler tests pinned the new invariants: no env in list/get
  responses, owner reveal happy-path + audit row, agent-actor 403,
  `****` sentinel preserves real values, UpdateAgent silently
  ignores `custom_env`, pure `mergeAgentEnv` cases.
- CLI tests pivot to the new flag surface: `agent update` MUST NOT
  expose the env flags; `agent env set` MUST expose
  --custom-env-stdin/--custom-env-file.
- Frontend test fixtures updated; pnpm typecheck / test / lint
  pass cleanly.

This is a breaking API change. Scripts that read `custom_env` from
`/api/agents` must migrate to `GET /api/agents/{id}/env`.

Co-authored-by: multica-agent <github@multica.ai>

* fix(agents): close actor-spoofing + audit fail-closed in env endpoints (MUL-2600)

Addresses Elon's review of #3209:

* Mint a task-scoped `mat_` token per claim, bound to (agent, task,
  workspace, owner). Daemon injects it into the agent process in place
  of its own credential. Auth middleware authoritatively rebuilds
  X-User-ID / X-Agent-ID / X-Task-ID from the token row and sets
  X-Actor-Source=task_token; that header is server-set only — incoming
  values are stripped before any auth branch runs. resolveActor honors
  the header so an agent that strips X-Agent-ID / X-Task-ID still
  resolves as actor=agent.
* GetAgentEnv / UpdateAgentEnv are now fail-closed on audit-log
  failures: GET refuses to return plaintext, PUT persists inside the
  same tx as the audit row so they commit/roll back together.
* PUT /api/agents/{id} returns 400 when the body carries custom_env
  instead of silently dropping it — directs callers to the audited env
  endpoint.
* Agent actors never see mcp_config, even when the underlying member
  is owner/admin; mutation broadcasts go through a redaction shim so
  WS subscribers don't pick it up either.
* Fix backend test that asserted dense JSON (jsonb::text renders
  whitespace) and frontend test that assumed a unique "Test User"
  match.

Co-authored-by: multica-agent <github@multica.ai>

* fix(agents): close residual MUL-2600 gaps from review (MUL-2600)

Migration 108 FK now correctly references agent_task_queue(id) instead
of the non-existent agent_task table; the previous name blocked CI
backend migrations.

Task-token-authenticated requests can no longer be re-routed at a
different workspace by passing workspace_slug / workspace_id /
?workspace_id / a URL workspace param. ResolveWorkspaceIDFromRequest
and resolveWorkspaceUUID both short-circuit on X-Actor-Source=task_token
and return only the token-bound X-Workspace-ID; buildMiddleware adds a
defence-in-depth 403 if any URL-resolved workspace disagrees with the
token binding.

mcp_config no longer leaks back to agent actors through UpdateAgent /
CreateAgent / ArchiveAgent / RestoreAgent HTTP responses — the same
redactAgentResponseForActor helper that GetAgent/ListAgents use is now
applied to mutation responses too. WS broadcasts were already redacted
via broadcastAgentResponse.

FailTask and every TaskService cancel path (CancelTask /
CancelTasksForIssue / CancelTasksForAgent / CancelTasksByTriggerComment
/ BroadcastCancelledTasks) now eagerly DeleteTaskTokensByTask so the
mat_ token's 24h window doesn't outlive a terminated task. Failure is
non-fatal — the FK cascade and expiry remain durable guards.

Doc-only: clarify that PUT /api/agents/{id} now hard-rejects bodies
that carry custom_env (was previously "silently ignores").

Tests:
- middleware: TestResolveWorkspaceIDFromRequest gains a task_token
  case asserting client-supplied slug/id/query cannot override the
  bound workspace.
- handler: TestUpdateAgent_RedactsMcpConfigForAgentActor and
  TestUpdateAgent_KeepsMcpConfigForMemberActor pin the mutation-
  response redaction contract per actor type.

Co-authored-by: multica-agent <github@multica.ai>

* fix(agents): match redacted mcp_config as JSON null, not Go nil (MUL-2600)

`AgentResponse.McpConfig` is `json.RawMessage` without `omitempty`, so
the redacted response serialises as `"mcp_config": null`. On decode,
`json.RawMessage` keeps the literal bytes `null` rather than collapsing
to Go nil, which made the assertion fire on a non-leak.

The product contract (field always present, distinguished from "no
config" via `mcp_config_redacted`) is intentional, so adjust the test
to check for "no secret-bearing content" instead of weakening the
contract via `omitempty`.

Co-authored-by: multica-agent <github@multica.ai>

---------

Co-authored-by: multica-agent <github@multica.ai>
2026-05-25 18:42:48 +08:00

333 lines
12 KiB
Go

package handler
import (
"encoding/json"
"log/slog"
"net/http"
"sort"
"github.com/go-chi/chi/v5"
"github.com/jackc/pgx/v5/pgtype"
"github.com/multica-ai/multica/server/internal/logger"
db "github.com/multica-ai/multica/server/pkg/db/generated"
"github.com/multica-ai/multica/server/pkg/protocol"
)
// envSentinel is the masked marker the UI / clients see in place of a
// real value. A PUT body carrying it for a given key means "do not
// overwrite the existing value for that key" — a defense-in-depth
// guard so a client that round-trips a partially-revealed map cannot
// silently destroy real secrets by saving the masked placeholder.
const envSentinel = "****"
// agentEnvActivityRevealed and agentEnvActivityUpdated are the
// activity_log `action` constants for the two env-management
// endpoints. Stored on rows where `issue_id IS NULL` (env access is not
// tied to any issue). Owners can later query them — a queryable audit
// UI is out of scope for this PR, but the rows are written now so the
// data is captured from day one. Workspace activity history will
// eventually surface them; for now they're forensic-only.
const (
agentEnvActivityRevealed = "agent_env_revealed"
agentEnvActivityUpdated = "agent_env_updated"
)
// AgentEnvResponse is the wire shape for the dedicated env-management
// endpoint. Kept distinct from `AgentResponse` so secrets cannot leak
// back into the generic agent resource by accident — a future
// refactor that adds a field to AgentResponse cannot accidentally
// pull env values along.
type AgentEnvResponse struct {
AgentID string `json:"agent_id"`
CustomEnv map[string]string `json:"custom_env"`
}
// UpdateAgentEnvRequest is the wire shape for `PUT
// /api/agents/{id}/env`. Only `custom_env` is accepted — fewer
// surfaces, less to misuse.
type UpdateAgentEnvRequest struct {
CustomEnv map[string]string `json:"custom_env"`
}
// authorizeAgentEnv enforces the per-request auth contract for the env
// endpoints:
//
// 1. The actor MUST resolve to a member (human). Any request authored
// by an agent token — even one whose backing member is a workspace
// owner — is rejected. This is the key fix for the
// impersonation/lateral-movement risk that motivated MUL-2600: an
// agent running in the workspace cannot use its host's owner
// credentials to reveal another agent's secrets.
// 2. The member must be a workspace owner or admin.
//
// Returns the loaded agent and the authenticated member on success.
// All non-2xx branches write their own response and return ok=false.
func (h *Handler) authorizeAgentEnv(w http.ResponseWriter, r *http.Request) (db.Agent, db.Member, bool) {
agentID := chi.URLParam(r, "id")
agent, ok := h.loadAgentForUser(w, r, agentID)
if !ok {
return db.Agent{}, db.Member{}, false
}
workspaceID := uuidToString(agent.WorkspaceID)
userID := requestUserID(r)
// Reject agent actors before anything else. resolveActor returns
// "agent" iff both X-Agent-ID and a valid X-Task-ID are present and
// the task belongs to that agent — so this guard is precise and
// cannot be tricked by a member-supplied header.
actorType, _ := h.resolveActor(r, userID, workspaceID)
if actorType == "agent" {
writeError(w, http.StatusForbidden, "agents may not access env management endpoints")
return db.Agent{}, db.Member{}, false
}
member, ok := h.requireWorkspaceRole(w, r, workspaceID, "agent not found", "owner", "admin")
if !ok {
return db.Agent{}, db.Member{}, false
}
return agent, member, true
}
// GetAgentEnv returns the plaintext custom_env map for a single agent
// after gating through authorizeAgentEnv. Every successful read writes
// an `agent_env_revealed` row to activity_log (keys only, never
// values) so workspace owners have a trail of who saw which keys.
//
// Audit semantics are fail-closed: if we cannot persist the audit row
// we MUST NOT serve the plaintext. A reveal we cannot record is
// indistinguishable from an unaudited reveal, which would silently
// break the MUL-2600 promise of "every reveal leaves a queryable
// trail". Operators who hit a 500 here see the audit-log outage and
// can fix it; the alternative — quietly handing out secrets — is
// invisible.
func (h *Handler) GetAgentEnv(w http.ResponseWriter, r *http.Request) {
agent, member, ok := h.authorizeAgentEnv(w, r)
if !ok {
return
}
customEnv := unmarshalCustomEnv(agent)
revealedKeys := sortedKeys(customEnv)
details, _ := json.Marshal(map[string]any{
"agent_id": uuidToString(agent.ID),
"agent_name": agent.Name,
"revealed_keys": revealedKeys,
"key_count": len(revealedKeys),
})
if _, err := h.Queries.CreateActivity(r.Context(), db.CreateActivityParams{
WorkspaceID: agent.WorkspaceID,
IssueID: pgtype.UUID{}, // env access is not tied to an issue
ActorType: pgtype.Text{String: "member", Valid: true},
ActorID: parseUUID(uuidToString(member.UserID)),
Action: agentEnvActivityRevealed,
Details: details,
}); err != nil {
slog.Error("agent_env_revealed audit write failed; refusing to serve plaintext",
append(logger.RequestAttrs(r), "error", err, "agent_id", uuidToString(agent.ID))...)
writeError(w, http.StatusInternalServerError, "audit log write failed; refusing to serve env without a recorded reveal")
return
}
writeJSON(w, http.StatusOK, AgentEnvResponse{
AgentID: uuidToString(agent.ID),
CustomEnv: customEnv,
})
}
// UpdateAgentEnv replaces an agent's custom_env wholesale. The **** marker is
// honoured per-key: any value equal to envSentinel is treated as
// "keep the existing value for that key", protecting against the
// scenario where a UI fetches the env, exposes some values but leaves
// others masked, and then naively PUTs the whole map back. A
// straightforward write would have stored literal `****` in place of
// the real secret. Audit log captures the symmetric difference between
// old and new keys but never values.
//
// Persist + audit run inside one DB transaction so they commit
// together or roll back together. An audit-write outage cannot leave
// an unaudited env mutation on disk, and a persist failure does not
// leave a phantom audit row claiming a change that never happened.
func (h *Handler) UpdateAgentEnv(w http.ResponseWriter, r *http.Request) {
agent, member, ok := h.authorizeAgentEnv(w, r)
if !ok {
return
}
var req UpdateAgentEnvRequest
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
writeError(w, http.StatusBadRequest, "invalid request body")
return
}
if req.CustomEnv == nil {
req.CustomEnv = map[string]string{}
}
existing := unmarshalCustomEnv(agent)
merged, audit := mergeAgentEnv(existing, req.CustomEnv)
envBytes, err := json.Marshal(merged)
if err != nil {
writeError(w, http.StatusInternalServerError, "failed to encode env")
return
}
tx, err := h.TxStarter.Begin(r.Context())
if err != nil {
slog.Error("agent_env update: begin tx failed",
append(logger.RequestAttrs(r), "error", err, "agent_id", uuidToString(agent.ID))...)
writeError(w, http.StatusInternalServerError, "failed to update env")
return
}
defer tx.Rollback(r.Context())
qtx := h.Queries.WithTx(tx)
updated, err := qtx.UpdateAgentCustomEnv(r.Context(), db.UpdateAgentCustomEnvParams{
ID: agent.ID,
CustomEnv: envBytes,
})
if err != nil {
slog.Warn("update agent custom_env failed",
append(logger.RequestAttrs(r), "error", err, "agent_id", uuidToString(agent.ID))...)
writeError(w, http.StatusInternalServerError, "failed to update env")
return
}
auditDetails := map[string]any{
"agent_id": uuidToString(agent.ID),
"agent_name": agent.Name,
"added_keys": audit.added,
"removed_keys": audit.removed,
"changed_keys": audit.changed,
"preserved_keys": audit.preserved,
}
details, _ := json.Marshal(auditDetails)
if _, err := qtx.CreateActivity(r.Context(), db.CreateActivityParams{
WorkspaceID: agent.WorkspaceID,
IssueID: pgtype.UUID{},
ActorType: pgtype.Text{String: "member", Valid: true},
ActorID: parseUUID(uuidToString(member.UserID)),
Action: agentEnvActivityUpdated,
Details: details,
}); err != nil {
slog.Error("agent_env_updated audit write failed; rolling back update",
append(logger.RequestAttrs(r), "error", err, "agent_id", uuidToString(agent.ID))...)
writeError(w, http.StatusInternalServerError, "audit log write failed; env update rolled back")
return
}
if err := tx.Commit(r.Context()); err != nil {
slog.Error("agent_env update: tx commit failed",
append(logger.RequestAttrs(r), "error", err, "agent_id", uuidToString(agent.ID))...)
writeError(w, http.StatusInternalServerError, "failed to update env")
return
}
// Broadcast an agent:status update so connected clients refresh the
// "N variables configured" indicator. Payload is the redacted
// AgentResponse — no env values are sent.
resp := agentToResponse(updated)
workspaceID := uuidToString(updated.WorkspaceID)
h.publish(protocol.EventAgentStatus, workspaceID, "member", uuidToString(member.UserID), map[string]any{"agent": broadcastAgentResponse(resp)})
writeJSON(w, http.StatusOK, AgentEnvResponse{
AgentID: uuidToString(updated.ID),
CustomEnv: merged,
})
}
// envAudit summarises the diff between an agent's existing env and the
// new one, broken down so an auditor can reconstruct exactly which
// keys an operation touched without leaking values. All slices are
// sorted to keep the activity row content deterministic for tests and
// downstream tooling.
type envAudit struct {
added []string
removed []string
changed []string
preserved []string
}
// mergeAgentEnv applies the **** sentinel rule and returns both the
// final map to persist and an audit summary of which keys changed.
// Behaviour:
// - request key present, value == "****", key exists in `existing`
// → keep the existing value, append to preserved
// - request key present, value == "****", key NOT in `existing`
// → drop the key (literal "****" is never a valid stored value)
// - request key present, value != "****", key already in existing
// with same value → no-op (not counted)
// - request key present, value != "****", different from existing
// → write new value, append to changed
// - request key present, value != "****", key NOT in existing
// → write new value, append to added
// - key in existing but absent from request → removed
func mergeAgentEnv(existing, request map[string]string) (map[string]string, envAudit) {
merged := make(map[string]string, len(request))
audit := envAudit{}
for k, v := range request {
if v == envSentinel {
if old, ok := existing[k]; ok {
merged[k] = old
audit.preserved = append(audit.preserved, k)
}
// else: drop. We never persist a literal "****".
continue
}
if old, ok := existing[k]; ok {
if old == v {
merged[k] = v
continue
}
merged[k] = v
audit.changed = append(audit.changed, k)
continue
}
merged[k] = v
audit.added = append(audit.added, k)
}
for k := range existing {
if _, ok := request[k]; !ok {
audit.removed = append(audit.removed, k)
}
}
sort.Strings(audit.added)
sort.Strings(audit.removed)
sort.Strings(audit.changed)
sort.Strings(audit.preserved)
return merged, audit
}
// unmarshalCustomEnv decodes an agent's stored custom_env bytea into a
// map, returning an empty (never nil) map so callers can iterate
// safely.
func unmarshalCustomEnv(a db.Agent) map[string]string {
out := map[string]string{}
if len(a.CustomEnv) == 0 {
return out
}
if err := json.Unmarshal(a.CustomEnv, &out); err != nil {
slog.Warn("failed to unmarshal agent custom_env", "agent_id", uuidToString(a.ID), "error", err)
return map[string]string{}
}
if out == nil {
return map[string]string{}
}
return out
}
func sortedKeys(m map[string]string) []string {
keys := make([]string, 0, len(m))
for k := range m {
keys = append(keys, k)
}
sort.Strings(keys)
return keys
}