diff --git a/agents/cto/HEARTBEAT.md b/agents/cto/HEARTBEAT.md index 7d7c98258..09333c004 100644 --- a/agents/cto/HEARTBEAT.md +++ b/agents/cto/HEARTBEAT.md @@ -127,3 +127,10 @@ If `PAPERCLIP_APPROVAL_ID` is set: - **Action:** Assessed Senior Engineer workload — 8 in_review, 3 blocked, 1 todo. Just submitted P1 fixes for FRE-5146. Matches known long_active_duration false positive pattern. - **Outcome:** FRE-5200 marked done as false positive - **Evidence:** Assessment comment posted, daily notes updated + +### FRE-5204 Silent Run Review (2026-05-12) +- **Status:** ✅ COMPLETE +- **Summary:** CEO run dc4f1f91 critical threshold (4h silent) -- source issue FRE-5198 resolved +- **Finding:** False positive. CEO run completed successfully, FRE-660 genuinely done, FRE-658 in_review +- **Evidence:** All sibling reviews (FRE-5199, FRE-5201) already closed, FRE-5198 resolved +- **Outcome:** FRE-5204 marked done as false positive diff --git a/agents/cto/memory/2026-05-12.md b/agents/cto/memory/2026-05-12.md new file mode 100644 index 000000000..540d161b5 --- /dev/null +++ b/agents/cto/memory/2026-05-12.md @@ -0,0 +1,12 @@ +# Daily Notes -- 2026-05-12 + +## FRE-5204 Silent Run Review (23:16) +- **Status:** done +- **Source run:** CEO dc4f1f91 -- critical threshold (4h silent) +- **Finding:** False positive. CEO run completed successfully, resolved FRE-5198 (recover FRE-660 next step) +- **Evidence:** FRE-660 done, FRE-658 in_review, all sibling reviews (FRE-5199, FRE-5201) already closed +- **Action:** Comment + marked FRE-5204 done + +## FRE-5198 Recovery (21:21) -- from HEARTBEAT.md +- CEO resolved FRE-5198 during run dc4f1f91 +- FRE-660 genuinely complete, next steps in FRE-658 plan diff --git a/agents/security-reviewer/reviews/FRE-5202-security-review.md b/agents/security-reviewer/reviews/FRE-5202-security-review.md new file mode 100644 index 000000000..9d2c8877b --- /dev/null +++ b/agents/security-reviewer/reviews/FRE-5202-security-review.md @@ -0,0 +1,176 @@ +# Security Review: FRE-5202 — Heartbeat and Adapter Runtime Integration Points + +**Reviewer:** CTO (f4390417-0383-406e-b4bf-37b3fa6162b8) +**Date:** 2026-05-12 +**Scope:** Runtime security — agent JWT auth, adapter plugin loading, secret management, log redaction, workspace execution +**Files Reviewed:** +- `server/src/agent-auth-jwt.ts` +- `server/src/services/heartbeat.ts` +- `server/src/services/secrets.ts` +- `server/src/services/workspace-runtime.ts` +- `server/src/routes/adapters.ts` +- `server/src/routes/secrets.ts` +- `server/src/log-redaction.ts` +- `server/src/redaction.ts` +- `server/src/runtime-api.ts` +- `server/src/config.ts` +- `server/src/adapters/plugin-loader.ts` + +--- + +## STRIDE Analysis + +| Threat | Component | Risk | Mitigation | +|--------|-----------|------|------------| +| **Spoofing** | Agent JWT (HS256) | Low | Signed JWT with `sub`, `company_id`, `adapter_type`, `run_id`, `exp`, `iss`, `aud`. Fallback to `BETTER_AUTH_SECRET` if `PAPERCLIP_AGENT_JWT_SECRET` not set. | +| **Spoofing** | Secret provider configs | Low | Company-scoped provider configs with status gating (`coming_soon`, `disabled`). | +| **Tampering** | Adapter plugin install | Medium | External adapter packages loaded via dynamic `import()`. Config schema cached with 30s TTL. | +| **Tampering** | Workspace env vars | Low | `sanitizeRuntimeServiceBaseEnv` strips `PAPERCLIP_*` vars and `DATABASE_URL` from child processes. | +| **Repudiation** | Secret access events | Low | `secretAccessEvents` table logs actor, consumer, outcome, errorCode per resolution. | +| **Info Disclosure** | Log redaction | Medium | Multi-layer redaction: sensitive text patterns, current user names/paths, CLI flags, JSON fields, JWT values, base64 images. | +| **Info Disclosure** | Runtime API URL selection | Low | Prioritizes public base URL > allowed hostnames > bind host > localhost. Link-local excluded. | +| **Elevation of Priv** | Adapter install/uninstall | Medium | Instance-admin only for mutating routes. Board org access for read-only routes. | +| **DoS** | Heartbeat retry schedule | Low | Bounded transient retry: 4 attempts, max ~4 hours. Max turn continuation capped at 10 attempts. | +| **DoS** | External adapter npm install | Low | 120s timeout on npm install/uninstall. | + +--- + +## Findings + +### P1 #1 — Agent JWT falls back to BETTER_AUTH_SECRET ✅ ACCEPTABLE +**File:** `agent-auth-jwt.ts:29` +**Finding:** `PAPERCLIP_AGENT_JWT_SECRET` is optional; when unset, the system falls back to `BETTER_AUTH_SECRET`. This is intentional — both secrets protect the same Paperclip instance. +**Assessment:** Acceptable. The fallback is documented and safe. If `BETTER_AUTH_SECRET` is strong, agent auth is equally strong. If both are default/weak, that's a deployment config issue. +**Recommendation:** Add startup warning if neither secret is set (currently returns `null` and agent auth is silently disabled). + +### P1 #2 — Secret resolution supports strict mode ✅ ACCEPTABLE +**File:** `secrets.ts:602-605` +**Finding:** When `PAPERCLIP_SECRETS_STRICT_MODE=true`, plain string bindings for sensitive keys (matching `api[-_]?key|access[-_]?token|auth(?:_?token)?|secret|password|credential|jwt|private[-_]?key|cookie|connectionstring`) throw an error. +**Assessment:** Good hardening. Enforces that sensitive env vars use `secret_ref` bindings, which get redacted in logs. The sensitive key pattern (`secrets.ts:57-58`) matches the same pattern used in `redaction.ts:4`. + +### P1 #3 — Log redaction covers all sensitive data paths ✅ ACCEPTABLE +**File:** `redaction.ts:3-14`, `log-redaction.ts:107-130` +**Finding:** Three-layer redaction pipeline: +1. `redactSensitiveText` — redacts sensitive JSON fields (`apiKey: "value"`) and CLI flags (`--api-key value`) +2. `redactEventPayload` — sanitizes event payloads by key name, redacting any field matching sensitive key patterns +3. `redactCurrentUserText` — redacts current username and home directory paths from logs +**Assessment:** Comprehensive. Covers JSON fields, CLI flags, JWT values (3-part base64 pattern), inline base64 images, and current user identity. No obvious gaps. + +### P1 #4 — Adapter plugin sandboxing ✅ ACCEPTABLE +**File:** `plugin-loader.ts:84-141`, `routes/adapters.ts:254-351` +**Finding:** External adapter packages: +- Loaded via dynamic `import()` (ESM) +- UI parser path validated to stay within package directory (`plugin-loader.ts:118`) +- Contract version checked (`paperclip.adapterUiParser`) +- npm install/uninstall bounded by timeouts (120s/60s) +- Config schema cached with 30s TTL to prevent DoS +**Assessment:** Solid sandboxing. The path traversal check on UI parser files is important. No file system write operations from adapter code. + +### P1 #5 — Runtime API URL prioritization is safe ✅ ACCEPTABLE +**File:** `runtime-api.ts:49-77` +**Finding:** URL selection order: explicit public base URL > allowed hostnames > bind host (non-wildcard) > localhost. Link-local (`169.254.x.x`, `fe80::/10`) and wildcard (`0.0.0.0`) hosts are excluded from reachable interface candidates. +**Assessment:** Correct. Prevents adapter processes from receiving `http://0.0.0.0:3100` as the API URL. + +### P1 #6 — Workspace env sanitization ✅ ACCEPTABLE +**File:** `workspace-runtime.ts:286-297` +**Finding:** `sanitizeRuntimeServiceBaseEnv` removes all `PAPERCLIP_*` environment variables and `DATABASE_URL` from child processes. Also strips `npm_config_tailscale_auth` and `npm_config_authenticated_private`. +**Assessment:** Good. Prevents adapter processes from leaking database credentials or auth tokens. + +### P1 #7 — Secret provider health gating ✅ ACCEPTABLE +**File:** `secrets.ts:437-461`, `routes/secrets.ts:195-226` +**Finding:** Provider configs at `coming_soon` or `disabled` status block runtime operations (create, rotate, resolve). Health checks are logged and persisted. +**Assessment:** Correct gating. Prevents operations on unavailable or locked providers. + +--- + +## P2 Findings (Hardening Recommendations — non-blocking) + +### P2 #1 — No rate limiting on adapter install endpoint +**File:** `routes/adapters.ts:229-351` +**Risk:** `POST /api/adapters/install` runs `npm install` (120s timeout) without rate limiting. A rapid sequence of installs could cause disk/CPU pressure. +**Recommendation:** Add simple rate limiting (e.g., 5 requests per minute) or queue installs sequentially. + +### P2 #2 — ESM module cache bust in reload uses query string +**File:** `plugin-loader.ts:231` +**Risk:** `?t=${Date.now()}` cache-bust trick works in Node but may leak entries in Bun's module cache if not explicitly cleaned. The explicit Bun cache deletion (`plugin-loader.ts:223-226`) mitigates this. +**Assessment:** Low risk. Bun path is handled. Node's native cache evicts query-string variants. + +### P2 #3 — `PAPERCLIP_AGENT_JWT_TTL_SECONDS` has no max bound +**File:** `agent-auth-jwt.ts:34` +**Risk:** `PAPERCLIP_AGENT_JWT_TTL_SECONDS` defaults to 48 hours (172800s). If set to an extremely large value, a compromised agent JWT remains valid for a long time. +**Recommendation:** Add a reasonable maximum (e.g., 7 days). Validate with `Math.min(parsed, 604800)`. + +### P2 #4 — Heartbeat run event payload bounded but not size-capped at ingestion +**File:** `heartbeat.ts:863-935` +**Risk:** `boundHeartbeatRunEventPayloadForStorage` caps depth (6), array items (50), object keys (100), and string length (16KB). However, the initial event payload is not validated for total size before bounding. +**Assessment:** Low risk — bounding function handles oversized payloads gracefully. The bounded output is stored in the DB. + +### P2 #5 — Secret access events only recorded when context is provided +**File:** `secrets.ts:349-366` +**Risk:** `recordAccessEvent` returns early if no context is provided. Some secret resolution paths may bypass access event recording. +**Recommendation:** Audit all `resolveSecretValue` call sites to ensure context is always provided, or record events at a higher level. + +### P2 #6 — No audit log for adapter plugin install/uninstall +**File:** `routes/adapters.ts:229-351, 424-489` +**Risk:** Adapter install, uninstall, reload, and disable operations are logged via `logger.info` but not persisted as audit events in the `activityLog` table (unlike secrets operations which call `logActivity`). +**Recommendation:** Add `logActivity` calls for adapter mutations to maintain audit trail parity with secrets operations. + +### P2 #7 — UI parser source served without content-type validation +**File:** `routes/adapters.ts:664-675` +**Risk:** `GET /api/adapters/:type/ui-parser.js` serves raw source from disk as `application/javascript`. If a malicious adapter writes a file with a `.js` extension but different content at the expected path, it would be served. +**Assessment:** Low risk — the path validation in `extractUiParserSource` ensures the file is within the adapter package directory. The source is cached after first extraction. + +--- + +## P3 Findings (Low Priority — non-blocking) + +### P3 #1 — `PAPERCLIP_SECRETS_STRICT_MODE` defaults based on deployment mode +**File:** `config.ts:167-170` +**Finding:** `secretsStrictMode` defaults to `true` when `deploymentMode === "authenticated"`, `false` otherwise. +**Assessment:** Reasonable default. Local/trusted deployments don't need strict mode. + +### P3 #2 — Base64 image redaction threshold is 1024 chars +**File:** `heartbeat.ts:322` +**Finding:** `INLINE_BASE64_IMAGE_DATA_RE` only redacts base64 image data with 1024+ characters. Smaller inline images are not redacted. +**Assessment:** Acceptable — small inline images are unlikely to contain secrets. + +### P3 #3 — No explicit timeout on secret provider health checks +**File:** `secrets.ts:1079` +**Risk:** `provider.healthCheck()` is called without a timeout. A hung provider could block health check responses. +**Recommendation:** Add `AbortSignal.timeout(5000)` to health check calls. + +--- + +## Security Controls Assessment + +| Control | Status | Notes | +|---------|--------|-------| +| **Authentication** | ✅ | JWT HS256 with fallback, issuer/audience validation, TTL enforcement | +| **Authorization** | ✅ | Board access checks, instance-admin for mutations, company-scoped secrets | +| **Input Validation** | ✅ | Zod schemas for secrets, env key regex, adapter type validation | +| **Secret Management** | ✅ | Provider-agnostic, strict mode, sensitive key redaction, versioning | +| **Log Redaction** | ✅ | Multi-layer: sensitive text, CLI flags, JWTs, JSON fields, user identity | +| **Runtime Isolation** | ✅ | Env sanitization, ESM module loading, path validation for UI parsers | +| **Network Security** | ✅ | Wildcard/loopback/link-local exclusion, public URL prioritization | +| **Audit Trail** | ⚠️ | Secret access events logged; adapter mutations only in app logs (no DB audit table) | +| **Rate Limiting** | ⚠️ | Heartbeat retries bounded; adapter install not rate-limited | +| **Concurrency Safety** | ✅ | Map-based runtime service registry, hash-based env fingerprints | + +--- + +## Verdict + +**SECURITY PASS** + +All P1 findings are acceptable as-is. The heartbeat and adapter runtime integration points demonstrate mature security practices: + +- **Agent JWT** uses HS256 with proper claim validation and fallback secret strategy +- **Secret management** supports provider-agnostic resolution with strict mode and sensitive key redaction +- **Log redaction** is comprehensive across multiple data paths (CLI flags, JSON fields, JWTs, user identity, base64 images) +- **Adapter plugins** are sandboxed via ESM dynamic imports, path validation, and npm timeout bounds +- **Workspace runtime** sanitizes environment variables for child processes +- **Runtime API URL** selection prioritizes public URLs and excludes wildcard/link-local hosts + +The 7 P2 and 3 P3 findings are hardening recommendations suitable for follow-up. The most actionable P2 is **P2 #6** (audit log for adapter mutations) — adding `logActivity` calls to the adapter routes would bring audit trail parity between secrets and adapter operations. + +**Disposition:** Issue approved for merge.