diff --git a/changelog.d/2609.fixed.md b/changelog.d/2609.fixed.md new file mode 100644 index 000000000..24a367845 --- /dev/null +++ b/changelog.d/2609.fixed.md @@ -0,0 +1 @@ +Opening, previewing or reading the bytes of a workflow whose id is not a plain file name no longer reaches a JSON file outside the workspace's `workflows/` folder: the read path now accepts only the same ids the save path always did, so an id such as `../outside/stolen` — or a stored default naming one — answers `workflow/not-found` instead of returning that file's definition ([#2609](https://github.com/ThinkFlowLab/FlyBot/issues/2609)). diff --git a/core/src/host/workflow-store.js b/core/src/host/workflow-store.js index d62131a53..cfc995a79 100644 --- a/core/src/host/workflow-store.js +++ b/core/src/host/workflow-store.js @@ -93,7 +93,12 @@ export function createWorkflowStore({ dir, presets = PRESETS, team = () => null, } } + // A file name is a file name on both sides of the store (#2609): `save` has always refused an id + // that is not one, and the read joins the same id onto the folder, so `../outside/stolen` would + // read a JSON file outside `workflows/` — including the stored `default` key `list()` feeds + // back in here. Every read entry point comes through this function, so the check lives here once. const readDefinition = (id) => { + if (!isWorkflowId(id)) return null const preset = presets.map(normaliseWorkflow).find((definition) => definition.id === id) if (preset !== undefined) return { definition: preset, source: 'preset' } const own = readOwn(id) diff --git a/core/test/workflow-store.test.js b/core/test/workflow-store.test.js index ab4ea4e97..a40eaf148 100644 --- a/core/test/workflow-store.test.js +++ b/core/test/workflow-store.test.js @@ -461,3 +461,63 @@ test('#2159: a hard-linked project file refuses to become the default', (t) => { assert.equal(lstatSync(leaf).nlink, 2, 'the file is still another name of the same inode') assert.deepEqual([...new Set(warnings)], [leaf], 'the warning names the file, once') }) + +// --------------------------------------------------------------------------- +// #2609: the read path asks the same question the write path does. An id was joined onto the +// workflow folder unchecked, so `../outside/stolen` read a JSON file written outside the workspace's +// `workflows/` folder and served it as this workspace's own definition. Every read entry point goes +// through `readDefinition`, which is where the guard lives. + +const folderWithOutsideDefinition = (t) => { + const root = mkdtempSync(join(tmpdir(), 'flybot-workflows-')) + t.after(() => rmSync(root, { recursive: true, force: true })) + const dir = join(root, 'workflows') + mkdirSync(dir, { recursive: true }) + const outside = join(root, 'outside') + mkdirSync(outside, { recursive: true }) + writeFileSync(join(outside, 'stolen.json'), JSON.stringify({ ...blankWorkflow, id: 'stolen', name: 'SECRET-OUTSIDE-WORKFLOWS' }, null, 2)) + return { root, dir } +} + +test('#2609: a read of an id that leaves the workflow folder answers not-found', (t) => { + const { dir } = folderWithOutsideDefinition(t) + const store = createWorkflowStore({ dir }) + const id = '../outside/stolen' + const opened = store.open(id) + assert.equal(opened.ok, false) + assert.equal(opened.code, 'workflow/not-found') + assert.equal(JSON.stringify(opened).includes('SECRET-OUTSIDE-WORKFLOWS'), false, 'the outside definition never comes back') + assert.equal(store.bytesOf(id), null, 'no bytes of a file outside the folder') + const copied = store.customize({ from: id, id: 'stolen-copy', name: 'Copy' }) + assert.equal(copied.ok, false) + assert.equal(copied.code, 'workflow/not-found') + assert.deepEqual(readdirSync(dir), [], 'nothing outside the folder was read or copied in') + // The control: a legal id still opens, so the guard did not switch the feature off. + assert.equal(store.customize({ from: 'blank', id: 'blank-mine', name: 'Mine' }).ok, true) + const mine = store.open('blank-mine') + assert.equal(mine.ok, true) + assert.equal(mine.workflow.source, 'workspace') + assert.equal(store.bytesOf('blank-mine').source, 'workspace') + assert.equal(store.open('blank').workflow.source, 'preset', 'a preset id still opens') +}) + +test('#2609: the workflows endpoint previews nothing for an id that leaves the folder', async (t) => { + const { root } = folderWithOutsideDefinition(t) + const rt = { state: { bots: {} }, ctx: { workspaceRegistry: { get: () => ({ path: root }) } }, warn: () => {} } + install(rt) + const previewed = await rt.workflowEndpoint('workflows', { op: 'preview', id: '../outside/stolen' }) + assert.equal(previewed.ok, false) + assert.equal(previewed.code, 'workflow/not-found') + assert.equal(JSON.stringify(previewed).includes('SECRET-OUTSIDE-WORKFLOWS'), false) + assert.equal((await rt.workflowEndpoint('workflows', { op: 'open', id: '../outside/stolen' })).code, 'workflow/not-found') + // The control, at the endpoint the page actually calls. + assert.equal((await rt.workflowEndpoint('workflows', { op: 'open', id: 'blank' })).ok, true) +}) + +test('#2609: a stored default that leaves the folder is answered as none', (t) => { + const { dir } = folderWithOutsideDefinition(t) + writeFileSync(join(dir, 'project.json'), `${JSON.stringify({ default: '../outside/stolen' }, null, 2)}\n`) + const listed = createWorkflowStore({ dir }).list() + assert.equal(listed.default, null, 'a stored key is a file name only when it is one') + assert.deepEqual(listed.own, []) +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 5f07b6645..7e0c14924 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -67,7 +67,7 @@ The server side of the patched `ds-bot` core (`core/`): the part of [ds-bot-core | `core/src/shared/workflow-doc.js` | The definition as a file (#1272 step 3): `workflowBytes(def)` is canonical — fixed key order, two spaces, a trailing newline, a field a definition does not state left out — so a load → save round trip is byte-identical, a definition diffs in git, and `column: false` is written while an absent column is not (they are two different decisions, #1265). `isWorkflowId` is the file-name rule (lowercase, digits, hyphens) and `PRESET_IDS` names the two ids a workspace file may not claim; `copyOfPreset(preset, { id, name })` is what *Customize* writes, and `differingFields(a, b)` is how the copy is held to "differs only in id/name" (acceptance 6). No I/O. Tests: `core/test/workflow-doc.test.js`. | | `core/src/shared/workflow-preview.js` | The guard preview the editor shows (#1272 step 3, acceptance 5): `previewNode(def, nodeId, answer)` asks `workflow.js`'s `guardRows` for one row per guard in the definition's order — `holds` (read and passing, no sentence), `refuse` (the producer's own sentence) or `unreadable` — and when the reader answered nothing (`{ ok: false, why }`) every row says it could not read the fact, with the reason named and no pass anywhere. It reads the same function `enterNode` reads, so the preview and a card's refusal cannot disagree. Tests: `core/test/workflow-preview.test.js`. | | `core/src/shared/workflow-run.js` | The running layer's pure half (#1316, #1272 step 4). `planJob({ definition, job, cards, facts, hold })` answers which node cards a job may create now: every decision is `enterNode`'s, so a node whose input is missing, stale or unreadable opens nothing and its refusal is that function's own sentence byte for byte, and a node that already has a card is in neither list — a card parented to another job is not this job's card, so `cardsOfJob` is what makes `(job, node)` one card across a replayed tick or a restarted host, even when a caller hands in the whole board. An open entry is the card the board already places (`parentId` the job card, `workflow`, `node`) plus the fields the pull queue reads (`role`, `queueRole`, `assignee: null`), so the hand-off is the claim and never an `@`. A held job opens nothing on any node, in the definition's own sentence: the planner renders that definition's `no-hold` guard (`pr` equal to `none`, refused for `hold`) through `renderRefusal`, which is the merge gate's wording byte for byte, and a definition guarding no held pull request gets `refusal: null` and the holder's name instead of a sentence this layer wrote. `jobDone` answers pin 6 — a job is done when its terminal node's outputs are present and current, never because a card moved — and `completions` is step (c)'s pure half: a done card whose declared outputs are current, append-only, an entry already in `history` never returned again. No fs, no clock, no timer, no network, no host import: `task-drive.js` `driveOnce` stays the only beat. Tests: `core/test/workflow-run.test.js`. | -| `core/src/host/workflow-store.js` | Where a definition lives (#1272 step 3): the presets read from their modules plus the owner's `workflows/.json` in the workspace, and the only writer. `save` validates with the engine's own `validateWorkflow` (the roster's roles are the team; an empty roster is no team to check), refuses a preset id (`workflow/preset-read-only` — *Customize* is the only way to start from one), writes `workflow-doc.js`'s bytes only when they changed, and answers `changed: false` for an unedited definition (acceptance 7). `customize`, `setDefault` (the project's `workflows/project.json`), `list`, `open` (definition + bytes + layout + preview) and `definitions()` (the presets plus the workspace files) round it out; `install` wires `rt.workflowEndpoint` for `workflows` (read) and `workflow-save` (write, in `authz.js`'s `TASK_ENDPOINTS`), so the client writes a file through the host and never to GitHub. A file that does not parse is skipped with one warning instead of breaking the list. The `workflows` folder itself must be a real directory too ([#2057](https://github.com/ThinkFlowLab/FlyBot/issues/2057), [#2068](https://github.com/ThinkFlowLab/FlyBot/issues/2068)): `dirKindSync` from `src/host/real-dir.js` is asked before every listing and read, so a link, a plain file or a check that cannot run lists nothing and opens nothing — `own` is empty, `default` is `null`, `open` answers `workflow/not-found`, `definitions()` answers the presets — which is exactly the answer a missing folder gives, and the linked folder is named once per folder and process in one warning (`ds-bot: workflow folder is not a plain directory …; keeping it empty`). The write side follows the same rule: `save`, `customize` and `setDefault` still create a missing folder as before and refuse a linked one instead of writing into it (`workflow/not-a-folder`), and a workflow file that does not parse is reported once per file and process. The file itself is asked as well ([#2079](https://github.com/ThinkFlowLab/FlyBot/issues/2079)): `save` and `setDefault` call `fileKindSync` on `workflows/.json` and `workflows/project.json`, still create a missing file and write a plain one, and refuse a link, a hard link or a directory with `workflow/not-a-file`, named once per file and process. Tests: `core/test/workflow-store.test.js`. | +| `core/src/host/workflow-store.js` | Where a definition lives (#1272 step 3): the presets read from their modules plus the owner's `workflows/.json` in the workspace, and the only writer. `save` validates with the engine's own `validateWorkflow` (the roster's roles are the team; an empty roster is no team to check), refuses a preset id (`workflow/preset-read-only` — *Customize* is the only way to start from one), writes `workflow-doc.js`'s bytes only when they changed, and answers `changed: false` for an unedited definition (acceptance 7). `customize`, `setDefault` (the project's `workflows/project.json`), `list`, `open` (definition + bytes + layout + preview) and `definitions()` (the presets plus the workspace files) round it out; `install` wires `rt.workflowEndpoint` for `workflows` (read) and `workflow-save` (write, in `authz.js`'s `TASK_ENDPOINTS`), so the client writes a file through the host and never to GitHub. A file that does not parse is skipped with one warning instead of breaking the list. The `workflows` folder itself must be a real directory too ([#2057](https://github.com/ThinkFlowLab/FlyBot/issues/2057), [#2068](https://github.com/ThinkFlowLab/FlyBot/issues/2068)): `dirKindSync` from `src/host/real-dir.js` is asked before every listing and read, so a link, a plain file or a check that cannot run lists nothing and opens nothing — `own` is empty, `default` is `null`, `open` answers `workflow/not-found`, `definitions()` answers the presets — which is exactly the answer a missing folder gives, and the linked folder is named once per folder and process in one warning (`ds-bot: workflow folder is not a plain directory …; keeping it empty`). The write side follows the same rule: `save`, `customize` and `setDefault` still create a missing folder as before and refuse a linked one instead of writing into it (`workflow/not-a-folder`), and a workflow file that does not parse is reported once per file and process. The file itself is asked as well ([#2079](https://github.com/ThinkFlowLab/FlyBot/issues/2079)): `save` and `setDefault` call `fileKindSync` on `workflows/.json` and `workflows/project.json`, still create a missing file and write a plain one, and refuse a link, a hard link or a directory with `workflow/not-a-file`, named once per file and process. The read path asks the same question the write path does ([#2609](https://github.com/ThinkFlowLab/FlyBot/issues/2609)): every read entry point (`open`, `preview`, `bytesOf`, `customize`, `setDefault`, and the stored `default` key `list()` feeds back in) goes through `readDefinition`, which accepts only an id `isWorkflowId` allows, so an id such as `../outside/stolen` answers `workflow/not-found` and no file outside `workflows/` is read. Tests: `core/test/workflow-store.test.js`. | | `core/src/host/workflow-facts.js` | The fact providers (#1272 step 2, acceptance 10): one function per fact a guard asks about — `github.labels`, `github.checkAtSha`, `github.verdictAtTip`, `github.mergeable`, `github.prBodyTip`, `task.progress`, `manual.tick` — each taking the answer a reader already got and returning one artifact (spec, revision, value), never fetching. No `GITHUB_TOKEN`, no answer, or an answer missing the field is `{ ok: false, reason: 'unreadable', why }`, never a guess: a fact nobody could read refuses the node it feeds instead of passing it. `workflowFact(name, args)` is the one spelling for a caller that has the name as data, and an unknown name is unreadable with the name in the reason. Tests: `core/test/workflow-repo-dev.test.js`. | | `core/src/shared/remote-label.js` | Owner-badge labels for a friend's Bot and “Shared room from …” titles. `friendBotTitle(bot)` / `friendBotOwner(bot)` name a friend's chat from the roster (`Chief (SelfB1233)`) and its owner (`SelfB1233`, the person an approval card names) with one formatter for every client surface (#498); `friendRecord` reads a published roster record (`roster.remoteBots`, name/owner/home at the top level) as well as a chat's `remote`, so the name does not change when the chat appears (#967). `remoteMentionLabel(record, others)` is the one name a mention wears — that title, and on a title two different handles share the home too (`Chief (Bob) · bob2.home`), so the menu, the bubble, the preview and the host's match cannot drift apart (#967). `remoteBotName` / `isAutoOwnerName` / `seatOwnerName` keep group and face-room labels in that format too (#526): Bot Social's ` · ` member label is not a Bot name, and an owner name made up from an install id (`FlyBot 682b8a43`) gives way to the username in the home handle (`Reviewer (flybot682b8a43)`). `isListedRemoteChat` / `listedChatBots` keep gone, hidden, and home-level (`@something.home`) remotes out of the **发给** picker, mentions, and `list_bots` friends lines: a share that ended is not a place to send to. The sidebar asks a different question through `isShownRemoteChat` (#956): a chat whose Bot left the published list keeps its row, marked *no longer shared*, while only a hide the user set (`bot.hidden`) takes a row out. History stays on disk either way. Since #1916 `cleanPeerName` bounds a peer name through the shared `cut` (`shared/text-boundary.js`), so a 60-unit label never ends in half of an emoji. | diff --git a/patches/ds-bot/files/src/host/workflow-store.js b/patches/ds-bot/files/src/host/workflow-store.js index d62131a53..cfc995a79 100644 --- a/patches/ds-bot/files/src/host/workflow-store.js +++ b/patches/ds-bot/files/src/host/workflow-store.js @@ -93,7 +93,12 @@ export function createWorkflowStore({ dir, presets = PRESETS, team = () => null, } } + // A file name is a file name on both sides of the store (#2609): `save` has always refused an id + // that is not one, and the read joins the same id onto the folder, so `../outside/stolen` would + // read a JSON file outside `workflows/` — including the stored `default` key `list()` feeds + // back in here. Every read entry point comes through this function, so the check lives here once. const readDefinition = (id) => { + if (!isWorkflowId(id)) return null const preset = presets.map(normaliseWorkflow).find((definition) => definition.id === id) if (preset !== undefined) return { definition: preset, source: 'preset' } const own = readOwn(id) diff --git a/patches/ds-bot/files/test/workflow-store.test.js b/patches/ds-bot/files/test/workflow-store.test.js index ab4ea4e97..a40eaf148 100644 --- a/patches/ds-bot/files/test/workflow-store.test.js +++ b/patches/ds-bot/files/test/workflow-store.test.js @@ -461,3 +461,63 @@ test('#2159: a hard-linked project file refuses to become the default', (t) => { assert.equal(lstatSync(leaf).nlink, 2, 'the file is still another name of the same inode') assert.deepEqual([...new Set(warnings)], [leaf], 'the warning names the file, once') }) + +// --------------------------------------------------------------------------- +// #2609: the read path asks the same question the write path does. An id was joined onto the +// workflow folder unchecked, so `../outside/stolen` read a JSON file written outside the workspace's +// `workflows/` folder and served it as this workspace's own definition. Every read entry point goes +// through `readDefinition`, which is where the guard lives. + +const folderWithOutsideDefinition = (t) => { + const root = mkdtempSync(join(tmpdir(), 'flybot-workflows-')) + t.after(() => rmSync(root, { recursive: true, force: true })) + const dir = join(root, 'workflows') + mkdirSync(dir, { recursive: true }) + const outside = join(root, 'outside') + mkdirSync(outside, { recursive: true }) + writeFileSync(join(outside, 'stolen.json'), JSON.stringify({ ...blankWorkflow, id: 'stolen', name: 'SECRET-OUTSIDE-WORKFLOWS' }, null, 2)) + return { root, dir } +} + +test('#2609: a read of an id that leaves the workflow folder answers not-found', (t) => { + const { dir } = folderWithOutsideDefinition(t) + const store = createWorkflowStore({ dir }) + const id = '../outside/stolen' + const opened = store.open(id) + assert.equal(opened.ok, false) + assert.equal(opened.code, 'workflow/not-found') + assert.equal(JSON.stringify(opened).includes('SECRET-OUTSIDE-WORKFLOWS'), false, 'the outside definition never comes back') + assert.equal(store.bytesOf(id), null, 'no bytes of a file outside the folder') + const copied = store.customize({ from: id, id: 'stolen-copy', name: 'Copy' }) + assert.equal(copied.ok, false) + assert.equal(copied.code, 'workflow/not-found') + assert.deepEqual(readdirSync(dir), [], 'nothing outside the folder was read or copied in') + // The control: a legal id still opens, so the guard did not switch the feature off. + assert.equal(store.customize({ from: 'blank', id: 'blank-mine', name: 'Mine' }).ok, true) + const mine = store.open('blank-mine') + assert.equal(mine.ok, true) + assert.equal(mine.workflow.source, 'workspace') + assert.equal(store.bytesOf('blank-mine').source, 'workspace') + assert.equal(store.open('blank').workflow.source, 'preset', 'a preset id still opens') +}) + +test('#2609: the workflows endpoint previews nothing for an id that leaves the folder', async (t) => { + const { root } = folderWithOutsideDefinition(t) + const rt = { state: { bots: {} }, ctx: { workspaceRegistry: { get: () => ({ path: root }) } }, warn: () => {} } + install(rt) + const previewed = await rt.workflowEndpoint('workflows', { op: 'preview', id: '../outside/stolen' }) + assert.equal(previewed.ok, false) + assert.equal(previewed.code, 'workflow/not-found') + assert.equal(JSON.stringify(previewed).includes('SECRET-OUTSIDE-WORKFLOWS'), false) + assert.equal((await rt.workflowEndpoint('workflows', { op: 'open', id: '../outside/stolen' })).code, 'workflow/not-found') + // The control, at the endpoint the page actually calls. + assert.equal((await rt.workflowEndpoint('workflows', { op: 'open', id: 'blank' })).ok, true) +}) + +test('#2609: a stored default that leaves the folder is answered as none', (t) => { + const { dir } = folderWithOutsideDefinition(t) + writeFileSync(join(dir, 'project.json'), `${JSON.stringify({ default: '../outside/stolen' }, null, 2)}\n`) + const listed = createWorkflowStore({ dir }).list() + assert.equal(listed.default, null, 'a stored key is a file name only when it is one') + assert.deepEqual(listed.own, []) +})