From 9a649abf85baca4823c1d335d6016fcbcc833794 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Thu, 12 Nov 2026 17:17:19 -0600 Subject: [PATCH] fix(packs): SP4 re-check, the board route method split, human role reserved, distinct step roles (#1873) - serve.mjs: only GET and HEAD /api/board stay the Board read; every other method goes to the pack route, so DELETE and PUT answer its 405 again. The owner-only check moves inside packsHTTP after the method check, so OPTIONS on a root Console stays 405 as before SP4. - runtime.mjs: a --as rename cannot target the definition name human, and a human step role never follows a rename. A step whose mapped roles collide refuses invalid-request at preview, not only at apply. - The no-op apply skip also needs the same pack sha256, so a changed pack with nothing to add still records its entry. - Tests: the --as refusals for human, a step role collision, a pack defining human, a changed saved pack recording its entry, board by PUT, a granted session refused owner-only on every pack route, and a Console without sign-in refused owner-only. --- packages/bus/src/runtime.mjs | 17 ++++++---- packages/bus/tests/packs.test.mjs | 41 +++++++++++++++++++++++- packages/webui/src/packs-http.mjs | 4 ++- packages/webui/src/serve.mjs | 10 +++--- packages/webui/tests/packs-http.test.mjs | 17 ++++++++-- 5 files changed, 74 insertions(+), 15 deletions(-) diff --git a/packages/bus/src/runtime.mjs b/packages/bus/src/runtime.mjs index c81035198..5ee6f6596 100644 --- a/packages/bus/src/runtime.mjs +++ b/packages/bus/src/runtime.mjs @@ -323,7 +323,8 @@ function packOps({ writer, rolesDir, dataRoot, system, registryFor, assertClean, if (!Array.isArray(as) || as.length > 32) bad(); for (const value of as) { const m = typeof value === 'string' ? PACK_AS.exec(value) : null; - if (!m || out[m[1]].has(m[2]) || [...out[m[1]].values()].includes(m[3])) bad(); + // `human` is the reserved step role: no rename may write a definition by that name. + if (!m || out[m[1]].has(m[2]) || [...out[m[1]].values()].includes(m[3]) || (m[1] === 'definitions' && m[3] === 'human')) bad(); out[m[1]].set(m[2], m[3]); } return out; @@ -416,9 +417,11 @@ function packOps({ writer, rolesDir, dataRoot, system, registryFor, assertClean, for (const [name, value] of Object.entries(pack.workflows)) { const target = as.workflows.get(name) ?? name; // The step ids are the pack's own; the organization's workflow keeps title, state and roles. - // A role naming one of this pack's definitions follows that definition's --as rename. - const role = r => Object.hasOwn(pack.definitions, r) ? as.definitions.get(r) ?? r : r; + // A role naming one of this pack's definitions follows that definition's --as rename; the + // reserved `human` role never moves. A step whose mapped roles collide refuses here, not at apply. + const role = r => r !== 'human' && Object.hasOwn(pack.definitions, r) ? as.definitions.get(r) ?? r : r; const workflow = { description: value.description, steps: value.steps.map(s => ({ title: s.title, state: s.state, roles: s.roles.map(role) })) }; + if (workflow.steps.some(s => new Set(s.roles).size !== s.roles.length)) bad(); const path = `${prefix}workflows.${target}`; const current = scope.workflows?.[target]; let mark; @@ -468,11 +471,13 @@ function packOps({ writer, rolesDir, dataRoot, system, registryFor, assertClean, if (typeof args.digest !== 'string') bad(); const preview = await build(id, args, actor, context); if (preview.digest !== args.digest) throw new BusError('pack-plan-stale'); - // An apply that adds nothing and proposes what the last apply of this pack proposed writes - // nothing, so a retry loop does not fill the packs list to its cap. The digest pins the revision. + // An apply of the same pack (same sha256) that adds nothing and proposes what the last apply of + // it proposed writes nothing, so a retry loop does not fill the packs list to its cap. A changed + // pack still records its entry. The digest pins the revision. const { plan } = preview; const last = (writer.read(id).document.packs ?? []).filter(e => sameApply(e, plan.packs.id, plan.packs.origin, plan.packs.project)).at(-1); - if (Object.keys(plan).every(k => k === 'packs') && last && canonicalJson(last.proposedHires ?? []) === canonicalJson(plan.packs.proposedHires)) + if (Object.keys(plan).every(k => k === 'packs') && last && last.sha256 === plan.packs.sha256 + && canonicalJson(last.proposedHires ?? []) === canonicalJson(plan.packs.proposedHires)) return { revision: preview.revision, added: [], pack: preview.pack.id }; return { ...write(id, plan, preview.revision, actor, {}), pack: preview.pack.id }; }, diff --git a/packages/bus/tests/packs.test.mjs b/packages/bus/tests/packs.test.mjs index afe37667e..eecb90cd5 100644 --- a/packages/bus/tests/packs.test.mjs +++ b/packages/bus/tests/packs.test.mjs @@ -203,7 +203,7 @@ test('SP4 --as renames a definition or workflow and pins the hire to the new nam assert.equal(file().definitions.coder, undefined); assert.ok(file().workflows.ship); // A rename onto another pack item's own name would show both as add while one replaced the other. - for (const as of [['definitions.nobody=x'], ['definitions.coder=a', 'definitions.coder=b'], ['definitions.coder=x', 'definitions.pm=x'], ['roles.coder=x'], 'definitions.coder=x', ['definitions.coder=reviewer']]) + for (const as of [['definitions.nobody=x'], ['definitions.coder=a', 'definitions.coder=b'], ['definitions.coder=x', 'definitions.pm=x'], ['roles.coder=x'], 'definitions.coder=x', ['definitions.coder=reviewer'], ['definitions.coder=human'], ['definitions.merge-gate=human']]) await assert.rejects(call('pack.preview.console', { pack: 'coding', as }), /invalid-request/); }); @@ -226,6 +226,45 @@ test('SP4 --as onto another workflow of the pack refuses; a renamed inline defin assert.ok(file().definitions.writer); assert.equal(file().definitions.scribe, undefined); assert.deepEqual(file().workflows.notes.steps[0].roles, ['writer', 'human']); + // A rename onto a role the same step already names refuses at preview, not only at apply. + install(variant('pairkit', p => { + p.definitions = { scribe }; + p.hires = []; + p.workflows = { notes: { description: 'Notes.', steps: [{ id: 'write', title: 'Write it', state: 'todo', roles: ['scribe', 'pm'] }] } }; + })); + await call('pack.preview.console', { pack: 'pairkit' }); + await assert.rejects(call('pack.preview.console', { pack: 'pairkit', as: ['definitions.scribe=pm'] }), /invalid-request/); + // The reserved human step role never follows a rename, even when the pack defines a definition by that name. + install(variant('humankit', p => { + p.definitions = { human: scribe }; + p.hires = []; + p.workflows = { signoff: { description: 'Sign-off.', steps: [{ id: 'sign', title: 'Sign it', state: 'todo', roles: ['human'] }] } }; + })); + const renamed = await call('pack.preview.console', { pack: 'humankit', as: ['definitions.human=bot'] }); + assert.deepEqual(renamed.plan.workflows.signoff.steps[0].roles, ['human']); +}); + +test('SP4 a changed pack with nothing to add still records its entry; an identical repeat writes nothing', async t => { + const { call, file, save } = await harness(t); + save('jason', 'mine', variant('mine')); + // The first apply pins the definitions it adds; the second pins the pack's template refs, so it + // records an entry too. From the second on, the proposals are settled. + for (let i = 0; i < 2; i++) { + const p = await call('pack.preview.console', { pack: 'saved:mine' }); + await call('pack.apply.console', { pack: 'saved:mine', digest: p.digest }); + } + assert.equal(file().packs.length, 2); + save('jason', 'mine', variant('mine', p => { p.version = '1.0.1'; p.description = 'A changed description.'; })); + const changed = await call('pack.preview.console', { pack: 'saved:mine' }); + assert.ok(changed.items.every(i => i.mark === 'same'), JSON.stringify(changed.items)); + await call('pack.apply.console', { pack: 'saved:mine', digest: changed.digest }); + assert.equal(file().packs.length, 3); + assert.equal(file().packs.at(-1).version, '1.0.1'); + assert.notEqual(file().packs.at(-1).sha256, file().packs[1].sha256); + const repeat = await call('pack.preview.console', { pack: 'saved:mine' }); + const out = await call('pack.apply.console', { pack: 'saved:mine', digest: repeat.digest }); + assert.deepEqual(out.added, []); + assert.equal(file().packs.length, 3); }); test('SP4 an instance by the hire name drops the proposal; launch.noHire marks it', async t => { diff --git a/packages/webui/src/packs-http.mjs b/packages/webui/src/packs-http.mjs index 5bdc826ba..e5369a9e4 100644 --- a/packages/webui/src/packs-http.mjs +++ b/packages/webui/src/packs-http.mjs @@ -51,7 +51,7 @@ async function body(req, required, optional) { return value; } -export async function packsHTTP(req, res, { path, read, write, session, live, origin }) { +export async function packsHTTP(req, res, { path, owner, read, write, session, live, origin }) { const refuse = (status, error) => json(res, status, { error }); const fail = error => { const code = codes[error?.code] ? error.code : 'outcome-unknown'; return refuse(codes[code] ?? 503, code); }; const match = PACKS_ROUTE.exec(path); @@ -64,6 +64,8 @@ export async function packsHTTP(req, res, { path, read, write, session, live, or const list = path === '/api/packs'; const allowed = list ? ['GET'] : name !== null ? ['POST', 'DELETE'] : ['POST']; if (!allowed.includes(req.method)) return refuse(405, 'method-not-allowed'); + // A method the route never serves answers 405 first; every served method is the root owner's. + if (!owner) return refuse(403, 'owner-only'); if (!session) return refuse(401, 'sign-in-required'); if (!read) return refuse(503, 'not-configured'); const writes = !list && step !== 'preview'; diff --git a/packages/webui/src/serve.mjs b/packages/webui/src/serve.mjs index bb23c7e9e..14ef40970 100644 --- a/packages/webui/src/serve.mjs +++ b/packages/webui/src/serve.mjs @@ -934,8 +934,8 @@ export async function startServer({ host = '127.0.0.1', port = 7330, board = DEF if (sso !== null && !named && !(path === '/healthz' && req.method === 'GET')) return json(res, 403, { error: 'public Host required' }); // No CORS. JSON content type and same-origin checks keep browser forms out. const decisionPath = DECISION_ROUTE.test(path), requestPath = REQUEST_ROUTE.test(path), remotePath = PROJECT_REMOTE_ROUTE.test(path), systemTypesPath = SYSTEM_TYPES_ROUTE.test(path), autonomyPath = AUTONOMY_ROUTE.test(path), - // GET /api/board stays the Board read; only POST /api/board is the pack route's board write. - packsPath = PACKS_ROUTE.test(path) && (path !== '/api/board' || req.method === 'POST'); + // GET and HEAD /api/board stay the Board read; every other method on it is the pack route's (POST writes, the rest 405). + packsPath = PACKS_ROUTE.test(path) && !(path === '/api/board' && (req.method === 'GET' || req.method === 'HEAD')); if (req.headers.origin && req.headers.origin !== (named ? (sso ? (localOrigin ?? sso.publicOrigin) : `https://${preview.host}`) : `http://${req.headers.host}`)) return decisionPath ? decisionRefuse(res, 'cross-origin') : requestPath ? requestRefuse(res, 'cross-origin') : json(res, 403, { error: 'cross-origin request refused' }); let signer = preview, ssoSession = null; @@ -1077,9 +1077,9 @@ export async function startServer({ host = '127.0.0.1', port = 7330, board = DEF } // Starter packs SP4 (#1873): the owner's pack, board and workflow routes on the selected organization. if (packsPath) { - if (tester || granted || !named || sso === null) return json(res, 403, { error: 'owner-only' }); - return packsHTTP(req, res, { path, read: requestRead, write: send ? packsWrite : null, - session: ssoSession, live: () => { const live = sso.session(req); return live?.rootOwner === true ? live : null; }, origin: localOrigin ?? sso.publicOrigin }); + const owner = !(tester || granted || !named || sso === null); + return packsHTTP(req, res, { path, owner, read: requestRead, write: send ? packsWrite : null, + session: ssoSession, live: () => { const live = sso.session(req); return live?.rootOwner === true ? live : null; }, origin: localOrigin ?? sso?.publicOrigin }); } if (tester && ['/project-remote.js', '/project-remote.css', '/files.js', '/files.css', '/system-types.js', '/system-types.css'].includes(path)) return json(res, 403, { error: 'owner-only' }); // Row 194 (#1692, D4.1): the tester also launches and terminates. diff --git a/packages/webui/tests/packs-http.test.mjs b/packages/webui/tests/packs-http.test.mjs index 5e25d2ee3..6ebdd96ec 100644 --- a/packages/webui/tests/packs-http.test.mjs +++ b/packages/webui/tests/packs-http.test.mjs @@ -143,6 +143,7 @@ test('SP4 the admission checks refuse before any bus call; a read-only Console s ['cross site', `${ORG}/board`, 'POST', { revision: revisionNow, board: BOARD }, { 'sec-fetch-site': 'cross-site' }, [403, 'cross-origin']], ['preview by GET', `${ORG}/packs/preview`, 'GET', undefined, {}, [405, 'method-not-allowed']], ['board by DELETE', `${ORG}/board`, 'DELETE', { revision: revisionNow }, {}, [405, 'method-not-allowed']], + ['board by PUT', `${ORG}/board`, 'PUT', { revision: revisionNow }, {}, [405, 'method-not-allowed']], ]) { const [status, result] = await call(f, path, method, body, headers); assert.deepEqual([status, result.error], expected, label); @@ -181,8 +182,9 @@ test('SP4 a session that is not the root owner reaches no pack route', async t = const before = readFileSync(f.file); f.revokeOwner(); for (const [path, method, body] of [[`${ORG}/packs`, 'GET'], [`${ORG}/packs/preview`, 'POST', { pack: 'coding' }], [`${ORG}/packs/apply`, 'POST', { pack: 'coding', digest: preview.digest }], [`${ORG}/workflows/one`, 'POST', { revision: preview.revision, workflow: FLOW('one') }], [`${ORG}/board`, 'POST', { revision: preview.revision, board: BOARD }]]) { - const [status] = await call(f, path, method, body); - assert.ok([401, 403, 404].includes(status), `${method} ${path} answered ${status}`); + // revokeOwner leaves a granted user's session: the route refuses it owner-only, POST /api/board included. + const [status, result] = await call(f, path, method, body); + assert.deepEqual([status, result.error], [403, 'owner-only'], `${method} ${path}`); } assert.equal(f.calls.length, asked, 'nothing reached the bus'); assert.deepEqual(readFileSync(f.file), before); @@ -207,3 +209,14 @@ test('SP4 a sign-out while the body is still arriving dispatches nothing', async assert.equal(f.calls.length, asked); assert.deepEqual(readFileSync(f.file), before); }); + +test('SP4 a Console without sign-in serves no pack route; a method the route never serves stays 405', async t => { + const web = await startServer({ port: 0, board: 'http://127.0.0.1:9', log: () => {} }); + t.after(() => close(web)); + const f = { base: `http://127.0.0.1:${web.address().port}` }; + for (const [path, method, body] of [['/api/packs', 'GET'], ['/api/packs/preview', 'POST', { pack: 'coding' }], ['/api/board', 'POST', { revision: '0'.repeat(64), board: BOARD }]]) { + const [status, result] = await call(f, path, method, body); + assert.deepEqual([status, result.error], [403, 'owner-only'], `${method} ${path}`); + } + for (const method of ['OPTIONS', 'PUT', 'DELETE']) assert.equal((await call(f, '/api/board', method))[0], 405, method); +});