fix(upgrade): Resolve symlinks before self-copy guard in installBinary#1046
Conversation
When the install dir is reached through a symlink (e.g. macOS /tmp -> /private/tmp), the source path (canonicalized by process.execPath) and the .download temp path point at the same file but differ as strings. The resolve()-based guard then failed to detect this, unlinked the source, and copyFile() crashed with ENOENT. Compare realpath-canonicalized paths instead, falling back to resolve() when the path does not exist yet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Codecov Results 📊✅ Patch coverage is 90.00%. Project has 4292 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 81.98% 82.05% +0.07%
==========================================
Files 329 329 —
Lines 23883 23917 +34
Branches 15603 15634 +31
==========================================
+ Hits 19581 19625 +44
- Misses 4302 4292 -10
- Partials 1651 1649 -2Generated by Codecov Action |
MathurAditya724
left a comment
There was a problem hiding this comment.
Reviewed the diff — the fix is correct and well-scoped.
What I verified:
realpathSyncresolves through symlinks (unlikeresolve()which only makes paths absolute), so the/tmp→/private/tmpmacOS case is correctly handled.- The
try/catchfallback toresolve()is necessary becauserealpathSyncthrowsENOENTon non-existent paths, which is the normal case whentempPathhasn't been created yet. - The new test correctly reproduces the bug scenario: canonical
sourcePathvs symlinkedinstallDirpointing to the same real directory. - All 49 tests in
binary.test.tspass. CI is green.
One minor note (non-blocking): The catch block in the canonical helper at src/lib/binary.ts:458 is silent (no log.debug). The repo's AGENTS.md prohibits silent catch blocks in src/, though the existing file already has several (lines 70, 280, 285, 416, 467). Since this is a pre-existing pattern in this file and the fallback behavior is clearly documented in the comment block above, this isn't worth blocking on — but a log.debug("realpathSync failed, falling back to resolve()", error) would be consistent with the stated policy if you want to tighten it up.
No findings.
Per AGENTS.md, catch blocks in src/ must not silently swallow errors. Guard the expected "path does not exist yet" case with existsSync (no log, since it fires on every normal install) and log.debug() only the genuinely unexpected realpathSync failures in the catch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the careful review! Addressed the silent-catch note in 7327753. One nuance worth calling out: I applied it slightly differently than the literal suggestion. Dropping const canonical = (p: string): string => {
if (!existsSync(p)) {
return resolve(p);
}
try {
return realpathSync(p);
} catch (error) {
logger.debug("realpathSync failed, falling back to resolve()", error);
return resolve(p);
}
};Left the pre-existing silent catches (lines 70, 280, 285, 416, 467) out of scope to keep this PR focused — happy to do a separate cleanup pass on them if that's useful. |
Adds a mocked test exercising the catch branch where realpathSync throws on an existing path (permission error or TOCTOU race), asserting installBinary falls back to resolve() and logs. Kept in a sibling .mocked.test.ts so the node:fs mock does not leak into binary.test.ts. Brings patch coverage above target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Replace realpathSync with async realpath from node:fs/promises for consistency with the surrounding async function - Replace existsSync guard with try/catch ENOENT pattern to avoid TOCTOU race condition - Trim excessive comments on canonical helper and mocked test header - Fix silent catch block in temp file cleanup (add logger.debug) - Update mocked test to mock node:fs/promises realpath instead of node:fs realpathSync
Address Seer review: ENOENT is the expected case (no leftover temp file from a previous interrupted operation), so skip the debug log for it.
installBinarydeletes the source file it is about to copy when the install directory is reached through a symlink, crashing the upgrade withENOENT. The self-copy guard compared paths withresolve(), which makes paths absolute but does not follow symlinks. On macOS/tmpis a symlink to/private/tmp, so a download at/private/tmp/.../sentry.download(the canonicalizedprocess.execPath) and the temp target/tmp/.../sentry.downloadlooked different despite being the same file — the guard misfired,unlink()removed the source, andcopyFile()then failed.This surfaces during
sentry cli upgradewhenSENTRY_INSTALL_DIRpoints through a symlink; a normal~/.local/bininstall is unaffected, which is why it was intermittent.Symlink-aware comparison
The guard now compares
realpathSync-canonicalized paths, falling back toresolve()when the path does not exist yet (the normal case where the temp file has not been created).Adds a regression test that installs through a symlinked dir and fails (ENOENT) without the fix.