Skip to content

Match and fix any external id that has escaped forward slashes#1478

Merged
jkasten2 merged 1 commit intomainfrom
fix/unescapeEID
Nov 5, 2021
Merged

Match and fix any external id that has escaped forward slashes#1478
jkasten2 merged 1 commit intomainfrom
fix/unescapeEID

Conversation

@adamschlesinger
Copy link
Copy Markdown
Contributor

@adamschlesinger adamschlesinger commented Nov 3, 2021

Fix for https://app.asana.com/0/1193807006058450/1201230887422941/f which explicitly only checks outbound external user ids.

This will NOT prevent the client from saving the EID with escaped slashes. This is why there is no if (matched != currentEID) check. We cannot rely on the current EID on the client to be unescaped.


This change is Reviewable

@adamschlesinger adamschlesinger requested review from a team, emawby, jkasten2 and jmadler November 3, 2021 15:28
Copy link
Copy Markdown
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @adamschlesinger, @emawby, and @jmadler)


OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignalRestClient.java, line 164 at r1 (raw file):

            String strJsonBody = jsonBody.toString();

            Pattern eidPattern = Pattern.compile("(?<=\"external_user_id\":\").*\\\\/.*?(?=\",|\"\\})");

Looks like you have an extra \ on the or operator when looking for }. I believe the full regex should be

(?<=\"external_user_id\":\").*\\\\/.*?(?=\",|\"\})

Tested with the following to confirm:
https://regex101.com/r/yAlOnr/1/


OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignalRestClient.java, line 167 at r1 (raw file):

            Matcher eidMatcher = eidPattern.matcher(strJsonBody);

            if (eidMatcher.find()) {

We be nice to split this code into it's own descriptive method so we don't have to add the implementation details don't have to live in the startHTTPConnection method itself.

Copy link
Copy Markdown
Contributor Author

@adamschlesinger adamschlesinger left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @adamschlesinger, @emawby, @jkasten2, and @jmadler)


OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignalRestClient.java, line 164 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Looks like you have an extra \ on the or operator when looking for }. I believe the full regex should be

(?<=\"external_user_id\":\").*\\\\/.*?(?=\",|\"\})

Tested with the following to confirm:
https://regex101.com/r/yAlOnr/1/

You need to escape the escape for Java.

Copy link
Copy Markdown
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @adamschlesinger, @emawby, and @jmadler)


OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignalRestClient.java, line 164 at r1 (raw file):

Previously, adamschlesinger (Adam Schlesinger) wrote…

You need to escape the escape for Java.

ah you are right, tested it in Java and you need to have exactly\\} for it to work.

Copy link
Copy Markdown
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

1 comment left, recommended code clean up but not required.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @adamschlesinger, @emawby, and @jmadler)

@jkasten2 jkasten2 merged commit dbed822 into main Nov 5, 2021
@jkasten2 jkasten2 deleted the fix/unescapeEID branch November 5, 2021 22:52
@jkasten2 jkasten2 mentioned this pull request Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants