fix(virtual_lab): secure evaluate_code endpoint with auth, rate limiting, and tests#1032
Open
arnavgogia20 wants to merge 1 commit intoalphaonelabs:mainfrom
Open
Conversation
…miting - add login_required protection - implement per-user rate limiting via Django cache - add safe JSON parsing with error handling - log rate limit violations - add test coverage for auth and throttling
Contributor
👀 Peer Review RequiredHi @arnavgogia20! This pull request does not yet have a peer review. Before this PR can be merged, please request a review from one of your peers:
Thank you for contributing! 🎉 |
Contributor
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Configuration Updates .pre-commit-config.yaml, poetry.toml |
Removed --parallel=4 flag from Django test command to run tests serially; enabled automatic virtual environment creation in Poetry. |
Feature Implementation web/virtual_lab/views.py |
Added @login_required decorator for authentication; implemented per-user rate limiting (5 requests/minute) via Django cache with 429 response; added try/except for JSON parsing errors returning 400 response; preserved existing error handling for downstream API failures. |
Test Coverage web/virtual_lab/tests.py |
Added EvaluateCodeTests suite with three test cases: authentication requirement verification, rate limiting enforcement (mocked external API), and invalid JSON payload handling. |
Sequence Diagram(s)
sequenceDiagram
participant Client as User/Client
participant View as Django View<br/>(evaluate_code)
participant Cache as Django Cache
participant API as External API<br/>(Piston)
Client->>View: POST /evaluate_code
alt Not Authenticated
View-->>Client: Redirect to /accounts/login/
else Authenticated
View->>Cache: Check rate limit<br/>(key: user_id)
alt Rate Limit Exceeded
Cache-->>View: 429 response
View-->>Client: 429 + error message
else Within Limit
View->>View: Parse JSON payload
alt Invalid JSON
View-->>Client: 400 + error message
else Valid JSON
View->>Cache: Increment request count
View->>API: POST code to evaluate
API-->>View: Response (200/5xx)
View-->>Client: Response
end
end
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately summarizes the main changes: adding authentication, rate limiting, and tests to the evaluate_code endpoint, which aligns with all major file modifications. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
📝 Coding Plan
- Generate coding plan for human review comments
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 @coderabbitai help to get the list of available commands and usage tips.
Tip
CodeRabbit can approve the review once all CodeRabbit's comments are resolved.
Enable the reviews.request_changes_workflow setting to automatically approve the review once all CodeRabbit's comments are resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/virtual_lab/tests.py`:
- Line 18: The test currently calls cache.clear(), which wipes the global cache;
instead target and delete only the rate-limit key for the test user. Replace the
cache.clear() call with a deletion of the specific key (e.g., use
cache.delete(rate_limit_key) or cache.pop(rate_limit_key, None)) where
rate_limit_key is the same key your rate limiter uses for this user within the
test setup; keep the rest of the cache untouched to avoid cross-test coupling.
In `@web/virtual_lab/views.py`:
- Around line 158-169: The rate limiter has a race on initialization and a
non-parameterized log; replace the get/set sequence with an atomic
cache.add(cache_key, 1, timeout=60) to initialize only one process, then call
cache.incr(cache_key) for subsequent requests and check the returned value
against the limit (>=5) to return the 429 JsonResponse; also change the logger
call in this branch to parameterized logging via logger.warning("Rate limit
exceeded for user %s", request.user.id) to avoid f-string evaluation and support
structured logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dcba5c22-e36f-411d-8e5f-cdfe06c47266
⛔ Files ignored due to path filters (1)
poetry.logis excluded by!**/*.log
📒 Files selected for processing (4)
.pre-commit-config.yamlpoetry.tomlweb/virtual_lab/tests.pyweb/virtual_lab/views.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security Fix: Protect
evaluate_codeEndpointThis PR addresses a critical security vulnerability in the
evaluate_codeendpoint by adding authentication, rate limiting, and improved request handling.Problem
The
evaluate_codeendpoint previously:Changes Made
1. Authentication Protection
@login_requiredto restrict access to authenticated users only2. Rate Limiting (Per-user)
429 Too Many Requestswhen exceeded3. Safe JSON Handling
json.loads(request.body)in atry/except400 Bad Requestfor invalid JSON payloads4. Test Coverage
Added tests to ensure reliability:
External API calls (Piston) are mocked in tests to ensure consistency.
Verification
234 tests)black,isort,flake8, etc.)Notes
Impact
Please let me know if you'd like any changes or improvements...i'll be happy to iterate!
Security Enhancement for evaluate_code Endpoint
This PR addresses a security vulnerability in the
evaluate_codeendpoint by introducing authentication, rate limiting, and input validation controls.Key Changes
Authentication & Rate Limiting (views.py)
@login_requireddecorator to restrict endpoint access to authenticated usersInput Validation (views.py)
Test Coverage (tests.py)
Build Configuration
Impact
These changes prevent abuse of external code execution functionality, improve error handling reliability, and align with secure coding best practices. All 234 existing tests pass along with pre-commit hooks validation.