Skip to content

Commit 16c75af

Browse files
manavgupclaude
andauthored
fix: Security & performance improvements for PR #555 (#563)
* fix(pr-555): Add authorization checks and improve security Addresses critical security vulnerabilities and code quality issues identified in PR #555 review: **1. Authorization Security** (CRITICAL FIX): - Add verify_user_authorization() helper function to validate user access - Implement authorization checks in ALL endpoints (create, get, update, delete, toggle, list) - Only admins can create GLOBAL configs - Users can only access their own configs (or admin can access any) - Proper logging for authorization violations **2. Exception Handling** (HIGH PRIORITY): - Replace generic Exception catching with specific exception types: - ValidationError for Pydantic validation failures (422 status) - IntegrityError for unique constraint violations (409 status) - ValueError for not found errors (404 status) - Use logger.exception() for unexpected errors (better stack traces) - Return generic "Internal server error" message for security **3. Performance Optimization** (HIGH PRIORITY): - Add composite database indexes for common query patterns: - idx_runtime_config_user_lookup (scope, category, user_id, is_active) - idx_runtime_config_collection_lookup (scope, category, collection_id, is_active) - idx_runtime_config_global_lookup (scope, category, is_active) - Optimizes get_effective_config() hierarchical queries **4. Code Quality Improvements**: - Import ConfigScope for scope validation - Import IntegrityError and ValidationError for proper exception handling - Improve docstrings with authorization documentation - Better error messages and logging - Ruff formatting applied **Testing**: Local validation pending **Related**: Addresses review feedback from PR #555 (Dynamic Configuration System) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: Add NotFoundError exception handling to prevent 500 errors - Add NotFoundError import from core.custom_exceptions - Update get_runtime_config() to catch NotFoundError → 404 - Update update_runtime_config() to catch NotFoundError → 404 - Update delete_runtime_config() to catch NotFoundError → 404 - Update toggle_runtime_config() to catch NotFoundError → 404 - ValueError now properly returns 400 Bad Request instead of 404 - Fixes critical bug where service NotFoundError was caught as generic Exception - Ensures proper HTTP status codes for all error conditions Addresses automated review feedback on PR #563 Related to PR #555 (Dynamic Configuration System) * docs: Update PR #555 review with NotFoundError fix - Document NotFoundError exception mapping bug (commit eebe1f7) - Add 4th critical issue to executive summary - Include new test recommendations for exception handling - Update diff summary with both commits - Add benefits of NotFoundError fix Comprehensive review now covers all 4 critical/high priority fixes * fix: Critical security vulnerabilities - IDOR prevention and GLOBAL config protection CRITICAL FIX #1: Path Parameter Validation (IDOR Prevention) - Add validation that user_id in request body matches path parameter - Prevents users from creating configs under other users' paths - Applied to create_runtime_config() endpoint - Security logging for mismatch attempts - Returns 400 Bad Request with clear error message CRITICAL FIX #2: GLOBAL Config Authorization Protection - Add verify_global_config_authorization() helper function - Enforce admin-only modification of GLOBAL scope configs - Applied to update_runtime_config(), delete_runtime_config(), toggle_runtime_config() - Fetch config before modification to check scope - Security logging for unauthorized modification attempts - Returns 403 Forbidden for non-admin users Security Impact: - Prevents IDOR (Insecure Direct Object Reference) attacks - Prevents privilege escalation via GLOBAL config modification - Non-admins can no longer modify system-wide configurations - Comprehensive audit trail for all violations Addresses automated review feedback from PR #563 Related to PR #555 (Dynamic Configuration System) --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c183637 commit 16c75af

File tree

3 files changed

+722
-69
lines changed

3 files changed

+722
-69
lines changed

0 commit comments

Comments
 (0)