Skip to content

fix(mods): read a Mods folder the user symlinked into an installation - #275

Open
Pixnop wants to merge 2 commits into
devfrom
fix/symlinked-mods-folder
Open

fix(mods): read a Mods folder the user symlinked into an installation#275
Pixnop wants to merge 2 commits into
devfrom
fix/symlinked-mods-folder

Conversation

@Pixnop

@Pixnop Pixnop commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Issue #237 reports that a profile whose Mods folder is a symbolic link shows an empty mod list, and that the open-folder button claims the folder does not exist. I reproduced it against a real temp tree before touching anything: an installation with Mods replaced by a link at a Mods folder living elsewhere. The file system side is fine, readdir follows the link and lists the archives inside, and the archives themselves are ordinary files rather than links. The only thing standing in the way is assertNoSymlinkComponents in the managed path policy, which walks every existing ancestor of a path and throws on the first link it finds. GET_INSTALLED_MODS runs that assertion outside its own try block, so the whole invoke rejects and the renderer's catch turns it into an empty list. ENSURE_PATH_EXISTS swallows the same throw and returns false, which is exactly the "folder doesn't exist" notification in the screenshot.

That walk earns its keep and I did not want to remove it. The grant check that decides whether a path belongs to the launcher is lexical, comparing resolved strings, and the symlink walk is what stops the lexical answer from drifting away from the real one: without it a link planted inside an installation points wherever it likes and still looks contained. So the change gives the policy a second grade, allowSymlinks, and puts only the read-only channels on it. Listing the mods in a folder, asking whether a path is there, and handing a folder to the file explorer are all reads, and a folder the user deliberately linked in is their own arrangement, not an attack. Everything that writes keeps the walk exactly as it was: deleting a mod, moving a path, downloading a replacement, extracting an archive, copying an icon or a background. Creating a folder is a write too, so ENSURE_PATH_EXISTS now answers on the read grade when the folder is already there and drops back to the strict grade when it has to make one, since a mkdir through a link would land outside the granted subtree.

Two things bound what the read grade can reach. The scan only ever opens .zip files, and the directory reader still refuses any entry that is itself a link, so nothing planted inside a Mods folder widens the set of archives that get opened. Dangling links were already skipped by that same check, and they now say so in the debug log rather than disappearing quietly, which is the kind of thing worth being able to find in a log when a mod goes missing. The trade the read grade does make is stated out loud in the tests: a link inside a granted subtree does let a read follow it out. Nothing that writes follows, and there are rows pinning that, including a link pointing at a system directory where the read is admitted and the delete is refused.

There is a deliberate limitation here. Installing or updating a mod into a linked Mods folder is still refused, because the download output path goes through the strict grade and the worker refuses to replace a link. Widening that would mean granting write access to a folder outside the installation, which is a bigger decision than this fix and deserves its own review. Listing, opening and inspecting all work now; a test pins the refusal so it is a recorded choice rather than an oversight. Happy to open a follow-up if you want writes to follow, resolving the real path once and containing the write targets inside it.

On the tests: a mod behind a linked folder is read with the same metadata as one in a plain folder, a linked Mods folder answers all three read channels, a dangling link is skipped without failing the scan, and the write boundary is pinned from both sides. I checked each guard by breaking it and confirming a row goes red: ignoring the new option fails five rows, dropping the strict grade from the create path fails one, removing the entry-link check fails two, and putting the mod scan back on the write grade fails one. The symlink rows are skipped on Windows, which is where the reporter hit this, because creating a link there needs Developer Mode or elevation the runners do not have. Issue #267 tracks that gap. None of the code involved branches on platform, so the Linux and macOS runs are what hold it.

Gates are green: typecheck, lint (no errors, the fifteen warnings are pre-existing), format check, and the full suite at 1649 passing with coverage above every floor.

The managed path policy walked every ancestor of a path and refused it
outright when any of them was a symbolic link. That walk is what keeps
the grant check honest for anything that writes, but it also ran on the
channels that only read, so a profile whose Mods folder is a link at a
Mods folder kept elsewhere came back as an empty mod list and a folder
button that reported the folder missing.

Give the policy a read grade that skips the walk and put the four
read-only channels on it: the installed-mods scan, the two existence
checks, and showing a folder in the file explorer. Creating a folder
stays on the strict grade inside ENSURE_PATH_EXISTS, and deleting,
moving, downloading and extracting are untouched, so a link still cannot
be used to reach past a grant with anything that writes.

The scan keeps dropping entries inside the folder that are themselves
links, dangling ones included, and now says so in the debug log.
@Pixnop
Pixnop requested a review from Zaldaryon August 28, 2026 22:13

@Zaldaryon Zaldaryon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes, but not on the security model. The allowSymlinks grade is sound: the grant check is untouched and still lexical over config-declared roots, no IPC channel creates a symlink (checked every write, delete, move, download, extract, compress path, they all keep the walk), and the sandboxed renderer has no way to plant one. Abusing the read grade needs an actor who already has fs read/write as the user, who gains nothing from a launcher-mediated oracle. The trade is stated honestly and the tests pin it from three sides. Local gates pass here: typecheck, lint:ci at 0 errors and 15 pre-existing warnings, format:check, test:coverage at 92.66 statements / 89.85 branches / 92.12 functions / 94.12 lines, all over the floors; the four touched suites are 145 passing.

Two correctness points that belong in this PR, not a follow-up.

1. ENSURE_PATH_EXISTS now returns true for a path that exists but is not a directory

src/ipc/handlers/pathsHandlers.ts around line 350:

const safePath = await assertManagedPath(pathValue, "path", { allowMissing: true, allowSymlinks: true })
if (await fse.pathExists(safePath)) return true

fse.pathExists is fs.access based, so it is true for a regular file, a FIFO, anything. Before this PR the path was assertManagedPath then fse.ensureDir(safePath), and ensureDir on an existing file threw EEXIST / ENOTDIR, so the handler returned false. CHECK_PATH_EMPTY right above it already does the right thing ((await fse.stat(safePath)).isDirectory() && ...); this new early return dropped that check.

AddInstallation.tsx line 157 is a free-text folder field. Type an existing file under a granted subtree (.../RiftLauncherInstallations/notes.txt). Before, the add was blocked at AddInstallation.tsx:97 with installationFolderCreateFailed. Now ensurePathExists returns true, the guard passes, and an installations[] entry whose path is a regular file lands in the config, which is exactly the "dangling entry with no data behind it" the comment at AddInstallation.tsx:93-96 says the guard exists to stop. That entry then feeds the game launch --dataPath, backup compression (hard failure, compression.ts refuses a non-directory source), and the mod scan.

stat rather than lstat keeps the #237 fix, since a link to a directory stats as one:

const existing = await fse.stat(safePath).catch(() => null)
if (existing?.isDirectory()) return true

A file then falls through to the strict arm, ensureDir throws EEXIST, and the handler returns false as before. No test covers this today.

2. CHECK_PATH_EMPTY is left strict, so the PR's own rule is half applied on a sibling read

src/ipc/handlers/pathsHandlers.ts:330. CHECK_PATH_EMPTY is a pure read (pathExists, stat, readdir) and its result feeds only a warning toast at all three call sites: usePathActions.ts:18, useVersionInstallFolder.ts:44, useConfigFolderPicker.ts:28, each if (!(await checkPathEmpty(p))) addNotification("folderNotEmpty", "warning"). By the rule this PR introduces it should have moved to the read grade with the others.

Leaving it strict is not neutral, because the failure is a throw, not a false, and none of the three call sites has a try/catch. A user who picks a symlinked folder in the settings pickers hits assertNoSymlinkComponents, the invoke rejects, pickFolder rejects before configDispatch, and the folder setting silently does not change. No error, no warning. That is the same bug as #237, one line away from the fix this PR already makes. Add allowSymlinks: true here too.

Worth stating, not blocking

  • #237 is half fixed. With <inst>/Mods a link, the mod list populates and the folder button opens, but every write on the Mods screen now fails: delete goes through assertManagedDeletionPath (strict, rejects), update and install go through DOWNLOAD_ON_PATH (strict, rejects). Your pathsHandlers.test.ts row pins the download refusal, so it is deliberate, but the user sees "my mods list works and every button errors," which reads as a launcher bug. The Mods screen should detect a linked folder and say what is going on, or the PR and issue should spell out that listing works and writing does not.
  • CHECK_PATH_EXISTS widening reaches past the Mods folder. It is the pre-flight for backup at MainMenu.tsx:154 / ListInstallations.tsx:178 and for launch at launch.ts:20. With a symlinked installation folder it now returns true and the flow proceeds to fail deeper in a strict channel, a rejected invoke rather than the clean folderDoesntExists toast. No containment issue, but the failure mode moved and it is broader than the Mods story. A test row for the installation-path-as-link case would be good.
  • Zero coverage on Windows, which is where #237 was reported. The new suites are all skipIf(process.platform === "win32") (modScan.test.ts:155, pathPolicy.test.ts:265, pathsHandlers.test.ts:333). #267 tracks the CI gap. The fix very likely works there, because Node's lstat().isSymbolicLink() is true for a directory junction (mklink /J, no Developer Mode), which is the reparse point a Windows user most likely has, but nothing verifies it and modScan.ts:322 also silently drops junction entries inside Mods on Windows.
  • assertNoSymlinkComponents has a pre-existing dangling-link blind spot (pathPolicy.ts:158, the ascend loop is driven by existsSync, which follows links, so a dangling intermediate is treated as "missing" and never lstated). Not introduced here, and I could not drive an escape through it (fs.mkdir recursive returns EEXIST on a dangling component), but the comment at pathsHandlers.ts:346-348 claims more than the walk guarantees. Worth a separate issue: drive that loop off lstatSync succeeding.

