webmaxru / node-red-contrib-web-push

A Node-RED node for sending Web Push notifications using VAPID
https://www.npmjs.com/package/node-red-contrib-web-push
6 stars 7 forks source link

Keys in credentials & button to generate keypair #3

Closed bartbutenaers closed 4 years ago

bartbutenaers commented 4 years ago

Hey Maxim,

Here are some of the changes I contacted you about. Unfortunately a few months later than expected ...

Button to generate keypair

To avoid the users having to use command line stuff, I have added a button to generate a keypair:

demo_button

Some remark:

Keys in credentials

Until now the 3 key fields were plain text fields, which has some disadvantages:

To solve that, every Node-RED node has a 'credentials' section where I'm going to store the keys. The major part of the work was to migrate the plain-text keys of old existing nodes to the new keys in the credentials, to make sure we have no impact on existing flows:

  1. I had to keep both the old html elements and the old node fields, otherwise Node-RED messed up the old values behind my back. The new fields have a postfix "2.

  2. As soon as you enter the keys manually (or via the new generation button), the keys will be temporarily be available in the browser:

    image

  3. But once you have deployed the flow, the keys are only available on the server. From then on Node-RED will only send to the browser whether the key fields have content or not:

    image

    So this is much better compared to passing the keys all the time between client and server ...

  4. As long as we keep using input fields of type 'text' you would see indeed that Node-RED only sends dummy text content to fill the html fields:

    image

  5. Therefore we will change the input fields to type "password", to display the dummy text like this:

    image

Should be much secure now...

But there is something that is still not correct ;-( Node-RED makes sure that nodes can only read their own credentials, so other nodes cannot access them! However the keys from your config node are also needed in your web-push node. This means I had to add a new function to the config node, to get access to the keys:

this.getKeys = function () {
        return {
            publicKey : node.credentials.publicKey2  || node.publicKey,
            privateKey: node.credentials.privateKey2 || node.privateKey,
            gcmApiKey : node.credentials.gcmApiKey2  || node.gcmApiKey
        }
 }

And that function is now called from the web-push node. But this means that all other nodes can call it to read the keys, which is NOT secure!!

Would be better if the config node had following two functions:

Do you have any thoughts about this? We can also start continuing with this pull-request, and afterwards fix this issue ...

P.S. when there are no subscriptions, an error was logged. However since it is very likely that there are no subscriptions at some moment in time, I have changed it to warning:

image image

Is that ok for you?

Thanks a lot for reviewing this pull request !!! Bart

webmaxru commented 4 years ago

Thank you for your work! Will publish the new version soon.