Closed Bug 831524 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla21
blocking-b2g tef+
Tracking Status
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: kumar, Assigned: ferjm)

References

Details

Attachments

(2 files)

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.
Blocks: 830450
Can you attach a logcat?
blocking-b2g: --- → tef?
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
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"
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.
blocking-b2g: tef? → ---
Component: DOM: Device Interfaces → Payments/Refunds
Product: Core → Marketplace
Version: Trunk → 1.0
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.
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.
(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?
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.
(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().
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"}"
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
Blake, is atob() supposed to support HTML encoding?
Flags: needinfo?(mrbkap)
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.
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.
Component: Payments/Refunds → DOM: Device Interfaces
Product: Marketplace → Core
Version: 1.0 → Trunk
Based on the comments I'm hearing above, this shouldn't block, but we should consider doing this in a future release.

Tracking nom.
tracking-b2g18: --- → ?
Summary: A long payment JWT is truncated somewhere in navigator.mozPay() → Payment JWT w/ encoded HTML causes decoding error in navigator.mozPay()
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.
(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.
Flags: needinfo?(mrbkap)
Blocks: basecamp-payments
No longer blocks: marketplace-payments
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.
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>'
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
Assignee: nobody → ferjmoreno
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?
Nevermind, ignore my previous comment. I misread  the "with the (non URL-safe) '=' padding characters omitted" from the above comment.
Summary: Payment JWT w/ encoded HTML causes decoding error in navigator.mozPay() → Payment JWT as URL-safe base64 causes decoding error in navigator.mozPay()
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>
Attached patch v1Splinter Review
I'll be adding tests for this to Bug 809219
Attachment #715182 - Flags: review?(fabrice)
Attachment #715182 - Flags: review?(fabrice) → review+
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
Attachment #715182 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/mozilla-central/rev/1ef37f5cf7d6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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.
Keywords: qawanted, verifyme
Keywords: qawanted
QA Contact: jsmith
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.
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Attachment #715182 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
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.
blocking-b2g: --- → tef?
We'll take this verified fix for uplift, flags set accordingly, please land asap.
blocking-b2g: tef? → tef+
You need to log in before you can comment on or make changes to this bug.