Repository navigation
[INFRA-1189] chore(security): make proxy hop count configurable for rate limits #10075
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -150,6 +150,14 @@ | |
| "SCHEMA_COERCE_PATH_PK": False, | ||
| } | ||
|
|
||
| # Number of reverse proxies in front of the API. DRF throttles use it to pick the | ||
| # client IP from X-Forwarded-For. Unset keeps DRF's default (None). | ||
| _num_proxies_raw = os.environ.get("NUM_PROXIES", "").strip() | ||
| if _num_proxies_raw.isdecimal() and len(_num_proxies_raw) <= 3: | ||
| REST_FRAMEWORK["NUM_PROXIES"] = int(_num_proxies_raw) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: this hop count only reaches DRF throttles. Several other places read the client IP through
In the README's "behind an additional load balancer" setup, the leftmost entry is whatever the client sent. So once this is configured, the IP recorded for logins and request logs can differ from the IP the throttle counts. This was already the case before this PR, but a follow-up could make
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| elif _num_proxies_raw: | ||
| _logger.warning("NUM_PROXIES: ignoring invalid value %r", _num_proxies_raw) | ||
|
|
||
| # API key throttle rate (DRF SimpleRateThrottle format, e.g. "60/minute") | ||
| API_KEY_RATE_LIMIT = os.environ.get("API_KEY_RATE_LIMIT", "60/minute") | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # Copyright (c) 2023-present Plane Software, Inc. and contributors | ||
| # SPDX-License-Identifier: AGPL-3.0-only | ||
| # See the LICENSE file for details. | ||
|
|
||
| """Unit tests for the optional NUM_PROXIES setting.""" | ||
|
|
||
| import importlib | ||
|
|
||
| import pytest | ||
|
|
||
| from plane.settings import common | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def load_rest_framework(monkeypatch): | ||
| """Re-execute settings/common.py with NUM_PROXIES set to ``value`` (None = unset).""" | ||
|
|
||
| def _load(value): | ||
| if value is None: | ||
| monkeypatch.delenv("NUM_PROXIES", raising=False) | ||
| else: | ||
| monkeypatch.setenv("NUM_PROXIES", value) | ||
| return importlib.reload(common).REST_FRAMEWORK | ||
|
|
||
| yield _load | ||
| monkeypatch.undo() | ||
| importlib.reload(common) | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| class TestNumProxiesSetting: | ||
| @pytest.mark.parametrize("value", [None, "", "abc", "-1", "9" * 5000]) | ||
| def test_unset_or_invalid_keeps_drf_default(self, load_rest_framework, value): | ||
| assert "NUM_PROXIES" not in load_rest_framework(value) | ||
|
|
||
| @pytest.mark.parametrize("value, expected", [("1", 1), ("0", 0)]) | ||
| def test_non_negative_integer_is_applied(self, load_rest_framework, value, expected): | ||
| assert load_rest_framework(value)["NUM_PROXIES"] == expected |
Uh oh!
There was an error while loading. Please reload this page.