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

fix for share of secured sites #630

Merged
merged 4 commits into from Sep 20, 2018

Conversation

Projects
None yet
8 participants
@sdbruder
Contributor

sdbruder commented Sep 14, 2018

This fixes the valet share issue for secured sites/apps (the already closed issue #148), its an implementation of this idea:

When an app/site is secure, instead of creating 2 server blocks, lets create 3. the current on port 80 redirecting to 443, the secure one on port 443 and a new one on port, say, 88 (any other port not 80 or 443 is ok) doing an usual non-SSL server block.

So the ngrok fix is now:
if site is secure redirect 88 else redirect 80

This fixes it and gets around the pro ngrok issue as we will be serving an usual http:// site, only not on the std issue port.

sdbruder added some commits Sep 14, 2018

@drbyte

This comment has been minimized.

Contributor

drbyte commented Sep 14, 2018

This works okay if I only hit the http URL given by ngrok.
However, if I attempt to use the https protocol on the ngrok URL then my browser points out "attempt to serve unsecure content" for all the CSS/JS assets.

It also echoes out the following to the console (visible when ending the ngrok connection):
listen 443 ssl http2;

I haven't dug any deeper than just replicating your code changes and attempting to visit the URL, so don't have any "solution" to offer at this point.

On the plus side it does make webhook testing better since it avoids a 301 redirect which can be a real nuisance.

@sdbruder

This comment has been minimized.

Contributor

sdbruder commented Sep 14, 2018

that output is a grep test that needs to be silenced (needs a —quiet parameter).

I think the ‘attempt to server insecure content’ is unsolvable because that is limited on how the page content is referencing assets, which needs to be done in a particular way for https:// sites to avoid exactly that issue.

@sdbruder

This comment has been minimized.

Contributor

sdbruder commented Sep 14, 2018

There, no grep output.

@drbyte

This comment has been minimized.

Contributor

drbyte commented Sep 18, 2018

Nice.

Works slick.

Notwithstanding that one must manually account for "mixed content" within their app. But, as you say, this isn't really the fault of Valet or Ngrok ... it's the nature of different domain names.

If this is not merged, then ngrok serves via a URL-redirected tunnel. Which on the surface looks fine.
However, webhooks typically expect a 200 response, and the URL-redirection throws a 301 in first, which is rejected by the webhooks I've tested. Merging this PR resolves that.

@drbyte

This comment has been minimized.

Contributor

drbyte commented Sep 18, 2018

Just noting here that #534 attempts to do something similar, albeit in a slightly different way.

@mattstauffer

This comment has been minimized.

Contributor

mattstauffer commented Sep 19, 2018

@drbyte I haven't followed all the conversations around ngrok very well. Could you give me an opinion on which you prefer, or whether you have an alternate idea?

@drbyte

This comment has been minimized.

Contributor

drbyte commented Sep 19, 2018

@mattstauffer at this stage I prefer this one.
Credits and thanks to earlier submissions; however, this latest one is the one I've tested and used myself.

@sjelfull

This comment has been minimized.

sjelfull commented Sep 19, 2018

This is a good fix for the ngrok issue since it just works.

However, #534 allows you to enable access on standard port 80 from any context which doesn't accept the self-signed certificate without remembering the port.

At this point, I'm favorable to this PR for fixing the share command - but it would be good if there is a way out of forced SSL, should someone have a need for that for any reason.

@mattstauffer

This comment has been minimized.

Contributor

mattstauffer commented Sep 19, 2018

Current plan is to tag 2.1.0 and then merge this in 2.1.1 so it's separated a bit.

@drbyte

This comment has been minimized.

Contributor

drbyte commented Sep 19, 2018

@sjelfull wrote:

it would be good if there is a way out of forced SSL, should someone have a need for that for any reason.

I'm favorable to that too. Perhaps after #630 is merged you can update #534 merge conflicts and we can review it then.

@drbyte drbyte referenced this pull request Sep 19, 2018

Closed

Remove port from url #601

@mattstauffer

This comment has been minimized.

Contributor

mattstauffer commented Sep 19, 2018

I'd like to merge this in to master, test it a bit, then tag it asv2.1.1. Are we waiting on anything for that?

@drbyte

This comment has been minimized.

Contributor

drbyte commented Sep 19, 2018

I'd say it's safe to merge and test. I've been using it with success.

@mattstauffer mattstauffer merged commit 10c5239 into laravel:master Sep 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mattstauffer

This comment has been minimized.

Contributor

mattstauffer commented Sep 20, 2018

Alright, let's do a bit of testing on dev-master and see if we want to get this into 2.1.2

@drbyte

This comment has been minimized.

Contributor

drbyte commented Sep 20, 2018

Alright.

@mattstauffer curious your thoughts on whether to also update the ngrok binary since the one in this distro is outdated.

@mattstauffer

This comment has been minimized.

Contributor

mattstauffer commented Sep 20, 2018

@drbyte ¯\(°_o)/¯ we should probably keep them all up to date--do you have much knowledge of what's changed?

@drbyte

This comment has been minimized.

Contributor

drbyte commented Sep 20, 2018

We're currently distributing v2.1.18; Latest is 2.2.8.
I've used both the old and the new without issue.

I'm not seeing anywhere that describes small iterative updates; however for major changes they list one at https://ngrok.com/docs#compat-2.2

Given that in the ngrok console they encourage you to just press CTRL+U to self-update when a new version is available, I think it makes sense to at least bump to the next mid-point-release. And keep our distro up to date 2-3 times per year probably.

@blaues0cke

This comment has been minimized.

blaues0cke commented Oct 3, 2018

Thank you very much. Can confirm that valet secure followed by valet share just works like a charm!

@simensen

This comment has been minimized.

Contributor

simensen commented Oct 24, 2018

Looking forward to this. :)

