Skip to content
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

Don't use React Native's version of okhttp3 to fix self signed SSL #9040

Merged
merged 1 commit into from
Jan 22, 2019

Conversation

jtreanor
Copy link
Contributor

Fixes #9032

Rect native uses okhttp version 3.11.0 while FluxC uses 3.9.0. Normally this would not be a major issue but there is a known problem with later versions of okhttp breaking self signed SSL (wordpress-mobile/WordPress-FluxC-Android#919).

We don't fully understand the scope of the issue but I think the safest thing to do is to use the version that we know works as expected (I have tested this). Gutenberg/React Native also seems to be working fine with this version (both binary and source versions).

To test:

Follow these steps on release/11.6 and on this branch:

  1. Open the app
  2. Tap "Login" and "Login by entering site address"
  3. Enter a self signed SSL site address
  4. Click next

On release/11.6, it will give an error saying the site does not exist. On this branch it will allow you to login as expected.

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@hypest
Copy link
Contributor

hypest commented Jan 22, 2019

On release/11.6, it will give an error saying the site does not exist.

👋 @jtreanor , I tried with the selfhosted site from http://do.wpmt.co/ (https://do2.wpmt.co/sslselfsigned/) and what I see is the "Invalid SSL certificate" popup, which asks about accepting the certificate or not. Any other step or site I can test that gives out a "site does not exist" error instead? Thanks!

@aforcier
Copy link
Contributor

@hypest you have to accept the certificate in any case - the error will occur after you've accepted the certificate.

@hypest
Copy link
Contributor

hypest commented Jan 22, 2019

Sorry, should have mentioned it before @aforcier. I actually had accepted the cert and moved on with logging in in my test. Everything seemed to work as normal.

@hypest
Copy link
Contributor

hypest commented Jan 22, 2019

Maybe I need to be on a particular build flavor? I tried wasabiDebug. I'll try vanillaRelease.

EDIT: can't repro on vanillaRelease either. Using commit hash: c5d268f.

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

Haven't been able to reproduce the originating issue but, this change makes sense to me so, 👍.

@aforcier
Copy link
Contributor

Confirmed fixed with the same self-signed SSL site I know you used to test @jtreanor - shared it with @hypest and he was able to confirm too.

:shipit: !

@aforcier aforcier merged commit 087e98f into release/11.6 Jan 22, 2019
@aforcier aforcier deleted the okhttp-conflict branch January 22, 2019 13:43
@jtreanor
Copy link
Contributor Author

jtreanor commented Jan 22, 2019

Thanks for testing @hypest and @aforcier ! As @aforcier mentioned the site I was testing with was different to the one you mentioned. It looks like https://do2.wpmt.co/sslselfsigned/ is working for me as well, so something about the other cert breaks with the later okhttp version.

This is a promising sign that the issue is not as widespread as I thought and will help in the investigation. I agree that we should still make this change until we are sure of the implications of upgrading okhttp in FluxC.

@aforcier
Copy link
Contributor

Just for future reference, I corrected a mistake in the config for https://do2.wpmt.co/sslselfsigned/ that allowed our self-hosted discovery process to connected to it via unencrypted HTTP and produce a false positive. It should now fail without this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants