HTTP_HOST is being overwritten by HTTP_X_ORIGINAL_HOST

[vicdelfant]

Just wondering, what's the reasoning behind server.php:L102-L014?

if (isset($_SERVER['HTTP_X_ORIGINAL_HOST'])) {
    $_SERVER['HTTP_HOST'] = $_SERVER['HTTP_X_ORIGINAL_HOST'];
}

I use Valet with a paid Ngrok plan, with a (generic) reserved hostname in de Ngrok config. Unfortunately, this line sets every incoming request to the reserved hostname, instead of the domain name configured using Ngrok's host_header setting.

I understand this might have something to do with Valet's own share capability, but this basically prevents anyone from using Ngrok standalone (and in accordance with its documentation) with Valet.

This could be circumvented by re-rewriting the HTTP_HOST in a custom server, but unfortunately the original hostname is no longer available in $_SERVER.

[adamwathan]

I can't remember the exact details, but the original commit is here:

067e02a

If I'm not mistaken, it was an issue with how URLs would get generated when using Laravel helpers like url() when sharing a site with ngrok. They would end up pointing at the wrong place if we didn't override that header.

If I had a site setup locally as my-site.dev, and then I shared that and got some ngrok URL, as soon as someone tried to click a link in the ngrok version of the site, it would try to take them to my-site.dev which of course wouldn't work for them.

[vicdelfant]

Ah! That makes sense indeed. In my case I'm having the exact opposite issue: I'm using Symfony with a host requirement in the routing configuration, which means that (with the rewritten hostname) the incoming HTTP_HOST never matches the my-site.dev hostname it expects.

Maybe we could store the original HTTP_HOST in a separate $_SERVER variable? This would allow folks like me to re-rewrite the hostname in a custom server without breaking backward-compatibility.

Something like:

if (isset($_SERVER['HTTP_X_ORIGINAL_HOST'])) {
    $_SERVER['HTTP_X_INBOUND_HOST'] = $_SERVER['HTTP_HOST']; // Some non-standard header so it won't conflict
    $_SERVER['HTTP_HOST'] = $_SERVER['HTTP_X_ORIGINAL_HOST'];
}

It might make more sense to use the X-Forwarded-Host HTTP header, and have Laravel (or whatever) use that to determine the correct hostname for URL generation. That'd require you to configure ngrok as a trusted proxy in Laravel/Symfony, though.

if (isset($_SERVER['HTTP_X_ORIGINAL_HOST']) && !isset($_SERVER['HTTP_X_FORWARDED_HOST'])) {
    $_SERVER['HTTP_X_FORWARDED_HOST'] = $_SERVER['HTTP_X_ORIGINAL_HOST'];
}

[vicdelfant]

I have prepared two branches, one for each possible solution:

• Add an X-Inbound-Host header and rewrite it for Symfony: feature-symfony-keep-host.
  • Pros: does not require any changes to existing applications, they simply won't notice that the application is being accessed through a proxy.
  • Cons: requires a case-by-case change to every driver.
• Use the X-Forwared-Host: feature/http-forwarded-host.
  • Pros: standards-compliant, behaves like any application sitting behind a proxy so there usually is a built-in solution (or configuration option) available.
  • Cons: breaks existing applications using Valet that rely on the "incorrectly" rewritten $_SERVER['HTTP_HOST'].

What do you think?

[drbyte]

I'd suggest a combination of both:

1. The X-Forwarded-Host approach you've proposed feels right. As you said, standards compliant, and for apps that already consider it, just works.

2. Also set the $_SERVER['HTTP_X_INBOUND_HOST'] var so that if someone's app doesn't consider the X-Fowarded-Host header, then they can simply update their own local app-specific valet driver to override HTTP_HOST (using your proposed symfony driver as an example

[vicdelfant]

Okay! I'll combine the two and submit a PR for further review.

[drbyte]

Have you had a chance to test this change on both free and paid ngrok accounts?

[vicdelfant]

Yes, but testing the changes from the PR actually got me thinking: with this PR we are trying to revert to standards-compliant behavior on a case-by-case basis. IMHO it'd make more sense to have the exception (the Laravel driver, in this case) require a change, not the other way around.

I went ahead and pushed 2d0c44d to the PR. As you can see, the changes to the Symfony driver have been reverted and the Laravel driver now contains the $_SERVER['HTTP_HOST'] = $_SERVER['HTTP_X_FORWARDED_HOST']. This is basically a shortcut for Laravel so one won't have to configure the trusted proxies, and also makes sure we won't be breaking existing applications.

Here are the test results with a paid ngrok account, tested with both a random and reserved hostname:

Unmodified driver (= standard behavior)

HTTP_HOST: test.dev
HTTP_X_ORIGINAL_HOST: abc123.eu.ngrok.io
HTTP_X_FORWARDED_HOST: abc123.eu.ngrok.io

Modified driver (= only for LaravelValetDriver)

HTTP_HOST: abc123.eu.ngrok.io
HTTP_X_ORIGINAL_HOST: abc123.eu.ngrok.io
HTTP_X_FORWARDED_HOST: abc123.eu.ngrok.io

What do you think?

[drbyte]

It feels simpler. I like that. It's definitely better for it to "just work" hands-free, no special coding required.

[vicdelfant]

Nice. Yeah, I like its simplicity too. Makes it easier to understand what's going on.

[adamwathan]

Do we know for sure that Laravel is the only bundled driver that uses that server variable for URL generation and stuff?

[vicdelfant]

Not quite sure, so there's a chance of this update breaking other applications unfortunately. Laravel's behavior actually isn't special or anything, but rewriting the HTTP_HOST in server.php introduced a more or less Laravel-specific change in the Valet core that's now affecting other frameworks.

Something like the Laravel Trusted Proxies library would probably have done a better job for a Laravel application, but of course requires some manual labor (loading and configuring it to treat ngrok as a trusted proxy).

It basically comes down to possibly breaking backwards-compatibility but becoming standards-compliant, or leaving the non-standard behavior in place and requiring changes to drivers for applications that expect standard behavior.

[adamwathan]

I'll merge it for now and we can deal with any issues it introduces with other drivers if they pop up 👍

[drbyte]

👍