831524 - Payment JWT as URL-safe base64 causes decoding error in navigator.mozPay()

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kumar McMillan [:kumar]]

When a JWT is very long it is truncated within navigator.mozPay() and this results in a base64 decoding error or a 'too few segments' error.

Here is a test to reproduce. Load this in a b2g browser and click the button: http://people.mozilla.com/~kmcmillan/trunc.html

Expected: this should open the Trusted UI to begin payment.
Actual: In the console you see a message with an error saying the JWT cannot be decoded.

I can see the JWT that arrives via the global callback is in tact but somewhere after cpmm.sendAsyncMessage() it has been truncated.

Comment 1

[Jason Smith [:jsmith]]

Can you attach a logcat?

Comment 2

[Jason Smith [:jsmith]]

I got a logcat:

01-16 15:39:48.916: I/IdleService(109): next timeout 1000 msec from now
01-16 15:39:48.916: I/IdleService(109): SetTimerExpiryIfBefore: next timeout 1000 msec from now
01-16 15:39:48.916: I/IdleService(109): reset timer expiry to 1009 msec from now
01-16 15:39:48.916: I/IdleService(109): Reset idle timeout: tell observer 489d7de0 user is back
01-16 15:39:49.457: E/GeckoConsole(1458): Content JS LOG at http://people.mozilla.com/~kmcmillan/trunc.html:11 in anonymous: FAIL
01-16 15:39:49.457: E/GeckoConsole(1458): Content JS LOG at http://people.mozilla.com/~kmcmillan/trunc.html:12 in anonymous: PAY_REQUEST_ERROR_ERROR_DECODING_JWT
01-16 15:39:49.927: I/IdleService(109): Get idle time: time since reset 829 msec
01-16 15:39:49.927: I/IdleService(109): Idle timer callback: current idle time 829 msec
01-16 15:39:49.927: I/IdleService(109): next timeout 171 msec from now
01-16 15:39:49.927: I/IdleService(109): SetTimerExpiryIfBefore: next timeout 170 msec from now
01-16 15:39:49.927: I/IdleService(109): reset timer expiry to 180 msec from now
01-16 15:39:50.117: I/IdleService(109): Get idle time: time since reset 1011 msec
01-16 15:39:50.117: I/IdleService(109): Idle timer callback: current idle time 1011 msec
01-16 15:39:50.117: I/IdleService(109): next timeout 4294967293987 msec from now
01-16 15:39:50.117: I/IdleService(109): SetTimerExpiryIfBefore: next timeout 4294967293987 msec from now
01-16 15:39:50.117: I/IdleService(109): reset timer expiry to 4294967293996 msec from now
01-16 15:39:50.117: I/IdleService(109): Idle timer callback: tell observer 489d7de0 user is idle
01-16 15:39:50.117: I/IdleService(109): Get idle time: time since reset 1016 msec

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

We are failing at https://mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.jsm#257 with a "Failed to decode base64 string!" exception thrown from https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsXPConnect.cpp#2349, which is expected for invalid JWTs.

