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

Create setting/flag to enable/disable X-Forwarded-Port #2473

Closed
wants to merge 7 commits into from

Conversation

victor-torres
Copy link

Summary

Create a new setting/flag to enable/disable X-Forwarded-Port to populate remote address info.

This flag is disabled by default. Command line options are:

  • --forwarded-port
  • --no-forwarded-port

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@victor-torres
Copy link
Author

victor-torres commented Sep 30, 2024

This pull request is addressing the issue described in #1974

@victor-torres
Copy link
Author

There's a failing test case in tests/supervisors/test_multiprocess.py that I don't think is related to this PR.
It fails on Python 3.9 running on macOS.

I think this PR is ready for review. Let me know if I can help with anything else or if there's anything that could be improved here :D

@Kludex
Copy link
Member

Kludex commented Oct 2, 2024

Do we need to add a new flag?

@victor-torres
Copy link
Author

victor-torres commented Oct 2, 2024

I don't mind considering X-Forwarded-Port (if available) by default if already checking for X-Forwarded-For.

My decision to include a new flag was to avoid changing the default behaviour, something that some maintainers and some users usually ask for.

But I can totally remove the flag and make it be considered by default +1

Just let me know.

@Kludex
Copy link
Member

Kludex commented Oct 9, 2024

Let's take a step back here, and continue the discussion on #1974 (comment), please.

@Kludex
Copy link
Member

Kludex commented Oct 10, 2024

I'll be closing this until we have a decision on the issue.

I don't think we want this to be a parameter anyway.

@Kludex Kludex closed this Oct 10, 2024
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