Fix crash when $
sign is in external user ID
#1589
Merged
+99
−14
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
One Line Summary
Fix (1) a crash when a user has a dollar sign ($) in the external_user_id, and (2) only un-escape forward slashes in the
external_user_id
and not from others values such astimezone
which can have a value with a slash likeEngland/London
.Question for Reviewers on Better Regex
I couldn't find the right regex that would match only
$1$\/abc\/de$f\/
from a string like the following. The previous regex of(?<=\"external_user_id\":\").*\\\\/.*?(?=\",|\"\\})
would result in$1$\/abc\/de$f\/","app_id":"b4f7","timezone":"$Europe\/London
So I just matched the value of the external user ID, regardless of having any slashes in the value, and went from there.
Details
Motivation
A customer reported crashes when using a dollar sign ($) in an external user ID. Examples of the exception raised are:
Related PRs
JSONObject.toString()
will escape any forward slashes/
with\/
, and this will be sent in a request, and show up in the dashboard as\/
. A previous related PR was made to to "unescape" this for a customer request where it is used in external user ID: #1478.Scope and Background
Affects JSON serialization when sending the json string in a request, and in cases where there are forward slashes in an external user ID, those slashes are not escaped. After the changes in the previous PR mentioned above, we may have inadvertently been escaping
timezone
for many requests, but it doesn't seem to have been any ill effects.Extracted the logic from the rest client to a helper method called
toUnescapedEUIDString()
.If a user has a dollar sign ($) in the external user ID, and our code is trying to escape the forward slashes via a string replacement, this will cause a crash as
$
has a non-literal meaning when used in the replacement string. The solution is to callquoteReplacement
to escape any $ or \ signs. See https://docs.oracle.com/javase/8/docs/api/java/lang/String.html#replaceAll-java.lang.String-java.lang.String-Also fix the pattern matching for external user ID. Previously in a JSONObject like
{"app_id": "abc", "external_user_id": "user1", "timezone": "Europe/London"}
, the regex would matchuser1", "timezone": "Europe/London
, grabbing the forward slash in ANY values that come after the external_user_id. This can lead to the above crash even if the user does not have any forward slashes themselves (because it can come from timezone). See https://regexr.com/6lmm0. This is fixed to match the external user ID value only.There may now be more calls to String methods than necessary as the method now finds the value for external_user_id in the JSON string first, and regardless of whether it has forward slashes or not, we replace all instances of
\/
with/
(just no changes happen if it doesn't exist in the string). There is probably a better regex to use, but after trying for a while, I couldn't come up with it.Testing
Unit testing
Added unit tests for the method
toUnescapedEUIDString()
, with different JSON values. The testing was primarily done via unit testing and examining the values throughout the life of the method call.Manual testing
No pertinent manual testing.
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is