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

[BrowserKit] fixed BC Break for HTTP_HOST header #26244

Merged
merged 1 commit into from Nov 26, 2018

Conversation

brizzz
Copy link
Contributor

@brizzz brizzz commented Feb 20, 2018

Q A
Branch? 2.7, 2.8, 3.x, 4.x
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes/no
Fixed tickets #22933
License MIT
Doc PR n/a

The situation well described in the original issue. I will add only:

I propose a compromise solution: add to HTTP_HOST header power to override URI when it is relative.

Proposed solution:

  • if the request URI is relative, then use the HTTP_HOST header passed to Client::request() to generate an absolute URI
  • if the request URI is absolute, then ignore the HTTP_HOST header (as it now works)
  • do the same with HTTPS server parameter

Profit:

Before:

$client->setServerParameters(['HTTP_HOST' => 'example.com']);
$client->request('GET', '/');
$this->assertEquals('http://example.com/', $client->getRequest()->getUri());

$client->request('GET', '/', [], [], ['HTTP_HOST' => 'example.com']);
$this->assertEquals('http://localhost/', $client->getRequest()->getUri());

Fixed (see last line):

$client->setServerParameters(['HTTP_HOST' => 'example.com']);
$client->request('GET', '/');
$this->assertEquals('http://example.com/', $client->getRequest()->getUri());

$client->request('GET', '/', [], [], ['HTTP_HOST' => 'example.com']);
$this->assertEquals('http://example.com/', $client->getRequest()->getUri());

@MathieuMindid
Copy link

Thanks for the fix.
As mentioned in #26257 the problems also exist in last version (3.x and 4.x)

@brizzz
Copy link
Contributor Author

brizzz commented Feb 22, 2018

Yes, this fix (if its correct) should be merged to all maintained versions. I have edited description to clarify.

@fabpot fabpot modified the milestones: 2.7, 2.8 May 28, 2018
@fabpot fabpot changed the base branch from 2.7 to 2.8 May 28, 2018 06:06
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I finally reviewed this PR, it makes sense to me!

@brizzz
Copy link
Contributor Author

brizzz commented Nov 25, 2018

this is great news!

@fabpot
Copy link
Member

fabpot commented Nov 26, 2018

Thank you @brizzz.

@fabpot fabpot merged commit 8c4a594 into symfony:2.8 Nov 26, 2018
fabpot added a commit that referenced this pull request Nov 26, 2018
This PR was merged into the 2.8 branch.

Discussion
----------

[BrowserKit] fixed BC Break for HTTP_HOST header

| Q             | A
| ------------- | ---
| Branch?       | 2.7, 2.8, 3.x, 4.x
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes/no
| Fixed tickets | #22933
| License       | MIT
| Doc PR        | n/a

The situation well described in the original issue. I will add only:

- #10549 - makes server parameters to take precedence over URI.
- #16265 - partially revererts  #10549. Makes server parameters do not affect URI. But this is only true for `Client::request()`. It is still possible to set host for URI by `Client::setServerParameters()` when URI is realative (see examples below).

I propose a compromise solution: add to HTTP_HOST header power to override URI when it is relative.

Proposed solution:
- if the request URI is relative, then use the HTTP_HOST header passed to Client::request() to generate an absolute URI
- if the request URI is absolute, then ignore the HTTP_HOST header (as it now works)
- do the same with HTTPS server parameter

Profit:
- fix BC Break
- the documentation will be correct
  - http://symfony.com/doc/2.8/routing/hostname_pattern.html#testing-your-controllers
  - https://symfony.com/doc/2.8/testing.html#testing-configuration

Before:

```
$client->setServerParameters(['HTTP_HOST' => 'example.com']);
$client->request('GET', '/');
$this->assertEquals('http://example.com/', $client->getRequest()->getUri());

$client->request('GET', '/', [], [], ['HTTP_HOST' => 'example.com']);
$this->assertEquals('http://localhost/', $client->getRequest()->getUri());
```

Fixed (see last line):

```
$client->setServerParameters(['HTTP_HOST' => 'example.com']);
$client->request('GET', '/');
$this->assertEquals('http://example.com/', $client->getRequest()->getUri());

$client->request('GET', '/', [], [], ['HTTP_HOST' => 'example.com']);
$this->assertEquals('http://example.com/', $client->getRequest()->getUri());
```

Commits
-------

8c4a594 [BrowserKit] fixed BC Break for HTTP_HOST header; implemented same behaviour for HTTPS server parameter
@Naktibalda
Copy link
Contributor

Naktibalda commented Nov 27, 2018

It broke my tests, so it is a BC-break.
I was posting Host header to localhost to see if some header functionality is working.

@c33s
Copy link

c33s commented Jul 28, 2019

@nicolas-grekas does this PR really make sense? this PR broke flexible testing of domain specific routes complete or is there a way to do this after the PR? see #32791

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.

None yet

7 participants