@simensen

This comment has been minimized.

Contributor

simensen commented Oct 24, 2018

(can confirm that dev-master works as expected for me...)

superdav42 added a commit to superdav42/valet-linux that referenced this pull request Nov 1, 2018

@simonhamp

This comment has been minimized.

simonhamp commented Nov 10, 2018

This appears to have broken nginx booting if you have secured sites, as Kerberos/Screen Sharing uses port 88.

Way around at the moment is to disable Kerberos and/or Screen Sharing (which may not be possible for some).

Can we try a different port that isn't reserved?

@Danny-G-Smith

This comment has been minimized.

Danny-G-Smith commented Nov 10, 2018

I am running into this issue on a clean install with port 88. Is there a previous version I can install in the mean time to get around this and get a working system?
thanks in advance.

@drbyte

This comment has been minimized.

Contributor

drbyte commented Nov 10, 2018

I think you could revert to 2.1.0 to bypass this change, but there might be other things you want from newer releases.

You could also just temporarily directly edit the 2 affected files:
~/.composer/vendor/laravel/valet/cli/stubs/secure.valet.conf
~/.composer/vendor/laravel/valet/valet
Both can be edited with a regular code editor, and all you need to change is the 88.

Using port 87 seems to test out okay.

Whether you edit the files directly or downgrade with composer, you'll need to rebuild your site configs:
A fast way to rebuild all site configs is to run valet tld test (where "test" is your configured tld)

@simonhamp

This comment has been minimized.

simonhamp commented Nov 10, 2018

That should work too, but it will likely cause problems when you come to update later... you shouldn't really edit vendor files.

@Danny-G-Smith

This comment has been minimized.

Danny-G-Smith commented Nov 10, 2018

So how would I install 2.1.0? I tried . valet install: "2.1.0" and valet install: 2.1.0
thanks

@drbyte

This comment has been minimized.

Contributor

drbyte commented Nov 11, 2018

@simonhamp wrote:

That should work too, but it will likely cause problems when you come to update later... you shouldn't really edit vendor files.

Of course. It will definitely be wiped out during a future update. But the request was "for in the meantime".

@drbyte

This comment has been minimized.

Contributor

drbyte commented Nov 11, 2018

@Danny-G-Smith wrote:

So how would I install 2.1.0? I tried . valet install: "2.1.0" and valet install: 2.1.0

composer global require laravel/valet:2.1.0

@simensen

This comment has been minimized.

Contributor

simensen commented Nov 14, 2018

AWESOME THIS WORKS AND I AM SO HAPPY.

(Yay)

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