Node.js module using Promises (client for KeePassHttp)

Question

This is my first Node module, as well as the first time using Promises in Javascript. It is a client for the KeePass plugin "KeePassHTTP" to expose passwords securely, which I am planning on using to pass credentials to gulp.

All feedback welcome!

// Code based on https://github.com/belaviyo/keepass-macpass-helper
// Keepass HTTP protocol documentation - https://github.com/pfn/keepasshttp

const sjcl = require('./sjcl').sjcl;                     // not using npm module because it doesn't have support for cbc
const nconf = require('nconf');
const rp = require('request-promise-native');

let port = null;
let key = null;
let id = null;

function iv(len = 16) {
  let iv = [];
  for (let i = 0; i < len; i++) {
    iv.push(String.fromCharCode(Math.floor(Math.random() * 256)));
  }
  return new Buffer(iv.join(''), 'binary').toString('base64');
}

function encrypt(data, iv) {
  const enc = sjcl.mode.cbc.encrypt(
      new sjcl.cipher.aes(sjcl.codec.base64.toBits(key)),
      sjcl.codec.utf8String.toBits(data),
      sjcl.codec.base64.toBits(iv)
  );
  return sjcl.codec.base64.fromBits(enc);
}

function decrypt(data, iv) {
  const dec = sjcl.mode.cbc.decrypt(
      new sjcl.cipher.aes(sjcl.codec.base64.toBits(key)),
      sjcl.codec.base64.toBits(data),
      sjcl.codec.base64.toBits(iv)
  );
  return sjcl.codec.utf8String.fromBits(dec);
}

function verify(request) {
  const nonce = iv();
  request['Nonce'] = nonce;
  request['Verifier'] = encrypt(nonce, nonce);
  if (id) {
    request['Id'] = id;
  }
  return request;
}

function post(request) {
  return rp({
    method: 'POST',
    uri: `http://localhost:${port}`,
    body: request,
    json: true
  });
}

function init() {
  return new Promise(function(resolve) {
    nconf.file({file: '.keepass'});

    nconf.defaults({
      port: 19455
    });

    port = nconf.get('port');
    key = nconf.get('key');
    id = nconf.get('id');

    if (!key) {
      key = iv(32);
      nconf.set('key', key);
      nconf.save();
    }
    resolve();
  });
}

function test() {
  return new Promise((resolve, reject) => {
    let request = {
      RequestType: 'test-associate',
      TriggerUnlock: false,
    };
    request = verify(request);
    post(request)
        .then(response => {
          if (response && response['Success']) {
            resolve(response);
          } else {
            reject(response);
          }
        })
        .catch(response => reject(response));
  });
}

function associate() {
  return new Promise((resolve, reject) => {
    let request = {
      RequestType: 'associate',
      Key: key
    };
    request = verify(request);
    post(request)
        .then(response => {
          if (response && response['Success']) {
            id = response['Id'];
            nconf.set('id', id);
            nconf.save();
            resolve(response);
          } else {
            reject(response);
          }
        })
        .catch(response => reject(response));
  });
}

function logins(url) {
  return new Promise((resolve, reject) => {
    let request = {
      RequestType: 'get-logins',
      TriggerUnlock: 'false',
      SortSelection: 'false',
    };
    request = verify(request);
    const iv = request['Nonce'];
    request['Url'] = encrypt(url, iv);

    post(request)
        .then(response => {
          if (response && response['Entries']) {
            let nonce = response.Nonce;
            response.Entries = response.Entries.map(entry => {
              return Object.assign(entry, {
                Login: decrypt(entry.Login, nonce),
                Name: decrypt(entry.Name, nonce),
                Password: decrypt(entry.Password, nonce)
              })
            });
            resolve(response);
          } else {
            reject(response);
          }
        })
        .catch(response => reject(response));
  });
}

// itl: init -> test -> logins

function itl(url) {
  return new Promise((resolve, reject) => {
    init()
        .then(() => test())
        .then(() => logins(url))
        .then(response => resolve(response))
        .catch(() => {
          associate()
              .then(() => logins(url))
              .then(response => resolve(response))
              .catch(response => reject(response));
        });
  });
}

module.exports = {
  init,
  test,
  associate,
  logins,
  itl,
};

Example Usage:

const keepass = require('./keepass/keepass.js');

keepass.itl('www.example.com')
    .then(result => console.log('Success', result))
    .catch(result => console.log('Error', result))

Thanks for all your comments. I've published the module to npm with most of the suggestions implemented. npmjs.com/package/keepass-http-client — michoel
– michoel, Commented Mar 15, 2017 at 2:49

John · Accepted Answer · 2017-03-11 08:14:35Z

Since no one else has commented on your use of promises I will do so.

Just mainly looking at your promise code I see one repeating pattern that could be Improved. Your use of new Promise() in your code is often not needed in the ways you are using it. Generally creating a new promise is only needed when working with asynchronous code that is not already using promises.

Let's look at one example of how your usage of new Promise is verbose, unnecessary, and a little redundant.

