Skip to content

Fix node diagnostic raw-surface auth boundary - #1886

Open
simple-agent-manager[bot] wants to merge 3 commits into
mainfrom
sam/fix-critical-node-wide-c6hc8n
Open

Fix node diagnostic raw-surface auth boundary#1886
simple-agent-manager[bot] wants to merge 3 commits into
mainfrom
sam/fix-critical-node-wide-c6hc8n

Conversation

@simple-agent-manager

Copy link
Copy Markdown
Contributor

Summary

  • Require node-scoped management authentication for VM-agent node-wide diagnostics (/events, /events/export, /metrics/export, /logs, /containers, /logs/stream, /debug-package, and existing shared /system-info).
  • Reject workspace browser sessions and workspace-scoped management JWTs for node-wide raw surfaces; preserve workspace-scoped /workspaces/{workspaceId}/events behavior for matching workspace auth.
  • Preserve control-plane node observability by keeping /api/nodes/:id/... proxy routes intact and sanitizing the log-stream proxy so browser Authorization/Cookie material is not forwarded to the VM agent.
  • Update VM-agent public docs/spec contracts for the corrected trust boundary.

Do not merge and do not deploy staging from this PR. This is a constituent fix for a coordinated review wave; the parent will build a mega PR and perform consolidated staging.

Validation

  • pnpm lint
  • pnpm typecheck
  • pnpm test
  • Additional validation run (if applicable)
  • If this PR changes candidate selection for a sweep/cron/alarm loop (WHERE clause, status set, join, or equivalent), expected candidate volume and worst-case per-candidate cost are stated in the summary or validation notes (see .claude/rules/47-control-loop-io-budget.md) — N/A: no sweep/cron/alarm candidate selection changed.

Additional validation:

  • cd packages/vm-agent && go test ./internal/server -run 'TestNodeWide|TestWorkspaceScopedEvents' — passed.
  • cd packages/vm-agent && go test ./internal/server — passed.
  • cd packages/vm-agent && go test ./... — passed.
  • cd packages/vm-agent && go vet ./... — passed.
  • cd packages/vm-agent && go test -race ./internal/server -run 'TestNodeWide|TestWorkspaceScopedEvents' — passed.
  • pnpm --filter @simple-agent-manager/api test -- tests/unit/routes/node-observability-logs.test.ts tests/unit/task-callback-auth-routing.test.ts tests/unit/routes/mcp-knowledge-policy-tools.test.ts tests/unit/routes/mcp-streamable-http.test.ts — passed, 4 files / 36 tests.
  • pnpm -s --filter @simple-agent-manager/api test — passed, 591 files / 7923 tests.
  • pnpm format:check — passed.
  • pnpm build — passed.
  • pnpm test — passed, 21 tasks successful; API 591/591 files and web 286/286 files.
  • cd packages/vm-agent && staticcheck ./... — not clean due pre-existing unrelated findings outside this patch, including ACP test unused values, persistence/publish unused helpers, server unused helpers in logs.go/workspace_routing.go, and TypeRegA deprecations. No new staticcheck finding is introduced by the touched code.

Staging Verification (REQUIRED for all code changes — merge-blocking)

All checkboxes below are mandatory for any PR that changes runtime code (.ts, .tsx, .go, etc.). Write N/A: docs-only ONLY if the PR contains zero runtime code changes. See .claude/rules/13-staging-verification.md.

  • Staging deployment green — skipped by explicit parent release contract; parent review wave will deploy consolidated mega PR.
  • Live app verified via Playwright — skipped by explicit parent release contract; parent review wave will perform consolidated staging verification.
  • Existing workflows confirmed working — local API/web/VM-agent contract tests passed; live staging skipped by explicit parent release contract.
  • New feature/fix verified on staging — local two-workspace canary and control-plane proxy tests passed; live staging skipped by explicit parent release contract.
  • Infrastructure verification completed — VM-agent runtime code changed, but staging/infrastructure verification is intentionally deferred to parent mega PR by user instruction.
  • Mobile and desktop verification notes added for UI changes — N/A: no UI changes.

Staging Verification Evidence

Staging was not deployed and not verified for this constituent PR by explicit user instruction: "DO NOT merge the PR and DO NOT deploy staging. The parent will build one mega PR and perform consolidated staging."

UI Compliance Checklist (Required for UI changes)

  • Mobile-first layout verified — N/A: no UI changes.
  • Accessibility checks completed — N/A: no UI changes.
  • Shared UI components used or exception documented — N/A: no UI changes.
  • Playwright visual audit run locally — N/A: no UI changes.

End-to-End Verification (Required for multi-component changes)

  • Data flow traced from user input to final outcome with code path citations (see .claude/rules/10-e2e-verification.md)
  • Capability test exercises the complete happy path across system boundaries
  • All spec/doc assumptions about existing behavior verified against code (not just "read the code")
  • If any gap exists between automated test coverage and full E2E, manual verification steps documented below

Data Flow Trace

  • Web/API user node observability: apps/web/src/lib/api/nodes.ts calls /api/nodes/:nodeId/...; apps/api/src/routes/nodes.ts verifies user/node ownership via requireNodeOwnership, then apps/api/src/services/node-agent.ts:nodeAgentRequest or nodeAgentRawRequest signs node-scoped management JWTs with signNodeManagementToken(userId, nodeId, null, env).
  • WebSocket log stream: apps/api/src/routes/nodes.ts mints a node-scoped management JWT, puts it on the VM-agent /logs/stream URL, sets backend Authorization: Bearer <node-management-token>, and forwards only allowlisted WebSocket handshake headers plus X-SAM-Node-Id.
  • VM-agent node-wide routes: packages/vm-agent/internal/server/server.go maps node-wide diagnostics to handlers in events.go, logs.go, debug_package.go, and system_info.go; those handlers call requireNodeEventAuth, which now accepts only node-scoped management JWTs and rejects workspace cookies/workspace-scoped management tokens before returning node-wide data or archives.
  • Workspace-scoped route: packages/vm-agent/internal/server/events.go:handleListWorkspaceEvents continues to call checkWorkspaceRequestAuth and then reads only the requested workspace events.

Capability coverage:

  • packages/vm-agent/internal/server/events_test.go builds a realistic two-workspace single-node fixture with workspace-B canary data across event store, in-memory events, Docker logs/containers, and debug archive paths.
  • The tests prove workspace-A cookie auth cannot read workspace-B data through direct HTTP routes or /logs/stream, management tokens still read node-wide observability, workspace-scoped events remain scoped, invalid/expired/replayed tokens fail closed, auth precedence is deterministic, and denied /debug-package responses do not begin partial archives.
  • apps/api/tests/unit/routes/node-observability-logs.test.ts proves the control-plane /api/nodes/:id/logs/stream proxy strips client Authorization/Cookie and sends node-management auth to the VM agent.

Untested Gaps

Live staging validation is intentionally deferred by explicit parent release contract. Automated local coverage exercises the affected direct VM-agent surfaces and the control-plane log-stream proxy behavior; the parent mega PR must perform live staging and VM/infrastructure verification before merge/deploy.

Post-Mortem (Required for bug fix PRs)

What broke

A workspace-authenticated browser session for one workspace on a multi-workspace node could directly request node-wide diagnostic endpoints and read other workspaces' logs, events, container metadata, raw SQLite exports, and debug-package archive content.

Root cause

packages/vm-agent/internal/server/events.go:requireNodeEventAuth treated any valid workspace session cookie as sufficient node-wide authorization. logs.go, debug_package.go, exports, and system-info shared that helper even though those endpoints expose node-wide/raw data.

Class of bug

Multi-tenant authorization boundary failure / confused trust boundary between workspace-scoped browser auth and node-scoped operator diagnostics.

Why it wasn't caught

The prior tests were source-string assertions that encoded browser workspace auth as expected behavior for node events. There was no adversarial two-workspace canary scenario covering direct HTTP/WebSocket/archive endpoints or control-plane proxy header precedence.

Process fix included in this PR

Added behavioral regression tests in packages/vm-agent/internal/server/events_test.go and apps/api/tests/unit/routes/node-observability-logs.test.ts, replacing source-string assertions with scenario-driven auth/leakage tests. Public docs/specs now state the node-scoped diagnostic trust boundary.

Post-mortem file

This PR body is the post-mortem artifact for this constituent fix; no separate task file was committed to keep the PR tightly scoped per release contract.

Specialist Review Evidence (Required for agent-authored PRs)

If local subagents were used during Phase 5, list every reviewer below. Do NOT merge until every row shows PASS or ADDRESSED. If any reviewer could not complete (timeout, workspace killed, error), you MUST add the needs-human-review label and stop — do not self-merge. See .claude/rules/25-review-merge-gate.md.

  • All local reviewers completed and findings addressed before merge
  • If any reviewer did NOT complete: needs-human-review label added and merge deferred to human — N/A: both local reviewers completed.
Reviewer Status Outcome
local skeptical implementation/security reviewer ADDRESSED Found API /api/nodes/:id/logs/stream forwarded client Authorization/Cookie, which could break VM-agent query-token auth precedence and leak client auth to the node. Fixed by allowlisting WebSocket headers and setting VM-agent node-management Authorization; added route regression test.
local test-quality reviewer ADDRESSED Requested invalid/expired query-token fallback tests, inverse auth-precedence coverage, positive WebSocket management-token coverage, and stronger no-partial-archive assertions. Added all requested cases.
security-auditor ADDRESSED Reviewed JWT/auth/WebSocket/IDOR boundary. Node-wide diagnostics now require node-scoped management JWTs; workspace cookies and workspace-scoped management JWTs are rejected; control-plane proxy auth material is not forwarded.
test-engineer ADDRESSED Verified scenario-driven two-workspace canary tests, direct HTTP/WebSocket/archive negative and positive cases, auth precedence, replay/expiry, workspace-scoped allowed data, and no source-string assertions.
go-specialist PASS Reviewed VM-agent Go auth path, route coverage, WebSocket pre-upgrade auth, and fail-closed behavior; no remaining blocking Go findings.
doc-sync-validator PASS Public VM-agent docs and specs updated to match the corrected trust boundary.
constitution-validator PASS No production hardcoded URL/timeout/limit/identifier violations added; new literal IDs/secrets are test fixtures only.

Exceptions (If any)

  • Scope: Staging/deployment verification omitted for this PR.
  • Rationale: Explicit parent release contract says not to deploy staging; parent wave will create the mega PR and perform consolidated staging.
  • Expiration: Expires when the parent mega PR performs staging verification.

Agent Preflight (Required)

  • Preflight completed before code changes

Classification

  • external-api-change
  • cross-component-change
  • business-logic-change
  • public-surface-change
  • docs-sync-change
  • security-sensitive-change
  • ui-change
  • infra-change

External References

N/A: no external API or third-party documentation was needed; the fix is internal VM-agent/API authorization behavior.

Codebase Impact Analysis

Affected paths: packages/vm-agent/internal/server/events.go for shared node-wide diagnostic auth, packages/vm-agent/internal/server/events_test.go for behavioral coverage across events/logs/exports/debug archive/WebSocket, apps/api/src/routes/nodes.ts for control-plane log-stream proxy header sanitization, and apps/api/tests/unit/routes/node-observability-logs.test.ts for the proxy regression. Public docs/spec paths updated under apps/www/src/content/docs/docs/reference/vm-agent.md, specs/020-node-observability/contracts/vm-agent-logs.md, and specs/014-multi-workspace-nodes/contracts/node-agent-api.md.

Documentation & Specs

Updated apps/www/src/content/docs/docs/reference/vm-agent.md to document node-scoped management auth for node-wide diagnostics. Updated specs/020-node-observability/contracts/vm-agent-logs.md and specs/014-multi-workspace-nodes/contracts/node-agent-api.md so contracts distinguish node-wide diagnostics from workspace-scoped event access.

Constitution & Risk Check

Checked Principle XI/no-hardcoded-values: production code adds no hardcoded deployment URL, timeout, limit, or identifier; route behavior continues deriving VM-agent protocol/port/domain and token material from existing config/services. Security risk is intentionally reduced by fail-closing node-wide diagnostics under workspace auth while preserving control-plane proxy workflows and route/response contracts. Staging risk is deferred by explicit parent contract and covered locally with focused VM-agent/API tests plus full monorepo validation.

@codspeed-hq

codspeed-hq Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 6 untouched benchmarks


Comparing sam/fix-critical-node-wide-c6hc8n (0e1e5de) with main (b672af2)

Open in CodSpeed

@simple-agent-manager
simple-agent-manager Bot force-pushed the sam/fix-critical-node-wide-c6hc8n branch from 02e0ad6 to 77e065f Compare August 23, 2026 04:41
@simple-agent-manager
simple-agent-manager Bot force-pushed the sam/fix-critical-node-wide-c6hc8n branch 2 times, most recently from 1647877 to 1f025e8 Compare August 23, 2026 05:00
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant