Entitlement verification: Fix request verification#839
Conversation
There was a problem hiding this comment.
Forgot to add the url path to this error.
There was a problem hiding this comment.
I thought about adding a test making sure this happens, but it's tricky since this all happens internally to this class and it seemed wrong to move that logic away from this class. I thought with the real data test would be enough to cover this case.
There was a problem hiding this comment.
Hmm why adding a line break? Responses always end with it, and if they stop adding it, then the signature needs to account for that.
In iOS I added the line break to the test to replicate what the real responses look like so the signature matches.
There was a problem hiding this comment.
Well, the way we are reading the body in android (per lines) actually removes that line break. Which was causing the signature verification to fail. Adding this line break at the end actually fixes the issue (it verifies correctly).
Having said that, you might be right... It might be more accurate to change how we read the body to avoid modifying it at all... Will work on that tomorrow. Thanks!
There was a problem hiding this comment.
Changed approach in latest commits @NachoSoto. Lmk what you think!
Codecov Report
@@ Coverage Diff @@
## entitlements-verification #839 +/- ##
==========================================================
Coverage 82.62% 82.62%
==========================================================
Files 137 137
Lines 4501 4501
Branches 587 587
==========================================================
Hits 3719 3719
Misses 562 562
Partials 220 220 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
NachoSoto
left a comment
There was a problem hiding this comment.
Unless I'm missing something I don't think we want to do this.
There was a problem hiding this comment.
Hmm why adding a line break? Responses always end with it, and if they stop adding it, then the signature needs to account for that.
In iOS I added the line break to the test to replicate what the real responses look like so the signature matches.
There was a problem hiding this comment.
This will read the whole text, it would not be that performant if we had multiple lines, but responses are usually minified anyway so it shouldn't really matter. This way we avoid removing the final new line from the body. CC @vegaro in case you have any thoughts about this approach.
Another thing I noticed is that we were not closing the BufferedReader but we are closing the inputStream later on, so I think we shouldn't be leaking it.
There was a problem hiding this comment.
I think this is much simpler 👍🏻
8619d16 to
125eefa
Compare
There was a problem hiding this comment.
I think this is much simpler 👍🏻
I tried adding that test before and it was difficult but then again, we only want to make sure the new lines are not stripped from the payload, so adding a test for that was easy 👍 |
This reverts commit 988a2c01699448e82a1cf519b4dfa37b5f4c2983.
71f99ed to
b7b2a25
Compare
### Description There was a mismatch in what body the signing expects and what the body we read from the response. We were missing an end of line at the end of the body when verifying the signature. This fixes that issue.
Description
There was a mismatch in what body the signing expects and what the body we read from the response. We were missing an end of line at the end of the body when verifying the signature. This fixes that issue.