fix(extension): resolve symlinks in containment check

[margaretjgu]

assertWithinInstallDir compared resolve()d paths, which normalizes ./.. but doesn't follow symlinks, so a symlinked entrypoint inside the install directory passed the check while pointing anywhere on disk. Switches to realpath on both sides of the comparison and calls the check from createLocalExtension too (it previously only ran on the install path).

Closes #500.

[github-actions]

✅MegaLinter analysis: Success

Descriptor	Linter	Files	Fixed	Errors	Warnings	Elapsed time
✅ COPYPASTE	jscpd	yes		no	no	0.4s
✅ REPOSITORY	gitleaks	yes		no	no	41.47s
✅ REPOSITORY	git_diff	yes		no	no	0.07s
✅ REPOSITORY	secretlint	yes		no	no	1.88s
✅ REPOSITORY	trivy	yes		no	no	15.87s
✅ TYPESCRIPT	eslint	2		0	0	2.51s

Notices

📣 MegaLinter 9.5.0 is out! Discover the new features and security recommendations in the release announcement. (Skip this info by defining SECURITY_SUGGESTIONS: false)

See detailed reports in MegaLinter artifacts
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security
Show us your support by starring ⭐ the repository

[JoshMock]

It looks like upgradeExtension may circumvent the new symlink check. git pull --ff-only could introduce new symlinks within an extension that's already installed, but assertWithinInstallDir is never called.

[JoshMock]

Suggested change

	await writeFile(payload, '#!/bin/sh\necho PAYLOAD RAN FROM OUTSIDE\n', { mode: 0o755 })
	await chmod(payload, 0o755)
	await writeFile(payload, '#!/bin/sh\necho PAYLOAD RAN FROM OUTSIDE\n', { mode: 0o755 })

nit: Setting mode twice is redundant.

[margaretjgu]

already fixed, mode is set once in the writeFile call now, no separate chmod after it.

[margaretjgu]

addressed both flagged issues:

@JoshMock the npm update path had the same gap as git pull, entrypoint wasn't reverified after upgrading. added the same assertWithinInstallDir check there, with a test that runs a real npm update against a symlinked entrypoint.

on the ai review's ENOENT claim for createLocalExtension: not actually a bug, the scaffolded entrypoint file is written before assertWithinInstallDir runs (the write happens inside the if block, the check happens after it), so realpath never sees a missing file. the createLocalExtension test suite covers the no path default scaffold case and passes.

rebased onto main to pick up ignore-scripts and the test run seam from #539, resolved conflicts by combining both.

[JoshMock]

nit: unintentional NOTICE regression?

[margaretjgu]

yess :( but fixed

[github-actions]

The fix is well-motivated and the implementation is correct overall. A few real issues:

Bug: symlink escape test for npm upgrade calls the wrong run path

The test 'rejects a stored entrypoint that is a symlink escaping the install directory after npm update' places the extension at extDir/elastic-npmupgrade but uses source: 'npm:elastic-npmupgrade'. The upgradeExtension npm branch calls run('npm', ['update', ...]) before calling assertWithinInstallDir. The _testSetRun seam is set up in a before() hook that makes run a no-op, so this test path doesn't actually invoke npm — that's fine. But this means the test is genuinely exercising the right code path. No bug here, though the test setup doesn't make that obvious.

Real issue: realpath.native fallback on platforms where it doesn't exist

realpathCb.native is undefined on some platforms (or in some older Node versions in sandboxed environments). promisify(undefined) will throw at module load time with a non-obvious error. It should at minimum guard:

const realpath = promisify(realpathCb.native ?? realpathCb)

The native variant is used specifically for Windows 8.3 short name expansion, so on non-Windows platforms falling back to the non-native version is semantically correct.

Real issue: TOCTOU window between assertWithinInstallDir and execution

After realpath resolves the symlink and the check passes, nothing prevents the symlink from being replaced before the entrypoint is actually executed. This is an inherent limitation of the approach, but worth a comment so future maintainers don't think the check is airtight.

Unnecessary dependency: cross-spawn for a single spawnSync call

cross-spawn is added as a runtime production dependency (dependencies, not devDependencies) solely to handle .cmd shims on Windows. Node 18+ spawnSync with shell: false does handle .cmd files on Windows when passed with the .cmd extension explicitly, or alternatively npm could be invoked as npm.cmd on Windows with a one-line platform check. Adding cross-spawn + 4 transitive packages (path-key, shebang-command, shebang-regex, which, isexe) as production runtime dependencies for this is over-engineering. A targeted fix:

const npmCmd = process.platform === 'win32' ? 'npm.cmd' : 'npm'

That said, if Windows CI was actually broken before this, the fix is pragmatic — just worth flagging.

Minor: shell: false removal

Removing the explicit shell: false is fine since cross-spawn handles it, but the security comment in the file header now says "never a shell-interpreted string" while the code no longer makes that guarantee explicit at the call site. The comment was updated, which is good.

Overall the core security fix (resolving symlinks before containment check) is correct and the tests cover the important cases.

[github-actions]

The diff looks correct and well-reasoned. A few real issues worth noting:

cross-spawn dependency is unnecessary

The stated reason for adding cross-spawn is to invoke npm's .cmd shim on Windows without shell: true. Node.js 16+ has shell: false + .cmd support via spawnSync natively when you pass the full path, and npm ships a Node.js wrapper script (npm.js) that can be invoked directly with process.execPath. More practically: cross-spawn is a 5-package sub-tree (cross-spawn, path-key, shebang-command, shebang-regex, isexe, which) added as a production dependency purely to handle Windows .cmd resolution. This is over-engineering per the ponytail rule — a helper that does process.platform === 'win32' ? cmd + '.cmd' : cmd or just locating the npm executable once would replace all five packages with ~3 lines.

realpath.native fallback behaviour

realpath.native is documented as not available on all platforms. On systems where it falls back to the JS implementation, the Windows 8.3 short-name problem it was added to fix reappears silently. The comment acknowledges the motivation but not this risk. At minimum a note or a runtime check would help.

TOCTOU acknowledged but not mitigated in the npm upgrade path

The comment in assertWithinInstallDir correctly flags the TOCTOU race. In the npm upgrade branch specifically, npm update runs, then assertWithinInstallDir is called on ext.entrypoint — but npm update could have replaced the file between the check and the actual execution call elsewhere. This is correctly flagged as known, but worth confirming no execution path runs the entrypoint between npm update returning and the assertion completing.

realpath error path conflates two callers

The catch block in assertWithinInstallDir uses entrypoint as the fallback path in the error message when err.path is absent, but the error could have been thrown by the realpath(installDir) call. The message might point to the wrong path in that case.

Test uses bare spawnSync from Node (no cross-spawn)

The test imports spawnSync from node:child_process while production code now uses cross-spawn's sync. On Windows CI this means the git bootstrap in the upgrade test might fail with the same .cmd problem the PR is solving. Consistent with using cross-spawn in production, the test should too, or this inconsistency should be documented.

Nothing else is wrong with the logic, the security fix itself is sound, and the tests cover the intended cases.