kojiromike / Subresource-Signing

User agents should validate resource signatures before processing
MIT License
1 stars 1 forks source link

First fixes and minor tweaks. #1

Closed patricknelson closed 8 years ago

patricknelson commented 8 years ago

Check this out, here's my stab at it. Basically I fixed a few minor redundancies, unused vars, tried newer approaches at things (since we're requiring modern [read: non-IE] browsers) and using HTML5, so we're free to use those dandy global ID's.

Also, biggest change (so far) was the revision of the promise.then(...) in the main srs.load() method. That's the resolution of a promise, so that closure (in as the then() param) is being passed the data processed by GPG, so I removed that extra "reject" parameter (since that's what you put into a Promise constructor anyway) and renamed it a bit. Also, more importantly, I had no way to handle errors, so I used promises as a return type for srs.load(...) as well so that, if I wanted to, I could handle them. Otherwise, they will propagate up as Error's anyway :+1:

I might make more tweaks as I look at it but this is what I've got so far, so here's a PR!

patricknelson commented 8 years ago

Oh! I forgot to mention one of the most important security improvements. I changed this:

var valid = resolve.signatures.filter(sig => sig.valid).length > 0;

to

// Consider the entire response invalid if any of the signatures are not valid.
var valid = data.signatures.filter(sig => sig.valid).length == data.signatures.length;

I figured that made the most sense, since apparently GPG will return multiple possible signatures and, if we're going to be filtering out and counting them anyway, we should be buzzing the alarms if ANY of them aren't valid, not if at least one is valid.

patricknelson commented 8 years ago

We may also want to consider throwing in maybe a gulp/grunt workflow (for shits/giggles) to transpile to Babel for the less fortunate (IE and beyond). But that's probably way out of scope :smile:

patricknelson commented 8 years ago

Another change is that I tweaked the format of the public key supplied inline (even though I know that's not important for final implementation) to follow the more modern standard seen by JavaScript templating where they'll typically encapsulate hidden templates (or text data strictly for JS use) in <script> tags. Like mustache for example.

'stache