Skip to content

fix: honor accept_only_proxied_requests=False when known_networks is set - #695

Open
sridhar-3009 wants to merge 1 commit into
Neoteroi:mainfrom
sridhar-3009:fix/accept-only-proxied-requests-operator-precedence
Open

fix: honor accept_only_proxied_requests=False when known_networks is set#695
sridhar-3009 wants to merge 1 commit into
Neoteroi:mainfrom
sridhar-3009:fix/accept-only-proxied-requests-operator-precedence

Conversation

@sridhar-3009

Copy link
Copy Markdown

Summary

BaseForwardedHeadersMiddleware.should_validate_client_ip() combines its conditions like this:

return (
    self.accept_only_proxied_requests
    and any(self.known_proxies)
    or any(self.known_networks)
)

Since and binds tighter than or in Python, this is actually evaluated as:

(self.accept_only_proxied_requests and any(self.known_proxies)) or any(self.known_networks)

So whenever known_networks is configured (non-empty), the method returns True regardless of accept_only_proxied_requests - contradicting the method's own docstring:

If accept_only_proxied_requests is set to False, it means the server will accept both requests that are proxied and requests that are hitting the web server directly.

In practice, this means a server configured with known_networks=[...] and accept_only_proxied_requests=False (intending to accept both proxied and direct requests) will still validate the client IP against the known proxies/networks for every direct request, and reject any direct request whose IP isn't in that list with a 400 Proxy IP not recognized error - even though the developer explicitly asked for direct requests to be accepted too.

This doesn't reproduce when only known_proxies is used (the default), since in that case both terms of the or depend on accept_only_proxied_requests correctly. It's specific to the known_networks configuration.

Fix

Parenthesize the intended grouping:

return self.accept_only_proxied_requests and (
    any(self.known_proxies) or any(self.known_networks)
)

Test plan

  • Reproduced the bug against main first (a direct, non-proxied request was rejected with 400 despite accept_only_proxied_requests=False) before applying the fix
  • Added test_x_forwarded_headers_middleware_accepts_direct_requests_with_known_networks, covering the known_networks + accept_only_proxied_requests=False combination
  • pytest tests/test_forwarding.py → 25 passed
  • Full test suite: pytest tests/ → 1923 passed, 1 skipped
  • black --check, isort --check-only, flake8 all clean on changed files

should_validate_client_ip() combined its conditions as:

    accept_only_proxied_requests and any(known_proxies) or any(known_networks)

Since 'and' binds tighter than 'or' in Python, this evaluates as
(accept_only_proxied_requests and any(known_proxies)) or
any(known_networks) - so whenever known_networks was non-empty, the
method returned True regardless of accept_only_proxied_requests,
contradicting its own docstring ("If accept_only_proxied_requests is
set to False... the server will accept both requests that are proxied
and requests that are hitting the web server directly").

In practice this meant that setting accept_only_proxied_requests=False
alongside known_networks still validated (and rejected) the client IP
of every direct, non-proxied request, since should_validate_client_ip()
never actually skipped validation for that configuration.

Parenthesize the intended grouping so the flag is honored regardless of
which of known_proxies/known_networks is configured.
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.

1 participant