feat(extension): add extension registry store module#299
Conversation
✅MegaLinter analysis: Success
See detailed reports in MegaLinter artifacts MegaLinter is graciously provided by OX Security |
|
Alternative design: filesystem as registry (no After looking at how
How the five operations become trivial:
Entrypoint discovery: require extension authors to declare it in {
"name": "@elastic/my-extension",
"bin": {
"elastic-my-extension": "./bin/elastic-my-extension"
}
}Security improvement: tampering with extension A now requires writing into extension A's own directory. A malicious extension can still do that (same user), but it can no longer silently poison all extensions by editing one shared file. Pairing with a hash stored in the per-extension This is a larger architectural call than what this PR is scoped to, so raising it now before the store interface is built on top of -- easier to change the shape here than after the install/remove/dispatch layers are wired in. |
| it('creates the parent directory if missing', async () => { | ||
| const nested = join(tmpDir, 'subdir', 'extensions.json') | ||
| _testSetRegistryPath(nested) | ||
| await writeExtensions([ext1]) | ||
| const raw = await readFile(nested, 'utf-8') | ||
| assert.deepEqual(JSON.parse(raw), [ext1]) | ||
| _testSetRegistryPath(registryFile) | ||
| }) |
There was a problem hiding this comment.
If this fails mid-test, the registry path won't be reset, which could leak into other tests, making them fail.
Consider using a try/finally block here to ensure that the state is always cleaned up
There was a problem hiding this comment.
good catch, fixed with try/finally
| const filtered = extensions.filter((e) => e.name !== name) | ||
| await writeExtensions(filtered) |
There was a problem hiding this comment.
| const filtered = extensions.filter((e) => e.name !== name) | |
| await writeExtensions(filtered) | |
| const filtered = extensions.filter((e) => e.name !== name) | |
| if (filtered.length === extensions.length) return | |
| await writeExtensions(filtered) |
We should only write the extensions if one was actually removed. Currently it writes regardless instead of no-oping
There was a problem hiding this comment.
fixed -- early return added so we skip the write when nothing changed
| if (idx === -1) { | ||
| extensions.push(entry) | ||
| } else { | ||
| extensions[idx] = entry | ||
| } | ||
| await writeExtensions(extensions) |
There was a problem hiding this comment.
| if (idx === -1) { | |
| extensions.push(entry) | |
| } else { | |
| extensions[idx] = entry | |
| } | |
| await writeExtensions(extensions) | |
| const updated = idx === -1 | |
| ? [...extensions, entry] | |
| : extensions.map((e, i) => (i === idx ? entry : e)) | |
| await writeExtensions(updated) |
Nit: prefer to treat arrays as immutable instead of mutating in-place
There was a problem hiding this comment.
went ahead and made the change... you're right it's cleaner. the array is a fresh local read on every call so there's no correctness difference, but prefer immutable too for consistency
|
|
||
| return { | ||
| name, | ||
| source: obj['source'] as string, |
There was a problem hiding this comment.
source field has no format validation. It accepts any non-empty string. If the expected formats are github:org/repo and npm:package, a regex validation here (like SAFE_NAME_RE for names) would catch invalid registry entries early and give callers better error messages.
There was a problem hiding this comment.
good point, added a loose regex check (SAFE_SOURCE_RE) that validates the prefix is a known format -- kept it intentionally loose so future source types (e.g. git:) aren't rejected. still tight enough to catch arbitrary strings like you described
- add try/finally in parent-dir test so registry path is always reset - skip write in removeExtension when no entry matched (no-op as documented) - add loose source format validation in validateEntry so tampered registry entries with unrecognised source strings are rejected on read
|
@MattDevy Really appreciate the detailed writeup, the gh comparison is useful context. The main reason we landed on a central json file over the directory structure is the On the security side i think both models have roughly the same exposure in the same-user threat category... in the filesystem approach a malicious extension can still overwrite its own `package.json` bin field and get re-spawned from a tampered entrypoint next time. The blast radius is narrower (one extension vs all) but the attack still works. The thing that would actually close the gap is storing an install-time hash and verifying it before spawn... that's worth a separate issue regardless of which design we go with. The one place the filesystem model does win is concurrent writes... if two CLI processes ran simultaneously the directory approach avoids the read-modify-write race on extensions.json, though in practice the CLI is single-invocation so this probably doesn't matter. I'll address this offline with you for further discussion about pros and cons. |
Summary
Closes #293. Part of #222.
src/extension/store.tswith five exports:readExtensions,writeExtensions,findExtension,upsertExtension,removeExtension~/.elastic/extensions.json; parent directory is created automatically_testSetRegistryPathseam lets tests redirect to a tmp dir without touching the real home directoryTest plan
npx tsc --noEmitpasses cleannode --import tsx/esm --test test/extension/store.test.ts-- 14/14 pass