Skip to content

Conversation

@diegolmello
Copy link
Member

Proposed changes

In the current state of the app, we don't enforce secure connections, but that causes more harm than good, making users to believe it's an issue on the app when it's really a misconfiguration.

This PR starts to enforce secure connections on servers.

Issue(s)

How to test or reproduce

Screenshots

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Further comments

url = `${ url }.rocket.chat`;
}

// Comment to test local server on Android device
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use __DEV__ here, just to new devs don't worry about that.

@Fishbowler
Copy link
Contributor

Should this be a server concern rather than enforced by the app?
I feel like there are some valid use cases. If I'm testing changes to the app, or in any way throwing together any proof-of-concept that involves Rocket.Chat, should I need to add HTTPS? Including to my local Docker RC?

@grahamsmith
Copy link
Contributor

I like the idea behind the intent here. 👍

That said I feel this sets developers up for failure as it hides the behaviour in rewriting URLs secretly in multiple places. While working on and using RocketChat this sort of change has cost lots of time and effort as a general theme.

My suggestions are:

  • Add a strong warning about the use HTTP rather than HTTPS in console or even on the client device.
  • If HTTPS is important, then both iOS and Android have feature support around this, then we need to look beyond the React Native code. In the Android Network Security Config the setting for ClearTextTrafficPermitted is true. This could be moved to debug builds only as well as support for user certificates (which are allowed in production builds so I can bypass the HTTPS really easily by MITM if I knew what I was doing). There are similar concepts in iOS. Devs can also implement certificate pinning and alike when using HTTPS in production.
  • If the decision has been taken to force HTTPS then make it configurable and easily changed and obvious (I can help update the docs if needed).

@diegolmello
Copy link
Member Author

@Fishbowler @grahamsmith Thanks for the comments, guys.
The intent here is to keep developer experience the same: it should work on insecure connections on development.

HTTP doesn't work on iOS production already and it works on Android just because we have ClearTextTrafficPermitted enabled (which we're going to fix in this PR).

I've created this PR as draft because we need a deeper discussion and testing :)

@diegolmello diegolmello closed this Sep 1, 2020
@diegolmello diegolmello deleted the improvement.enforce-https branch September 1, 2020 21:11
@diegolmello
Copy link
Member Author

Closed in favour of #2357 and #2449

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.

5 participants