function logins(url) {
  return new Promise((resolve, reject) => {
    let request = {
      RequestType: 'get-logins',
      TriggerUnlock: 'false',
      SortSelection: 'false',
    };
    request = verify(request);
    const iv = request['Nonce'];
    request['Url'] = encrypt(url, iv);

    post(request)
        .then(response => {
          if (response && response['Entries']) {
            let nonce = response.Nonce;
            response.Entries = response.Entries.map(entry => {
              return Object.assign(entry, {
                Login: decrypt(entry.Login, nonce),
                Name: decrypt(entry.Name, nonce),
                Password: decrypt(entry.Password, nonce)
              })
            });
            resolve(response);
          } else {
            reject(response);
          }
        })
        .catch(response => reject(response));
  });
}

The above can be simplified to this

function logins(url) {
    let request = {
        RequestType: 'get-logins',
        TriggerUnlock: 'false',
        SortSelection: 'false',
    };
    request = verify(request);
    const iv = request['Nonce'];
    request['Url'] = encrypt(url, iv);

    return post(request)
        .then(response => {
            if (response && response['Entries']) {
                let nonce = response.Nonce;
                response.Entries = response.Entries.map(entry => {
                    return Object.assign(entry, {
                    Login: decrypt(entry.Login, nonce),
                    Name: decrypt(entry.Name, nonce),
                    Password: decrypt(entry.Password, nonce)
                    })
                });
                return response;
            } else {
                throw new Error(response);
            }
        })
}

In both of those pieces of code a promise is being returned. In both cases the promise will need to be caught by the calling function when an error happens, but will provide the response when no error is there. All I did was refactor the code and it should behave the exact same way. There is now just less complexity. I would do this for all the other times you wrap a promise around another promise.

Note how I threw an error inside the then method of your promise this will just send the error to the next catch in your promise chain (which will be in the calling function since there is no catch there.) Also note that I removed the catch this means any error that would be caught by the catch that was there now must be caught by the calling function.

Hope that helped if you need further help or have any questions let me know.

Nice job cleaning up the promises. I'd prefer to return Promise.reject(response) over throwing an error in async code. See this StackOverflow post — Craig Ayre
– Craig Ayre, Commented Mar 14, 2017 at 13:27
@CraigAyre "Any time you are inside of a promise callback, you can use throw. However, if you're in any other asynchronous callback, you must use reject." That's a quote from the accepted answer. That throw is inside a promise not just any normal async code. So, it basically makes no difference, but if you want to get in a good habit that will work anywhere then you are probably making the right choice going with returning Promise.reject(). — John
– John, Commented Mar 14, 2017 at 19:14

Peter Taylor · Accepted Answer · 2017-03-08 11:12:51Z

1

I hope someone more qualified than me can give you feedback on the JavaScript style and the use of promises. What stands out to me is the iv method, for two reasons:

Math.floor(Math.random() * 256) is not appropriate for a security-related application. You're already using SJCL, so the only reason not to use sjcl.random would be if there's something even better available.
Less importantly, iv isn't the best name for a method which is also used to generate keys.

answered Mar 8, 2017 at 11:12

Peter Taylor

24.5k1 gold badge49 silver badges94 bronze badges

\$\begingroup\$ Thanks for your comments. The iv() routine is pretty much just copied from the Chrome extension I based the code on.. I am hoping someone can offer feedback on my use of Promises. \$\endgroup\$

michoel
– michoel

2017-03-08 22:34:36 +00:00
Commented Mar 8, 2017 at 22:34

Add a comment |

konijn · Accepted Answer · 2017-03-10 12:35:09Z

Interesting question:

Scope

I have problems with this code and where you declare id:
```
    id = response['Id'];
    nconf.set('id', id);
    nconf.save();
```
I would write this as
```
    nconf.set('id', response['Id']);
    nconf.save();
```
But that would break the code, because id is declared all the way on top, and you use it as a global variable (check function verify). That is not good practice.

Idioms and Custom

Dot notation is almost always better, use request.Nonce, not request['Nonce']
This is interesting: let port = null;, I would just declare var port, key, id;. I would never assign null unless I will actually compare to null later on in the code.

Naming

I am a believer in spartan coding, but you may have crossed the line here : function iv and function ivl

Comments

You have too few comments, and the code could definitely use some more

Don't repeat yourself

'Magic Numbers' like 19455 should be declared in a constant on top

JsHint.com

Your code fails on some minor items, consider using http://jshint.com/ to perfect your code

To be clear, all in all your code is readable, maintainable, I like it.

But that would break the code, because id is declared all the way on top, and you use it as a global variable (check function verify). That is not good practice. - are you sure about that? I was under the understanding that in node.js variables are scoped to the module, and is the correct way of having a variable available to the entire module (but not global). — michoel
– michoel, Commented Mar 14, 2017 at 0:09
You have too few comments, and the code could definitely use some more. Can you give an example of where you think the code would benefit from more comments? — michoel
– michoel, Commented Mar 14, 2017 at 0:12

Stack Exchange Network

Node.js module using Promises (client for KeePassHttp)

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Node.js module using Promises (client for KeePassHttp)

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions