950105 - [Wap push] SI messages which contain '&' symbol in the URL are not received

Status: VERIFIED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #944422 +++

While investigating bug 944422 I found an issue in how WBXML code is parsed and decoded into a regular XML file. The WAP-192-WBXML-20010725-a standard [1] mandates that during encoding "character entities in the textual markup (e.g., &) which
can be represented in the target character encoding MUST be converted to string form". In practice this means that an '&' sequence in the original markup will be turned into the '&' character in the WBXML message.

Once the WBXML message is parsed and turned again into XML this process must be reversed for text fields and the following characters need to be turned into their character entity form:

& -> &
< -> &lt;
> -> &gt;

If we fail to do this the resulting XML document will be invalid as &, < and > are not permitted to appear unescaped in a text field.

To fix this problem I believe we should detect and replace these characters with character entities in both WbxmlInlineString.decode() [2] and WbxmlStringTable.decode() [3]. The relevant unit-tests in test_si_pdu_helper.js and test_sl_pdu_helper.js should also be augmented to cover these cases.

This bug is currently causing WAP Push messages containing URL with search parameters (which thus include the '&' character) to be dropped by the WAP Push app as the DOMParser refuses the XML file delivered by gecko. I'm marking this bug as koi? since the original was a koi+.

I'd tackle this bug myself but I spent most of today trying to run the xpcshell test within the emulator with no success so I'm not sure how long it could take me. Maybe someone more familiar with this code can take over.

[1] http://technical.openmobilealliance.org/tech/affiliates/wap/wap-192-wbxml-20010725-a.pdf
[2] http://hg.mozilla.org/mozilla-central/file/8b5875dc7e31/dom/wappush/src/gonk/WbxmlPduHelper.jsm#l137
[3] http://hg.mozilla.org/mozilla-central/file/8b5875dc7e31/dom/wappush/src/gonk/WbxmlPduHelper.jsm#l119

Comment 1

[Jason Smith [:jsmith]]

Triage Notes:

Sounds like a blocker. Search parameters are a common use case for a URL, so this is could definitely happen.

Comment 2

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Escape reserved characters when converting a WBXML document to XML

This is a WIP patch that implements escaping reserved XML characters when parsing inline string and string table references. I'd like to add some tests to verify this works correctly but I've been unable to run the WAP Push xpcshell tests on my device or on the emulator.

I've followed the instructions on MDN https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/XPCShell but couldn't figure out what to pass to the --testing-modules-dir and --manifest parameters.

Chuck can you have a look at my patch and see if it looks reasonable? If you can also help me out with xpcshell tests I should be able to finish this one pretty soon.

Comment 3

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8347966 [details] [diff] [review]
[PATCH WIP] Escape reserved characters when converting a WBXML document to XML

Review of attachment 8347966 [details] [diff] [review]:
-----------------------------------------------------------------

Look good to me, and thanks for fixing this.

xpcshell for WAP Push can't run in emulator is caused by bug 948376, and is fixed now.
you can add test case to dom/wappush/tests/test_cp_pdu_helper.js, and use run test by following command after build emulator.

./test.sh xpcshell --test-path B2G_EMULATOR_PATH/objdir-gecko/dist/test-package-stage/xpcshell/tests/dom/wappush/tests/test_cp_pdu_helper.js

Comment 4

[Gabriele Svelto [:gsvelto]]

(In reply to Chuck Lee [:chucklee] from comment #3)
> xpcshell for WAP Push can't run in emulator is caused by bug 948376, and is
> fixed now.
> you can add test case to dom/wappush/tests/test_cp_pdu_helper.js, and use
> run test by following command after build emulator.
> 
> ./test.sh xpcshell --test-path
> B2G_EMULATOR_PATH/objdir-gecko/dist/test-package-stage/xpcshell/tests/dom/
> wappush/tests/test_cp_pdu_helper.js

Ah, great! Thanks!

Comment 5

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Escape reserved characters when converting a WBXML document to XML

Here's an update patch complete with an additional xpcshell test to exercise this functionality.

Enpei can you check if this patch fixes the issue you tested in bug 944422? The patch I'm attaching applies cleanly to gecko in the 1.2 branch. The behavior should now be identical to Android: when the 'href' field points to an URL with the '&' symbol it will be properly translated and the resulting link in the WAP Push app should be shown and should be identical to what you see in Android.

Comment 6

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8349469 [details] [diff] [review]
[PATCH] Escape reserved characters when converting a WBXML document to XML

Review of attachment 8349469 [details] [diff] [review]:
-----------------------------------------------------------------

Look good to me, Thanks!
But I am not a peer, add vicamo as reviewer.

Comment 7

[Enpei]

Hi Gabriele,

Verified on Buri with attached patch.
Gaia      c82c957e9d4078cd4043de6b7d21a2667a73adf7
Gecko     65dfa0844f0abbe4cfc90708f8620df4811afe6a
BuildID   20131219140435
Version   26.0
ro.build.version.incremental=291
ro.build.date=Wed Dec  4 09:48:04 CST 2013

Nicely work and it will display exactly what user send no matter "&" or "&amp". Also verified other possible special characters(! * ' ( ) ; : @ & = + $ , / ? % # [ ]- _ . ~).

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8349469 [details] [diff] [review]
[PATCH] Escape reserved characters when converting a WBXML document to XML

Review of attachment 8349469 [details] [diff] [review]:
-----------------------------------------------------------------

r=chulee,me. Thank you :)

Comment 9

[Gabriele Svelto [:gsvelto]]

Thanks Chuck & Vicamo for the review and big thanks to Enpei for her thorough testing!

The green try-run is here: https://tbpl.mozilla.org/?tree=Try&rev=0272bad783dc so setting checkin-needed; this needs to land on master and on mozilla-b2g26_v1_2, the patch should apply cleanly everywhere. I've set the flags accordingly, once landed the issue should be fixed across all affected versions.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/778e7a7b267e

Comment 11

[Tony Chung [:tchung]]

koi+. please uplift.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/778e7a7b267e

Comment 13

[Enpei]

(In reply to Wes Kocher (:KWierso) from comment #12)
> https://hg.mozilla.org/mozilla-central/rev/778e7a7b267e

Hi Wes, could you also help to uplift to v1.2 due to this is a koi+ bug. Thanks.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f9a3bb870d9c
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/931864adcf68

Comment 15

[Enpei]

Verified on Buri v1.2 and passed.

Gaia      01e9da49be2cc4bc134eeefc434740d572ec2246
Gecko     252332f9d779541d82b87b9e17b57a4d5a978443
BuildID   20131223095701
Version   28.0a2