-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Symlinks with absolute paths in temp folder on macOS with NW.js 0.22.x #6
Comments
I think we possibly can merge Problem with nwjs 0.22.0 is known by me, it is kind of headache. |
Hi @arudnev, I am unable to solve the issue by monkey patch:
Also for what reason we want to use |
I created new PR #7. |
Without (monkey)patching of https://github.com/bower/decompress-zip/blob/master/lib/extractors.js#L109 use of -l in rsync did not fix it for me, that's what actually lead me to creation of this ticket. When -l is used then links in destination folder (i.e. somewhere under app home) are pointed to source folder (i.e. somewhere in temp folder). I don't remember now if it the app even starts after that if it was properly signed before and update was downloaded from the internet and we end up with this mess of symlinks. Also, once you do backup after upgrade, the symlinks in the backup will point to locations in the temp folder. Then when you cleanup that temp folder as part of the next upgrade (i.e. when migrating from 0.22.0 to 0.22.1) those links in the backup will become broken. So, all those timestamped backups are going to be broken after first upgrade to the next version of nwjs. When -L is used then it does not matter that decompress-zip created links with absolute paths instead of relative paths as there are no symlinks in destination folder, the contents of the symlink target are copied recursively into destination folders, creating duplicates that you need to cleanup and potentially breaking code signature (I have not tested that, but most likely that would be the case, unless you get rid of symlinks as part of packaging, before signing, but then we don't have this problem with symlinks to begin with). In regards to As for (monkey)patching of that symlink operation in decompress-zip/extractors - I tried fixing it manually in node_modules after npm install and it seemed to be working when applied in build phase, but I ended up with a hack that I apply in runtime instead. I did not want to include it here as this is ugly hack and should really be correctly fixed in decompress-zip (I don't expect that guys added extra code to switch from relative path to absolute path without some reason behind it, so relative path might not always be right answer, but it would be nice to add some switch for it if that's the case). Anyway, until proper fix is in place, here is what worked for me:
...
const extractors = require('decompress-zip/lib/extractors');
extractors.symlink = require('./extractors.symlink.hack');
...
const fs = require('graceful-fs');
const Q = require('q');
const mkpath = Q.denodeify(require('mkpath'));
const symlink = Q.denodeify(fs.symlink);
const path = require('path');
function mkdir(dir, cache, mode) {
dir = path.normalize(path.resolve(process.cwd(), dir) + path.sep);
if (mode === undefined) {
mode = parseInt('777', 8) & (~process.umask());
}
if (!cache[dir]) {
var parent;
if (fs.existsSync(dir)) {
parent = new Q();
} else {
parent = mkdir(path.dirname(dir), cache, mode);
}
cache[dir] = parent.then(function () {
return mkpath(dir, mode);
});
}
return cache[dir];
}
function getLinkLocation(file, destination, zip, basePath) {
var parent = path.dirname(destination);
return zip.getBuffer(file._offset, file._offset + file.uncompressedSize)
.then(function (buffer) {
var linkTo = buffer.toString();
var fullLink = path.resolve(parent, linkTo);
if (path.relative(basePath, fullLink).slice(0, 2) === '..') {
throw new Error('Symlink links outside archive');
}
return linkTo;
});
}
function symlinkHack(file, destination, zip, basePath) {
var parent = path.dirname(destination);
return mkdir(parent, zip.dirCache)
.then(function () {
return getLinkLocation(file, destination, zip, basePath);
})
.then(function (linkTo) {
return symlink(linkTo /* path.resolve(parent, linkTo) */, destination)
.then(function () {
return {symlink: file.path, linkTo: linkTo};
});
});
}
module.exports = symlinkHack; By the way, with this monkey-patch I did not need to add -l option to rsync, I think relative links are correctly rsync'ed. |
@dsheiko are you sure in https://github.com/dsheiko/nw-autoupdater/blob/master/index.js#L51 ? Possibly should be |
Oh hell, you are right.I shall stop working at nights. backupDir belongs to
the options. Pushed the hot fix
|
I've changed this.accumulativeBackup to this.options.accumulativeBackup |
@arudnev I like your approach, looks clean. Can you please merge to the master branch and make a Pull Request? |
@cryomi Well, with last changes on Linux it's not good. Switching to release-1.1.1 (I shall have done on the first place) |
This is related to nwjs/nw.js#5873.
Starting with 0.22.0 nwjs includes symlinks in distro for macOS.
Those symlinks with relative paths are included into update packages after fixes done for evshiron/nwjs-builder-phoenix#30.
When extracted into temp location those symlinks have absolute path due to https://github.com/bower/decompress-zip/blob/master/lib/extractors.js#L109.
Once extracted update package is rsync'ed into original (app home) location as part of swap.sh it has symlinks pointing to absolute locations of the folders from temp directory, which leads to easy to guess consequences of clearing that temp folder.
We are currently hacking this by replacing decompress-zip/lib/extractors.symlink operation in runtime with a monkey-patched version that has the following code:
It produces symlinks with relative paths in the temporary update location and they are rsync'ed correctly.
Alternatively, swap.sh could be changed to include -L option for rsync to recursively copy content behind those symlinks, but then one probably would have to deal with duplicate copies and potentially some other issues.
It would be great to make sure that symlinks with relative paths are supported in unpacking as it seems that this is how it's going to be packaged for macOS moving forward.
The text was updated successfully, but these errors were encountered: