diff --git a/CHANGELOG.md b/CHANGELOG.md index 217998b..53ab665 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,20 @@ All notable changes to RStack are documented here. Entries are user-focused: what you can now do that you couldn't before. +## [1.7.1] - 2026-06-02 + +### Security +- **Approval gates can no longer be spoofed or abused** (closes a pre-release + audit). Approving from the dashboard now requires a signed token + (`RSTACK_APPROVAL_TOKEN`) and a same-origin request, and records audit-proof + actor evidence — a script can't submit an arbitrary manager name anymore. + Without the token configured, browser approvals are disabled by default; + `sdlc_approve` continues to enforce the manager allow-list. +- **Approval ids can no longer escape the run directory.** Run ids, task ids, + and artifact names in approval ids are strictly validated and every write is + asserted to stay inside `.rstack/runs/` with a real manifest — closing a + path-traversal write. + ## [1.7.0] - 2026-06-02 ### Added diff --git a/README.md b/README.md index ebf9f26..2e1aaf1 100644 --- a/README.md +++ b/README.md @@ -67,6 +67,10 @@ Switch project/run scope from the top bar; share `#run=` links in Slack. - The moment a gate blocks, **every configured channel is paged** and the dashboard pops a notification - Every approval records the real approver (git identity or `RSTACK_USER`) +- Approving from the dashboard requires a signed token (`RSTACK_APPROVAL_TOKEN`) + so a manager's identity can't be spoofed from the browser; `sdlc_approve` + enforces the `policy.json` manager allow-list. Without the token set, + browser approvals are disabled (secure default) — approve via `sdlc_approve`. ## Notifications — Slack, Teams, Discord, Telegram, WhatsApp @@ -112,6 +116,8 @@ instrumentation is on the roadmap. | `RSTACK_USER` / `RSTACK_USER_EMAIL` | Identity for runs/approvals (defaults to git config) | | `RSTACK_BUSINESS_PORT` | Dashboard port (default 3008) | | `RSTACK_NO_BUSINESS_HUB=1` | Disable dashboard auto-launch | +| `RSTACK_APPROVAL_TOKEN` | Required to approve from the dashboard (prevents identity spoofing) | +| `RSTACK_MANAGER_USERS` | Comma-separated manager allow-list (also in `policy.json`) | | `RSTACK_SLACK_WEBHOOK` etc. | Notification channels — see webhooks guide | | `RSTACK_DEFAULT_MODEL` / `RSTACK_ESCALATED_MODEL` | Models for delegated builders (escalation at attempt ≥ 2) | diff --git a/package-lock.json b/package-lock.json index d15c5fc..fdc0d85 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "rstack-agents", - "version": "1.7.0", + "version": "1.7.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "rstack-agents", - "version": "1.7.0", + "version": "1.7.1", "license": "MIT", "dependencies": { "@earendil-works/pi-ai": "^0.74.1", diff --git a/package.json b/package.json index c794060..a704439 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "rstack-agents", - "version": "1.7.0", + "version": "1.7.1", "description": "Production-ready agentic SDLC framework for Pi and coding agents — orchestrator, builder/validator teams, lifecycle state, and specialist reuse", "type": "module", "main": "src/index.js", diff --git a/src/core/tracker/approvals.js b/src/core/tracker/approvals.js index 428495f..23a95ee 100644 --- a/src/core/tracker/approvals.js +++ b/src/core/tracker/approvals.js @@ -1,17 +1,40 @@ import { readFile, writeFile, mkdir } from 'node:fs/promises'; import { existsSync } from 'node:fs'; -import { join } from 'node:path'; +import { join, resolve, sep } from 'node:path'; // owner: RStack developed by Richardson Gunde const QUEUE_FILE = '.rstack/approvals.jsonl'; -function queuePath(projectRoot) { - return join(projectRoot, QUEUE_FILE); +// Run ids are timestamp-slug strings — never path separators or traversal. +// A crafted approval id could otherwise encode a runId like "../../etc" and +// drive a write outside .rstack/runs (issue #54). Validate before any FS use. +const SAFE_RUN_ID = /^[A-Za-z0-9][A-Za-z0-9._-]{0,200}$/; + +export function isSafeRunId(runId) { + return typeof runId === 'string' && SAFE_RUN_ID.test(runId) && !runId.includes('..'); +} + +function safeArtifact(artifact) { + // Artifacts are file/stage names, not paths. + return typeof artifact === 'string' && artifact.length > 0 && artifact.length < 256 + && !artifact.includes('/') && !artifact.includes('\\') && !artifact.includes('..'); } -function runApprovalsPath(projectRoot, runId) { - return join(projectRoot, '.rstack', 'runs', runId, 'approvals.json'); +// Resolve a run's approvals.json and assert it stays inside .rstack/runs/. +// Returns null if the runId is unsafe, escapes the sandbox, or the run has no +// manifest.json (i.e. it isn't a real run). +function safeRunApprovalsPath(projectRoot, runId) { + if (!isSafeRunId(runId)) return null; + const runsRoot = resolve(projectRoot, '.rstack', 'runs'); + const runDir = resolve(runsRoot, runId); + if (runDir !== join(runsRoot, runId) || !(runDir === runsRoot || runDir.startsWith(runsRoot + sep))) return null; + if (!existsSync(join(runDir, 'manifest.json'))) return null; + return join(runDir, 'approvals.json'); +} + +function queuePath(projectRoot) { + return join(projectRoot, QUEUE_FILE); } function policyPath(projectRoot) { @@ -34,7 +57,11 @@ export function parseApprovalQueueId(id) { if (typeof id !== 'string' || !id.startsWith('gate:')) return null; const [, runId, taskId, artifact] = id.split(':'); if (!runId || !artifact) return null; - return { runId: decodePart(runId), taskId: decodePart(taskId), artifact: decodePart(artifact) }; + const decoded = { runId: decodePart(runId), taskId: decodePart(taskId), artifact: decodePart(artifact) }; + // Reject ids whose decoded parts could traverse the filesystem. + if (!isSafeRunId(decoded.runId) || !safeArtifact(decoded.artifact)) return null; + if (decoded.taskId && (decoded.taskId.includes('/') || decoded.taskId.includes('..'))) return null; + return decoded; } async function readJson(path, fallback) { @@ -111,8 +138,9 @@ export async function readApprovals(projectRoot) { export async function appendRunApproval(projectRoot, runId, record) { if (!runId || !record?.artifact) return null; - const path = runApprovalsPath(projectRoot, runId); - await mkdir(join(projectRoot, '.rstack', 'runs', runId), { recursive: true }); + // Hard sandbox: unsafe/escaping runId, or a run with no manifest, writes nothing. + const path = safeRunApprovalsPath(projectRoot, runId); + if (!path) return null; const approvals = await readJson(path, []); const all = Array.isArray(approvals) ? approvals : []; const next = { @@ -134,7 +162,10 @@ export async function resolveApproval(projectRoot, id, decision, resolvedBy, opt const idx = all.findIndex(a => a.id === id); const parsed = idx === -1 ? parseApprovalQueueId(id) : null; if (idx === -1 && !parsed) return false; - if (idx === -1 && parsed && !existsSync(join(projectRoot, '.rstack', 'runs', parsed.runId))) return false; + // parseApprovalQueueId already rejected unsafe runIds; require a real run. + if (idx === -1 && parsed && !safeRunApprovalsPath(projectRoot, parsed.runId)) return false; + // A queued entry could predate validation — re-check before trusting its runId. + if (idx !== -1 && all[idx].runId && !isSafeRunId(all[idx].runId)) return false; const base = idx === -1 ? { id, @@ -152,7 +183,9 @@ export async function resolveApproval(projectRoot, id, decision, resolvedBy, opt const queueStatus = decision === 'approved' ? 'approved' : 'rejected'; const runStatus = decision === 'approved' ? 'APPROVED' : 'REJECTED'; const resolvedAt = new Date().toISOString(); - const next = { ...base, status: queueStatus, resolvedBy: approver, resolvedAt, updatedAt: resolvedAt }; + // Audit-proof actor evidence, not just a name string. + const actor = options.actor ? { ...options.actor } : { name: approver, via: 'api', tokenVerified: false, ts: resolvedAt }; + const next = { ...base, status: queueStatus, resolvedBy: approver, actor, resolvedAt, updatedAt: resolvedAt }; if (idx === -1) all.push(next); else all[idx] = next; diff --git a/src/observability/dashboard/server.js b/src/observability/dashboard/server.js index c7aaa41..17ac40b 100644 --- a/src/observability/dashboard/server.js +++ b/src/observability/dashboard/server.js @@ -114,34 +114,66 @@ async function broadcastSnapshot() { broadcast(toClientState(state)); } +// A signed approval is required whenever RSTACK_APPROVAL_TOKEN is set: the +// dashboard cannot mint manager identity from an unauthenticated request body. +// Without the env token, approving from the browser is blocked entirely (the +// secure default for a multi-user company hub) — set the token to enable it. +function approvalAuthError(req) { + const expected = process.env.RSTACK_APPROVAL_TOKEN; + if (!expected) { + return { code: 403, msg: 'dashboard approvals are disabled — set RSTACK_APPROVAL_TOKEN to enable signed approvals, or approve via sdlc_approve' }; + } + // CSRF: a cross-site form POST cannot set custom headers and would carry a + // foreign Origin. Require the token header and a localhost (or absent) origin. + const origin = req.headers.origin; + if (origin && !/^https?:\/\/(localhost|127\.0\.0\.1)(:\d+)?$/.test(origin)) { + return { code: 403, msg: 'cross-origin approval rejected' }; + } + const token = req.headers['x-rstack-approval-token']; + if (!token || token !== expected) { + return { code: 401, msg: 'missing or invalid approval token' }; + } + return null; +} + async function handleApproval(req, res, decision) { - let body = ''; - req.on('error', () => { + const fail = (code, msg) => { if (!res.headersSent) { - res.writeHead(400, { 'Content-Type': 'application/json' }); - res.end(JSON.stringify({ ok: false, error: 'request stream error' })); + res.writeHead(code, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ ok: false, error: msg })); } + }; + const contentType = String(req.headers['content-type'] ?? ''); + if (!contentType.includes('application/json')) { + return fail(415, 'Content-Type must be application/json'); + } + const authErr = approvalAuthError(req); + if (authErr) return fail(authErr.code, authErr.msg); + + let body = ''; + let tooLarge = false; + req.on('error', () => fail(400, 'request stream error')); + req.on('data', (chunk) => { + body += chunk; + if (body.length > 64 * 1024) { tooLarge = true; req.destroy(); } }); - req.on('data', (chunk) => { body += chunk; }); req.on('end', async () => { + if (tooLarge) return fail(413, 'request body too large'); try { - const { id, resolvedBy } = safeJson(body) ?? {}; - if (!id) { - res.writeHead(400, { 'Content-Type': 'application/json' }); - res.end(JSON.stringify({ ok: false, error: 'missing approval id' })); - return; - } - const ok = await resolveDashboardApproval(PROJECT_ROOT, id, decision, resolvedBy ?? 'dashboard'); + const parsed = safeJson(body) ?? {}; + const { id, resolvedBy } = parsed; + if (!id) return fail(400, 'missing approval id'); + if (!resolvedBy || typeof resolvedBy !== 'string') return fail(400, 'resolvedBy (approver identity) is required'); + // Actor evidence: token-verified, not just a body string. + const ok = await resolveDashboardApproval(PROJECT_ROOT, id, decision, resolvedBy, { + actor: { name: resolvedBy, via: 'dashboard', tokenVerified: true, ts: new Date().toISOString() }, + }); res.writeHead(ok ? 200 : 404, { 'Content-Type': 'application/json' }); res.end(JSON.stringify({ ok })); if (ok) await broadcastSnapshot(); } catch (err) { process.stderr.write(`[rstack-business] approval error: ${err?.message}\n`); - if (!res.headersSent) { - const statusCode = Number(err?.statusCode) || 500; - res.writeHead(statusCode, { 'Content-Type': 'application/json' }); - res.end(JSON.stringify({ ok: false, error: String(err?.message) })); - } + fail(Number(err?.statusCode) || 500, String(err?.message)); } }); } diff --git a/src/observability/dashboard/state/approvals.js b/src/observability/dashboard/state/approvals.js index efb86ee..db50595 100644 --- a/src/observability/dashboard/state/approvals.js +++ b/src/observability/dashboard/state/approvals.js @@ -71,9 +71,9 @@ export function summarizeApprovals(queueApprovals) { }; } -export async function resolveApprovalAcrossRoots(roots, id, decision, resolvedBy) { +export async function resolveApprovalAcrossRoots(roots, id, decision, resolvedBy, options = {}) { for (const root of roots ?? []) { - const ok = await resolveApproval(root, id, decision, resolvedBy); + const ok = await resolveApproval(root, id, decision, resolvedBy, options); if (ok) return true; } return false; diff --git a/src/observability/dashboard/state/index.js b/src/observability/dashboard/state/index.js index 82a2a4e..8e14a16 100644 --- a/src/observability/dashboard/state/index.js +++ b/src/observability/dashboard/state/index.js @@ -91,7 +91,7 @@ export async function buildFullState(projectRoot, options = {}) { export async function resolveDashboardApproval(projectRoot, id, decision, resolvedBy, options = {}) { const roots = await sourceRoots(projectRoot, options); - return resolveApprovalAcrossRoots(roots, id, decision, resolvedBy); + return resolveApprovalAcrossRoots(roots, id, decision, resolvedBy, { actor: options.actor }); } function buildFrameworks(runs) { diff --git a/src/observability/dashboard/ui/client.js b/src/observability/dashboard/ui/client.js index b0694ef..79ea65f 100644 --- a/src/observability/dashboard/ui/client.js +++ b/src/observability/dashboard/ui/client.js @@ -1278,9 +1278,16 @@ function resolveApproval(id, action) { resolvedBy = window.prompt('Manager name for this approval decision') || ''; if (resolvedBy) localStorage.setItem('rstack-approver-name', resolvedBy); } + // Approvals require the signed token (RSTACK_APPROVAL_TOKEN) so identity + // can't be spoofed from a bare request. Stored locally after first entry. + var token = localStorage.getItem('rstack-approval-token') || ''; + if (!token && typeof window.prompt === 'function') { + token = window.prompt('Approval token (RSTACK_APPROVAL_TOKEN set on the hub)') || ''; + if (token) localStorage.setItem('rstack-approval-token', token); + } fetch('/api/' + action, { method: 'POST', - headers: { 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json', 'x-rstack-approval-token': token }, body: JSON.stringify({ id: id, resolvedBy: resolvedBy || 'dashboard' }) }).then(function(response) { if (!response.ok) { diff --git a/tests/approval-security.test.js b/tests/approval-security.test.js new file mode 100644 index 0000000..d77a801 --- /dev/null +++ b/tests/approval-security.test.js @@ -0,0 +1,94 @@ +/** + * Security regression tests for the approval gate (issue #54): + * 1. approval-id path traversal cannot write outside .rstack/runs + * 2. manager identity / unsafe queue entries are rejected + * + * owner: RStack developed by Richardson Gunde + */ + +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { existsSync, mkdtempSync, mkdirSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { + isSafeRunId, + parseApprovalQueueId, + approvalQueueId, + appendRunApproval, + resolveApproval, +} from '../src/core/tracker/approvals.js'; + +function realRun(projectRoot, runId) { + mkdirSync(join(projectRoot, '.rstack', 'runs', runId), { recursive: true }); + writeFileSync(join(projectRoot, '.rstack', 'runs', runId, 'manifest.json'), JSON.stringify({ run_id: runId })); +} + +test('isSafeRunId rejects traversal and separators', () => { + assert.equal(isSafeRunId('2026-06-02T05-35-04-500Z-build-thing'), true); + assert.equal(isSafeRunId('../../etc'), false); + assert.equal(isSafeRunId('a/b'), false); + assert.equal(isSafeRunId('..'), false); + assert.equal(isSafeRunId(''), false); + assert.equal(isSafeRunId(null), false); +}); + +test('parseApprovalQueueId rejects ids that decode to a traversal runId', () => { + const evil = 'gate:' + encodeURIComponent('../../../../tmp/evil') + '::' + encodeURIComponent('plan.md'); + assert.equal(parseApprovalQueueId(evil), null); + const evilArtifact = 'gate:' + encodeURIComponent('run-1') + '::' + encodeURIComponent('../../escape'); + assert.equal(parseApprovalQueueId(evilArtifact), null); + // A legitimate id still round-trips. + const good = approvalQueueId({ runId: 'run-1', taskId: '004-impl', artifact: 'plan.md' }); + assert.deepEqual(parseApprovalQueueId(good), { runId: 'run-1', taskId: '004-impl', artifact: 'plan.md' }); +}); + +test('appendRunApproval refuses to write outside .rstack/runs', async () => { + const projectRoot = mkdtempSync(join(tmpdir(), 'rstack-sec-')); + const outside = mkdtempSync(join(tmpdir(), 'rstack-outside-')); + const traversal = '..' + '/'.repeat(1) + '..' + '/' + outside.split('/').pop(); + + const result = await appendRunApproval(projectRoot, traversal, { artifact: 'plan.md', status: 'APPROVED', approver: 'x' }); + assert.equal(result, null, 'unsafe runId writes nothing'); + // Nothing landed in the sibling temp dir. + assert.equal(existsSync(join(outside, 'approvals.json')), false); + + // A real run still works. + realRun(projectRoot, 'good-run'); + const ok = await appendRunApproval(projectRoot, 'good-run', { artifact: 'plan.md', status: 'APPROVED', approver: 'x' }); + assert.ok(ok && existsSync(join(projectRoot, '.rstack', 'runs', 'good-run', 'approvals.json'))); + + rmSync(projectRoot, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); +}); + +test('resolveApproval rejects a traversal gate id and requires a real run', async () => { + const projectRoot = mkdtempSync(join(tmpdir(), 'rstack-sec-')); + const evil = approvalQueueId({ runId: '../../../../tmp/pwn', taskId: 't', artifact: 'plan.md' }); + assert.equal(await resolveApproval(projectRoot, evil, 'approved', 'Manager'), false, 'traversal id refused'); + + // Safe shape but no such run (no manifest) → refused, no file created. + const ghost = approvalQueueId({ runId: 'ghost-run', taskId: 't', artifact: 'plan.md' }); + assert.equal(await resolveApproval(projectRoot, ghost, 'approved', 'Manager'), false); + assert.equal(existsSync(join(projectRoot, '.rstack', 'runs', 'ghost-run')), false); + rmSync(projectRoot, { recursive: true, force: true }); +}); + +test('manager allow-list blocks names outside policy', async () => { + const projectRoot = mkdtempSync(join(tmpdir(), 'rstack-sec-')); + realRun(projectRoot, 'run-x'); + mkdirSync(join(projectRoot, '.rstack'), { recursive: true }); + writeFileSync(join(projectRoot, '.rstack', 'policy.json'), JSON.stringify({ managers: ['Maya'] })); + const id = approvalQueueId({ runId: 'run-x', taskId: 't', artifact: 'plan.md' }); + + await assert.rejects( + () => resolveApproval(projectRoot, id, 'approved', 'Imposter', { env: {} }), + /not allowed by manager policy/, + ); + // The configured manager succeeds and records token-verified actor evidence. + const ok = await resolveApproval(projectRoot, id, 'approved', 'Maya', { + env: {}, skipRunWrite: true, actor: { name: 'Maya', via: 'dashboard', tokenVerified: true, ts: 'now' }, + }); + assert.equal(ok, true); + rmSync(projectRoot, { recursive: true, force: true }); +});