Improve installer recovery diagnostics and path validation#23
Improve installer recovery diagnostics and path validation#23Vladyslav-Kuksiuk wants to merge 1 commit into
Conversation
Oleg-Melnik
left a comment
There was a problem hiding this comment.
Reviewed the recovery-diagnostics + path-validation refactor. The extraction of InstallationDirectory, the preserved IOException cause, and the new functional test all look good; detekt and the new test pass locally.
Verdict: APPROVE WITH CHANGES. No hard blockers, but two items are worth resolving before merge — see the inline comments:
- Should fix (security): the installation root is no longer re-validated for symlink/junction redirection on each filesystem operation — only once in
prepare(). Issue #16 asked to consolidate this validation without weakening symlink protection. - Should fix (behavior): turning the restore
catchinto athrowalso changes the pre-download reuse attempt ininstall(), which no longer falls back to a fresh download on anIOException. Plus a wording nit on the new message.
| /** | ||
| * Rejects symbolic links in every existing component from the installation root. | ||
| */ | ||
| fun requireNoSymbolicLinks(path: Path) { |
There was a problem hiding this comment.
Should fix (security) — symlink/junction protection on the installation root is weakened.
Before this refactor, requireNoSymbolicLinks(...) and createDirectoriesSafely(...) each called prepareInstallationDirectory(root), which re-ran isRedirectingFileSystemEntry(root) on every filesystem operation. Now the root is validated only once, in prepare() (line 162). Neither method re-checks it afterwards:
- the component walk starts at
current = rootand only inspects root's children (root.relativize(path)is empty for the root itself), and requireRealPathInsidecallsroot.toRealPath()on both sides, so a root symlink is followed identically and cannot be detected.
Effect: if build/embed-code (the fixed root) is swapped for a symlink/junction after prepare() but before a later write — e.g. during the network-download window — subsequent moveSafely/writeStoredValue calls resolve through it and can write outside the root. The old code rejected this on the next operation; the new code does not. This is exactly the regression issue #16 warned against ("consolidate ... without weakening ... symlink protection"). Precondition is a concurrent local writer in the build tree, so the practical window is narrow, but it is a real reduction of a defense the prior code provided.
Suggested fix: re-validate the root at the start of requireNoSymbolicLinks / createDirectoriesSafely (isRedirectingFileSystemEntry(root) + !Files.isDirectory(root, NOFOLLOW_LINKS)), or include root itself in the walked components, so root redirection is rechecked per operation as before.
| return true | ||
| } catch (_: IOException) { | ||
| return false | ||
| } catch (exception: IOException) { |
There was a problem hiding this comment.
Should fix (behavior to confirm). Changing this catch from return false to throw also affects the pre-download reuse attempt in install() — the installFromVerifiedAsset(...) call before the network download (around line 207). Previously an IOException there returned false and the task fell through to a fresh download; now it aborts the whole install with no re-download fallback.
Issue #16 targets the generic post-download restoration path, so surfacing the cause there and in the offline path is clearly intended. But please confirm you also intend the online reuse attempt to stop self-healing via re-download. In practice a corrupt/mismatched cached asset still returns false (the SHA-256 check), so only genuine I/O errors change behavior here — still worth a deliberate decision.
Nit: this message (Could not restore the verified Embed Code asset from ...) is worded differently from the sibling failure at line 259 (The verified Embed Code asset could not be restored from ...) for essentially the same condition; consider aligning the two.
Oleg-Melnik
left a comment
There was a problem hiding this comment.
@Vladyslav-Kuksiuk LGTM with comments to address.
This PR improves installer recovery diagnostics and path validation.
Resolves this issue.