Not checked here

Windows behaviour (junction case, F5), ENSURE_PATH_EXISTS against a real file at runtime, and the settings-picker abort at runtime.

…er answer the empty check

ENSURE_PATH_EXISTS answered true for anything the read grade could see,
because fse.pathExists is fs.access based and a regular file passes it.
Before the read grade existed, an existing file reached fse.ensureDir and
threw EEXIST, so the handler returned false and AddInstallation's guard
stopped the add. Without that, typing an existing file path into the
folder field puts an installation entry on a regular file, which is the
dangling entry the guard is there to prevent, and that path then feeds
the launch --dataPath, the backup compression and the mod scan. Stat the
path and require a directory: a link at a directory still stats as one,
so the #237 case keeps working, and a file falls through to the strict
arm and comes back false as it used to.

CHECK_PATH_EMPTY was left on the strict grade while its three siblings
moved, and it is a read like they are. Its result only raises a warning
toast, but the failure is a throw and none of the three call sites
catches, so picking a linked folder in the settings pickers or the
version install folder took the whole pick down: no warning, no error,
and the setting quietly kept its old value. Put it on the read grade too.

Also drop the claim in the ENSURE_PATH_EXISTS comment that the strict
arm keeps a mkdir inside the grant. The walk refuses a link among the
existing ancestors, which is what it actually promises.
@Pixnop

Pixnop commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

Both correctness points are fixed on the branch, and I took the non-blocking notes as far as they go inside this PR.

You are right about what pathExists answers. ENSURE_PATH_EXISTS now stats the path and only calls it present when it comes back a directory:

const safePath = await assertManagedPath(pathValue, "path", { allowMissing: true, allowSymlinks: true })
const existing = await fse.stat(safePath).catch(() => null)
if (existing?.isDirectory()) return true

A regular file falls through to the strict arm, ensureDir throws EEXIST, and the handler answers false the way it did before this branch, so the guard at AddInstallation.tsx:97 blocks the add again and no installations[] entry lands on a file. stat rather than lstat is what keeps #237, since the link at the Mods folder stats as a directory. Two rows hold it: an existing file under a granted root has to give false, and the linked folder row already in the suite has to stay true. Putting pathExists back turns the file row red and leaves everything else green. Swapping stat for lstat turns the #237 row red instead, which is the other half of the same fix.

CHECK_PATH_EMPTY is on the read grade now. Your reading of the failure is what makes it worth doing rather than tidy: the throw is not a false, it takes the whole pick down, so the folder setting quietly keeps its old value and the user gets neither the warning nor an error. The new row drives a linked folder through the channel both ways round, non-empty giving false and empty giving true. Dropping the option again turns that row red.

The CHECK_PATH_EXISTS reach past the Mods folder has the row you asked for: an installation whose own folder is a link, which is the shape the backup pre-flight and launch.ts see. It reports true. Removing allowSymlinks from that handler turns it red along with the linked Mods row.

The comment on the create arm claimed the strict path keeps a mkdir inside the grant, which is more than the walk gives you once you account for the dangling blind spot you found. It now says what the walk actually does, which is refuse a link it finds among the existing ancestors. The loop itself I left alone, since it predates this branch and driving it off lstatSync deserves its own change. Happy to open that issue.

On #237 being half fixed, I agree that "the list works and every button errors" reads like a launcher bug to whoever hits it. The description records the boundary and the download refusal has a row pinning it, but nothing tells the user. A notice on the Mods screen when the folder is a link is the right answer, and it is a renderer change with its own strings, so I would rather do it separately than grow this one. Say the word and I pick it up next, or it goes into the issue if you would rather it wait.

Windows is still uncovered and #267 is the tracker. The junction point is a good one and I have nothing here to check it against.

Gates on the branch: typecheck clean, lint:ci at 0 errors and the same 15 pre-existing warnings, format:check clean, full suite 1652 passing and 2 skipped, coverage at 92.68 statements, 89.89 branches, 92.13 functions, 94.12 lines.

@Pixnop
Pixnop requested a review from Zaldaryon August 29, 2026 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants