Initial commit: 9 security patterns for code review
Fundamentals: secure-defaults, input-validation, credential-handling, audit-logging Identity: authentication, authorization Attack Prevention: injection-prevention, dos-prevention, prompt-injection
This commit is contained in:
@@ -0,0 +1,134 @@
|
||||
# Authorization
|
||||
|
||||
## Rule
|
||||
|
||||
Verify permissions on every request. Default deny. Check at the resource, not just the route.
|
||||
|
||||
**Source:** [OWASP Authorization Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Authorization_Cheat_Sheet.html)
|
||||
|
||||
## Correct Pattern
|
||||
|
||||
```python
|
||||
from enum import Enum
|
||||
from functools import wraps
|
||||
|
||||
class Permission(Enum):
|
||||
READ = "read"
|
||||
WRITE = "write"
|
||||
DELETE = "delete"
|
||||
ADMIN = "admin"
|
||||
|
||||
def check_permission(user_id: str, resource_type: str,
|
||||
resource_id: str, permission: Permission) -> bool:
|
||||
"""Check if user has permission on specific resource."""
|
||||
# Get user's roles
|
||||
roles = get_user_roles(user_id)
|
||||
|
||||
# Check resource-level permissions
|
||||
resource_perms = get_resource_permissions(resource_type, resource_id)
|
||||
|
||||
for role in roles:
|
||||
if permission in resource_perms.get(role, []):
|
||||
return True
|
||||
|
||||
# Check ownership
|
||||
if get_resource_owner(resource_type, resource_id) == user_id:
|
||||
if permission in [Permission.READ, Permission.WRITE]:
|
||||
return True
|
||||
|
||||
return False # Default deny
|
||||
|
||||
def require_permission(resource_type: str, permission: Permission):
|
||||
"""Decorator to enforce authorization."""
|
||||
def decorator(func):
|
||||
@wraps(func)
|
||||
def wrapper(*args, **kwargs):
|
||||
user_id = get_current_user_id()
|
||||
resource_id = kwargs.get("resource_id") or args[0]
|
||||
|
||||
if not check_permission(user_id, resource_type, resource_id, permission):
|
||||
log_access(user_id, f"{resource_type}/{resource_id}",
|
||||
permission.value, allowed=False)
|
||||
raise PermissionDenied()
|
||||
|
||||
log_access(user_id, f"{resource_type}/{resource_id}",
|
||||
permission.value, allowed=True)
|
||||
return func(*args, **kwargs)
|
||||
return wrapper
|
||||
return decorator
|
||||
|
||||
@require_permission("document", Permission.READ)
|
||||
def get_document(resource_id: str):
|
||||
return Document.query.get(resource_id)
|
||||
```
|
||||
|
||||
## Incorrect Pattern
|
||||
|
||||
```python
|
||||
# Wrong: checking only authentication, not authorization
|
||||
@login_required
|
||||
def delete_document(doc_id):
|
||||
Document.query.get(doc_id).delete() # Any logged-in user can delete!
|
||||
|
||||
# Wrong: client-side only checks
|
||||
if user.role == "admin": # Checked in JavaScript only
|
||||
show_admin_panel()
|
||||
|
||||
# Wrong: IDOR vulnerability
|
||||
@app.route("/api/users/<user_id>/profile")
|
||||
def get_profile(user_id):
|
||||
return User.query.get(user_id).to_dict() # No ownership check!
|
||||
|
||||
# Wrong: relying on hidden URLs
|
||||
@app.route("/admin/secret/delete-all") # Security through obscurity
|
||||
def delete_all():
|
||||
...
|
||||
```
|
||||
|
||||
## IDOR Prevention
|
||||
|
||||
```python
|
||||
# Insecure Direct Object Reference - always verify ownership
|
||||
|
||||
# Wrong
|
||||
@app.route("/api/orders/<order_id>")
|
||||
def get_order(order_id):
|
||||
return Order.query.get(order_id) # Any user can view any order
|
||||
|
||||
# Correct
|
||||
@app.route("/api/orders/<order_id>")
|
||||
def get_order(order_id):
|
||||
order = Order.query.get(order_id)
|
||||
if order.user_id != current_user.id:
|
||||
if not current_user.has_permission("orders.view_all"):
|
||||
raise PermissionDenied()
|
||||
return order
|
||||
```
|
||||
|
||||
## Privilege Escalation Prevention
|
||||
|
||||
```python
|
||||
def update_user_role(actor_id: str, target_user_id: str, new_role: str):
|
||||
"""Prevent privilege escalation."""
|
||||
actor = get_user(actor_id)
|
||||
|
||||
# Can't grant roles higher than your own
|
||||
if ROLE_HIERARCHY[new_role] > ROLE_HIERARCHY[actor.role]:
|
||||
raise PermissionDenied("Cannot grant role higher than your own")
|
||||
|
||||
# Can't modify users with higher roles
|
||||
target = get_user(target_user_id)
|
||||
if ROLE_HIERARCHY[target.role] >= ROLE_HIERARCHY[actor.role]:
|
||||
raise PermissionDenied("Cannot modify user with equal or higher role")
|
||||
|
||||
target.role = new_role
|
||||
log_role_change(actor_id, target_user_id, target.role, new_role)
|
||||
```
|
||||
|
||||
## Edge Cases
|
||||
|
||||
- Time-of-check to time-of-use (TOCTOU) race conditions
|
||||
- Horizontal privilege escalation (user A accesses user B's data)
|
||||
- Vertical privilege escalation (user becomes admin)
|
||||
- Permission caching leading to stale authz
|
||||
- Implicit permissions from group membership
|
||||
Reference in New Issue
Block a user