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

browsersync should default to HTTPS, fixes #19 #21

Merged
merged 5 commits into from
Nov 30, 2022
Merged

Conversation

tyler36
Copy link
Collaborator

@tyler36 tyler36 commented Sep 27, 2022

Fixes

Currently, browsersync is configured to run on the HTTP. This is not optimal.

This PR changes browsersync to use the HTTPS protocal instead.

Note: When starting browsersync, it reports the "External URL" has HTTP. I tried adding HTTPS: true to the config file but this resulted in a broken DDEV (502)

@tyler36 tyler36 requested a review from rfay September 27, 2022 01:23
@rfay
Copy link
Member

rfay commented Sep 27, 2022

Does this mean you figured it out, or that you still couldn't make it work on https?

@tyler36
Copy link
Collaborator Author

tyler36 commented Sep 27, 2022

This is working locally for me.

@tyler36
Copy link
Collaborator Author

tyler36 commented Sep 27, 2022

Browsersync is running on HTTPS protocal in the browser. The CLI reports to running on HTTP though. It would be nice to have that correct but I think it might be the reverse proxy stuff?

@rfay
Copy link
Member

rfay commented Sep 27, 2022

Looks like maybe some work remaining to do on tests?

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Well, it definitely works, but it will be too confusing to people.

Could we at least change ddev browsersync command to output the true information? Maybe it can say at the top, before launching browsersync "IT WILL BE HTTP NOT HTTP" :) and give a good URL. It might be possible to do something more sophisticated than that.

I think you already know that the reason it won't work by telling browsersync to use https is that then the ddev-router can't trust (or even decrypt) the https content.

So the problem is just that we're letting browsersync provide inadequate information, and we need to figure out a way around that.

I think it would be good to add more to the README about editing browser-sync.js as well. And how about adding "web" as one of the listed items/

@addison74
Copy link

I did the changes to expose the new ports in docker-compose.browsersync.yaml. Here are the test results for different scenarios.

1 - Opening https://browsersync.ddev.site:3000 in the browser. As you may see any changes in the index.html file are loaded in the browser window.

1 - HTTPS

2 - By opening http://browsersync.ddev.site:3000 in the browser (the BrowserSync External link) we get a 400 error.

2 - BS LINK

3 - By opening http://browsersync.ddev.site in the browser (the DDEV link using HTTP protocol) BrowserSync detects the changes in the file but it is not reloading the browser window content.

3 - HTTP LINK

In order to display https instead of http in the External link of the BrowserSync console we should add this option https: true in the browser-sync.js file, but if we do this you may see what happens bellow. BrowserSync detects the changes in the index.html file, but accessing the External link in the browser produces a 502 error.

4 - HTTPS ON

As a first conclusion after these scenarios. Changing the ports was beneficial, now the addon can also be used with HTTPS. The issue that needs to be solved is related to the External link displayed by BrowserSync in the console and which creates confusion but also errors in the situation in which it is used.

@tyler36
Copy link
Collaborator Author

tyler36 commented Sep 28, 2022

Not sure why the tests are failing :(

@rfay
Copy link
Member

rfay commented Oct 3, 2022

FYI it's worth thinking about allowing HTTPS communication between ddev-webserver and ddev-router, but that wouldn't fundamentally solve the problem here, because the bound port (443) by default might be changed.

Thanks for all the work on this.

@rfay
Copy link
Member

rfay commented Nov 3, 2022

This PR can be tested with ddev get https://github.com/tyler36/ddev-browsersync/tarball/tyler36/https-support/

@ursbraem
Copy link

ursbraem commented Nov 7, 2022

I got the PR and encountered no issues so far

@mandrasch
Copy link

This PR can be tested with ddev get https://github.com/tyler36/ddev-browsersync/tarball/tyler36/https-support/

Used it in this demo repo: https://github.com/mandrasch/ddev-laravelmix-browsersync, worked fine (quick test).

@tyler36 tyler36 requested a review from rfay November 11, 2022 00:37
@tyler36 tyler36 force-pushed the tyler36/https-support branch from 152e743 to 67b473d Compare November 11, 2022 00:47
@rfay
Copy link
Member

rfay commented Nov 11, 2022

Doggone those tests :)

@rfay
Copy link
Member

rfay commented Nov 15, 2022

Will definitely review and take a look at tests. Just under the gun to get the traefik stuff done for a major release. Thanks for your patience!

@rfay
Copy link
Member

rfay commented Nov 29, 2022

Thanks so much for the good work on this. Please take a look at

and if you're OK with it, merge it in here.

@rfay rfay changed the title browsersync should default to HTTPS browsersync should default to HTTPS, fixes #19 Nov 29, 2022
* Improve output of ddev browsersync to show https URL

* Update tests and github setup to make work with https

* Do mkcert -install so we can trust certs
@tyler36
Copy link
Collaborator Author

tyler36 commented Nov 30, 2022

Closing this in favor of #31

@tyler36 tyler36 closed this Nov 30, 2022
@tyler36 tyler36 reopened this Nov 30, 2022
@tyler36
Copy link
Collaborator Author

tyler36 commented Nov 30, 2022

Sorry. #31 was a PR to this PR. Re-opening.

@tyler36 tyler36 merged commit 3cd36d9 into main Nov 30, 2022
@tyler36 tyler36 deleted the tyler36/https-support branch November 30, 2022 00:28
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