-
Notifications
You must be signed in to change notification settings - Fork 240
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
Add cleartext traffic check #122
base: main
Are you sure you want to change the base?
Conversation
scarlet-websocket-okhttp/src/main/java/com/tinder/scarlet/websocket/okhttp/OkHttpClientUtils.kt
Outdated
Show resolved
Hide resolved
scarlet-websocket-okhttp/src/main/java/com/tinder/scarlet/websocket/okhttp/OkHttpClientUtils.kt
Outdated
Show resolved
Hide resolved
scarlet-websocket-okhttp/src/main/java/com/tinder/scarlet/websocket/okhttp/OkHttpClientUtils.kt
Outdated
Show resolved
Hide resolved
thanks for the contribution @rogerthat94! Just a few nits |
Thank you for making this helpful library! I'm happy to give back with a small contribution. I hope this addresses the nits. |
It throws an exception when the URL starts with `ws:.
@rogerthat94 sorry it's been a while. if you can resolve conflicts, i'd be more than happy to accept this change. Thanks! |
@aaronweihe No problem! I have just resolved conflicts. |
It looks like the version of this cannot be specified separately OkHttp specifies this in a single string. See here: https://github.com/square/okhttp/blob/68241851c197a67cdcdd0e150e239f55d1e556f4/build.gradle#L28
ping @aaronweihe |
@aaronweihe Are you still interested in this change? I can rebase it on top of the latest version of the code if so. |
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.
looks good to me. Can you sign the CLA?
|
||
fun OkHttpClient.newWebSocketFactory(requestFactory: RequestFactory): WebSocket.Factory { | ||
return OkHttpWebSocket.Factory(OkHttpClientWebSocketConnectionEstablisher(this, requestFactory)) | ||
} | ||
|
||
fun OkHttpClient.newWebSocketFactory(url: String): WebSocket.Factory { | ||
if (url.startsWith("ws:", ignoreCase = true)) { | ||
try { | ||
if ((Build.VERSION.SDK_INT > 23 && |
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 like the Platform#isCleartextTrafficPermitted
abstraction. we could tackle it in the next PR
Thanks! I have signed the CLA. Is there anything else that you would like me to do? |
@aaronweihe Are you able to merge this? |
When someone tries to open a Websocket connection without using TLS (ie "ws:" instead of "wss:") on Android 9 and newer, the connection will fail and Scarlet will not provide any information about this to the developer. This confused me as well as the author of this post.
Okhttp has a similar check here.
I check for a ClassNotFoundException, so it should not cause any issues for non-Android apps that use Scarlet.