Make the Acceptable Ads link clickable

[Felix Dahlke, 2013-11-26 09:21:51]

http://codereview.adblockplus.org/6196972490850304/diff/5707702298738688/src/...
File src/org/adblockplus/android/Preferences.java (right):

http://codereview.adblockplus.org/6196972490850304/diff/5707702298738688/src/...
src/org/adblockplus/android/Preferences.java:160: final int padding = 10;
I'm not a big fan of hard coding style properties, but I think the only
alternative would be to have a proper xml-defined dialog. I'd rather do that in
a follow-up.

[Wladimir Palant, 2013-11-26 09:35:54]

http://codereview.adblockplus.org/6196972490850304/diff/5707702298738688/res/...
File res/values/strings.xml (right):

http://codereview.adblockplus.org/6196972490850304/diff/5707702298738688/res/...
res/values/strings.xml:38: <string
name="acceptable_ads_url">https://adblockplus.org/en/acceptable-ads</string>
This URL shouldn't be hardcoded, libadblockplus already has all the necessary
information:

FilterEngine.GetPref("documentation_link")->AsString()

Here %LINK% needs to be replaced by "acceptable_ads" and %LANG% by the
application locale (e.g.
https://adblockplus.org/redirect?link=acceptable_ads&lang=en-US).

http://codereview.adblockplus.org/6196972490850304/diff/5707702298738688/src/...
File src/org/adblockplus/android/Preferences.java (right):

http://codereview.adblockplus.org/6196972490850304/diff/5707702298738688/src/...
src/org/adblockplus/android/Preferences.java:156: .replace("<a>", "<a href=\"" +
getString(R.string.acceptable_ads_url) + "\">");
This is a security vulnerability, please don't allow localizers to inject random
HTML tags into our UI. You need to escape entities in the string and then
replace &lt;a&gt;(.*?)&lt;/a&gt; by <a href="...">\1</a> - like we do it in
Firefox or Chrome. Note that HTML entities need to be escaped in the URL you
insert as well. The right function to do this seems to be
android.text.TextUtils.htmlEncode().

[Felix Dahlke, 2013-11-26 10:25:50]

Issues addressed.

http://codereview.adblockplus.org/6196972490850304/diff/5707702298738688/res/...
File res/values/strings.xml (right):

http://codereview.adblockplus.org/6196972490850304/diff/5707702298738688/res/...
res/values/strings.xml:38: <string
name="acceptable_ads_url">https://adblockplus.org/en/acceptable-ads</string>
On 2013/11/26 09:35:54, Wladimir Palant wrote:
> This URL shouldn't be hardcoded, libadblockplus already has all the necessary
> information:
> 
> FilterEngine.GetPref("documentation_link")->AsString()
> 
> Here %LINK% needs to be replaced by "acceptable_ads" and %LANG% by the
> application locale (e.g.
> https://adblockplus.org/redirect?link=acceptable_ads&lang=en-US).

Done.

http://codereview.adblockplus.org/6196972490850304/diff/5707702298738688/src/...
File src/org/adblockplus/android/Preferences.java (right):

http://codereview.adblockplus.org/6196972490850304/diff/5707702298738688/src/...
src/org/adblockplus/android/Preferences.java:156: .replace("<a>", "<a href=\"" +
getString(R.string.acceptable_ads_url) + "\">");
On 2013/11/26 09:35:54, Wladimir Palant wrote:
> This is a security vulnerability, please don't allow localizers to inject
random
> HTML tags into our UI. You need to escape entities in the string and then
> replace &lt;a&gt;(.*?)&lt;/a&gt; by <a href="...">\1</a> - like we do it in
> Firefox or Chrome. Note that HTML entities need to be escaped in the URL you
> insert as well. The right function to do this seems to be
> android.text.TextUtils.htmlEncode().

Done. I had a feeling I wouldn't get away with this... :)

[Wladimir Palant, 2013-11-26 10:48:56]

It's up to you whether you want to improve the API now or leave it for later.
The second issue (HTML encoding of the URL) needs to be fixed however. LGTM if
you fix it.

http://codereview.adblockplus.org/6196972490850304/diff/5685265389584384/jni/...
File jni/abpEngine.cpp (right):

http://codereview.adblockplus.org/6196972490850304/diff/5685265389584384/jni/...
jni/abpEngine.cpp:461: JNIEnv *env, jobject object)
This is a fairly useless API. It would make more sense to either expose
FilterEngine::GetPref() (obviously complicated because multiple return types are
possible) or the equivalent of Utils.getDocLink() JavaScript function (currently
implemented in adblockplus and adblockpluschrome repositories). Implementing the
latter in JavaScript would require a change to libadblockplus (the change itself
is trivial, updating libadblockplus binaries unfortunately isn't). Alternatively
the locale parameter could be cached in the init function and reused in the C++
code here.

http://codereview.adblockplus.org/6196972490850304/diff/5685265389584384/src/...
File src/org/adblockplus/android/Preferences.java (right):

http://codereview.adblockplus.org/6196972490850304/diff/5685265389584384/src/...
src/org/adblockplus/android/Preferences.java:159:
.replaceAll("&lt;a&gt;(.*?)&lt;/a&gt;", "<a href=\"" + url + "\">$1</a>");
TextUtils.htmlEncode(url)?

[Felix Dahlke, 2013-11-26 13:11:45]

http://codereview.adblockplus.org/6196972490850304/diff/5685265389584384/jni/...
File jni/abpEngine.cpp (right):

http://codereview.adblockplus.org/6196972490850304/diff/5685265389584384/jni/...
jni/abpEngine.cpp:461: JNIEnv *env, jobject object)
On 2013/11/26 10:48:57, Wladimir Palant wrote:
> This is a fairly useless API. It would make more sense to either expose
> FilterEngine::GetPref() (obviously complicated because multiple return types
are
> possible) or the equivalent of Utils.getDocLink() JavaScript function
(currently
> implemented in adblockplus and adblockpluschrome repositories). Implementing
the
> latter in JavaScript would require a change to libadblockplus (the change
itself
> is trivial, updating libadblockplus binaries unfortunately isn't).
Alternatively
> the locale parameter could be cached in the init function and reused in the
C++
> code here.

Yeah, I agree that it makes sense to expose something better here, I favour
Utils.getDocLink(). But that does indeed require a libadblockplus change, I'll
create a follow-up for it.

http://codereview.adblockplus.org/6196972490850304/diff/5685265389584384/src/...
File src/org/adblockplus/android/Preferences.java (right):

http://codereview.adblockplus.org/6196972490850304/diff/5685265389584384/src/...
src/org/adblockplus/android/Preferences.java:159:
.replaceAll("&lt;a&gt;(.*?)&lt;/a&gt;", "<a href=\"" + url + "\">$1</a>");
On 2013/11/26 10:48:57, Wladimir Palant wrote:
> TextUtils.htmlEncode(url)?

Why? That'd just mess up the link and it wouldn't work anymore. We get this
value from libadblockplus, so XSS is highly unlikely.

[Wladimir Palant, 2013-11-26 13:33:44]

http://codereview.adblockplus.org/6196972490850304/diff/5685265389584384/src/...
File src/org/adblockplus/android/Preferences.java (right):

http://codereview.adblockplus.org/6196972490850304/diff/5685265389584384/src/...
src/org/adblockplus/android/Preferences.java:159:
.replaceAll("&lt;a&gt;(.*?)&lt;/a&gt;", "<a href=\"" + url + "\">$1</a>");
On 2013/11/26 13:11:46, Felix H. Dahlke wrote:
> Why? That'd just mess up the link and it wouldn't work anymore.

No, it will make sure the link is properly encoded. Feel free to check the
following two links with an HTML validator of your choice:

<a href="http://example.com/?foo=bar&x=y">test</a>
<a href="http://example.com/?foo=bar&amp;x=y">test</a>

[Felix Dahlke, 2013-11-26 13:48:01]

http://codereview.adblockplus.org/6196972490850304/diff/5685265389584384/src/...
File src/org/adblockplus/android/Preferences.java (right):

http://codereview.adblockplus.org/6196972490850304/diff/5685265389584384/src/...
src/org/adblockplus/android/Preferences.java:159:
.replaceAll("&lt;a&gt;(.*?)&lt;/a&gt;", "<a href=\"" + url + "\">$1</a>");
On 2013/11/26 13:33:44, Wladimir Palant wrote:
> On 2013/11/26 13:11:46, Felix H. Dahlke wrote:
> > Why? That'd just mess up the link and it wouldn't work anymore.
> 
> No, it will make sure the link is properly encoded. Feel free to check the
> following two links with an HTML validator of your choice:
> 
> <a href="http://example.com/?foo=bar&x=y">test</a>
> <a href="http://example.com/?foo=bar&amp;x=y">test</a>

Ouch, you're right.

[Wladimir Palant, 2013-11-26 14:38:52]

LGTM

[Felix Dahlke, 2013-11-26 15:18:26]

Follow-ups:
https://trello.com/c/iEBrT3cV/342-use-an-xml-defined-dialog-for-the-first-run...
https://trello.com/c/xhq9qC30/343-use-utils-getdoclink-instead-of-duplicating...