Repository navigation
fix(plugin): validate installed.json records instead of casting them #2307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
79f9b1b
d191f66
f6127ae
d76badf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,6 +84,18 @@ describe('PluginManager', () => { | |
| ]); | ||
| }); | ||
|
|
||
| it('fails the load with the offending index when installed.json holds a bad record', async () => { | ||
| // `load()` dereferences `entry.id` and `entry.root` for every record, so a | ||
| // malformed one has to be rejected at the store instead of crashing here. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This new inline test comment is inside a v2 test body, but the scoped guide allows comments only in the top-of-file AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L13-L13 Useful? React with 👍 / 👎. |
||
| await writeFile( | ||
| join(home, 'plugins', 'installed.json'), | ||
| JSON.stringify({ version: 1, plugins: [null] }), | ||
| 'utf8', | ||
| ); | ||
| const manager = new PluginManager({ kimiHomeDir: home }); | ||
| await expect(manager.load()).rejects.toThrow(/plugins\[0\]/); | ||
| }); | ||
|
|
||
| it('installs a local-path plugin into the managed root', async () => { | ||
| const sourceRoot = await mkdtemp(join(tmpdir(), 'plugin-install-source-')); | ||
| try { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| import { mkdir, readFile, rename, writeFile } from 'node:fs/promises'; | ||
| import path from 'node:path'; | ||
|
|
||
| import { z } from 'zod'; | ||
|
|
||
| import type { | ||
| PluginCapabilityState, | ||
| PluginGithubMetadata, | ||
|
|
@@ -9,6 +11,49 @@ import type { | |
|
|
||
| const INSTALLED_REL = path.join('plugins', 'installed.json'); | ||
|
|
||
| const PluginSourceSchema: z.ZodType<PluginSource> = z.enum([ | ||
| 'local-path', | ||
| 'zip-url', | ||
| 'github', | ||
| ]); | ||
|
|
||
| const PluginGithubMetadataSchema: z.ZodType<PluginGithubMetadata> = z.object({ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a newer install record has extra fields inside Useful? React with 👍 / 👎. |
||
| owner: z.string(), | ||
| repo: z.string(), | ||
| ref: z.object({ | ||
| kind: z.enum(['branch', 'tag', 'sha']), | ||
| value: z.string(), | ||
| }), | ||
| installedSha: z.string().optional(), | ||
| }); | ||
|
|
||
| const PluginCapabilityStateSchema: z.ZodType<PluginCapabilityState> = z.object({ | ||
| mcpServers: z.record(z.string(), z.object({ enabled: z.boolean() })).optional(), | ||
| }); | ||
|
|
||
| /* | ||
| Records are validated field by field rather than trusted from disk: `installed.json` | ||
| is hand-editable, survives downgrades, and is written by older versions, so a | ||
| structurally valid file can still carry records that violate the type. Consumers | ||
| dereference these fields unconditionally — `PluginManager.load` reads `entry.id` | ||
| and `entry.root`, and the TUI renders `github.ref.value` — so an unchecked cast | ||
| turns a bad record into a crash far from the file that caused it. | ||
|
|
||
| Unknown keys are kept (`loose`) so a record written by a newer version round-trips | ||
| through an older one instead of losing fields on the next write. | ||
| */ | ||
| const InstalledRecordSchema = z.looseObject({ | ||
| id: z.string(), | ||
| root: z.string(), | ||
| source: PluginSourceSchema, | ||
| enabled: z.boolean(), | ||
| installedAt: z.string(), | ||
| updatedAt: z.string().optional(), | ||
| originalSource: z.string().optional(), | ||
| capabilities: PluginCapabilityStateSchema.optional(), | ||
| github: PluginGithubMetadataSchema.optional(), | ||
| }); | ||
|
|
||
| export interface InstalledRecord { | ||
| readonly id: string; | ||
| readonly root: string; | ||
|
|
@@ -42,7 +87,17 @@ export async function readInstalled(kimiHomeDir: string): Promise<InstalledFile> | |
| if (typeof parsed !== 'object' || parsed === null || !Array.isArray(parsed.plugins)) { | ||
| throw new Error('installed.json is not a valid InstalledFile object'); | ||
| } | ||
| return parsed; | ||
| const plugins = parsed.plugins.map((entry, index) => { | ||
| const record = InstalledRecordSchema.safeParse(entry); | ||
| if (!record.success) { | ||
| throw new Error( | ||
| `plugins[${index}] is not a valid installed record: ${record.error.message}`, | ||
| { cause: record.error }, | ||
| ); | ||
| } | ||
| return record.data as InstalledRecord; | ||
| }); | ||
| return { ...parsed, plugins }; | ||
| } catch (error) { | ||
| throw new Error( | ||
| `Failed to parse ${filePath}: ${(error as Error).message}`, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the v2 package, comments are constrained to the top-of-file
/** */module header, but this new block comment sits in the body next to schema declarations and explains implementation rationale. Please remove it or fold only the module-level responsibility into the header so the file stays within the v2 comment convention.AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L13-L13
Useful? React with 👍 / 👎.