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: URL auto-encoding improvements #9178
Conversation
- No longer encodes legal chars '?', '/', '@', and ':' in query params. - No longer encodes legal chars '$', '&', '+', ',', ';', and '=' in URL's "username:password" component. - No longer encodes legal chars '$', '&', '+', ',', ';', '=', ':', '@', '/', and '?' in URL #fragment. - Spaces in the URL's query paramaters are now encoded as '+' instead of "%20". - Given URL's %-encoded characters are now always preserved. * Used to decode "%26" and "%3D" to '&' and '=' in query params, breaking the query. * Used to decode "%2F" to '/' in URL path, which can break the path.
- No longer encodes legal chars '!', '$', ''', '(', ')', '+', ',', and ';' in query params. - Worked-around Android bug caused by TIMOB-24695 fix where a URL without a path would wrongly copy query param characters proceeding an unencoded '/' into the URL's path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CR: PASS
Looks great!
queryString = queryString.replace("%27", "'"); | ||
queryString = queryString.replace("%28", "("); | ||
queryString = queryString.replace("%29", ")"); | ||
queryString = queryString.replace("%2B", "+"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, possibility to use regex here:
// this will cover %2B and %2b
queryString = queryString.replaceAll("(?i)%2b", "+");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea. I'll go do it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to punt on this. The String.replaceAll() regex always does a string allocation, even if no substring replacements are done (I just verified this today). Versus String.replace() will return a reference to itself if no substrings were found, avoiding an unnecessary allocation. Hmm...
FR Passed. Ran the code above & the auto encoding for the URL's works as expected. Studio Ver: 4.9.0.201705302345 |
JIRA: https://jira.appcelerator.org/browse/TIMOB-24695
Changes:
!
,$
,'
,(
,)
,+
,,
,/
,:
,;
,?
, and@
in query params.$
,&
,+
,,
,;
, and=
in URL's "username:password" component.$
,&
,+
,,
,;
,=
,:
,@
,/
, and?
in URL #fragment.+
instead of%20
.%2F
to/
in URL path, which can break the path.Test Procedure:
Received HTTP Request: POST /Space%20Test?test=Space+Test HTTP/1.1
Received HTTP Request: POST /Slash%2FTest HTTP/1.1
Received HTTP Request: POST /?test=%22(!$'+,/:;?@)%22 HTTP/1.1
Received HTTP Request: POST /?test=abc/xyz HTTP/1.1
Test Code: