Skip to content

IBX-11181: Trusted Proxies is not set on Ibexa Cloud #699

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vidarl wants to merge 3 commits into
base: 4.6
Choose a base branch
from
Open

IBX-11181: Trusted Proxies is not set on Ibexa Cloud #699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb8e4b2
IBX-11181: Trusted Proxies is not set on Ibexa Cloud
vidarl Jan 14, 2026
d53633c
Fixed tests
vidarl Jan 19, 2026
8e6ec00
IBX-11181: PHPStan
barw4 Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions phpstan-baseline.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41122,7 +41122,7 @@ parameters:
-
message: '#^Call to method PHPUnit\\Framework\\Assert\:\:assertInstanceOf\(\) with ''Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\Location'' and Ibexa\\Contracts\\Core\\Repository\\Values\\Content\\Location will always evaluate to true\.$#'
identifier: method.alreadyNarrowedType
count: 10
count: 9
path: tests/integration/Core/Repository/LocationServiceTest.php

-
Expand Down Expand Up @@ -41152,7 +41152,7 @@ parameters:
-
message: '#^Call to static method PHPUnit\\Framework\\Assert\:\:assertInstanceOf\(\) with ''Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\Location'' and Ibexa\\Contracts\\Core\\Repository\\Values\\Content\\Location will always evaluate to true\.$#'
identifier: staticMethod.alreadyNarrowedType
count: 2
count: 1
path: tests/integration/Core/Repository/LocationServiceTest.php

-
Expand Down
33 changes: 2 additions & 31 deletions src/bundle/Core/EventSubscriber/TrustedHeaderClientIpEventSubscriber.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,6 @@

final class TrustedHeaderClientIpEventSubscriber implements EventSubscriberInterface

@vidarl vidarl Jan 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
final class TrustedHeaderClientIpEventSubscriber implements EventSubscriberInterface
final class IbexaCloudTrustedProxiesEventSubscriber implements EventSubscriberInterface

Or this is considered a BC ? Class is final though, so maybe not a problem?

@barw4 barw4 Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't care

@ViniTou ViniTou Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isnt IbexaCloud too generic tho? this whole class is about Upsun & Fastly, right?

@vidarl vidarl Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ViniTou IbexaCloud is on Upsun, and only there. We could call it FastlyOnIbexaCloudTrustedProxiesEventSubscriber or something if you like? Personally, I prefer IbexaCloudTrustedProxiesEventSubscriber though

@vidarl vidarl Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ViniTou : You are good and thread can be closed, or you still want changes?

{
private const PLATFORM_SH_TRUSTED_HEADER_CLIENT_IP = 'X-Client-IP';

private ?string $trustedHeaderName;

public function __construct(
?string $trustedHeaderName
) {
$this->trustedHeaderName = $trustedHeaderName;
}

public static function getSubscribedEvents(): array
{
return [
Expand All @@ -36,28 +26,9 @@ public function onKernelRequest(RequestEvent $event): void
{
$request = $event->getRequest();

$trustedProxies = Request::getTrustedProxies();
$trustedHeaderSet = Request::getTrustedHeaderSet();

$trustedHeaderName = $this->trustedHeaderName;
if (null === $trustedHeaderName && $this->isPlatformShProxy($request)) {
$trustedHeaderName = self::PLATFORM_SH_TRUSTED_HEADER_CLIENT_IP;
if ($this->isPlatformShProxy($request) && $request->headers->get('Client-Cdn') === 'fastly') {
Comment thread
vidarl marked this conversation as resolved.
Request::setTrustedProxies(['REMOTE_ADDR'], Request::getTrustedHeaderSet());

@konradoboza konradoboza Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't that fallback to Symfony's default if framework.trusted_proxies was never set? Shouldn't we be more specific here by passing: HEADER_X_FORWARDED_FOR | HEADER_X_FORWARDED_HOST | HEADER_X_FORWARDED_PROTO | HEADER_X_FORWARDED_PORT combo?

@barw4 barw4 Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Via Symfony's code:

$trustedHeaderSet = $trustedHeaders ?? (Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_PORT | Request::HEADER_X_FORWARDED_PROTO);

Host and Prefix are omitted in this case but the question is - do we really need them? It's not like previous Subscriber added Host so imo this change is proper. Subscriber didn't pass Host and we aren't passing Host now.

Host has the following note in Symfony docs:

Enabling the Request::HEADER_X_FORWARDED_HOST option exposes the application to [HTTP Host header attacks](https://www.skeletonscribe.net/2013/05/practical-http-host-header-attacks.html). Make sure the proxy really sends an x-forwarded-host header.

@barw4 barw4 Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On the other hand, HOST was set manually in https://github.com/ibexa/fastly-forwarded-host/blob/main/src/bundle/EventSubscriber/KernelRequestSubscriber.php and if we are deprecating this bundle I would say we also need to pass X_FORWARDED_HOST explicitly? I believe we can pass it safely because proxy sends it safely and cannot be spoofed due to Client-Cdn existance that guarantess request is actually coming from proxy?

@barw4 barw4 Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Additionally, if I understand correctly fastly-forwarded-host bundle was created some time ago because of the need to handle a lot of Upsun domains and overcome some routing (??) limitations. Perhaps the FORWARDED-HOST should be enabled explicitly in this subscriber via a dedicated config option if such a limitation occurs?

@vidarl vidarl Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The previous implementation enabled X_FORWARDED_FOR as a trusted header. But it did not change $trustedProxies ( the actually IPs that where trusted ) and there was no way to configure that requests coming from Fastly should be trusted (because of the spoofing of the IP that takes place on Upsun).
So AFAIK, this subscriber didn't really change the behavior in any way, ie: adding X_FORWARDED_FOR as trusted header had no effect what-so-ever.

With my change, the requests is now trusted if it comes via Fastly. My idea was this simply that this enables site integrators to enable the trusted headers they need - it makes that part possible. Then it is up to site integrators to decide what headers to enable.

We could of course make things more convenient and add some default trusted headers. But this should then be treated as BC and be documented IMO.

}

if (null === $trustedHeaderName) {
return;
}

$trustedClientIp = $request->headers->get($trustedHeaderName);

if (null !== $trustedClientIp) {
if ($trustedHeaderSet !== -1) {
$trustedHeaderSet |= Request::HEADER_X_FORWARDED_FOR;
}
$request->headers->set('X_FORWARDED_FOR', $trustedClientIp);
}

Request::setTrustedProxies($trustedProxies, $trustedHeaderSet);
}

private function isPlatformShProxy(Request $request): bool
Expand Down
2 changes: 0 additions & 2 deletions src/bundle/Core/Resources/config/services.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,8 +312,6 @@ services:
$cache: '@ibexa.cache_pool'

Ibexa\Bundle\Core\EventSubscriber\TrustedHeaderClientIpEventSubscriber:
arguments:
$trustedHeaderName: '%ibexa.trusted_header_client_ip_name%'
tags:
- {name: kernel.event_subscriber}

Expand Down
111 changes: 19 additions & 92 deletions tests/bundle/Core/EventSubscriber/TrustedHeaderClientIpEventSubscriberTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,10 @@

final class TrustedHeaderClientIpEventSubscriberTest extends TestCase
{
private const PLATFORM_SH_TRUSTED_HEADER_CLIENT_IP = 'X-Client-IP';

private ?string $originalRemoteAddr;

private const PROXY_IP = '127.100.100.1';

private const REAL_CLIENT_IP = '98.76.123.234';

private const CUSTOM_CLIENT_IP = '234.123.78.98';

/**
* @param array<mixed> $data
*/
Expand All @@ -53,114 +47,47 @@ protected function tearDown(): void
public function getTrustedHeaderEventSubscriberTestData(): array
{
return [
'default behaviour' => [
self::REAL_CLIENT_IP,
self::REAL_CLIENT_IP,
],
'use custom header name with valid value' => [
self::REAL_CLIENT_IP,
self::PROXY_IP,
'X-Custom-Header',
['X-Custom-Header' => self::REAL_CLIENT_IP],
'request from random client received on non-Upsun platform' => [
false,
[],
[],
],
'use custom header name without valid value' => [
self::PROXY_IP,
self::PROXY_IP,
'X-Custom-Header',
'request from random client, forging Client-Cdn received on non-Upsun platform' => [
false,
['Client-Cdn' => 'fastly'],
[],
],
'use custom header value without custom header name' => [
self::PROXY_IP,
self::PROXY_IP,
null,
['X-Custom-Header' => self::REAL_CLIENT_IP],
],
'default platform.sh behaviour' => [
self::REAL_CLIENT_IP,
self::PROXY_IP,
null,
['X-Client-IP' => self::REAL_CLIENT_IP],
['PLATFORM_RELATIONSHIPS' => true],
],
'use custom header name without valid value on platform.sh' => [
self::PROXY_IP,
self::PROXY_IP,
'X-Custom-Header',
[self::PLATFORM_SH_TRUSTED_HEADER_CLIENT_IP => self::REAL_CLIENT_IP],
'request from random client received on Upsun platform' => [
false,
[],
['PLATFORM_RELATIONSHIPS' => true],
],
'use custom header with valid value on platform.sh' => [
self::CUSTOM_CLIENT_IP,
self::PROXY_IP,
'X-Custom-Header',
[
self::PLATFORM_SH_TRUSTED_HEADER_CLIENT_IP => self::REAL_CLIENT_IP,
'X-Custom-Header' => self::CUSTOM_CLIENT_IP,
],
['PLATFORM_RELATIONSHIPS' => true],
],
'use valid value without custom header name on platform.sh' => [
self::REAL_CLIENT_IP,
self::PROXY_IP,
null,
[
self::PLATFORM_SH_TRUSTED_HEADER_CLIENT_IP => self::REAL_CLIENT_IP,
'X-Custom-Header' => self::CUSTOM_CLIENT_IP,
],
'request via Fastly received on Upsun platform' => [
true,
['Client-Cdn' => 'fastly'],
['PLATFORM_RELATIONSHIPS' => true],
],
];
}

public function testTrustedHeaderEventSubscriberWithoutTrustedProxy(): void
{
$_SERVER['REMOTE_ADDR'] = self::PROXY_IP;

$eventDispatcher = new EventDispatcher();
$eventDispatcher->addSubscriber(
new TrustedHeaderClientIpEventSubscriber('X-Custom-Header')
);

$request = Request::create('/', 'GET', [], [], [], array_merge(
$_SERVER,
['PLATFORM_RELATIONSHIPS' => true],
));
$request->headers->add([
'X-Custom-Header' => self::REAL_CLIENT_IP,
]);

$event = $eventDispatcher->dispatch(new RequestEvent(
self::createMock(KernelInterface::class),
$request,
HttpKernelInterface::MAIN_REQUEST
), KernelEvents::REQUEST);

/** @var \Symfony\Component\HttpFoundation\Request $request */
$request = $event->getRequest();

self::assertEquals(self::PROXY_IP, $request->getClientIp());
}

/**
* @dataProvider getTrustedHeaderEventSubscriberTestData
*/
public function testTrustedHeaderEventSubscriberWithTrustedProxy(
string $expectedIp,
string $remoteAddrIp,
?string $trustedHeaderName = null,
bool $isFromTrustedProxy,
array $headers = [],
array $server = []
): void {
$_SERVER['REMOTE_ADDR'] = $remoteAddrIp;
Request::setTrustedProxies(['REMOTE_ADDR'], Request::getTrustedHeaderSet());
$_SERVER['REMOTE_ADDR'] = self::REAL_CLIENT_IP;

$eventDispatcher = new EventDispatcher();
$eventDispatcher->addSubscriber(
new TrustedHeaderClientIpEventSubscriber($trustedHeaderName)
new TrustedHeaderClientIpEventSubscriber()
);

$request = Request::create('/', 'GET', [], [], [], array_merge(
$server,
['REMOTE_ADDR' => $remoteAddrIp],
['REMOTE_ADDR' => self::REAL_CLIENT_IP],
));
$request->headers->add($headers);

Expand All @@ -173,6 +100,6 @@ public function testTrustedHeaderEventSubscriberWithTrustedProxy(
/** @var \Symfony\Component\HttpFoundation\Request $request */
$request = $event->getRequest();

self::assertEquals($expectedIp, $request->getClientIp());
self::assertEquals($isFromTrustedProxy, $request->isFromTrustedProxy());
}
}