#743 doesn't respect TrustedProxy settings

[ceejayoz]

Q	A
Bug?	yes
New Feature?	no
Framework	Laravel
Framework version	10.3.3
Package version	13.4.0
PHP version	8.2

Actual Behaviour

Insertion fails, as it attempts to insert the (correctly) multi-IP forwarding header.

Illuminate\Database\QueryException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for type inet: "175.157.86.118, 10.0.16.108" (Connection: pgsql, SQL: insert into "audits" ("old_values", "new_values", "event", "auditable_id", "auditable_type", "user_id", "user_type", "tags", "ip_address", "user_agent", "url", "updated_at", "created_at") values (... 175.157.86.118, 10.0.16.108...)

Expected Behaviour

Laravel Auditing should only use Request::ip(). This already respects X-Forwarded-For headers internally, if the TrustedProxy middleware is correctly configured. That's where this should be handled.

Note that #743 has introduced a security hole, as X-Forwarded-For can be modified by a malicious user, resulting in an incorrect IP being logged in the audit trail.

Steps to Reproduce

Run Laravel Auditing in a multi-proxy scenario (in our case: nginx --> Docker).

Alternatively, use cURL to spoof the header (curl -H "X-Forwarded-For: 123.123.123.123" http://example.com) on an endpoint that will cause an audit log entry. The faked IP should appear in the database. curl -H "X-Forwarded-For: 123.123.123.123 10.0.0.1" http://example.com will cause the SQL error.

Possible Solutions

Revert change from #743.

[systemsolutionweb]

if the TrustedProxy middleware is correctly configured. That's where this should be handled.

Hi, examples? Can you share a link where everyone can see that configuration? It is to prevent it from happening again and keep information for the future. Thanks

Seems like you update it

[ceejayoz]

@systemsolutionweb

Trusted Proxy docs: https://laravel.com/docs/10.x/requests#configuring-trusted-proxies

It uses Symfony's handling behind the scenes (https://github.com/laravel/framework/blob/136ed754f2d80e5c7cd407a4262eb2eac478e7e8/src/Illuminate/Http/Request.php#L318).

https://symfony.com/doc/current/create_framework/http_foundation.html

The $_SERVER['HTTP_X_FORWARDED_FOR'] value cannot be trusted as it can be manipulated by the end user when there is no proxy. So, if you are using this code in production without a proxy, it becomes trivially easy to abuse your system. That's not the case with the getClientIp() method as you must explicitly trust your reverse proxies by calling setTrustedProxies()

[MortenDHansen]

That makes sense. The package should do as basic as possible, and rely on users to extend when needed.

[ceejayoz]

@MortenDHansen Thanks!

Just to be clear, extending isn't necessary for @lincolnpjames's issue (and would introduce the same security hole); they just need to configure their TrustedProxy middleware correctly for their load balancer / proxy setup.