fix(tasks)!: contain the server filesystem tasks by default - #843
Merged
sroussey merged 1 commit intoAug 20, 2026
Merged
Conversation
BREAKING CHANGE: `registerCommonTasks()` in the node and electron builds no
longer registers `FileGrepTask`, `FileLoaderTask` or `FileSedTask`. Call the
new `registerFileSystemTasks()` to restore them. Separately, a local path now
has to resolve inside `config.roots`, which defaults to `[process.cwd()]`
rather than to "anywhere"; pass explicit `roots`, or `allowAnyRoot: true`, to
read outside the working directory. The classes are exported unchanged — only
ambient registry availability and the containment default moved.
Two states existed and neither contained anything. `resolveLocalFilePath`
skipped containment whenever `roots` was `undefined`, on the stated grounds
that "the enforced control is the `filesystem:read` entitlement". It is not:
`TaskGraphRunner` consults an enforcer only under `enforceEntitlements`, and
`ENTITLEMENT_ENFORCER` has no default factory, so the flag without a
registered enforcer throws. Neither is a default. An embedder that called
`registerCommonTasks()` and ran a graph therefore had a task in the ambient
registry that read any path the process could open, and
`assertResolvedPathDeclared` did not narrow it — it recomputes the path from
the same input through the same resolver, so with no roots both sides agree
and every path passes.
`FileLoaderTask.server` was worse than the other two and is why the fix cannot
stop at `roots`: it declared no entitlement at all, honoured no roots, and
reached the filesystem through `url.slice(7)`, which neither percent-decodes
nor rejects a `file://` host. It now runs the same resolver and carries the
same `configSchema()` / static + instance `entitlements()` pair its siblings
do, with `metadata.url` still the caller's path so the output shape is
unchanged.
The registration split is the half that survives untrusted input. A serialized
node is built as `{...item.config, id, defaults}`, so a graph that names
`FileGrepTask` supplies its own config and can state `roots: ["/"]` itself —
no default this package picks constrains it. The only control left is the type
not resolving. The cwd default is the defence for the other case, a trusting
embedder that authored the graph itself.
Root resolution is also order-independent now. `realpathSync(root)` ran inside
the `some()` predicate, which short-circuits: `[good, missing]` returned true
without ever resolving the broken root while `[missing, good]` threw, on
identical input. Every root is resolved before any containment verdict, so a
misconfigured one fails always rather than sometimes — the deterministic
direction and the safer one.
The ~35 existing server tests that constructed these tasks with no `roots`
now state `roots: [testDir]`. That churn is the point: each one says which
directory it means to read.
Coverage Report
File CoverageNo changed files found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two published behaviours in
@workglow/taskschange. Both are the fix.1.
registerCommonTasks()no longer registers the filesystem tasks. In the node and electron builds it used to addFileGrepTask,FileLoaderTaskandFileSedTasktoTaskRegistry. It no longer does.The classes are exported exactly as before — constructing one directly is unchanged. Only ambient registry availability became opt-in.
2. A local path must now resolve inside
config.roots, which defaults to[process.cwd()]. It used to default to no containment at all. An absolute path outside the working directory now needs explicit roots, or the new explicit opt-out:allowAnyRootmust be the literaltrue; omittingrootsnever implies it.In-repo callers of
registerCommonTasks()(8 sites across examples and tests) use none of the file tasks, so nothing in this repo needed the new call.Why
F1 — the server file tasks defaulted to unrestricted filesystem read
resolveLocalFilePathskipped containment wheneverrootswasundefined, on the stated grounds that "the enforced control is thefilesystem:readentitlement". It is not.TaskGraphRunnerconsults an enforcer only underenforceEntitlements, andENTITLEMENT_ENFORCERhas no default factory — so setting the flag without registering one throws. Neither is a default, which leaves exactly two states: off, and explicitly configured. In the off state an embedder that calledregisterCommonTasks()had a task in the ambient registry that read any path the process could open.assertResolvedPathDeclareddid not narrow this. It recomputes the path from the same input through the same resolver — a declare-then-swap guard, not containment — so with no roots both sides agree and every path passes.FileLoaderTask.serverwas strictly worse, and is why the fix cannot stop atroots: it declared no entitlement at all, honoured no roots, and reached the filesystem viaurl.slice(7), which neither percent-decodes nor rejects afile://host. Fixing grep and sed while leaving it registered would have been theatre. It now runs the same resolver and carries the sameconfigSchema()plus static/instanceentitlements()pair its siblings do.metadata.urlis still the caller's path, so the output shape is unchanged.The registration split is the half that survives untrusted input. A serialized node is built as
{...item.config, id, defaults}, so a graph namingFileGrepTasksupplies its own config and can stateroots: ["/"]itself — no default this package picks constrains it. The only control left against hostile graph JSON is the type not resolving at all. The cwd default is the defence for the other case: a trusting embedder that authored the graph itself.F7 — root resolution was order-dependent
realpathSync(root)ran inside thesome()predicate, andsomeshort-circuits. Reproduced:["good", "missing"]returned true without ever resolving the broken root, while["missing", "good"]threw — same config, same input, opposite outcome by array order. Every root is now resolved before any containment verdict is taken, so a misconfigured root fails always rather than sometimes. That is both the deterministic direction and the safer one.The two findings share
LocalFilePath.server.ts, and F7 is meaningless until F1 makesrootsload-bearing, so they ship together.Tests
Every new test was verified to fail before the source fix and pass after (source changes stashed, tests kept):
promise resolved "{ text: 'classified', …}"— it read the filepromise resolvedfor the[good, missing]orderingAdditional property allowAnyRoot is not allowedRegisterFileSystemTasks.test.ts(all 4 registry assertions)registerFileSystemTasksdid not exist; the three types were in the registry afterregisterCommonTasks()Additional property roots is not allowed— the task supported no roots at allNew
packages/test/src/test/task/RegisterFileSystemTasks.test.tspins both directions of the split and that the classes stay constructible without registering.~35 existing server tests that constructed these tasks with no
rootsnow stateroots: [testDir]. That churn is the point — each one now says which directory it means to read. Two exceptions state their intent differently: the/dev/zeronon-regular-file tests useroots: ["/dev"]so the device is inside the root and still refused, and the entitlement tests'/etc/passwddenial usesroots: ["/etc"]so the enforcer is what refuses it rather than containment — otherwise the policy under test would never be consulted.Test output (after the fix)
Whole
packages/test/src/test/task/directory: 1216 passed, 24 skipped, 1 failed. The one failure isFetchTask.test.ts > an HTTP error cancels the response body instead of abandoning it, which fails identically onmainwith these changes stashed — it is a separate, unreleased finding scoped to a follow-up PR.Also clean:
turbo run build-types --filter=@workglow/tasks(5 packages),eslintandprettieron every changed file, andtsgo --noEmitover the six changed test files.Also updated
packages/tasks/README.mdgains a Filesystem Tasks (server builds) section namingregisterFileSystemTasksas the migration and documenting the root policy.node.ts/electron.tsdocblocks explain why registration — not a config default — is the boundary against untrusted graph JSON.rootsdocblock inLocalFilePathOptionsand both task config interfaces: the old "undefined means unrestricted — the enforced control is thefilesystem:readentitlement" sentence was itself the defect and is replaced with the real rule plus a note that the entitlement path is off unlessenforceEntitlementsis set with a registered enforcer.electron.tscarries the identical server registration and is split the same way;browser.tsis untouched, since the cross-platform classes reach http(s) throughFetchUrlTaskand touch no filesystem.Generated by Claude Code