fix(plugins): reject path traversal in manifest identity before the install rm - #548
Merged
Conversation
…nstall rm
`identity.id` and `identity.version` from an uploaded manifest are used as
path segments (`<packagesDir>/<id>/<version>`) and that directory is
`fs.rm -rf`'d before the staged package is renamed onto it. Neither field
was charset-validated, so a manifest with id `..` and version `migrations`
resolved to a sibling of the packages root, deleted it, and replaced it with
attacker-controlled content. Reachable without operator interaction via the
remote registry install and via vendored plugins in an imported profile
bundle.
Two independent layers:
- manifestLoader rejects (not sanitises) an id outside the lowercase
npm-name charset and a version that is not semver — a hard `null`,
which the upload path surfaces as `package.manifest_invalid`;
- packageUploadService re-proves containment immediately before the
destructive `fs.rm`, and additionally refuses ids that collide with the
reserved packages-root entries (`node_modules` — the host symlink —
and `index.json`).
6 tasks
Weegy
added a commit
that referenced
this pull request
Jul 30, 2026
…ncy-safe (#552) * feat(plugins): allow .sql in distributed plugin packages A distributed plugin ships its own schema as `migrations/*.sql`, but the zip extractor rejected the extension, so the package failed ingest with `zip.forbidden_extension`. Adding `.sql` was previously refused because it would have escalated the manifest-identity path traversal (an id of `..` with version `migrations` resolved onto the real migrations directory) from a directory delete into arbitrary SQL executed at boot. That traversal was closed in 09ff9cd, and the remaining routes from uploaded content to a migrator-scanned path were re-verified before this change: - all eight SQL migrators resolve their directory from their own `import.meta.url`, the fixed `middleware/migrations`, or an operator env override — never from package content; - the three extraction call sites land only under state dirs (uploaded-packages staging/final, builder previews), never in the shipped code tree; - the `node_modules` symlink at the packages root is charset-valid as an id and is blocked by the reserved-root check, not by the charset gate. Tests pin those conditions rather than just the happy path: `.sql` accepted by the default allowlist, still rejected under an explicit override that omits it, and no `.sql` surviving outside the packages root for a traversing identity, the reserved `node_modules` id, or a Zip-Slip entry name. Shared package-zip fixtures move to `test/_helpers/pluginPackageZip.ts` so both install-pipeline suites use one builder. * fix(migrations): make all 8 SQL migrators safe against concurrent multi-replica boot Every migrator was read-ledger -> filter -> apply with no mutual exclusion, so two replicas booting together both executed the same pending list. `CREATE ... IF NOT EXISTS` masks that; `ALTER TABLE ... ADD CONSTRAINT` does not — the loser gets 42710 and its boot fails. `conductorWebhookEndpointStore.pg.test.ts` already carried a test-level `migrateWithRetry` workaround for the same race. Design (identical in all eight, no shared helper — that is separate work): - Read the ledger BEFORE locking and return early when nothing is pending, so the steady-state boot takes no lock and can never queue behind a migrating replica. - Acquire with `pg_try_advisory_lock(4410, hashtext(<ledger table>))` in a bounded 2s poll, never the blocking `pg_advisory_lock`: three of the eight run inside a plugin `activate()` that ToolPluginRuntime hard-caps at 10s, and the knowledge-graph plugin can already have spent 6s in `waitForPostgres` first. The try-variant also returns a boolean the code actually reads, so the session always knows whether it holds the lock. - Re-read the ledger UNDER the lock, so a replica that queued behind a winner never re-applies what the winner just applied. - A loser that never gets the lock re-reads the ledger: if the winner finished, it continues; otherwise it throws a retryable error naming the pending files. The message says "timed out" so `bootstrap.retryErroredPlugins` classifies it as transient instead of latching the plugin `errored`. - `pg_advisory_unlock` runs on the success path only, inside `try`, and its boolean is read. It is never awaited in `finally`, where it could hang on a half-open connection (these pools set no statement_timeout) or replace the original migration error. Any path that cannot prove the lock was released ends at `client.release(true)`, which destroys the connection and releases the session lock with it. `client.release()` always runs. - `ensureLedger` retries once on 42P07/23505: `CREATE TABLE IF NOT EXISTS` is not atomic against a concurrent `CREATE TABLE` of the same name. Tests cover, for all eight: no lock when there is no work, lock taken before the first migration, the under-lock re-read, the bounded loser error, the loser whose winner finished, an original error preserved through a failing migration, a throwing unlock, an unlock that reports not-held, a driver that models no advisory locks, the ledger DDL race, and that 6s + 2s fits the 10s activate cap. * feat(plugins): allow .sql in packages + make the 8 migrators concurrency-safe Two of three wave-2 packets shipped after review; the third is held back again. .SQL IN THE ZIP ALLOWLIST — unblocked by #548. It was rejected in the first batch because it would have escalated the then-live path traversal from directory delete/replace into arbitrary SQL execution. #548 closed that, and the reviewer verified independently that no route remains from uploaded content to any migrator-scanned path: all 8 migrators derive their directory from import.meta.url or an operator env var, never from package content, and the 3 extraction destinations are staging/preview paths with generated names. The stronger argument, which the analysis had buried: .js/.mjs/.cjs are already allowlisted and dynamic-import()ed by the runtime, so anyone who can get a zip to ingest() already holds in-process code execution. .sql is strictly weaker than what the trust boundary already grants — which makes it security-inert independently of the migrator analysis. Also adds `migrations` to both boilerplate build-zip.mjs INCLUDE lists. Without it the directory is silently dropped at packaging time: the copy loop skips anything not on the list, so an author following the boilerplate gets a green install and no schema. The allowlist entry alone was necessary but not sufficient. MIGRATOR CONCURRENCY — shipped on the third attempt. Every migrator was read-ledger -> filter -> apply with no mutual exclusion, so two replicas booting together both execute; IF NOT EXISTS masks it, ADD CONSTRAINT does not (42710). Two earlier designs were rejected, and both rejections shaped this one: an unbounded pg_advisory_lock is unusable because three of the eight migrators run inside a plugin activate() that ToolPluginRuntime caps at 10s — turning a rare race into a deterministic boot failure; and a retry that called pg_advisory_unlock without reading its boolean return could not distinguish "released" from "never held", making the mechanism silently inert. STILL HELD BACK: the DynamicAgentRuntime rollback, now on its second rejection. It does not cover the timeout path — withTimeout is a bare Promise.race and does not cancel, so after the rollback runs and clears the in-flight marker, the orphaned activate() continues and re-registers. That is the same defect class already documented for ToolPluginRuntime, and it wants a cancellation token rather than another rollback tweak. 5,224 pass, typecheck clean, ratchet held at 3,303. * chore(470): resync wave2 with main (#549) — baseline 3,303 → 3,306 The channel-api work added 3 dev-platform references (test/packages). Same legitimate raise as on the C5 branch.
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.
Fixes a path traversal in the plugin install pipeline that is exploitable today, independent of any epic. Found while planning #470; landing it standalone so it does not wait on unrelated decisions.
The chain
manifestLoader.tsreadsidentity.id/identity.versionfrom the uploaded manifest with no charset validation — the only check isif (!id || !name || !version) return null.packageUploadService.tsbuildsfinalDir = path.join(packagesDir, plugin.id, plugin.version).fs.rm(finalDir, { recursive: true, force: true })followed byfs.rename(packageRoot, finalDir).Verified on this machine:
So a manifest declaring
identity.id: '..'andidentity.version: 'migrations'deletes the real migrations directory and replaces it with attacker-controlled package content. At next boot the migrator does areaddirof exactly that path and executes every.sqlit does not already have in its ledger.Not operator-only. The same
ingest()path is reached fromroutes/registryInstall.ts(ZIPs fetched from a remote plugin registry),profileBundleImporter.ts(plugin ZIPs vendored inside an imported profile bundle),dependencyChainResolver.tsandbuilder/installCommit.ts.Today
.sqlis not in the ZIP extension allowlist, which limits this to arbitrary directory delete/replace rather than SQL execution. That allowlist entry was proposed as part of #470 — this fix is a hard prerequisite for it, and the reason it was pulled from that batch.The fix — two independent layers
Layer 1,
manifestLoader.ts: strict patterns foridentity.id(npm-style, optionally scoped) andidentity.version. Reject, never sanitise.Layer 2,
packageUploadService.ts: re-assert containment before the destructivefs.rm—path.resolve(finalDir)must stay underpath.resolve(packagesDir). Defence in depth: the service layer does not rely on the loader having stayed correct. Also guards a small set of reserved root entries (notably thenode_modulessymlink the store plants, where a rename would land in the host's realnode_modules).Verification
An independent adversarial review pass reproduced every claim on a clean checkout and found no critical, high or medium issues. It probed the patterns directly —
..,.,....,..a,a/../..,/etc/passwd,a/b,@../a,@a/..,%2e%2e, backslash forms, NUL bytes, spaces, uppercase, leading-/_all rejected — and confirmed that values which pass and look suspicious (a..,1.0.0-..,@a../b) still resolve to a single contained path segment.It also checked a symlink bypass is unreachable:
zipExtractorrejects symlink entries outright, and every intermediate path segment is created byfs.mkdir, never from archive content.Compatibility: all 19 in-repo
manifest.yamlidentities and all 8 inomadia-byte5-pluginspass both patterns.AgentIdSchema/SemverSchemain the Builder are strict subsets, so no Builder output is rejected.Test plan
middleware/test/pluginInstallPathTraversal.test.ts, stable across 3 isolated runs..,a/../.., absolute paths, URL-encoded, backslash separators, NUL@omadia/plugin-office) still installfs.rmis proven not reached for a rejected packageKnown follow-ups (low, not blocking)
identity.versionhas no length cap, so a semver-legal 600-char version reachesfs.rmand throwsENAMETOOLONGas a 500 rather than the 422 every other malformed-manifest path returns.package.manifest_invalid("schema_version"), which misdiagnoses the cause; the charset is also undocumented indocs/creating-plugins.md.active. No in-repo or byte5 plugin is affected.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.