Stripped non-printing, non-ASCII chars from user agent #1819
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HTTP headers support Latin-1 only. See this okhttp PR: square/okhttp#891
Tests results for SalesforceSDKGenerated by 🚫 Danger |
wmathurin
reviewed
Nov 30, 2018
| SDK_VERSION, Build.VERSION.RELEASE, Build.MODEL, appName, getAppVersion(), | ||
| appTypeWithQualifier, uid, TextUtils.join(".", features)); | ||
|
|
||
| // Strip out all non-printing non-ASCII characters |
There was a problem hiding this comment.
Those characters probably come from the appname.
They might not be readable but if we strip them, it will change the app name in our logs - and impact our stats.
What problem are they causing?
There was a problem hiding this comment.
1. The MobileSDK is getting the app name
2. One of our customers is not using a regular quotation mark, but rather \u026F, which is `'`
3. Android is okay with this (which explains why the build worked on the pipeline before) but OkHTTP isn’t okay with it - it doesn’t like unencoded unicode
4. Trying to send a `User-agent` header with this value fails
so the fix is in the MobileSDK and it’s to encode the user agent properly, which requires ISO-8859-1 encoding (ASCII)
5. The customer can't login.
Root cause is here.
There was a problem hiding this comment.
@bhariharan Are you saying we should encode it better or just strip the characters?
There was a problem hiding this comment.
After talking to Bharath, I've taken a less aggressive approach and moved the logic to get the app's display name into an overrideable method. This allows me to fix this at the app level.
wmathurin
approved these changes
Nov 30, 2018
bhariharan
approved these changes
Dec 1, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
HTTP headers support Latin-1 only. See this okhttp PR