Skip to content

Commit

Permalink
Fix #19290: Fix Request::getHostInfo() doesn’t return the port if a…
Browse files Browse the repository at this point in the history 
… Host header is used
  • Loading branch information
@lesha724
lesha724 committed Apr 15, 2022
1 parent 4f1ffd2 commit 8046d3a
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 7 deletions.
1 change: 1 addition & 0 deletions framework/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Yii Framework 2 Change Log
- Enh #19270: Replace deprecated `scss` converter in `yii\web\AssetConverter::$commands` (WinterSilence)
- Enh #19254: Support specifying custom characters for `yii.validation.trim()` and replace deprecated `jQuery.trim()` (WinterSilence)
- Bug #19291: Reset errors and validators in `yii\base\Model::__clone()` (WinterSilence)
- Bug #19290: Fix `Request::getHostInfo()` doesn’t return the port if a Host header is used (lesha724)
- Enh #19295: Added alias `text/rtf` for mime-type `application/rtf` (lesha724)
- Enh #19308: Add `yii\web\UploadedFile::$fullPath` represents 'full_path' key added in PHP 8.1 (WinterSilence)
- Bug #19303: Fix serialization in `yii\caching\Dependency::generateReusableHash()` (WinterSilence)
Expand Down
21 changes: 14 additions & 7 deletions framework/web/Request.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -749,13 +749,20 @@ public function getHostInfo()
$this->_hostInfo = $http . '://' . trim(explode(',', $this->headers->get('X-Forwarded-Host'))[0]);
} elseif ($this->headers->has('X-Original-Host')) {
$this->_hostInfo = $http . '://' . trim(explode(',', $this->headers->get('X-Original-Host'))[0]);
} elseif ($this->headers->has('Host')) {
$this->_hostInfo = $http . '://' . $this->headers->get('Host');
} elseif (isset($_SERVER['SERVER_NAME'])) {
$this->_hostInfo = $http . '://' . $_SERVER['SERVER_NAME'];
$port = $secure ? $this->getSecurePort() : $this->getPort();
if (($port !== 80 && !$secure) || ($port !== 443 && $secure)) {
$this->_hostInfo .= ':' . $port;
} else {
if ($this->headers->has('Host')) {
$this->_hostInfo = $http . '://' . $this->headers->get('Host');
} elseif (filter_has_var(INPUT_SERVER, 'SERVER_NAME')) {
$this->_hostInfo = $http . '://' . filter_input(INPUT_SERVER, 'SERVER_NAME');
} elseif (isset($_SERVER['SERVER_NAME'])) {
$this->_hostInfo = $http . '://' . $_SERVER['SERVER_NAME'];
}

if ($this->_hostInfo !== null && !preg_match('/:\d+$/', $this->_hostInfo)) {
$port = $secure ? $this->getSecurePort() : $this->getPort();
if (($port !== 80 && !$secure) || ($port !== 443 && $secure)) {
$this->_hostInfo .= ':' . $port;
}
}
}
}
Expand Down
64 changes: 64 additions & 0 deletions tests/framework/web/RequestTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,48 @@ public function getHostInfoDataProvider()
'example1.com',
]
],
// HTTP header missing with port 80
[
[
'HTTP_HOST' => 'example1.com',
'SERVER_PORT' => 80,
],
[
'http://example1.com',
'example1.com',
]
],
// normal with nonstandart port 8080
[
[
'HTTP_HOST' => 'example1.com',
'SERVER_PORT' => 8080,
],
[
'http://example1.com:8080',
'example1.com',
]
],
[
[
'HTTP_HOST' => 'example1.com:8081',
'SERVER_PORT' => 8080,
],
[
'http://example1.com:8081',
'example1.com',
]
],
[
[
'HTTP_HOST' => 'example1.com:8080',
'SERVER_PORT' => 8080,
],
[
'http://example1.com:8080',
'example1.com',
]
],
// HTTP header missing
[
[
Expand All @@ -296,6 +338,28 @@ public function getHostInfoDataProvider()
'example2.com',
]
],
// HTTP header missing with nonstandart port 8080
[
[
'SERVER_NAME' => 'example1.com',
'SERVER_PORT' => 8080,
],
[
'http://example1.com:8080',
'example1.com',
]
],
// HTTP header missing with port 80
[
[
'SERVER_NAME' => 'example1.com',
'SERVER_PORT' => 80,
],
[
'http://example1.com',
'example1.com',
]
],
// forwarded from untrusted server
[
[
Expand Down

10 comments on commit 8046d3a

@Myakun
Copy link

@Myakun Myakun commented on 8046d3a Apr 25, 2022

Choose a reason for hiding this comment

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

We use the following scheme in the project

  1. The request is handled by the NGINX proxy
  2. NGINX proxy distributes requests to a cluster of nodes unreachable from the outside
  3. This commit caused host info in the application to be defined as http://example.com:8080 instead of https://example.com (detected during redirects)

@bizley
Copy link
Member

@bizley bizley commented on 8046d3a Apr 25, 2022

Choose a reason for hiding this comment

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

@lesha724 please take a look, maybe we should rollback this in total?

@lesha724
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Myakun, Could you give an example of what's in the request header?
@bizley, please look description of #19290, we can rollback, but this issue will not fixed

@Myakun
Copy link

@Myakun Myakun commented on 8046d3a Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@lesha724

json_encode(getallheaders()) return result example

{"Authorization":"Bearer apikey","Content-Type":"application\/json","User-Agent":"GuzzleHttp\/7","Content-Length":"44","Connection":"close","Host":"appdev.my.secret.domain","X-Forwarded-For":"my-secret-ip","X-Real-Ip":"same-secret-ip"}

@lesha724
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Myakun , and what is in $_SERVER? Maybe SERVER_PORT isn`t empty?

@Myakun
Copy link

@Myakun Myakun commented on 8046d3a Apr 25, 2022

Choose a reason for hiding this comment

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

@lesha724 you are right.

[SERVER_PORT] => 8080

@lesha724
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bizley, What you think? I think it is correct and we shouldn't rollback this changes.

@bizley
Copy link
Member

@bizley bizley commented on 8046d3a Apr 25, 2022

Choose a reason for hiding this comment

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

Looks like it, let's keep it. Thanks.

@lesha724
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Myakun, try use X-Forwarded-Host, this must help you

@Myakun
Copy link

@Myakun Myakun commented on 8046d3a Apr 25, 2022

Choose a reason for hiding this comment

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

@Myakun, try use X-Forwarded-Host, this must help you

Thx, @lesha724
My system administrator has already found a solution to the problem. The most important thing for me was to report a possible problem to yii team.

Please sign in to comment.