Answers Filbert's round 1 (#1518 comment 26724): launch.by must hold role.launch, launch is null when limits.authority drops it, a test for cross-role narrowing, and the business file checked on its open descriptor. Co-Authored-By: Claude Opus 5.5 <[email protected]>
249 lines
12 KiB
Diff
249 lines
12 KiB
Diff
--- r1/packages/business/README.md
|
|
+++ r2/packages/business/README.md
|
|
@@ -69,7 +69,9 @@
|
|
it to `<configDir>/businesses/mosaic-stack.json`, put in the bot ids and
|
|
token dates from `docs/guides/slice-1-identities.md`, fix the token paths
|
|
and run `validate`. The file must belong to you and must not be writable
|
|
-by group or other.
|
|
+by group or other. The loader checks the owner and mode on the descriptor
|
|
+it reads from, so the file can't be swapped between the check and the
|
|
+read.
|
|
|
|
Each role instance names a `definition` (a version 2 role file), an
|
|
optional `holder`, its Vikunja bot (`tracker`, required when the definition
|
|
@@ -87,9 +89,10 @@
|
|
`rotateBy`, prints a warning.
|
|
|
|
`launch` lets one instance (`by`) start the listed instances, up to `max`
|
|
-sessions per model family (Opus 4, Sonnet 4 at most). Only that instance
|
|
-keeps `role.launch` as within-role. Every other instance loses it, so for
|
|
-them it's gated.
|
|
+sessions per model family (Opus 4, Sonnet 4 at most). `by` must name an
|
|
+instance whose definition holds `role.launch` within-role; otherwise the
|
|
+file refuses. Only that instance keeps `role.launch`. Every other instance
|
|
+loses it, so for them it's gated.
|
|
|
|
### Project file
|
|
|
|
@@ -143,7 +146,10 @@
|
|
`authority.crossRole`), `credentials` (references only), `tracker` (`bot`,
|
|
`botId` or null), `launch` (the launch block, or null) and `digest`, the
|
|
SHA-256 of the record's canonical JSON. `classify` refuses an action
|
|
-outside the vocabulary.
|
|
+outside the vocabulary. `launch` is set exactly when `classify(record,
|
|
+"role.launch")` is `within`. If `limits.authority` leaves out
|
|
+`role.launch` for the launcher, `launch` is null. A launcher should
|
|
+still ask `classify`, not read `launch` alone.
|
|
|
|
Errors are `BusinessError` with `exitCode` 2 (invalid) or 4 (a required
|
|
file missing). The CLI adds 3 for a system config problem and 4 for usage.
|
|
--- r1/packages/business/src/business.mjs
|
|
+++ r2/packages/business/src/business.mjs
|
|
@@ -3,7 +3,6 @@
|
|
// it and never writes it. A missing or invalid file refuses (lead decision
|
|
// 46, 6.1).
|
|
|
|
-import { lstatSync } from "node:fs";
|
|
import { dirname, isAbsolute, join, normalize } from "node:path";
|
|
import { homedir } from "node:os";
|
|
import { refuse } from "./errors.mjs";
|
|
@@ -112,12 +111,16 @@
|
|
return { roles: out, definitions };
|
|
}
|
|
|
|
-function checkLaunch(launch, roles, file) {
|
|
+function checkLaunch(launch, roles, definitions, file) {
|
|
const where = `${file} launch`;
|
|
requireObject(launch, where);
|
|
rejectUnknownKeys(launch, ["by", "instances", "max"], where);
|
|
requireId(launch.by, `${where}.by`);
|
|
if (!Object.hasOwn(roles, launch.by)) refuse(`${where}.by names ${launch.by}, which the business file doesn't declare`);
|
|
+ const launcher = definitions[roles[launch.by].definition];
|
|
+ if (!launcher.authority.withinRole.includes("role.launch")) {
|
|
+ refuse(`${where}.by names ${launch.by}, whose role ${launcher.name} doesn't hold role.launch within-role`);
|
|
+ }
|
|
const instances = requireDistinctList(launch.instances, `${where}.instances`, (name) => {
|
|
requireId(name, `${where}.instances entry`);
|
|
if (!Object.hasOwn(roles, name)) refuse(`${where}.instances names ${name}, which the business file doesn't declare`);
|
|
@@ -180,7 +183,7 @@
|
|
refuse(`${file} tracker.sync must use its own bot, not the one roles.${instance} uses`);
|
|
}
|
|
}
|
|
- const launch = document.launch === undefined ? null : checkLaunch(document.launch, roles, file);
|
|
+ const launch = document.launch === undefined ? null : checkLaunch(document.launch, roles, definitions, file);
|
|
|
|
return deepFreeze({
|
|
businessVersion: 1, id: document.id, file, human, arbiters, projects, vars, tracker, roles, launch, definitions,
|
|
@@ -193,9 +196,9 @@
|
|
export function loadBusiness(id, { dir = configDir(), rolesDir }) {
|
|
if (!rolesDir) throw new Error("loadBusiness needs rolesDir");
|
|
const file = businessFilePath(id, dir);
|
|
- const document = readJsonFile(file, "business file");
|
|
- const stat = lstatSync(file);
|
|
- if (stat.uid !== process.getuid()) refuse(`business file must belong to uid ${process.getuid()}: ${file}`);
|
|
- if ((stat.mode & 0o022) !== 0) refuse(`business file must not be writable by group or other (mode ${(stat.mode & 0o777).toString(8)}): ${file}`);
|
|
+ const document = readJsonFile(file, "business file", (stat) => {
|
|
+ if (stat.uid !== process.getuid()) refuse(`business file must belong to uid ${process.getuid()}: ${file}`);
|
|
+ if ((stat.mode & 0o022) !== 0) refuse(`business file must not be writable by group or other (mode ${(stat.mode & 0o777).toString(8)}): ${file}`);
|
|
+ });
|
|
return validateBusinessDocument(document, file, { rolesDir });
|
|
}
|
|
--- r1/packages/business/src/resolve.mjs
|
|
+++ r2/packages/business/src/resolve.mjs
|
|
@@ -68,8 +68,10 @@
|
|
crossRole = crossRole.filter((a) => vars["limits.authority"].includes(a));
|
|
}
|
|
// role.launch needs the business file's launch block naming this
|
|
- // instance (addendum A section 8). Without it the verb is gated.
|
|
- const launch = business.launch && business.launch.by === instance ? business.launch : null;
|
|
+ // instance (addendum A section 8), and the verb must survive
|
|
+ // limits.authority. Otherwise the verb is gated and `launch` is null,
|
|
+ // so the two never disagree.
|
|
+ const launch = business.launch && business.launch.by === instance && withinRole.includes("role.launch") ? business.launch : null;
|
|
if (!launch) {
|
|
withinRole = withinRole.filter((a) => a !== "role.launch");
|
|
crossRole = crossRole.filter((a) => a !== "role.launch");
|
|
--- r1/packages/business/src/util.mjs
|
|
+++ r2/packages/business/src/util.mjs
|
|
@@ -1,4 +1,4 @@
|
|
-import { lstatSync, readFileSync } from "node:fs";
|
|
+import { closeSync, constants, fstatSync, openSync, readFileSync } from "node:fs";
|
|
import { createHash } from "node:crypto";
|
|
import { refuse } from "./errors.mjs";
|
|
import { ID_PATTERN } from "./vocabulary.mjs";
|
|
@@ -59,17 +59,29 @@
|
|
}
|
|
|
|
// Read a JSON file that must be a regular file, not a symbolic link.
|
|
-// Missing or not a regular file is 4; unparseable is 2.
|
|
-export function readJsonFile(file, what) {
|
|
- let stat;
|
|
+// Missing or not a regular file is 4; unparseable is 2. One descriptor,
|
|
+// opened without following a link, serves every check and the read.
|
|
+// `checkStat` sees that descriptor's stat before the read, so the file
|
|
+// can't be swapped between the check and the read.
|
|
+export function readJsonFile(file, what, checkStat) {
|
|
+ let fd;
|
|
try {
|
|
- stat = lstatSync(file);
|
|
- } catch {
|
|
- refuse(`${what} not found: ${file}`, 4);
|
|
+ fd = openSync(file, constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK);
|
|
+ } catch (error) {
|
|
+ if (error.code === "ENOENT") refuse(`${what} not found: ${file}`, 4);
|
|
+ refuse(`${what} must be a regular, non-symbolic-link file: ${file}`, 4);
|
|
+ }
|
|
+ let text;
|
|
+ try {
|
|
+ const stat = fstatSync(fd);
|
|
+ if (!stat.isFile()) refuse(`${what} must be a regular, non-symbolic-link file: ${file}`, 4);
|
|
+ checkStat?.(stat);
|
|
+ text = readFileSync(fd, "utf8");
|
|
+ } finally {
|
|
+ closeSync(fd);
|
|
}
|
|
- if (!stat.isFile() || stat.isSymbolicLink()) refuse(`${what} must be a regular, non-symbolic-link file: ${file}`, 4);
|
|
try {
|
|
- return JSON.parse(readFileSync(file, "utf8"));
|
|
+ return JSON.parse(text);
|
|
} catch (error) {
|
|
refuse(`${what} is not valid JSON (${file}): ${error.message}`);
|
|
}
|
|
--- r1/packages/business/tests/business.test.mjs
|
|
+++ r2/packages/business/tests/business.test.mjs
|
|
@@ -1,7 +1,9 @@
|
|
import { test } from "node:test";
|
|
import assert from "node:assert/strict";
|
|
-import { chmodSync, lstatSync, readdirSync, readFileSync, symlinkSync, writeFileSync } from "node:fs";
|
|
+import { execFileSync, spawnSync } from "node:child_process";
|
|
+import { chmodSync, lstatSync, mkdirSync, readdirSync, readFileSync, symlinkSync, writeFileSync } from "node:fs";
|
|
import { join } from "node:path";
|
|
+import { pathToFileURL } from "node:url";
|
|
import { BusinessError, businessFilePath, configDir, loadBusiness, validateBusinessDocument } from "../src/index.mjs";
|
|
import { businessDoc, REPO, REPO_ROLES, rolesCopy, tmp, writeJson } from "./helpers.mjs";
|
|
|
|
@@ -118,6 +120,7 @@
|
|
test("launch", () => {
|
|
assert.equal(check((d) => { delete d.launch; }).launch, null);
|
|
refuses(() => check((d) => { d.launch.by = "ghost"; }), /launch\.by names ghost/);
|
|
+ refuses(() => check((d) => { d.launch.by = "cto"; d.launch.instances = ["coder"]; }), /launch\.by names cto, whose role cto doesn't hold role\.launch within-role/);
|
|
refuses(() => check((d) => { d.launch.instances = []; }), /must not be empty/);
|
|
refuses(() => check((d) => { d.launch.instances = ["coder", "coder"]; }), /duplicate/);
|
|
refuses(() => check((d) => { d.launch.instances = ["ghost"]; }), /instances names ghost/);
|
|
@@ -152,9 +155,31 @@
|
|
|
|
writeFileSync(join(dir, "businesses", "broken.json"), "{ not json");
|
|
refuses(() => loadBusiness("broken", { dir, rolesDir: REPO_ROLES }), /not valid JSON/);
|
|
+ // The owner and mode checks run on the opened file before the read, so
|
|
+ // a group-writable file refuses for its mode even when it won't parse.
|
|
+ chmodSync(join(dir, "businesses", "broken.json"), 0o660);
|
|
+ refuses(() => loadBusiness("broken", { dir, rolesDir: REPO_ROLES }), /writable by group or other \(mode 660\)/);
|
|
assert.throws(() => loadBusiness("acme", { dir }), /needs rolesDir/);
|
|
});
|
|
|
|
+// A directory or a FIFO under the business name refuses without a read.
|
|
+// Opening a FIFO without O_NONBLOCK would block the whole process, which
|
|
+// node:test can't time out, so that case runs in a child with a timeout.
|
|
+test("loadBusiness: not a regular file", () => {
|
|
+ const dir = join(tmp(), "config");
|
|
+ mkdirSync(join(dir, "businesses", "folder.json"), { recursive: true });
|
|
+ refuses(() => loadBusiness("folder", { dir, rolesDir: REPO_ROLES }), /must be a regular, non-symbolic-link file/, 4);
|
|
+ execFileSync("mkfifo", [join(dir, "businesses", "pipe.json")]);
|
|
+ const index = pathToFileURL(join(REPO, "packages", "business", "src", "index.mjs")).href;
|
|
+ const child = spawnSync(process.execPath, ["--input-type=module", "-e", `
|
|
+ import { loadBusiness } from ${JSON.stringify(index)};
|
|
+ try { loadBusiness("pipe", { dir: ${JSON.stringify(dir)}, rolesDir: ${JSON.stringify(REPO_ROLES)} }); }
|
|
+ catch (error) { console.log(error.exitCode, error.message); }
|
|
+ `], { encoding: "utf8", timeout: 5000 });
|
|
+ assert.equal(child.signal, null, "opening the FIFO waited for a writer");
|
|
+ assert.match(child.stdout, /^4 .*must be a regular, non-symbolic-link file/);
|
|
+});
|
|
+
|
|
test("loading writes nothing", () => {
|
|
const root = tmp();
|
|
const dir = join(root, "config");
|
|
--- r1/packages/business/tests/resolve.test.mjs
|
|
+++ r2/packages/business/tests/resolve.test.mjs
|
|
@@ -97,8 +97,39 @@
|
|
const noLaunch = resolve(setup((d) => { delete d.launch; }), "pm");
|
|
assert.equal(classify(noLaunch, "role.launch"), "gated");
|
|
assert.equal(noLaunch.launch, null);
|
|
- const otherLauncher = resolve(setup((d) => { d.launch.by = "cto"; d.launch.instances = ["coder"]; }), "pm");
|
|
- assert.equal(classify(otherLauncher, "role.launch"), "gated");
|
|
+
|
|
+ // A second pm instance is the launcher, so the first one may not launch.
|
|
+ const two = setup((d) => {
|
|
+ d.roles.pm2 = { ...d.roles.pm, holder: undefined, tracker: { bot: "bot-acme-pm2", botId: 7 } };
|
|
+ d.launch.by = "pm2";
|
|
+ });
|
|
+ assert.equal(classify(resolve(two, "pm"), "role.launch"), "gated");
|
|
+ assert.equal(resolve(two, "pm").launch, null);
|
|
+ assert.equal(classify(resolve(two, "pm2"), "role.launch"), "within");
|
|
+ assert.equal(resolve(two, "pm2").launch.by, "pm2");
|
|
+});
|
|
+
|
|
+test("limits.authority without role.launch leaves the launcher with no launch block", () => {
|
|
+ for (const layer of ["agent", "project"]) {
|
|
+ const authority = ["task.create", "task.assign", "message.send"];
|
|
+ const s = layer === "agent"
|
|
+ ? setup((d) => { d.roles.pm.vars["limits.authority"] = authority; })
|
|
+ : setup(undefined, { roles: { pm: { vars: { "limits.authority": authority } } } });
|
|
+ const pm = resolve(s, "pm");
|
|
+ assert.equal(classify(pm, "role.launch"), "gated", layer);
|
|
+ assert.equal(pm.launch, null, layer);
|
|
+ }
|
|
+});
|
|
+
|
|
+test("limits.authority narrows cross-role actions too", () => {
|
|
+ const s = setup((d) => {
|
|
+ d.roles.coder.vars["limits.authority"] = ["task.update.assigned", "git.push.working", "task.scope.change"];
|
|
+ });
|
|
+ const coder = resolve(s, "coder");
|
|
+ assert.deepEqual(coder.limits.authority, { withinRole: ["task.update.assigned", "git.push.working"], crossRole: ["task.scope.change"] });
|
|
+ assert.equal(classify(coder, "task.reassign"), "gated");
|
|
+ assert.equal(classify(coder, "task.scope.change"), "cross");
|
|
+ assert.equal(classify(coder, "review.request"), "gated");
|
|
});
|
|
|
|
test("classify", () => {
|