PR 1: Python — Security Middleware tests#439
Conversation
Cover all four security subsystems in web/security.py: - CSRF protection: safe method exemption, loopback bypass, origin/referer matching, port normalization, proxy header detection - Rate limiting: per-IP tracking, limit enforcement, stale sweep, SSE exemption, disable flag, X-Forwarded-For extraction - Security headers: nosniff, X-Frame-Options, CSP, Referrer-Policy - API key auth: enabled/disabled states, exempt paths, SSE query param, header validation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a comprehensive unit test suite for web security features covering CSRF protection, rate limiting logic, security response headers, and API key authentication mechanisms, validating edge cases including proxy header handling, port normalization, and trusted IP selection. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nitrobass24
changed the title
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/python/tests/unittests/test_web/test_security.py`:
- Around line 326-385: Both tests currently register custom before_request hooks
instead of exercising the real middleware; replace those hooks to call
install_rate_limiting so you validate production behavior. Modify
test_rate_limit_returns_429_with_retry_after to call install_rate_limiting(app,
max_requests=2, window_seconds=60) (or monkeypatch the _RateLimiter constructor
to return a limiter with those thresholds) and then drive requests with TestApp
and assert status 429 and presence of "Retry-After". Modify
test_x_forwarded_for_used_when_trusted to call install_rate_limiting(app,
max_requests=2, window_seconds=60, trust_x_forwarded_for=True) (or patch
configuration so X-Forwarded-For is trusted), then send three requests with
X-Forwarded-For and assert the third returns 429; remove the duplicated custom
hook logic in both tests and rely on install_rate_limiting, keeping assertions
for Retry-After and 429 as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 173e7bf4-6e59-4725-8289-2518824cac7b
📒 Files selected for processing (1)
src/python/tests/unittests/test_web/test_security.py
| def test_rate_limit_returns_429_with_retry_after(self): | ||
| app = _make_app_with_route() | ||
| limiter = _RateLimiter(max_requests=2, window_seconds=60) | ||
|
|
||
| @app.hook("before_request") | ||
| def _rate_limit(): | ||
| ip = bottle.request.environ.get("REMOTE_ADDR", "127.0.0.1") | ||
| if not limiter.is_allowed(ip): | ||
| retry = limiter.retry_after(ip) | ||
| resp = bottle.HTTPError(429, "Rate limit exceeded") | ||
| resp.headers["Retry-After"] = str(retry) | ||
| raise resp | ||
|
|
||
| test_app = TestApp(app) | ||
| test_app.get("/test", extra_environ={"REMOTE_ADDR": "10.0.0.1"}) | ||
| test_app.get("/test", extra_environ={"REMOTE_ADDR": "10.0.0.1"}) | ||
| resp = test_app.get( | ||
| "/test", | ||
| extra_environ={"REMOTE_ADDR": "10.0.0.1"}, | ||
| expect_errors=True, | ||
| ) | ||
| self.assertEqual(resp.status_int, 429) | ||
| self.assertIn("Retry-After", resp.headers) | ||
|
|
||
| def test_x_forwarded_for_used_when_trusted(self): | ||
| app = _make_app_with_route() | ||
| limiter = _RateLimiter(max_requests=2, window_seconds=60) | ||
|
|
||
| @app.hook("before_request") | ||
| def _rate_limit(): | ||
| ip = None | ||
| forwarded = bottle.request.get_header("X-Forwarded-For") | ||
| if forwarded: | ||
| ip = forwarded.split(",")[0].strip() | ||
| if not ip: | ||
| ip = bottle.request.environ.get("REMOTE_ADDR", "127.0.0.1") | ||
| if not limiter.is_allowed(ip): | ||
| raise bottle.HTTPError(429, "Rate limit exceeded") | ||
|
|
||
| test_app = TestApp(app) | ||
|
|
||
| # Requests with X-Forwarded-For should use the forwarded IP | ||
| test_app.get( | ||
| "/test", | ||
| headers={"X-Forwarded-For": "192.168.1.100"}, | ||
| extra_environ={"REMOTE_ADDR": "127.0.0.1"}, | ||
| ) | ||
| test_app.get( | ||
| "/test", | ||
| headers={"X-Forwarded-For": "192.168.1.100"}, | ||
| extra_environ={"REMOTE_ADDR": "127.0.0.1"}, | ||
| ) | ||
| # Third request from same forwarded IP should be rejected | ||
| resp = test_app.get( | ||
| "/test", | ||
| headers={"X-Forwarded-For": "192.168.1.100"}, | ||
| extra_environ={"REMOTE_ADDR": "127.0.0.1"}, | ||
| expect_errors=True, | ||
| ) | ||
| self.assertEqual(resp.status_int, 429) |
There was a problem hiding this comment.
Rate-limiting middleware behavior is not fully validated here.
test_rate_limit_returns_429_with_retry_after and test_x_forwarded_for_used_when_trusted build custom before_request hooks instead of invoking install_rate_limiting(...). That means these assertions can still pass even if the real middleware’s IP extraction / 429 / Retry-After logic regresses.
Please switch these to exercise install_rate_limiting directly (configure or patch limiter thresholds as needed) so the tests verify production behavior, not duplicated test-local logic.
Based on learnings: "When adding a feature, add unit tests for all new logic covering happy path, edge cases, and error/validation paths; for new API endpoints, add integration tests using WebTest/TestApp".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/tests/unittests/test_web/test_security.py` around lines 326 - 385,
Both tests currently register custom before_request hooks instead of exercising
the real middleware; replace those hooks to call install_rate_limiting so you
validate production behavior. Modify
test_rate_limit_returns_429_with_retry_after to call install_rate_limiting(app,
max_requests=2, window_seconds=60) (or monkeypatch the _RateLimiter constructor
to return a limiter with those thresholds) and then drive requests with TestApp
and assert status 429 and presence of "Retry-After". Modify
test_x_forwarded_for_used_when_trusted to call install_rate_limiting(app,
max_requests=2, window_seconds=60, trust_x_forwarded_for=True) (or patch
configuration so X-Forwarded-For is trusted), then send three requests with
X-Forwarded-For and assert the third returns 429; remove the duplicated custom
hook logic in both tests and rely on install_rate_limiting, keeping assertions
for Retry-After and 429 as-is.
1 participant
Summary
web/security.pycovering all four security subsystems_origin_tuplehelper parsing_RateLimiterper-IP tracking, limit enforcement, stale entry sweep, SSE stream exemption,disableflag,X-Forwarded-Forextraction, 429 + Retry-After response/server/config/getexempt, SSE query param and header authTest plan
cd src/python && PYTHONPATH=. python3 -m pytest tests/unittests/test_web/test_security.py -v— 47 passedpython3 -m ruff check tests/unittests/test_web/test_security.py— all checks passedpython3 -m ruff format --check tests/unittests/test_web/test_security.py— already formatted🤖 Generated with Claude Code
Summary by CodeRabbit