Skip to content

Commit

Permalink
minor #52070 [HttpFoundation] Cache trusted values (Toflar)
Browse files Browse the repository at this point in the history
This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[HttpFoundation] Cache trusted values

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT

Found another potential performance bottleneck. I personally detected it in the context of the `RouterListener` that sets the current request context and resets it, after the request is finished:

1. `onKernelRequest`: https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/HttpKernel/EventListener/RouterListener.php#L100 (then calls https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/Routing/RequestContext.php#L70).
2. `onKernelFinishRequest`: https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/HttpKernel/EventListener/RouterListener.php#L93 (also calls `RequestContext::fromRequest()`).

The subsequent `RequestContext::fromRequest()` always calls  `$request->getPort()` and `$request->isSecure()` where the trusted values are re-evaluated over and over again although as long as you don't change the trusted headers, they will always be the same.

I noticed it in the context of the routing process because I use quite a few subrequests but I think the optimization can affect many parts of an application. It's true for all the request methods that rely on the trusted headers like `$request->getHost()`, `$request->getClientIps()`, `$request->getBaseUrl()` etc.

So I figured, it would be best to cache the internal `getTrustedValues()` so all of those methods are optimized at once. This method is a rather heavy one because [it splits headers](https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/HttpFoundation/Request.php#L2077) and [combines parts](https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/HttpFoundation/Request.php#L2081) again etc.

This only needs to be done once per trusted header set.

Commits
-------

73abd62 [HttpFoundation] Cache trusted values
  • Loading branch information
fabpot committed Oct 16, 2023
2 parents eadd41a + 73abd62 commit 48133f8
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 4 deletions.
21 changes: 17 additions & 4 deletions src/Symfony/Component/HttpFoundation/Request.php
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,8 @@ class Request
private bool $isForwardedValid = true;
private bool $isSafeContentPreferred;

private array $trustedValuesCache = [];

private static int $trustedHeaderSet = -1;

private const FORWARDED_PARAMS = [
Expand Down Expand Up @@ -1997,8 +1999,20 @@ public function isFromTrustedProxy(): bool
return self::$trustedProxies && IpUtils::checkIp($this->server->get('REMOTE_ADDR', ''), self::$trustedProxies);
}

/**
* This method is rather heavy because it splits and merges headers, and it's called by many other methods such as
* getPort(), isSecure(), getHost(), getClientIps(), getBaseUrl() etc. Thus, we try to cache the results for
* best performance.
*/
private function getTrustedValues(int $type, string $ip = null): array
{
$cacheKey = $type."\0".((self::$trustedHeaderSet & $type) ? $this->headers->get(self::TRUSTED_HEADERS[$type]) : '');
$cacheKey .= "\0".$ip."\0".$this->headers->get(self::TRUSTED_HEADERS[self::HEADER_FORWARDED]);

if (isset($this->trustedValuesCache[$cacheKey])) {
return $this->trustedValuesCache[$cacheKey];
}

$clientValues = [];
$forwardedValues = [];

Expand All @@ -2011,7 +2025,6 @@ private function getTrustedValues(int $type, string $ip = null): array
if ((self::$trustedHeaderSet & self::HEADER_FORWARDED) && (isset(self::FORWARDED_PARAMS[$type])) && $this->headers->has(self::TRUSTED_HEADERS[self::HEADER_FORWARDED])) {
$forwarded = $this->headers->get(self::TRUSTED_HEADERS[self::HEADER_FORWARDED]);
$parts = HeaderUtils::split($forwarded, ',;=');
$forwardedValues = [];
$param = self::FORWARDED_PARAMS[$type];
foreach ($parts as $subParts) {
if (null === $v = HeaderUtils::combine($subParts)[$param] ?? null) {
Expand All @@ -2033,15 +2046,15 @@ private function getTrustedValues(int $type, string $ip = null): array
}

if ($forwardedValues === $clientValues || !$clientValues) {
return $forwardedValues;
return $this->trustedValuesCache[$cacheKey] = $forwardedValues;
}

if (!$forwardedValues) {
return $clientValues;
return $this->trustedValuesCache[$cacheKey] = $clientValues;
}

if (!$this->isForwardedValid) {
return null !== $ip ? ['0.0.0.0', $ip] : [];
return $this->trustedValuesCache[$cacheKey] = null !== $ip ? ['0.0.0.0', $ip] : [];
}
$this->isForwardedValid = false;

Expand Down
17 changes: 17 additions & 0 deletions src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2550,6 +2550,23 @@ public function testTrustedProxiesRemoteAddr($serverRemoteAddr, $trustedProxies,
$this->assertSame($result, Request::getTrustedProxies());
}

public function testTrustedValuesCache()
{
$request = Request::create('http://example.com/');
$request->server->set('REMOTE_ADDR', '3.3.3.3');
$request->headers->set('X_FORWARDED_FOR', '1.1.1.1, 2.2.2.2');
$request->headers->set('X_FORWARDED_PROTO', 'https');

$this->assertFalse($request->isSecure());

Request::setTrustedProxies(['3.3.3.3', '2.2.2.2'], Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_HOST | Request::HEADER_X_FORWARDED_PORT | Request::HEADER_X_FORWARDED_PROTO);
$this->assertTrue($request->isSecure());

// Header is changed, cache must not be hit now
$request->headers->set('X_FORWARDED_PROTO', 'http');
$this->assertFalse($request->isSecure());
}

public static function trustedProxiesRemoteAddr()
{
return [
Expand Down

0 comments on commit 48133f8

Please sign in to comment.