Changes in ProxyFix are not documented

[remilapeyre]

Hi, it seems to me that ProxyFix got changes that were not documented in 0.15. While the documentation give information about the module having changed, it's signature and behavior changed to. The new signature is:

ProxyFix(app, num_proxies=None, x_for=1, x_proto=0, x_host=0, x_port=0, x_prefix=0)

while the old ProxyFix had this behavior: was behaving like this:

ProxyFix(app, num_proxies=None, x_for=1, x_proto=1, x_host=1, x_port=1, x_prefix=1)

As far as I can see, there is no warning in the documentation regarding the change in behavior and since this is not something that is tested with our unit tests, we did not caught it.

Is the change in behavior wanted?
Could we document it more clearly? Especially at https://werkzeug.palletsprojects.com/en/0.15.x/contrib/fixers/ wich only says:

class werkzeug.contrib.fixers.ProxyFix(*args, **kwargs)

Deprecated since version 0.15: werkzeug.contrib.fixers.ProxyFix has moved to werkzeug.middleware.proxy_fix. This import will be removed in 1.0.

Would it be possible that importing from contrib.fixers give the old behavior and for the documentation and the error message give more details about this change?

Thanks a lot!

[davidism]

The signature of the previous version of ProxyFix from 0.14 is:

werkzeug/werkzeug/contrib/fixers.py

Line 120 in f1d15a2

def __init__(self, app, num_proxies=1):

The x_ parameters are all new in 0.15.

[davidism]

The fact that you need to specify the count for each one is documented:

werkzeug/src/werkzeug/middleware/proxy_fix.py

Lines 38 to 39 in 129b3fa

	You must tell the middleware how many proxies set each header so it
	knows what values to trust. It is a security issue to trust values

werkzeug/src/werkzeug/middleware/proxy_fix.py

Lines 62 to 63 in 129b3fa

	argument is deprecated. Each header is configured with a
	separate number of trusted proxies.

[remilapeyre]

Hi @davidism, thanks for the response. I did not see a mention that the default behavior changed though. There was a default behavior, there is still one but they differ. Can't we add a warning regarding this?

[rascalking]

Not for nothing, but by changing the default behavior, you're going to cause anything that's validating a signed http request (like Twilio webhooks) to suddenly decide that all of the signatures it's seeing are wrong.

[davidism]

In most server setups, the default is probably what you want, or you might enable x_host as well. The various headers are set in various combinations by different platforms and proxies. It is dangerous to make a broad recommendation to set every value to 1, you must set the values according to your specific hosting setup, as stated in the documentation.

[rascalking]

When working from a clean slate, absolutely. But I think it might make sense to have the contrib wrapper that throws the deprecation warning supply defaults that match what the previous version of the class did.

Some context for why folks might be cranky about this, since this is what happened to me: a fresh pip install of the same project and code, where flask was pinned to 1.0.2, but werkzeug wasn't pinned, can result in a broken site.