Conversation
Pixnop
left a comment
There was a problem hiding this comment.
I ran both paths against real folder layouts on Linux, dev against this branch, each time in a throwaway appData, and put the new checks through a mutation pass.
The config.json part is a real fix. On dev, a VS Launcher config.json that is an absolute link gets copied as a link, and the first settings save follows it: writeJsonAtomic goes through write-file-atomic, which resolves the link, so RiftLauncher rewrites the file VS Launcher reads. I saved a config after migrating on both: on dev the file behind the link ended up holding RiftLauncher's JSON, on this branch it was untouched and the new profile had a plain copy. A relative link (config.json -> config.real.json) was worse on dev, since the copied link dangles in the new folder and the player starts with no config at all. Here the content comes across.
Two things need to change before it goes in.
Blocking
1. The default path refuses without saying why, and never tries again
Both paths now refuse the same layouts, but they still answer differently, which is what #583 was about. The portable path stops with "RiftLauncher could not start" and gives the reason ("Legacy profile is not a folder: …", "Legacy profile belongs to another user: …"). The default path starts on an empty folder, and the reason is swallowed by the bare catch in setUpUserDataFolder. The log line is the generic one, "Could not copy the VS Launcher user data folder. Starting on an empty RiftLauncher folder." #583 asked for the same reason in the log on both paths.
Default profile, first launch:
| VS Launcher folder | dev | this branch |
|---|---|---|
| a link to a folder the player owns | config.json and Icons copied |
empty profile |
| a link to a root-owned folder | Icons copied |
empty profile |
| a real folder owned by another uid | config.json and Icons copied |
empty profile |
Icons is a link, or holds a link to a file or a folder |
both copied, links kept as links | empty profile |
Nothing is lost: the VS Launcher folder and every link target came out unchanged, and no RiftLauncher.migrating was left behind. The trouble is afterwards. The empty RiftLauncher folder makes planUserDataMigration return use-existing on the next launch, so the copy is never tried again, whatever the comment in that catch says ("still there to try again from on the next run"). The player gets a launcher with none of their installations, nothing on screen, and a log line that doesn't help. The installation pages under docs/get-started/installation/ (the README, "Where RiftLauncher keeps its data" in linux.md, "Portable data folder" in windows.md) still promise the copy with no conditions.
Windows players get this as well. lstat reports a junction as a link, and the branch's own junction test ("refuses a symlinked VS Launcher folder and starts on an empty folder") runs and passes on the Windows CI leg. So a VS Launcher folder moved to another drive with mklink /J now leaves RiftLauncher empty, or keeps it from starting in portable mode.
What I'd like: the error message carried in UserDataSetup and printed by describeUserDataSetup, the comment fixed, and a sentence in the docs saying which VS Launcher folders are not copied and how to get the copy once that is fixed (move the new RiftLauncher folder aside and start again).
Before doing that, have another look at #583's other option for this one folder. It is only ever read, it sits in the player's own appData where only they (or an administrator) can write, and copyLegacyUserDataEntries already keeps links out of the new profile. Accepting the folder itself, linked or not and whoever owns it, while still refusing links inside it would remove both the empty start and the portable boot failure for the first three rows. I'm fine either way, as long as the default path says why it started empty.
2. The owner check on the portable source profile reads the link, not the folder it copies
setUpPortableUserDataFolder takes the owner from fse.lstatSync(currentProfilePath) and then copies from realpathSync.native(currentProfilePath). A linked RiftLauncher folder is supported on purpose (the "copies a linked source profile without writing through its symlink" test), and when it is one, the check reads the owner of the link, which is the player who made it, while the copy reads the target. Linked to a root-owned folder, the target was copied into the portable profile, as on dev. Linked to a folder owned by another uid holding a config.json and an account-secrets.json, both files were copied, as on dev.
The check also opens a new split between the two paths. A real RiftLauncher folder owned by another uid now stops the portable path with "Profile folder belongs to another user", while the default path keeps using that same folder (use-existing, here and on dev).
Taking the owner from fse.statSync instead (or from the realpathSync.native result) checks the folder that is actually copied, and the suite stays green with that change. For the default path, either give an existing RiftLauncher folder the same check or leave this one out of the PR, but the two paths should agree.
Not blocking
- A
config.jsonlink target is read without checking what it is. A link to a FIFO blocks the migration for good (I killed it after 20 s, and the next launch blocked again). A link to/dev/zerois read until the process hits a 1 GiB memory cap. A 600 MiB file is read whole (680 MB peak) and written into the new profile, and a 3 GiB file only stops at Node's 2 GiB limit. None of this is new for the launcher as a whole, because on dev the link was copied as a link and the firstreadJSONSyncinindex.tsblocked or blew up the same way. But "safely as regular files" is not what the code checks. Anfse.statSync(source).isFile()before the read turns the FIFO and/dev/zerocases into a plain refusal, and a size cap would cover the big files. - A dangling
config.jsonlink now fails the whole default migration,Iconsincluded. Dev skipped the dangling link and copiedIcons. - The two helpers were inserted between
setUpUserDataFolder's doc comment and the function, so that comment,@param appDataPathand all, now documentsassertTrustedLegacyProfile, andsetUpUserDataFolderhas none. - "Legacy profile is not a folder" is what a player reads for a link or junction pointing at a folder. "is a link or junction" would say what was refused.
- The test "refuses a symlinked Icons folder in the default VS Launcher migration" plants a link inside
Icons.Iconsbeing a link itself is refused too (I checked), but no test covers it.
Validation
- 35 layouts, each run on dev and on this branch, through
selectUserDataFolderwith a throwaway appData, then the first config readindex.tsdoes, then a second launch. Folders owned by another account were real ones (a different uid), not a mockedgetuid. Apart from the two layouts where I swapped the folder on purpose, the VS Launcher folder and its link targets were unchanged in every run. - The check and the copy are separate path lookups. Swapping the VS Launcher folder for a link between them goes unnoticed and the copy reads the link target. Only someone who can already write the player's appData can do that, so I'm not asking for anything there.
- Hard links come across as independent files (new inode, source left with its two links) on both branches. Hard-linking another account's file into the folder is refused by the kernel (EPERM, with
protected_hardlinkson). - Mutation: of 11 hand mutants, the 10 that remove or weaken one of the new checks all fail the two touched test files on Linux. Three of them (dropping the legacy owner check, the source profile owner check, or the
config.jsoncopy as a file) are caught only by tests that skip on Windows, so only the Ubuntu leg guards them. Switching the source profile check fromlstattostat, the fix in point 2, survives: nothing pins either behaviour. - The 8 new tests fail against dev's source and pass here.
- The 14 Windows skips in the two files are the 11
skipIf(win32)tests and the Linux-only unreadable-folder test inuserDataMigration.test.ts, 4 of them new (the defaultconfig.jsonlink test and the three owner tests), plus the two existing marker tests inprofileChoice.test.ts. On Windows the owner checks do nothing (process.getuidis undefined), so what Windows players get from this PR is: a junction or directory link at%APPDATA%\VSLauncheror insideIconsnow stops the copy, a linkedconfig.json(which needs Developer Mode or admin rights to create) is copied as a file with no Windows test behind it, and ownership is unchanged. - On this head:
npm ci,npm run typecheck,npm run lint:ci(0 errors, the same 12 warnings as dev),npm run format:check,npm run test:coverage(280 files, 5048 passed, 4 skipped; 94.32% statements, 90.4% branches, 95.36% functions, 96.18% lines). The first full run had one failure intests/ipc/compression.test.ts, unrelated to this PR: it passed 3 times out of 3 on its own on dev and here, and in the full rerun. - CI is green on every leg that ran. dev has not moved since the base commit, so it merges without conflicts. No locale strings are touched.
Requesting changes for points 1 and 2.
Verify that legacy user data folders and current portable profile folders are regular directories owned by the current user before migrating them. Refuse symlinks in Icons, copy config.json symlinks safely as regular files, and start on an empty folder when migration fails. Fixes #583
5920556 to
d298246
Compare
|
Addressed the review feedback in commit \d298246f:
|
|
The remaining migration feedback is in signed commit The branch already carries the refusal reason in the startup log, documents the retry after moving the empty profile aside, and checks a linked portable source by its resolved target. I kept the documented refusal of linked legacy folders. The packaged headless launcher logged the specific link/junction reason, started on an empty profile, and left the legacy source unchanged. On Linux with Node 26.10.0, typecheck, lint, formatting, coverage and the packaged build passed. The full suite has 5,120 passed and 5 skipped; coverage is 94.43% statements, 90.58% branches, 95.41% functions and 96.37% lines. Lint has 0 errors and 12 existing warnings in untouched files. Targeted migration/profile tests passed 59 cases with 2 platform skips. |
Summary
Carry migration refusal details into the startup log and document how to retry after fixing an unsupported legacy folder. Check the resolved source folder's owner in portable mode and apply the same ownership rule to an existing default profile. Legacy folders and Icons links remain refused; source data stays untouched.
Config copies now use a bounded, nonblocking read of a regular file, up to 16 MiB. A dangling config link is skipped so Icons can still migrate. Added regressions for FIFO and device targets, oversized configs, direct Icons directory links, and default-profile ownership.
Type
Checklist
dev, notmain.npm run typecheckpasses.npm run lint:cipasses.npm run format:checkpasses.npm run test:coveragepasses, coverage at or above the floor invitest.config.ts.npm run build:unpackpasses.Testing
Linux, Node 26.10.0, head
711438dc: all five repository gates passed. Lint reports 0 errors and 12 existing warnings in untouched files. Coverage suite: 281 files, 5,120 passed, 5 skipped; 94.43% statements, 90.58% branches, 95.41% functions, 96.37% lines. Migration/profile targeted tests: 59 passed, 2 skipped.The packaged headless launcher started with a linked VS Launcher folder, logged the specific link/junction refusal, used an empty RiftLauncher profile, and left the source config unchanged. Windows was not run locally; final-head CI covers the Windows matrix.
Related issues
Fixes #583