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

[10.x] Update TrustProxies to rely on $headers if properly set #47844

Merged
merged 2 commits into from Jul 26, 2023

Conversation

inxilpro
Copy link
Contributor

Right now the match statement in TrustProxies::getTrustedHeaderNames only handles single headers. This means that if you set TrustProxies::$headers to something like Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_PORT it will ignore that and instead fall back on the default (which is to trust all headers).

There is a test for this, but the test was using the default values for TrustProxies::$headers, so it was passing because of the fallback, not because the it was working as expected.

The solution is to only rely on the match() statement if TrustProxies::$headers is not an integer. That way if someone has configured it correctly using bitmasks, it will respect the configuration, and only use string matching if the headers attribute is misconfigured.

This is a security issue and should be merged asap. Currently, anyone with custom TrustProxies headers that don't match the middleware defaults or the ELB/Traefik presets are impacted by this issue.

Until this PR is merged, anyone who is impacted can temporarily patch the bug by replacing the getTrustedHeaderNames method in TrustProxies with the following (this assumes that TrustProxies::$headers is properly set using the Request::HEADER_X_* constants):

protected function getTrustedHeaderNames()
{
  return $this->headers;
}

@taylorotwell taylorotwell merged commit 8c72035 into laravel:10.x Jul 26, 2023
16 checks passed
mzur added a commit to biigle/core that referenced this pull request Aug 21, 2023
This can be reverted when upgrading to Laravel 10.

References: laravel/framework#47844
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants