bunqCommunity / bunqJSClient

A javascript SDK for the bunq API
MIT License
58 stars 22 forks source link

Fix signature verification #14

Closed robbertkl closed 6 years ago

robbertkl commented 6 years ago

I noticed a mistake in the signature verification code and fixed it according to forge's own examples.

I guess this is why response verification is disabled?

https://github.com/BunqCommunity/BunqJSClient/blob/66be2aa45ca17352a14293f38ef4444c31a02c7f/src/ApiAdapter.ts#L369-L377

Also, together with re-enabling response verification, this will probably fix #4, unless there are other things preventing it from successfully verifying the response.

codecov[bot] commented 6 years ago

Codecov Report

Merging #14 into master will increase coverage by 0.68%. The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
+ Coverage   92.78%   93.47%   +0.68%     
==========================================
  Files          49       49              
  Lines        1456     1472      +16     
  Branches      295      304       +9     
==========================================
+ Hits         1351     1376      +25     
+ Misses        105       95      -10     
- Partials        0        1       +1
Impacted Files Coverage Δ
src/Api/SandboxUser.ts 100% <ø> (ø) :arrow_up:
src/Api/Installation.ts 100% <ø> (ø) :arrow_up:
src/BunqJSClient.ts 85.51% <ø> (ø) :arrow_up:
src/Api/DeviceRegistration.ts 100% <ø> (ø) :arrow_up:
src/Helpers/ErrorCodes.ts 100% <100%> (ø) :arrow_up:
src/Crypto/Sha256.ts 100% <100%> (ø) :arrow_up:
src/Api/SessionServer.ts 100% <100%> (ø) :arrow_up:
src/ApiAdapter.ts 78.62% <88.88%> (+10.52%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e3705f2...41cc60e. Read the comment docs.

Crecket commented 6 years ago

Thanks! I'll look into this later today

EDIT: And just to clarify, the code you quoted is disabled intentionally yes since the verification function wasn't functional yet

Crecket commented 6 years ago

It seems to be working, I'll add some tests when I get home and then create a new release

robbertkl commented 6 years ago

Great, thanks

robbertkl commented 6 years ago

@Crecket Haven't checked if you're testing this, but make sure to verify the signature verification is also working with binary data, e.g. GET /v1/attachment-public/<uuid>/content. Since the forge verification happens on a string, there's a chance the binary data got encoded as a UTF-8 string somewhere along the way.

Crecket commented 6 years ago

Yeah I'm working on this now 👍 I actually just created an issue for it so I'll disable it for now and fix it later this week when I have more time