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

[9.x] Backport: Update TrustProxies to rely on $headers if properly set #48129

Closed
wants to merge 1 commit into from

Conversation

mzur
Copy link
Contributor

@mzur mzur commented Aug 21, 2023

This is a backport of #47844 which can be considered a security issue.

…aravel#47844)

* Update trust proxies to rely on configuration if set

* Remove constants from match statement since they'll never match
@driesvints
Copy link
Member

Thanks. Please resend this with passing tests.

@driesvints driesvints closed this Aug 21, 2023
@mzur
Copy link
Contributor Author

mzur commented Aug 21, 2023

@driesvints Thanks for the quick reply. The type check error comes from a file that was not modified by this PR (Illuminate/Http/Client/Factory.php). Also the tests fail in an unrelated test case (HttpRequestTest.php) and rather seem to be an error in the test itself:

1) Illuminate\Tests\Http\HttpRequestTest::testDateMethod
Failed asserting that two DateTime objects are equal.
--- Expected
+++ Actual
@@ @@
-2020-01-01T16:30:25.000000+0000
+2020-01-01T16:30:25.889845+0000

The TrustProxiesTest passes.

@driesvints
Copy link
Member

Right now we can't merge any PR with failing tests and we're not actively developing Laravel v9 anymore. Please fix any failing tests before sending in a PR, thanks.

@mzur
Copy link
Contributor Author

mzur commented Aug 21, 2023

I considered this a security fix which are promised to be provided until next year for 9.x. But it's your call, of course. I'll see what I can do but fixing my own application has priority.

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.

3 participants