Support whitelisting dirs

ericfranz commented 5 years ago

There are definitely cleaner ways to do this, or more robust ways.

Two UX fixes:

[ ] 1. Try to download a zip of the selected directory that is not in the whitelist, and a 403 error occurs but a "Forbidden" error alert doesn't display like in the other Ajax requests that get a 403.
[ ] 2. If doing a non AJAX request (like enter the forbidden URL into the browser) instead of the page showing "forbidden" it should probably present a list of links the user can access (the whitelisted directories) or go to home directory (or root directory that is set for the tree).

A more robust solution would be to add checks at the source - the actual code that hits the filesystem could raise the same exceptions that occur when a file or directory is actually not readable by a user. But that would take more time than this.

ericfranz commented 5 years ago

Slightly cleaner might be a whitelist object:

whitelist = {
    paths:  process.env.WHITELIST ? process.env.WHITELIST.split(":") : [],
    enabled: ( ) => { return this.paths.length > 0; },
    contains: (path) => {
        // whitelist path contains function here
    }
}

// then 
if(whitelist.enabled){
    app.use( //...

      if(whitelist.contains(filepath))
          next();
      else
          // error

ericfranz commented 5 years ago

This should also change. Instead of blacklisting the request urls that contain file system paths outside the list of whitelisted paths, I should explicitly specify all the request urls that are whitelist, and black list everything else. That way if we miss a URL it is just denied.

MorganRodgers commented 5 years ago

Branch whitelist_nodejs6_morgans fixes a few things including the not-a-js-comment, the middleware never getting defined (and thus not used), and adds both realpath and tilda expansion.

ericfranz commented 5 years ago

In regards to "the middleware never getting defined (and thus not used)" that is by design. If no whitelist is defined, there is no need to define the middleware whose only purpose is to enforce the whitelist. I know this works, at least from testing on my laptop. When I run the app like this:

For example:

diff --git a/app.js b/app.js
index 0aaaf67..4ae155c 100644
--- a/app.js
+++ b/app.js
@@ -97,6 +97,7 @@ socket = io.listen(server, {
 // url: /fs/Applications/
 // path: /Applications/
 if(whitelist.enabled()) {
+    console.log("define whitelist middleware");
     app.use(function(req, res, next) {
         var request_url = url.parse(req.url).pathname,
             rx = /(?:\/oodzip|\/api\/v1\/fs|\/fs)(.*)(?:)/,

then you can see:

➜  files git:(whitelist_nodejs6_morgans_erics) ✗ node app.js
^C
➜  files git:(whitelist_nodejs6_morgans_erics) ✗ WHITELIST=/ node app.js
define whitelist middleware
^C

Though I need to fix it to be WHITELIST_PATH.

The tilde expansion is a step forward, but it just blindly substitutes ~ for home:

> var expandTilde = require('expand-tilde');
undefined
>
> expandTilde
[Function: expandTilde]
> expandTilde('~')
'/Users/efranz'
> expandTilde('~fakeuser')
'/Users/efranz/fakeuser'
> expandTilde('~efranz')
'/Users/efranz/efranz'

Not sure if anyone is opening paths in the files app that have the form ~username though. We could just open a bug or just let people know in whitelist mode opening paths like these might not work as expected (though it probably would - if we whitelist /Users then ~fakeuser would pass the whitelist even if we resolve the path incorrectly.

The realpathSync call raises an exception if called on a path that does not exist, so that needs to be rescued and the original path used. Otherwise, things like renaming a file would fail since we call whitelist.contains() on both the current file path and the file path that we will be renaming/moving the file to, which by definition doesn't exist yet.

Error: ENOENT: no such file or directory, lstat '/Users/efranz/tmp/bar2.txt'
    at Object.realpathSync (fs.js:1684:15)
    at resolvePath (/Users/efranz/dev/files/app.js:37:15)
    at /Users/efranz/dev/files/app.js:52:17
    at Array.filter (<anonymous>)
    at Object.contains (/Users/efranz/dev/files/app.js:48:27)
    at onPUT (/Users/efranz/dev/files/lib/cloudcmd/lib/server/rest.js:217:111)
    at /Users/efranz/dev/files/lib/cloudcmd/lib/server/rest.js:112:25
    at IncomingMessage.onEnd (/Users/efranz/dev/files/lib/cloudcmd/node_modules/pipe-io/lib/pipe.js:203:13)
    at IncomingMessage.emit (events.js:160:13)
    at endReadableNT (_stream_readable.js:1101:12)

ericfranz commented 5 years ago

@MorganRodgers I opened your branch as a PR to merge into this branch: https://github.com/OSC/ood-fileexplorer/pull/183

I also branched off of yours and added a PR addressing all the issues I mentioned in my previous comment except for the tilde expansion issue, which IMHO can wait. https://github.com/OSC/ood-fileexplorer/pull/184 is set to merge into #183 which can merge into this #182

OSC / ood-fileexplorer

Support whitelisting dirs #182