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

Update config docs to match DEFAULT_CONFIG #1803

Conversation

KevinGDialpad
Copy link
Contributor

The docs indicate that the default value of REAL_IP_HEADER is "X-Real-IP" but the code never sets it to that value.

@Tronic
Copy link
Member

Tronic commented Mar 10, 2020

The default value was changed to None for security reasons. Otherwise an attacker could send any spoofed addresses to Sanic servers that are not behind proxies but that are using the default config. Also, each service provider seems to use their own names for this header, so a default value would not work for all. For more information, see https://sanic.readthedocs.io/en/latest/sanic/config.html#proxy-configuration

However, the table in the documentation lists wrong values for many variables, and should be updated to match with current codebase. Good catch! Could you change this PR to do that?

@andreymal
Copy link
Contributor

When I made #1539 I meant that REAL_IP_HEADER will work only if PROXIES_COUNT has non-zero value. It seems the logic is changed now, so the docs should be updated instead

@Tronic
Copy link
Member

Tronic commented Mar 10, 2020

#1638 is the relevant PR for changes in semantics and default values. Prior to that PROXIES_COUNT defaulted to -1 (i.e. use the last proxy), so in effect the real IP header was also always enabled.

@KevinGDialpad
Copy link
Contributor Author

KevinGDialpad commented Mar 10, 2020

The default value was changed to None for security reasons. Otherwise an attacker could send any spoofed addresses to Sanic servers that are not behind proxies but that are using the default config. Also, each service provider seems to use their own names for this header, so a default value would not work for all. For more information, see https://sanic.readthedocs.io/en/latest/sanic/config.html#proxy-configuration

Ah, fair enough.
I'll make this PR a config change then!

Also, my bad, I should have noticed it. I thought the tests were passing locally yesterday but I probably didn't pay close enough attention to the tox output.

@KevinGDialpad
Copy link
Contributor Author

I noticed that the WEBSOCKET_ default config values weren't documented, so I added them. Let me know if they were left out on purpose.

@KevinGDialpad KevinGDialpad changed the title Set REAL_IP_HEADER's default value to "X-Real-IP" Update config docs to match DEFAULT_CONFIG Mar 10, 2020
@yunstanford yunstanford merged commit 60b4efa into sanic-org:master Mar 14, 2020
@KevinGDialpad KevinGDialpad deleted the set_default_value_for_REAL_IP_HEADER branch January 30, 2021 01:42
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.

4 participants