fix(cli): make Matrix recovery paths tolerate stale plugin config d...#53180
Conversation
…claw#52899) 1. Narrow loadConfigForInstall() to catch only INVALID_CONFIG errors, letting real failures (fs permission, OOM) propagate. 2. Assert allow array is properly cleaned in stale-cleanup test. 3. Add comment clarifying version-resolution is already addressed via the shared VERSION constant. 4. Run cleanStaleMatrixPluginConfig() during install so persistPluginInstall() → writeConfigFile() does not fail validation on stale Matrix load paths.
openclaw-barnacle
bot
added
cli
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🔵 Prototype pollution via object spread in removePluginFromConfig when processing user-controlled config
Description
This is now more reachable because the new Matrix “stale config cleanup” path calls Vulnerable code (object spread over untrusted object): const pluginsConfig = cfg.plugins ?? {};
...
const newPlugins = {
...pluginsConfig,
entries,
installs,
allow,
load,
slots,
};
...
const config: OpenClawConfig = {
...cfg,
plugins: Object.keys(cleanedPlugins).length > 0 ? cleanedPlugins : undefined,
};Impact depends on how polluted objects are later used, but typical risks include:
RecommendationAvoid spreading/assigning untrusted objects into plain Example fix: function pickPluginsConfig(input: unknown) {
const out: Record<string, unknown> = Object.create(null);
if (!input || typeof input !== "object") return out;
const obj = input as Record<string, unknown>;
// Copy only known-safe keys
for (const key of ["entries", "installs", "allow", "load", "slots"]) {
if (Object.prototype.hasOwnProperty.call(obj, key)) out[key] = obj[key];
}
return out as any;
}
const pluginsConfig = pickPluginsConfig(cfg.plugins);
const newPlugins = Object.assign(Object.create(null), pluginsConfig, {
entries,
installs,
allow,
load,
slots,
});
const config = Object.assign(Object.create(null), cfg, {
plugins: Object.keys(cleanedPlugins).length > 0 ? cleanedPlugins : undefined,
});Additionally, consider rejecting configs containing 2. 🔵 Terminal escape/log injection via untrusted filesystem path in doctor notes
Description
An attacker who can influence the config (or a synced/shared config) could embed control characters (e.g., ANSI escape sequences, newlines, carriage returns) in the path value. When
Vulnerable code: changes: [
`Removed stale Matrix plugin references (${removed.join(", ")}). ` +
`The previous install path no longer exists: ${issue.path}`,
],and the note sink: clackNote(wrapNoteMessage(message), stylePromptTitle(title));No output neutralization is performed to remove control characters from interpolated values. RecommendationNeutralize control characters before writing untrusted values to terminal/log output. Options:
function sanitizeForTerminal(value: string): string {
// Strip C0/C1 control chars and ESC (\u001b) used for ANSI sequences.
return value.replace(/[\u0000-\u001F\u007F-\u009F\u001B]/g, "");
}
// ...
`The previous install path no longer exists: ${sanitizeForTerminal(issue.path)}`
export function note(message: string, title?: string) {
if (isSuppressedByEnv(process.env.OPENCLAW_SUPPRESS_NOTES)) return;
const safe = message.replace(/[\u0000-\u001F\u007F-\u009F\u001B]/g, "");
clackNote(wrapNoteMessage(safe), stylePromptTitle(title));
}Also consider shortening or redacting home directories (e.g., Analyzed PR: #53180 at commit Last updated on: 2026-03-23T22:39:09Z |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b91b344a38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (err) { | ||
| if (extractErrorCode(err) !== "INVALID_CONFIG") { | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
loadConfigForInstall now swallows every INVALID_CONFIG error and proceeds, which means plugins install can run download/copy installers even when the config is invalid for unrelated reasons (for example schema mismatches outside Matrix). In that case the command only fails later during writeConfigFile validation, after filesystem side effects have already happened, leaving partially installed plugin directories from a failed install attempt. Please gate this fallback to the specific stale-Matrix-path case (or revalidate before running installers).
Useful? React with 👍 / 👎.
| if (!issue || issue.kind !== "missing-path") { | ||
| return { config: cfg, changes: [] }; | ||
| } | ||
| const { config, actions } = removePluginFromConfig(cfg, "matrix"); |
There was a problem hiding this comment.
This cleanup delegates to removePluginFromConfig, but that helper only removes plugins.load.paths via install.sourcePath; if a stale source: "path" install record has only installPath (which the type allows), detectPluginInstallPathIssue will still report missing-path while the stale load path survives. The resulting config remains invalid, so the intended reinstall/doctor recovery path can still fail to persist.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes two related Matrix upgrade recovery issues: stale plugin install records that block config validation, and plugin runtime version reporting that could disagree with the CLI's own version string. Key changes:
Confidence Score: 5/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/commands/doctor-config-flow.ts
Line: 93-102
Comment:
**Duplicate install-path diagnostics for stale Matrix config**
When the Matrix install path is stale/missing, users running `openclaw doctor` will see two separate messages about the same underlying issue:
1. A warning emitted by `collectMatrixInstallPathWarnings` (called inside `runMatrixDoctorSequence` above) — e.g. _"Matrix plugin path is missing. Run `openclaw plugins install @openclaw/matrix`…"_
2. The "Doctor changes: Removed stale Matrix plugin references…" note emitted here.
Both messages are accurate and complementary (the warning explains the problem; this note says what `--fix` will clean up), so there's no correctness issue. But a first-time user may be puzzled seeing a "you need to reinstall" warning immediately followed by a "stale refs removed" change note.
A low-friction option is to pass the stale-cleanup result back into `runMatrixDoctorSequence` (or run the check before it) so `collectMatrixInstallPathWarnings` can skip the install-path warning for the `missing-path` case when cleanup is already pending. Alternatively, a small prose note alongside the Doctor-changes output clarifying "After cleanup, reinstall with `openclaw plugins install @openclaw/matrix`" would remove the ambiguity.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(plugins): address review feedback fo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const staleMatrixCleanup = await cleanStaleMatrixPluginConfig(candidate); | ||
| if (staleMatrixCleanup.changes.length > 0) { | ||
| note(staleMatrixCleanup.changes.join("\n"), "Doctor changes"); | ||
| ({ cfg, candidate, pendingChanges, fixHints } = applyDoctorConfigMutation({ | ||
| state: { cfg, candidate, pendingChanges, fixHints }, | ||
| mutation: staleMatrixCleanup, | ||
| shouldRepair, | ||
| fixHint: `Run "${doctorFixCommand}" to remove stale Matrix plugin references.`, | ||
| })); | ||
| } |
There was a problem hiding this comment.
When the Matrix install path is stale/missing, users running openclaw doctor will see two separate messages about the same underlying issue:
- A warning emitted by
collectMatrixInstallPathWarnings(called insiderunMatrixDoctorSequenceabove) — e.g. "Matrix plugin path is missing. Runopenclaw plugins install @openclaw/matrix…" - The "Doctor changes: Removed stale Matrix plugin references…" note emitted here.
Both messages are accurate and complementary (the warning explains the problem; this note says what --fix will clean up), so there's no correctness issue. But a first-time user may be puzzled seeing a "you need to reinstall" warning immediately followed by a "stale refs removed" change note.
A low-friction option is to pass the stale-cleanup result back into runMatrixDoctorSequence (or run the check before it) so collectMatrixInstallPathWarnings can skip the install-path warning for the missing-path case when cleanup is already pending. Alternatively, a small prose note alongside the Doctor-changes output clarifying "After cleanup, reinstall with openclaw plugins install @openclaw/matrix" would remove the ambiguity.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/commands/doctor-config-flow.ts
Line: 93-102
Comment:
**Duplicate install-path diagnostics for stale Matrix config**
When the Matrix install path is stale/missing, users running `openclaw doctor` will see two separate messages about the same underlying issue:
1. A warning emitted by `collectMatrixInstallPathWarnings` (called inside `runMatrixDoctorSequence` above) — e.g. _"Matrix plugin path is missing. Run `openclaw plugins install @openclaw/matrix`…"_
2. The "Doctor changes: Removed stale Matrix plugin references…" note emitted here.
Both messages are accurate and complementary (the warning explains the problem; this note says what `--fix` will clean up), so there's no correctness issue. But a first-time user may be puzzled seeing a "you need to reinstall" warning immediately followed by a "stale refs removed" change note.
A low-friction option is to pass the stale-cleanup result back into `runMatrixDoctorSequence` (or run the check before it) so `collectMatrixInstallPathWarnings` can skip the install-path warning for the `missing-path` case when cleanup is already pending. Alternatively, a small prose note alongside the Doctor-changes output clarifying "After cleanup, reinstall with `openclaw plugins install @openclaw/matrix`" would remove the ambiguity.
How can I resolve this? If you propose a fix, please make it concise.
|
Landed via temp rebase onto
Thanks @lupuletic! |
2 participants
Matrix upgrades can leave config in a state that blocks reinstall/repair, and plugin runtime version reporting can disagree with the actual OpenClaw version.
Closes #52899
Changes:
plugins install, using best-effort config instead of hard-failing in preflight.Testing:
src/cli/plugins-cli.test.ts,src/cli/program/config-guard.test.ts,src/commands/doctor-config-flow.test.ts, andsrc/plugins/runtime/index.test.ts, then run those scoped tests pluspnpm buildto verify the runtime/version boundary still compiles cleanly.AI-assisted (Claude + Codex committee consensus, fully tested).
AI-Assisted PR Checklist