FS sandboxing

[andremedeiros]

Overview

TL;DR
This PR adds FS sandboxing to only allow our FS calls to run inside the DApp root or the OS' temporary directory.

@corpetty to review too.

Cool Spaceship Picture

[andremedeiros]

stat has some weirdness with the graceful-fs package for some reason, so I'm skipping it here.

[michaelsbradleyjr]

I researched/experimented and found that to test stat, a callback must be used. Here's how I rewrote the relevant test:

it('should not throw exceptions on paths inside the temporary dir root', async () => {
  try {
    if (func === 'stat') {
      await new Promise((resolve, reject) => {
        fs[func](os.tmpdir() + '/foo', (err) => {
          if (err) {
            reject(err);
          } else {
            resolve();
          }
        });
      });
    } else {
      await fs[func](os.tmpdir() + '/foo');
    }
  } catch (e) {
    assert.doesNotThrow(() => { if (e) throw e; }, /EPERM/);
  }
});

You can verify it's working by changing /EPERM/ to /ENOENT/ — the tests for fs.stat and fs.statSync will fail. Note that the function passed to it() has become an async function. Trying to assert.doesNotThrow([func]) where [func] is an async function can't work, since async functions always return promises.

The callback problem has to do with our old version of fs-extra. With recent versions, fs.stat is fine to call without a callback, since all fs-extra functions return promises if no callback is supplied.

[andremedeiros]

Would it make more sense to update fs-extra instead?

[michaelsbradleyjr]

Probably a good thing to do, but it's seen several major releases since 2.1.2 (the version we're using currently); fs-extra 7.0.0 may require some additional changes to embark, but maybe not. In any case, seems like a good opportunity and justification for a major upgrade of that dependency.

[jrainville]

Can't we assume the function will always be binded to itself? So that we can get rid of a param in each call.

[andremedeiros]

We can't. Some aren't bound to self, like writeFile is bound to writeFileSync.

[jrainville]

Why?

[andremedeiros]

I'm not sure. There are a couple of cases like that, and I'm assuming there's a reason why.

[jrainville]

For the count, can't we just do args.length?
And if it's not a path arg, just continue.

[andremedeiros]

We don't know how many args are paths or not.

[jrainville]

I think we could run path.resolve on the arg, and if it throws an error, we continue.

[andremedeiros]

I think I see what you're saying. I'm not sure this would work, because of the many different cases that we're trying to cover:

• Some functions have path as the single argument we're validating, but some others have src, dest.
• If we went through the args and, say, checked whether the path existed or not, we wouldn't be able to create new files or new directories
• path.resolve is actually incredibly forgiving, as it doesn't validate to check what a valid path is, so for instance, things like these work:

'/Users/andremedeiros/src/github.com/embark-framework/embark/{"foo": "bar"}}'
> path.resolve('{"foo": "bar"}, \n{"omg": "noway"}')
'/Users/andremedeiros/src/github.com/embark-framework/embark/{"foo": "bar"}, \n{"omg": "noway"}'
> path.resolve('{"foo": "bar"}, \n{"omg": "noway"}', './')
'/Users/andremedeiros/src/github.com/embark-framework/embark/{"foo": "bar"}, \n{"omg": "noway"}'
> path.resolve('{"foo": "bar"}, \n{"omg": "noway"}', '../')
'/Users/andremedeiros/src/github.com/embark-framework/embark'

[michaelsbradleyjr]

Rather than passing a count, in case there's a situation where all the path args are not consecutive, what if you pass an array of positions. Assuming count is renamed to pos, then you could do:

for (const i of pos) {
...
}

[jrainville]

Yeah, that would be more flexible, but in the case of optional parameters, it would still not work. Don't think we have any functions that have those now, but we need to keep it in mind just in case.

[michaelsbradleyjr]

Definitely. In such a case, pos would need to be computed against arguments with a helper function before passing it to restrictPath. The helper would be function-specific and need to apply the same rules as receiver does to figure out optional parameters.

[michaelsbradleyjr]

Would the binding for fs.mkdirpSync be fs? Same question re: the others.

Would it make more sense to pass a lamba receiver like

(...args) => fs.mkdirpSync(...args)

and then call it with receiver(...args)?

[andremedeiros]

Sometimes the binding is different then the caller :-(

[michaelsbradleyjr]

Then passing a lamba may be the simplest thing.

[andremedeiros]

By lambda do you mean the actual call?

[michaelsbradleyjr]

Yes, an arrow function: e.g. (...args) => fs.mkdirpSync(...args).

If that arrow function is passed as the receiver argument, then you can call it with receiver(...args) inside restrictPath without needing .apply and a binding.

[alaibe]

Have you tried to use: https://github.com/yahoo/fs-lock

[andremedeiros]

I have not. That might have been a good idea. 👀

[michaelsbradleyjr]

Re: fs-lock

YahooArchive/fs-lock#3 (comment)

[iurimatias]

@andremedeiros please rebase