Key the client address on the connection, not a header the caller sends
client_ip() read the first X-Forwarded-For hop unconditionally. The login rate limit and the account lockout are keyed on that value and the audit log records it, so rotating the header per request bought an unbounded budget and an untraceable trail. This walked around TRUSTED_PROXY_COUNT, which exists to decide exactly this. The app leaves ProxyFix off by default and says why, ten lines of comment away: ProxyFix trusts X-Forwarded-*, a client can send those itself, and the login limiter is keyed on the address. Then client_ip read the header anyway, so the stated protection never applied. Resolution is now from remote_addr, which ProxyFix has already filled from the header wherever a proxy is genuinely configured. Both cases end at the same trustworthy source, and the port still comes off because ARR forwards clientip:port and ProxyFix passes it through verbatim. geenforce and tools had each hand-rolled the same mistake for their own limiters. Rather than duplicate security-relevant logic a third time, client_ip and strip_port join the shopdb.api surface (contract 0.28.0). geenforce's _trusted_client_ip, which already refused the raw header for its IP allowlist, is untouched and was the model for this. The stability table was also missing rows for 0.26.0 and 0.27.0. INSTALL-WINDOWS-IIS.md never mentioned TRUSTED_PROXY_COUNT, so a site installed from it runs behind IIS with the setting at 0: every request looks like it came from IIS, the whole site shares one rate-limit bucket, and the audit log records the proxy for every action. It now says so, and says not to set it when nothing is in front. Three test modules simulated separate callers by setting X-Forwarded-For, which is the header a real attacker rotates to escape the limit. One of them, test_client_ip_prefers_forwarded_first_hop_ stripped, asserted the defect outright. The suite could not have caught this. They now drive REMOTE_ADDR, with a regression guard that forging the header buys nothing. Scope: product
This commit is contained in:
@@ -104,6 +104,12 @@ def collector_authorized(pluginname: str):
|
||||
# as script in this origin - see shopdb/utils/uploads.py.
|
||||
from shopdb.utils.uploads import send_upload, harden_upload_response
|
||||
|
||||
# Caller IP for rate limiting and audit. On the surface so a plugin does not
|
||||
# reimplement it: the obvious hand-rolled version reads X-Forwarded-For, which
|
||||
# the caller supplies, so a rate limit keyed on it can be rotated away to
|
||||
# nothing. See shopdb/utils/clientip.py.
|
||||
from shopdb.utils.clientip import client_ip, strip_port
|
||||
|
||||
# Import-mode helpers: preserve legacy timestamps during a bulk data import
|
||||
from shopdb.utils.import_mode import (
|
||||
apply_import_timestamps,
|
||||
@@ -346,6 +352,8 @@ __all__ = [
|
||||
# Serving uploads
|
||||
'send_upload',
|
||||
'harden_upload_response',
|
||||
'client_ip',
|
||||
'strip_port',
|
||||
# Import-mode helpers
|
||||
'apply_import_timestamps',
|
||||
'import_mode_active',
|
||||
|
||||
Reference in New Issue
Block a user