search

Learnosity / learnosity-sdk-php

Learnosity SDK for PHP
Apache License 2.0
10 stars 10 forks source link

[BUG] Signature mismatch with large floats #22

Closed klauste closed 6 years ago

klauste commented 6 years ago

LRN-17413

ekcolysp commented 6 years ago

Hi @klauste,

Given that our server back-end is also in PHP, do we need to make this change to the PHP SDK? I understand that the Java SDK exhibits this problem, tho my thinking is that given both the SDK and the server is in PHP, it should resolve itself out. There is a potential risk in making this modification.

If it is needed, then you can ignore this comment.

klauste commented 6 years ago

@ekcolysp problem is that the front end passes the request json as a string, where the floats are not in scientific notation. This gets hashed to get the signature (so it differs from the signature in the sdk where the floats are encoded in scientific notation). What we could do is json_encode(json_decode(requestString)) in the backend before hashing, that should also fix the issue. I haven't thought about this before, let me give it a try, and if it works we can decide which way is better.

klauste commented 6 years ago

@shtrom @ekcolysp if we add these lines:

$jsonOptions = JSON_UNESCAPED_UNICODE|JSON_UNESCAPED_SLASHES; $requestBody = json_encode(json_decode($requestBody, true), (int)$jsonOptions);

here: https://github.com/Learnosity/learnositypackages/blob/master/LearnosityPackages/Consumer/Authentication.php#L202

this would be resolved without changing all the sdks. Before I fix the review comments here, let me check if anyone has objections against this.

What do you guys think? Would that be a better solution or do you see any issues?

klauste commented 6 years ago

Ok, after giving this some more thought, I think we should stick with fixing the sdks. Too many unknowns otherwise, e.g. what if the front end does not generate scientific notation or if Java encodes different floats in scientific notation than PHP. So I reckon it's better if we just say that floats are not allowed to be in scientific notation.

klauste commented 6 years ago

@shtrom fixed up your comments, could you have another look?

klauste commented 6 years ago

@ekcolysp that's fixed. I really learnt something from that comment, probably made the mistake of passing the result as an argument in several of my recursive functions.