Symlinks with absolute paths in temp folder on macOS with NW.js 0.22.x about nw-autoupdater HOT 10 OPEN

I think we possibly can merge -L key for rsync, but problem is in third party decompress-zip lib to my mind, why it converts initial relative symlinks to absolute? Seems support for decompress-zip is outdated and we have no options except changing rsync.

Problem with nwjs 0.22.0 is known by me, it is kind of headache.

from nw-autoupdater.

Hi @arudnev, I am unable to solve the issue by monkey patch:
return symlink(linkTo /* path.resolve(parent, linkTo) */, destination)
It makes corrupted symlinks while decompress:

$ spctl -a -vvvv my.app
my.app: a sealed resource is missing or invalid

Also for what reason we want to use -L added to rsync, wheather decompress produces broken symlinks?

from nw-autoupdater.

I created new PR #7.
As well you should keep in mind fix for https://github.com/bower/decompress-zip/blob/master/lib/extractors.js#L109.
@dsheiko, you may close this issue.

from nw-autoupdater.

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.
If you run some cleanup procedure after upgrade where you remove temporary update directory, then your current install is going to be broken.

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 spctl -a -vvvv my.app, I believe with (monkey)patch it all works, as long as you have content of your update package (.zip) signed correctly as part of the build / packaging flow. For 0.22.x I had to switch to nwjs-builder-phoenix and extend it a bit to include extra steps, signing being one of them, that are executed before producing .zip and .dmg targets. With that the app remains correctly signed after upgrade and does not produce warnings.

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:

./updater.service.js (somewhere in your code, before using AutoUpdater)

...
const extractors = require('decompress-zip/lib/extractors');
extractors.symlink = require('./extractors.symlink.hack');
...

./extractors.symlink.hack.js (minimalistic rewrite of symlink operation and its dependencies)

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.

from nw-autoupdater.

@dsheiko are you sure in https://github.com/dsheiko/nw-autoupdater/blob/master/index.js#L51 ? Possibly should be
this.backupDir += this.options.accumulativeBackup ? _${Math.floor(Date.now() / 1000)} : ``;

from nw-autoupdater.

from nw-autoupdater.

Seems night is still ongoing) for you & me.
Look please

So it should be
this.options.backupDir += this.options.accumulativeBackup ? _${Math.floor(Date.now() / 1000)} : ``;

from nw-autoupdater.

I've changed this.accumulativeBackup to this.options.accumulativeBackup
I checked the instance state, yeah now it really expands the backupDir value when options accumulativeBackup set true.
But what weird- in you snapshot backupDir is also presented as first-level property of the update instance. I do not have it there, only in options. Actually by the code (master branch) I see no way it sneaks there...

from nw-autoupdater.

@arudnev I like your approach, looks clean. Can you please merge to the master branch and make a Pull Request?

from nw-autoupdater.

@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)

from nw-autoupdater.