Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIMOB-24695] Android: Fix URL decoding #9097

Closed
wants to merge 1 commit into from

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented May 30, 2017

  • Use decoded URL for requests
var win = Ti.UI.createWindow(),
    iv = Ti.UI.createImageView({
        image: 'http://scontent.xx.fbcdn.net.rsz.io/v/t1.0-9/s720x720/17795851_419371511758070_7481565181564022529_n.jpg?oh=7772aea44d2f40fdbd42285ca61d7aac&oe=59771D60?mode=crop&width=333&height=250'
    });

win.add(iv);
win.open();

JIRA Ticket

@garymathews garymathews added this to the 6.2.0 milestone May 30, 2017
@garymathews garymathews requested a review from ypbnv May 30, 2017 13:22
@jquick-axway
Copy link
Contributor

@garymathews, you shouldn't send an HTTP request with a "decoded" URL. It can create an invalid URL. For example, if the URL contains an encoded space character (ex: "%20" or '+'), then the URLDecoder would turn that into a literal space ' ' character, making the URL invalid.

@garymathews
Copy link
Contributor Author

@jquick-axway Good catch! But URLDecoder shouldn't invalidate the URL like that

var win = Ti.UI.createWindow(),
    iv = Ti.UI.createImageView({
        image: 'http://garymathews.com/pin%20wheel+.jpg'
        //image: 'http://garymathews.com/pin wheel+.jpg'
        //image: 'http://garymathews.com/pin%20wheel%2B.jpg'
        //image: 'http://garymathews.com/pin wheel%2B.jpg'
    });

win.add(iv);
win.open();

Should all still be valid

@jquick-axway
Copy link
Contributor

Okay. Thanks for testing it.
Perhaps the URI constructor re-encodes the URL again making this a non-issue.

Side Note:
The Axway JIRA site (link below; if you have access) has recently received an issue regarding the Titanium HttpClient double encoding URLs (ex: "%" -> "%%") on Android. I initially blew it off as an Appcelerator Studio logging issue (which is in fact a logging bug), but it now sounds like this may actually be an HTTP request bug similar to what you are fixing.
https://techweb.axway.com/jira/browse/SWAGGEN-133

Copy link
Contributor

@ypbnv ypbnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. Just the empty catch block are bugging me.

@@ -54,6 +55,10 @@ protected TiDownloadManager()

public void download(URI uri, TiDownloadListener listener)
{
try {
uri = new URI(URLDecoder.decode(uri.toString(), "utf8"));
} catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add logging the exception?

if (TiResponseCache.peek(uri)) {
InputStream stream = TiResponseCache.openCachedStream(uri);
if (stream != null) {
// Fallback to actual download when null
return stream;
}
}
u = uri.toURL();
} catch (URISyntaxException uriException) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here?

@jquick-axway
Copy link
Contributor

So, I've spent most of the day digging into this.

First, I've confirmed that the URLDecoder.decode() method in your code can produce an invalid URL that will cause the URI() constructor to throw a "URISyntaxException". For example, your "http://garymathews.com/pin%20wheel+.jpg" URL when decoded (%20 and '+' turn into spaces) will cause an exception when fed into the URI() constructor because a space character is not valid in a URL. Your current code catches it and ends up using the old URL instead.

Second, the "URLEncoder" and "URLDecoder" Java classes are only intended to encode/decode URL "parameters" (ie: the part to the right of the '?' in the URL). They are not intended to encode/decode the URL host name and path (I just learned this today as well). Query parameters are expected to use the "application/x-www-form-urlencoded" MIME format. The rest of the URL is expected to follow the "RFC 2373" spec which can be encoded via the Java Uri.encode() and Uri.decode() methods. And in my testing, all of these Java classes encode some characters they shouldn't and don't fully support both specs. Unfortunately, I haven't found any "magic" class/method that'll easily fully encode URLs for us without issues either. :(

So, the root cause of this bug (where question marks in the query parameters are being percent encoded when they shouldn't) lies in our TiUrl.getCleanUri() method. This is the method that gets called by our ImageView and HttpClient to encode URLs. A simple solution to this particular problem would be to modify the line here...
https://github.com/appcelerator/titanium_mobile/blob/master/android/titanium/src/java/org/appcelerator/titanium/util/TiUrl.java#L327

...to the following (I've added '?' and '/' to the do-not-encode list)...

builder.encodedQuery(Uri.encode(Uri.decode(base.getQuery()), "&=?/"));

But that said, there are other URL escaping issues in this code as well that's affecting ArrowDB queries in the following JIRA case assigned to me.
https://techweb.axway.com/jira/browse/SWAGGEN-133

I'm already re-working TiUrl.getCleanrUri() now. Do you mind if I take over fixing this? Otherwise our code changes are going to collide.

@garymathews
Copy link
Contributor Author

@jquick-axway Thanks for looking into this, you should take over this issue. I'll close this PR and assign TIMOB-24695 to you 👍

@garymathews garymathews closed this Jun 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants