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

[11.x] Fixes trust proxy REMOTE_ADDR not working in Swoole #52889

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

chuoke
Copy link
Contributor

@chuoke chuoke commented Sep 23, 2024

When using REMOTE_ADDR set the trust proxy in Swoole, like this:

      $middleware->trustProxies(
            at: [
                'REMOTE_ADDR',
            ],
            headers: Request::HEADER_X_FORWARDED_FOR
        );

Is nothing set to request->trustedProxies because $_SERVER['REMOTE_ADDR'] does not exist in Swoole. But Laravel internally converts the server information in Swoole into the request->server attribute, which includes REMOTE_ADDR, and then request()->server->get('REMOTE_ADDR') can get the corresponding value normally.

This leads to some logical ambiguity and increases the cognitive burden and learning cost.

So we need to unify the logic to avoid inconsistency. I can't change this in the Request class because of the method attribute restrictions, so the solution I've come up with so far is this, it works for me, and I hope there's another better way to solve it.

@crynobone crynobone changed the title fix: trust proxy REMOTE_ADDR not working in Swoole [11.x] Fixes trust proxy REMOTE_ADDR not working in Swoole Sep 24, 2024
@crynobone crynobone changed the title [11.x] Fixes trust proxy REMOTE_ADDR not working in Swoole [11.x] Fixes trust proxy REMOTE_ADDR not working in Swoole Sep 24, 2024
@taylorotwell
Copy link
Member

Can you add a test then mark as ready for review?

@taylorotwell taylorotwell marked this pull request as draft September 25, 2024 15:24
@chuoke chuoke marked this pull request as ready for review September 26, 2024 06:47
@chuoke
Copy link
Contributor Author

chuoke commented Sep 27, 2024

hi @taylorotwell I added one test case. the checks were unsuccessful, but not caused by this change, it seems some other network is wrong. Can you recheck this?

@taylorotwell taylorotwell merged commit 7aabb89 into laravel:11.x Sep 27, 2024
31 checks passed
@chuoke chuoke deleted the fix-trust-proxy-remote_addr-swoole branch September 29, 2024 03:16
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.

2 participants