Remove cryptiles dependency #102
Conversation
| } | ||
| catch (err) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Might be a good idea to propagate this error?
| return cryptiles.fixedTimeComparison(audience[0].textContent, realm); | ||
|
|
||
| try { | ||
| return crypto.timingSafeEqual(Buffer.from(audience[0].textContent), Buffer.from(realm)); |
There was a problem hiding this comment.
While this change is benign (cryptiles code is inlined here), the calling code should check if compared buffers have the same length. Lack of this validation by the caller results in an exception being thrown.
While use of timingSafeEqual is admirable, I don't see how this might be used as an oracle, and I think a simple string comparison would be just as good.
There was a problem hiding this comment.
Ok, so first you want a length check on ´audience[0].textContent´ and ´realm´ and then you want to replace the ´timingSafeEqual´ with a simple ´===´ comparison. Is this correct?
|
@CalRobert, @MarcinHoppe: I saw that the only thing 'Cryptiles' was doing was to wrap the 'timingSafeEqual' method, so what I did was just to move the wrapped method into our library. While I agree with you I did not want to complicate this more than necessary. The functionality should be the exact same as before. |
|
@david-nossebro While I'm with Marcin on this one where timingSafeEqual is overkill, I'm ok with this change going out as it seems to be backwards compatible with the current implementation and getting the ball rolling. My reasoning behind this being overkill is that we're not encapsulating all checks into a single timing-safe timeframe so this single check being timingsafe does not provide any useful purpose. |
Since Cryptiles contains a security issue mentioned here hapijs/cryptiles#34 I have removed the dependency.
This solves the issue #101 I reported.