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

[8.x] Implement TrustProxies middleware #38295

Merged
merged 4 commits into from
Aug 9, 2021
Merged

[8.x] Implement TrustProxies middleware #38295

merged 4 commits into from
Aug 9, 2021

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Aug 9, 2021

This PR ports the TrustedProxies package by Chris Fidao to Laravel. This will help us reduce one more external dependency we used to rely on.

In contrary to the original package, all configuration is done through overwriting properties. I'll send in an additional PR to laravel/laravel after this is merged and tagged.

@GrahamCampbell
Copy link
Member

Why must this be abstract?

@driesvints
Copy link
Member Author

driesvints commented Aug 9, 2021

@GrahamCampbell TrustHosts is also abstract. We extend the middleware in the skeleton.

@GrahamCampbell
Copy link
Member

A common thing i like to do is delete middlewares from the skeleton and use the default. Less app code to worry about and upgrade later.

@driesvints
Copy link
Member Author

Done

@taylorotwell
Copy link
Member

Will this have any breaking changes?

@taylorotwell taylorotwell merged commit af0b69a into 8.x Aug 9, 2021
@taylorotwell taylorotwell deleted the trusted-proxies branch August 9, 2021 18:55
@driesvints
Copy link
Member Author

@taylorotwell no. It's an entire new class.

@rrpadilla
Copy link
Contributor

@driesvints The $headers should accept "int|string|null"

    /**
     * The proxy header mappings.
     *
     * @var int|string|null
     */
    protected $headers = Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_HOST | Request::HEADER_X_FORWARDED_PORT | Request::HEADER_X_FORWARDED_PROTO | Request::HEADER_X_FORWARDED_AWS_ELB;

As you can see in the "getTrustedHeaderNames()" function.

   /**
     * Retrieve trusted header name(s), falling back to defaults if config not set.
     *
     * @return int A bit field of Request::HEADER_*, to set which headers to trust from your proxies.
     */
    protected function getTrustedHeaderNames()
    {
        switch ($this->headers) {
            case 'HEADER_X_FORWARDED_AWS_ELB':
            case Request::HEADER_X_FORWARDED_AWS_ELB:
                return Request::HEADER_X_FORWARDED_AWS_ELB;

            case 'HEADER_FORWARDED':
            case Request::HEADER_FORWARDED:
                return Request::HEADER_FORWARDED;

            case 'HEADER_X_FORWARDED_FOR':
            case Request::HEADER_X_FORWARDED_FOR:
                return Request::HEADER_X_FORWARDED_FOR;

            case 'HEADER_X_FORWARDED_HOST':
            case Request::HEADER_X_FORWARDED_HOST:
                return Request::HEADER_X_FORWARDED_HOST;

            case 'HEADER_X_FORWARDED_PORT':
            case Request::HEADER_X_FORWARDED_PORT:
                return Request::HEADER_X_FORWARDED_PORT;

            case 'HEADER_X_FORWARDED_PROTO':
            case Request::HEADER_X_FORWARDED_PROTO:
                return Request::HEADER_X_FORWARDED_PROTO;

            default:
                return Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_HOST | Request::HEADER_X_FORWARDED_PORT | Request::HEADER_X_FORWARDED_PROTO | Request::HEADER_X_FORWARDED_AWS_ELB;
        }

        return $this->headers;
    }

This is very useful when we need to get the $headers and $proxies from a config file. For example if the site is hosted on AWS behind AWS Elastic Load Balancer.

<?php

namespace App\Http\Middleware;

use Illuminate\Http\Middleware\TrustProxies as Middleware;
use Illuminate\Http\Request;
use Illuminate\Contracts\Config\Repository;

class TrustProxies extends Middleware
{
    /**
     * The trusted proxies for this application.
     *
     * @var array|string|null
     */
    protected $proxies;

    /**
     * The headers that should be used to detect proxies.
     *
     * @var int
     */
    protected $headers;

    /**
     * Create a new trusted proxies middleware instance.
     *
     * @param  \Illuminate\Contracts\Config\Repository  $config
     * @return void
     */
    public function __construct(Repository $config)
    {
        $this->proxies = $config->get('trustedproxy.proxies');
        $this->headers = $config->get('trustedproxy.headers');
    }
}

The "config/trustedproxy.php" file

<?php

return [

    'proxies' => env('TRUSTEDPROXY_PROXIES'),

    'headers' => env('TRUSTEDPROXY_HEADERS', 'HEADER_X_FORWARDED_AWS_ELB'),

];

Will be great if we can add this to the Laravel documentation.

Thanks.

@driesvints
Copy link
Member Author

@rrpadilla the current type seems correct to me.

@rrpadilla
Copy link
Contributor

rrpadilla commented Nov 22, 2021

@driesvints
keep in mind that the $headers attribute is used as a mapping to get the trusted header value, it is not the final header.
The function getTrustedHeaderNames() will return the trusted header to use based on the $headers attribute.
That type (@var null|string|int) is exactly what the TrustedProxies package by Chris Fidao is using because you can use a string value that you get from the config file.

@ttrig
Copy link
Contributor

ttrig commented Nov 23, 2021

I suspect this might be a breaking change since the possibility to configure this middleware with the trustedproxy.proxies config option was left out.

https://github.com/fideloper/TrustedProxy/blob/master/src/TrustProxies.php#L67

@driesvints
Copy link
Member Author

There's no breaking change. This is a new middleware to the framework.

LukeTowers added a commit to wintercms/storm that referenced this pull request Aug 4, 2022
This reinstates the behaviour originally present in fideveloper/trustedproxy where setting ** as the value for app.trustedProxies would allow all proxies vs * which would only allow the most recent one in a chain of proxies (as determined by $_SERVER['REMOTE_ADDR']). See fideloper/TrustedProxy@6018dfb for when & why it was originally added.

The '**' wildcard was removed in v4 of that package (fideloper/TrustedProxy@1d09591) with no explanation and was never added back in when Laravel merged it into the core in laravel/framework#38295.

This causes problems for environments where you have multiple proxies in a chain (i.e. Amazon CloudFront in front of Amazon ELB). These problems are documented in fideloper/TrustedProxy#115 & fideloper/TrustedProxy#107, and spawned fideloper/TrustedProxy#142 & https://github.com/ge-tracker/laravel-vapor-trusted-proxies to resolve them.

Ultimately, this commit serves to reintroduce the original behaviour of fideveloper/trustproxies v3 and make it so that you can use `**` as the value for app.trustProxies in order to get the correct client IP address when running Winter on Laravel Vapor.
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

5 participants