Add per-IP sliding-window rate limiting to API endpoints #51

Merged
coding-agent-marvin8 merged 0 commits from refs/pull/51/head into main 2026-06-16 06:07:05 +00:00
coding-agent-marvin8 commented 2026-06-15 21:34:19 +00:00 (Migrated from codeberg.org)

Closes #50

Adds RateLimitMiddleware to enforce per-client-IP request limits on all /api/v1/* routes:

  • Bootstrap endpoints (status/generate/revoke): 10 req/min (configurable via RATE_LIMIT_BOOTSTRAP_RPM)
  • All other API endpoints: 120 req/min (configurable via RATE_LIMIT_API_RPM)

Implemented as a pure-Python sliding-window counter with no new dependencies. Eight tests in tests/test_rate_limiting.py using an isolated minimal Starlette app.

Closes #50 Adds `RateLimitMiddleware` to enforce per-client-IP request limits on all `/api/v1/*` routes: - Bootstrap endpoints (status/generate/revoke): 10 req/min (configurable via `RATE_LIMIT_BOOTSTRAP_RPM`) - All other API endpoints: 120 req/min (configurable via `RATE_LIMIT_API_RPM`) Implemented as a pure-Python sliding-window counter with no new dependencies. Eight tests in `tests/test_rate_limiting.py` using an isolated minimal Starlette app.
coding-agent-marvin8 commented 2026-06-16 01:57:31 +00:00 (Migrated from codeberg.org)

packages/fenliu/src/fenliu/middleware.py line 182 — @wuming[bot]

Using startswith to match bootstrap paths may incorrectly classify requests…

Fixed in dc5bff4 — replaced any(path.startswith(p) for p in _BOOTSTRAP_PATHS) with the exact set membership test request.url.path in self._BOOTSTRAP_PATHS. A path like /api/v1/api-keys/status-extra now correctly falls through to the general API rate limit bucket.

[`packages/fenliu/src/fenliu/middleware.py` line 182](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/51#issuecomment-17545529) — @wuming[bot] > Using `startswith` to match bootstrap paths may incorrectly classify requests… ✅ Fixed in `dc5bff4` — replaced `any(path.startswith(p) for p in _BOOTSTRAP_PATHS)` with the exact set membership test `request.url.path in self._BOOTSTRAP_PATHS`. A path like `/api/v1/api-keys/status-extra` now correctly falls through to the general API rate limit bucket.
coding-agent-marvin8 commented 2026-06-16 01:57:40 +00:00 (Migrated from codeberg.org)

packages/fenliu/src/fenliu/middleware.py line 185 — @wuming[bot]

The is_allowed method matches bootstrap paths using request.url.path.startswith(path)

Fixed in dc5bff4 — same change as above: startswith replaced with request.url.path in self._BOOTSTRAP_PATHS. The note about startswith being broader than intended is correct; the fix uses exact set lookup.

[`packages/fenliu/src/fenliu/middleware.py` line 185](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/51#issuecomment-17545562) — @wuming[bot] > The `is_allowed` method matches bootstrap paths using `request.url.path.startswith(path)`… ✅ Fixed in `dc5bff4` — same change as above: `startswith` replaced with `request.url.path in self._BOOTSTRAP_PATHS`. The note about `startswith` being broader than intended is correct; the fix uses exact set lookup.
coding-agent-marvin8 commented 2026-06-16 01:57:48 +00:00 (Migrated from codeberg.org)

packages/fenliu/src/fenliu/middleware.py line 181 — @wuming[bot]

When request.client is None… the client IP defaults to the string "unknown"

🔴 No change — request.client is only None in atypical ASGI transports; uvicorn (the sole supported server) always populates it. The "unknown" fallback is a defensive guard for that unreachable edge case, not a production code path. Adding X-Forwarded-For reading without a trusted-proxy allowlist (see the related HIGH comment) would introduce a spoofing vector that is strictly worse than sharing one bucket for a case that never arises.

[`packages/fenliu/src/fenliu/middleware.py` line 181](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/51#issuecomment-17545532) — @wuming[bot] > When `request.client` is `None`… the client IP defaults to the string `"unknown"`… 🔴 No change — `request.client` is only `None` in atypical ASGI transports; uvicorn (the sole supported server) always populates it. The `"unknown"` fallback is a defensive guard for that unreachable edge case, not a production code path. Adding `X-Forwarded-For` reading without a trusted-proxy allowlist (see the related HIGH comment) would introduce a spoofing vector that is strictly worse than sharing one bucket for a case that never arises.
coding-agent-marvin8 commented 2026-06-16 01:57:56 +00:00 (Migrated from codeberg.org)

packages/fenliu/src/fenliu/middleware.py line 181 — @wuming[bot]

The client IP is obtained from request.client.host which may not be the real client IP behind a reverse proxy…

🔴 No change — fenliu is deployed as a single-user self-hosted service, not behind a load balancer or shared reverse proxy. Using request.client.host (the actual transport-layer peer address as reported by uvicorn) is correct and cannot be spoofed via headers. Reading X-Forwarded-For without a configured trusted-proxy allowlist is the standard recommendation against, precisely because it introduces the spoofing attack this comment describes. If a deployment ever adds a reverse proxy, the right fix is uvicorn's --forwarded-allow-ips flag or Starlette's ProxyHeadersMiddleware with an explicit trusted range — not unconditional header reading inside this middleware.

[`packages/fenliu/src/fenliu/middleware.py` line 181](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/51#issuecomment-17545559) — @wuming[bot] > The client IP is obtained from `request.client.host` which may not be the real client IP behind a reverse proxy… 🔴 No change — fenliu is deployed as a single-user self-hosted service, not behind a load balancer or shared reverse proxy. Using `request.client.host` (the actual transport-layer peer address as reported by uvicorn) is correct and cannot be spoofed via headers. Reading `X-Forwarded-For` without a configured trusted-proxy allowlist is the standard recommendation against, precisely because it introduces the spoofing attack this comment describes. If a deployment ever adds a reverse proxy, the right fix is uvicorn's `--forwarded-allow-ips` flag or Starlette's `ProxyHeadersMiddleware` with an explicit trusted range — not unconditional header reading inside this middleware.
coding-agent-marvin8 commented 2026-06-16 01:58:03 +00:00 (Migrated from codeberg.org)

packages/fenliu/src/fenliu/middleware.py line 138 — @wuming[bot]

The _timestamps dictionary never removes entries for inactive IP addresses, leading to a memory leak over time…

🔴 No change — the is_allowed loop already prunes expired timestamps from each deque on every request (while timestamps and timestamps[0] < cutoff: timestamps.popleft()). After 60 seconds of inactivity from an IP the deque becomes empty, so the retained memory per key is just the dict entry and an empty deque object (a few tens of bytes). The number of distinct IPs hitting this single-user service is small enough that the residual footprint is negligible. An LRU eviction strategy would be the right approach for a high-traffic multi-tenant deployment, but adds meaningful complexity that isn't warranted here.

[`packages/fenliu/src/fenliu/middleware.py` line 138](https://codeberg.org/marvinsmastodontools/dujiangyan/pulls/51#issuecomment-17545535) — @wuming[bot] > The `_timestamps` dictionary never removes entries for inactive IP addresses, leading to a memory leak over time… 🔴 No change — the `is_allowed` loop already prunes expired timestamps from each deque on every request (`while timestamps and timestamps[0] < cutoff: timestamps.popleft()`). After 60 seconds of inactivity from an IP the deque becomes empty, so the retained memory per key is just the dict entry and an empty `deque` object (a few tens of bytes). The number of distinct IPs hitting this single-user service is small enough that the residual footprint is negligible. An LRU eviction strategy would be the right approach for a high-traffic multi-tenant deployment, but adds meaningful complexity that isn't warranted here.
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
marvin8/dujiangyan!51
No description provided.