Google JWT decoder (https://developers.google.com/commerce/wallet/digital/docs/jwtdecoder) is also failing with the JWT passed to the mozPay() call from http://people.mozilla.com/~kmcmillan/trunc.html: 

"eyJhbGciOiAiSFMyNTYiLCAidHlwIjogIkpXVCJ9.eyJhdWQiOiAibWFya2V0cGxhY2UuZmlyZWZveC5jb20iLCAiaXNzIjogIm1hcmtldHBsYWNlLWRldi5hbGxpem9tLm9yZyIsICJyZXF1ZXN0IjogeyJuYW1lIjogIktydXBhJ3MgcGFpZCBhcHAgMSIsICJjaGFyZ2ViYWNrVVJMIjogImh0dHA6Ly9sb2NhbGhvc3Q6ODAwMi90ZWxlZm9uaWNhL3NlcnZpY2VzL3dlYnBheS9jaGFyZ2ViYWNrIiwgInBvc3RiYWNrVVJMIjogImh0dHA6Ly9sb2NhbGhvc3Q6ODAwMi90ZWxlZm9uaWNhL3NlcnZpY2VzL3dlYnBheS9wb3N0YmFjayIsICJwcm9kdWN0RGF0YSI6ICJhZGRvbl9pZD04NSZzZWxsZXJfdXVpZD1kNDg1NWRmOS02Y2UwLTQ1Y2QtODFjYi1jZjg3MzdlMWU3YWEmY29udHJpYl91dWlkPTIwMTg2OGI3YWMyY2RhNDEwYTk5YjNlZDRjMTFhOGVhIiwgInByaWNlUG9pbnQiOiAxLCAiaWQiOiAibWF1ZGU6ODUiLCAiZGVzY3JpcHRpb24iOiAiVGhpcyBhcHAgaGFzIGJlZW4gYXV0b21hdGljYWxseSBnZW5lcmF0ZWQgYnkgPGEgaHJlZj1cImh0dHA6Ly9vdXRnb2luZy5tb3ppbGxhLm9yZy92MS9iYTdmMzczYWUxNjc4OWVmZjNhYmZkOTVjYThkM2MxNWQxOGRjOTAwOWFmYTIwNGRjNDNmODVhNTViMWY2ZWYxL2h0dHAlM0EvL3Rlc3RtYW5pZmVzdC5jb21cIiByZWw9XCJub2ZvbGxvd1wiPnRlc3RtYW5pZmVzdC5jb208L2E-In0sICJleHAiOiAxMzU4Mzc5MTQ3LCAiaWF0IjogMTM1ODM3NTU0NywgInR5cCI6ICJtb3ppbGxhL3BheW1lbnRzL3BheS92MSJ9.kgSt636OSRBezMGtm9QLeDxlEOevL4xcOoDj8VRJyD8"

Comment 4

[Jason Smith [:jsmith]]

If Google's JWT decoder and ours are doing the same behavior, then gecko is probably doing this right. This is now a marketplace bug.

Comment 5

[Kumar McMillan [:kumar]]

Google's decoder just says "invalid JWT" but it doesn't say why. It's probably because we're missing some of their required fields (Mozilla's format is a little different). If you open the source of trunc.html and take the JWT string, try decoding it with a JWT library, e.g. Python's pyjwt:

import jwt
jwt.decode('...', verify=False)

It works fine for me. It's a valid JWT so I don't see why we shouldn't make it work in mozPay(). When I compared string lengths I saw truncation. You don't see that? Check in pay() before cpmm.sendAsyncMessage() passes the JWT through.

Comment 6

[Kumar McMillan [:kumar]]

After taking a closer look, it's the encoded HTML that is causing a problem in gecko. The atob function is throwing "String contains an invalid character". When I remove HTML from the test in trunc.html it passes.

The base64 decoder in Python (and probably other ones) doesn't have this problem. There is no reason why mozPay() should choke on encoded HTML.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Kumar McMillan [:kumar] from comment #6)
> After taking a closer look, it's the encoded HTML that is causing a problem
> in gecko. The atob function is throwing "String contains an invalid
> character". When I remove HTML from the test in trunc.html it passes.
> 
> The base64 decoder in Python (and probably other ones) doesn't have this
> problem. There is no reason why mozPay() should choke on encoded HTML.

Why are using encoded HTML in the first place?

Comment 8

[Kumar McMillan [:kumar]]

HTML is useful in app descriptions because it provides a way for buyers to get more info about the app. We have both a well tested HTML sanitizer (for malicious input) as well a link proxy server to block malware so there is no reason why we should disallow encoded HTML in JWTs.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Kumar McMillan [:kumar] from comment #6) 
> The base64 decoder in Python (and probably other ones) doesn't have this
> problem. There is no reason why mozPay() should choke on encoded HTML.

FTR the issue is in the JS atob() function implementation, not in mozPay().

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

atob() WebKit implementation does not fail decoding a payload containing HTML.

From Google Chrome JS console:

atob("eyJhdWQiOiAibW9ja3BheXByb3ZpZGVyLnBocGZvZ2FwcC5jb20iLCAiaXNzIjogIkVudGVyIHlvdSBhcHAga2V5IGhlcmUhIiwgInJlcXVlc3QiOiB7Im5hbWUiOiAiUGllY2Ugb2YgQ2FrZSIsICJwcmljZSI6ICIxMC41MCIsICJwcmljZVRpZXIiOiAxLCAicHJvZHVjdGRhdGEiOiAidHJhbnNhY3Rpb25faWQ9ODYiLCAiY3VycmVuY3lDb2RlIjogIlVTRCIsICJkZXNjcmlwdGlvbiI6ICJWaXJ0dWFsIGNob2NvbGF0ZSBjYWtlIHRvIGZpbGwgeW91ciB2aXJ0dWFsIHR1bW15In0sICJleHAiOiAxMzUyMjMyNzkyLCAiaWF0IjogMTM1MjIyOTE5MiwgInR5cCI6ICJtb2NrL3BheW1lbnRzL2luYXBwL3YxIn0");

"{"aud": "mockpayprovider.phpfogapp.com", "iss": "Enter you app key here!", "request": {"name": "Piece of Cake", "price": "10.50", "priceTier": 1, "productdata": "transaction_id=86", "currencyCode": "USD", "description": "Virtual chocolate cake to fill your virtual tummy"}, "exp": 1352232792, "iat": 1352229192, "typ": "mock/payments/inapp/v1"}"

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Actually, forget about my previous comment.

atob('eyJhdWQiOiAibWFya2V0cGxhY2UuZmlyZWZveC5jb20iLCAiaXNzIjogIm1hcmtldHBsYWNlLWRldi5hbGxpem9tLm9yZyIsICJyZXF1ZXN0IjogeyJuYW1lIjogIktydXBhJ3MgcGFpZCBhcHAgMSIsICJjaGFyZ2ViYWNrVVJMIjogImh0dHA6Ly9sb2NhbGhvc3Q6ODAwMi90ZWxlZm9uaWNhL3NlcnZpY2VzL3dlYnBheS9jaGFyZ2ViYWNrIiwgInBvc3RiYWNrVVJMIjogImh0dHA6Ly9sb2NhbGhvc3Q6ODAwMi90ZWxlZm9uaWNhL3NlcnZpY2VzL3dlYnBheS9wb3N0YmFjayIsICJwcm9kdWN0RGF0YSI6ICJhZGRvbl9pZD04NSZzZWxsZXJfdXVpZD1kNDg1NWRmOS02Y2UwLTQ1Y2QtODFjYi1jZjg3MzdlMWU3YWEmY29udHJpYl91dWlkPTIwMTg2OGI3YWMyY2RhNDEwYTk5YjNlZDRjMTFhOGVhIiwgInByaWNlUG9pbnQiOiAxLCAiaWQiOiAibWF1ZGU6ODUiLCAiZGVzY3JpcHRpb24iOiAiVGhpcyBhcHAgaGFzIGJlZW4gYXV0b21hdGljYWxseSBnZW5lcmF0ZWQgYnkgPGEgaHJlZj1cImh0dHA6Ly9vdXRnb2luZy5tb3ppbGxhLm9yZy92MS9iYTdmMzczYWUxNjc4OWVmZjNhYmZkOTVjYThkM2MxNWQxOGRjOTAwOWFmYTIwNGRjNDNmODVhNTViMWY2ZWYxL2h0dHAlM0EvL3Rlc3RtYW5pZmVzdC5jb21cIiByZWw9XCJub2ZvbGxvd1wiPnRlc3RtYW5pZmVzdC5jb208L2E-In0sICJleHAiOiAxMzU4Mzc5MTQ3LCAiaWF0IjogMTM1ODM3NTU0NywgInR5cCI6ICJtb3ppbGxhL3BheW1lbnRzL3BheS92MSJ9');
Error: INVALID_CHARACTER_ERR: DOM Exception 5

It seems that it is also failing in Google Chrome atob implementation

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Blake, is atob() supposed to support HTML encoding?

Comment 13

[Jason Smith [:jsmith]]

What's the risk of not supporting HTML encoding for v1?

We're really late in the game right now, so it's going to be a tough sell in triage to get this block unless this is a critical showstopper.

Comment 14

[Kumar McMillan [:kumar]]

I think it's fine to work around this by stripping HTML from app descriptions for v1. It might trip up developers when they encounter this error for in-app payments though (since the decode error does not explain much). We can add a warning in the docs.

Comment 15

[Jason Smith [:jsmith]]

Based on the comments I'm hearing above, this shouldn't block, but we should consider doing this in a future release.

Tracking nom.

Comment 16

[Kumar McMillan [:kumar]]

We worked around bug 830450 by stripping HTML so this isn't a major blocker. Seems like something worth fixing though since Python's base64 decoder can handle it.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #12)
> Blake, is atob() supposed to support HTML encoding?

There seems to be a bunch of confusion here. atob should accept any valid Base64 string and decode it to its original form. If that form was HTML, atob doesn't care. Our implementation of base 64 encoding doesn't accept non-7-bit-clean strings, so that is a potential concern (https://developer.mozilla.org/en-US/docs/DOM/window.btoa has more information about that). 

That being said, I don't believe the problem here is necessarily that we're encoding HTML, but that there are two different Base64 alphabets in play. In Gecko (and presumably WebKit), Base64 encoded values can have letters, numbers, + and /. In the Base64 value found at the URL (it would be good to attach that testcase to this bug for posterity, btw) there's a -, which is invalid for us but presumably valid for some other Base64 library. In fact, if I replace the hyphen in that input with a forward slash and feed it to Gecko's atob, the decoding works and I get a JSON string.

It seems to me like we should insist on the Base64 values passed to us being valid according to our Base64 decoder.

Comment 18

[Kumar McMillan [:kumar]]

Attachment: html page to reproduce the issue

Comment 19

[Kumar McMillan [:kumar]]

Thanks Blake, this was really helpful. Funny enough, it seems that using dashes was done on purpose by our jwt lib to make things url safe http://docs.python.org/2/library/base64.html#base64.urlsafe_b64encode Interesting, I'll see what it would mean to patch our jwt lib.

Comment 20

[Kumar McMillan [:kumar]]

Here's a simple case in Python to illustrate the problem.

This fails (similar to what's going on in atob()):

>>> import base64 as b
>>> b.standard_b64decode(b.urlsafe_b64encode('<a href="http://outgoing.mozilla.org/v1/ba7f373ae16789eff3abfd95ca8d3c15d18dc9009afa204dc43f85a55b1f6ef1/http%3A//testmanifest.com">testmanifest.com</a>'))
Traceback (most recent call last):
...
TypeError: Incorrect padding

This works:

>>> b.urlsafe_b64decode(b.urlsafe_b64encode('<a href="http://outgoing.mozilla.org/v1/ba7f373ae16789eff3abfd95ca8d3c15d18dc9009afa204dc43f85a55b1f6ef1/http%3A//testmanifest.com">testmanifest.com</a>'))
'<a href="http://outgoing.mozilla.org/v1/ba7f373ae16789eff3abfd95ca8d3c15d18dc9009afa204dc43f85a55b1f6ef1/http%3A//testmanifest.com">testmanifest.com</a>'

Comment 21

[Kumar McMillan [:kumar]]

This is turning into a real problem that I think we need to fix. All off the shelf JWT libraries use base64 *URL* encoding which breaks atob(). Can we do a pre-conversion in nav.mozPay() that converts - and _ back to + and / ?


From the spec, JWT segments are meant to always base64 URL encoded:

Base64url Encoding
    For the purposes of this specification, this term always refers to the URL- and filename-safe Base64 encoding described in RFC 4648 [RFC4648], Section 5, with the (non URL-safe) '=' padding characters omitted, as permitted by Section 3.2. (See Appendix C of [JWS] for notes on implementing base64url encoding without padding.) 

http://openid.net/specs/draft-jones-json-web-token-07.html

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

According to Blake's description (thanks Blake!) "Base64 encoded values can have letters, numbers, + and /", Gecko implements the "Modified Base64 encoding for UTF-7" as described in http://en.wikipedia.org/wiki/Base64#Implementations_and_history

Since we are supposed to support Base64 encoded string as described in RFC4648 for our payment JWTs, we should also take care of the pad char (=), right?

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Nevermind, ignore my previous comment. I misread  the "with the (non URL-safe) '=' padding characters omitted" from the above comment.

Comment 24

[Kumar McMillan [:kumar]]

Thanks ferjm. I can't think of any easy way to work around this so fixing it seems like the right thing. 

Keep in mind that after navPay() converts URL-safe base64 to standard base64 and does the atob() then it should also convert it back to URL-safe base64 since it gets passed to the payment processor like https://payserver.net/?req=<jwt>

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

I'll be adding tests for this to Bug 809219

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ef37f5cf7d6

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 715182 [details] [diff] [review]
v1

[Approval Request Comment]
User impact if declined: No HTML descriptions. Comment 8
Testing completed: Local testing and unit tests added to Bug 809219
Risk to taking this patch (and alternatives if risky): Very low risk
String or UUID changes made by this patch: None

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ef37f5cf7d6

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

Would be great to get confirmation from QA that this is no longer an issue on nightlies before we consider for uplift to v1-train.

Comment 30

[Kumar McMillan [:kumar]]

jsmith, can you re-run the STR?

To clarify comment 27 : this affects way more than just HTML descriptions. All third party libs that implement the JWT spec correctly use URL-safe base64. Without this fix all of in-app payments will be broken in cases where the output is not compatible with standard base64.

Comment 31

[Jason Smith [:jsmith]]

The STR in comment 0 on a central build with this patch works for me - the trusted UI is coming up, nothing scary in the logs. Good for uplift.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b37615faee29

Comment 33

[Jason Smith [:jsmith]]

This wouldn't block ship, but we might want to uplift this to v1.01 for these reasons:

1. This is a very low risk patch and mozPay (gecko-level) has been stable with few issues - so I'm not worried about regressions here. The payment issues we've had client-side typically have been on the gaia side.

2. This has already been tested and verified by QA

3. According to Kumar, this will allow third-party developers to test our in-app payment system without hitting unexpected errors. Not fixing this will force workarounds on the marketplace side in the API docs, as we're not matching the JWT spec

My gut is to uplift this on the risk vs. reward, but I'll let triage make the call.

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

We'll take this verified fix for uplift, flags set accordingly, please land asap.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8358ad0f29f5