From d77957c670b15cf21646d6dfb8ef982de5c082fe Mon Sep 17 00:00:00 2001 From: Greg Pstrucha <875316+gricha@users.noreply.github.com> Date: Wed, 4 Feb 2026 14:41:54 -0800 Subject: [PATCH] feat(skills): Add Django access control and IDOR security review skill Introduces a new skill for reviewing Django codebases for access control vulnerabilities, with focus on IDOR detection. Unlike pattern-matching scanners, this skill emphasizes investigation-based review: understanding the authorization model, tracing data flows, and confirming gaps through code analysis. Includes reference docs for Django ORM, views, DRF permissions, and tenant isolation patterns. Co-Authored-By: Claude --- django-access-review/SKILL.md | 341 ++++++++++++++++++ django-access-review/references/django-orm.md | 71 ++++ .../references/django-views.md | 76 ++++ .../references/drf-permissions.md | 79 ++++ .../references/tenant-isolation.md | 67 ++++ 5 files changed, 634 insertions(+) create mode 100644 django-access-review/SKILL.md create mode 100644 django-access-review/references/django-orm.md create mode 100644 django-access-review/references/django-views.md create mode 100644 django-access-review/references/drf-permissions.md create mode 100644 django-access-review/references/tenant-isolation.md diff --git a/django-access-review/SKILL.md b/django-access-review/SKILL.md new file mode 100644 index 0000000..66b494a --- /dev/null +++ b/django-access-review/SKILL.md @@ -0,0 +1,341 @@ +--- +name: django-access-review +description: Django access control and IDOR security review. Use when reviewing Django views, DRF viewsets, ORM queries, or any Python/Django code handling user authorization. Trigger keywords: "IDOR", "access control", "authorization", "Django permissions", "object permissions", "tenant isolation", "broken access". +model: sonnet +allowed-tools: Read Grep Glob Bash Task +license: LICENSE +--- + + + +# Django Access Control & IDOR Review + +Find access control vulnerabilities by investigating how the codebase answers one question: + +**Can User A access, modify, or delete User B's data?** + +## Philosophy: Investigation Over Pattern Matching + +Do NOT scan for predefined vulnerable patterns. Instead: + +1. **Understand** how authorization works in THIS codebase +2. **Ask questions** about specific data flows +3. **Trace code** to find where (or if) access checks happen +4. **Report** only what you've confirmed through investigation + +Every codebase implements authorization differently. Your job is to understand this specific implementation, then find gaps. + +--- + +## Phase 1: Understand the Authorization Model + +Before looking for bugs, answer these questions about the codebase: + +### How is authorization enforced? + +Research the codebase to find: + +``` +□ Where are permission checks implemented? + - Decorators? (@login_required, @permission_required, custom?) + - Middleware? (TenantMiddleware, AuthorizationMiddleware?) + - Base classes? (BaseAPIView, TenantScopedViewSet?) + - Permission classes? (DRF permission_classes?) + - Custom mixins? (OwnershipMixin, TenantMixin?) + +□ How are queries scoped? + - Custom managers? (TenantManager, UserScopedManager?) + - get_queryset() overrides? + - Middleware that sets query context? + +□ What's the ownership model? + - Single user ownership? (document.owner_id) + - Organization/tenant ownership? (document.organization_id) + - Hierarchical? (org -> team -> user -> resource) + - Role-based within context? (org admin vs member) +``` + +### Investigation commands + +```bash +# Find how auth is typically done +grep -rn "permission_classes\|@login_required\|@permission_required" --include="*.py" | head -20 + +# Find base classes that views inherit from +grep -rn "class Base.*View\|class.*Mixin.*:" --include="*.py" | head -20 + +# Find custom managers +grep -rn "class.*Manager\|def get_queryset" --include="*.py" | head -20 + +# Find ownership fields on models +grep -rn "owner\|user_id\|organization\|tenant" --include="models.py" | head -30 +``` + +**Do not proceed until you understand the authorization model.** + +--- + +## Phase 2: Map the Attack Surface + +Identify endpoints that handle user-specific data: + +### What resources exist? + +``` +□ What models contain user data? +□ Which have ownership fields (owner_id, user_id, organization_id)? +□ Which are accessed via ID in URLs or request bodies? +``` + +### What operations are exposed? + +For each resource, map: +- List endpoints - what data is returned? +- Detail/retrieve endpoints - how is the object fetched? +- Create endpoints - who sets the owner? +- Update endpoints - can users modify others' data? +- Delete endpoints - can users delete others' data? +- Custom actions - what do they access? + +--- + +## Phase 3: Ask Questions and Investigate + +For each endpoint that handles user data, ask: + +### The Core Question + +**"If I'm User A and I know the ID of User B's resource, can I access it?"** + +Trace the code to answer this: + +``` +1. Where does the resource ID enter the system? + - URL path: /api/documents/{id}/ + - Query param: ?document_id=123 + - Request body: {"document_id": 123} + +2. Where is that ID used to fetch data? + - Find the ORM query or database call + +3. Between (1) and (2), what checks exist? + - Is the query scoped to current user? + - Is there an explicit ownership check? + - Is there a permission check on the object? + - Does a base class or mixin enforce access? + +4. If you can't find a check, is there one you missed? + - Check parent classes + - Check middleware + - Check managers + - Check decorators at URL level +``` + +### Follow-Up Questions + +``` +□ For list endpoints: Does the query filter to user's data, or return everything? + +□ For create endpoints: Who sets the owner - the server or the request? + +□ For bulk operations: Are they scoped to user's data? + +□ For related resources: If I can access a document, can I access its comments? + What if the document belongs to someone else? + +□ For tenant/org resources: Can User in Org A access Org B's data by changing + the org_id in the URL? +``` + +--- + +## Phase 4: Trace Specific Flows + +Pick a concrete endpoint and trace it completely. + +### Example Investigation + +``` +Endpoint: GET /api/documents/{pk}/ + +1. Find the view handling this URL + → DocumentViewSet.retrieve() in api/views.py + +2. Check what DocumentViewSet inherits from + → class DocumentViewSet(viewsets.ModelViewSet) + → No custom base class with authorization + +3. Check permission_classes + → permission_classes = [IsAuthenticated] + → Only checks login, not ownership + +4. Check get_queryset() + → def get_queryset(self): + → return Document.objects.all() + → Returns ALL documents! + +5. Check for has_object_permission() + → Not implemented + +6. Check retrieve() method + → Uses default, which calls get_object() + → get_object() uses get_queryset(), which returns all + +7. Conclusion: IDOR - Any authenticated user can access any document +``` + +### What to look for when tracing + +``` +Potential gap indicators (investigate further, don't auto-flag): +- get_queryset() returns .all() or filters without user +- Direct Model.objects.get(pk=pk) without ownership in query +- ID comes from request body for sensitive operations +- Permission class checks auth but not ownership +- No has_object_permission() and queryset isn't scoped + +Likely safe patterns (but verify the implementation): +- get_queryset() filters by request.user or user's org +- Custom permission class with has_object_permission() +- Base class that enforces scoping +- Manager that auto-filters +``` + +--- + +## Phase 5: Report Findings + +Only report issues you've confirmed through investigation. + +### Confidence Levels + +| Level | Meaning | Action | +|-------|---------|--------| +| **HIGH** | Traced the flow, confirmed no check exists | Report with evidence | +| **MEDIUM** | Check may exist but couldn't confirm | Note for manual verification | +| **LOW** | Theoretical, likely mitigated | Do not report | + +### Suggested Fixes Must Enforce, Not Document + +**Bad fix**: Adding a comment saying "caller must validate permissions" +**Good fix**: Adding code that actually validates permissions + +A comment or docstring does not enforce authorization. Your suggested fix must include actual code that: +- Validates the user has permission before proceeding +- Raises an exception or returns an error if unauthorized +- Makes unauthorized access impossible, not just discouraged + +Example of a BAD fix suggestion: +```python +def get_resource(resource_id): + # IMPORTANT: Caller must ensure user has access to this resource + return Resource.objects.get(pk=resource_id) +``` + +Example of a GOOD fix suggestion: +```python +def get_resource(resource_id, user): + resource = Resource.objects.get(pk=resource_id) + if resource.owner_id != user.id: + raise PermissionDenied("Access denied") + return resource +``` + +If you can't determine the right enforcement mechanism, say so - but never suggest documentation as the fix. + +### Report Format + +```markdown +## Access Control Review: [Component] + +### Authorization Model +[Brief description of how this codebase handles authorization] + +### Findings + +#### [IDOR-001] [Title] (Severity: High/Medium) +- **Location**: `path/to/file.py:123` +- **Confidence**: High - confirmed through code tracing +- **The Question**: Can User A access User B's documents? +- **Investigation**: + 1. Traced GET /api/documents/{pk}/ to DocumentViewSet + 2. Checked get_queryset() - returns Document.objects.all() + 3. Checked permission_classes - only IsAuthenticated + 4. Checked for has_object_permission() - not implemented + 5. Verified no relevant middleware or base class checks +- **Evidence**: [Code snippet showing the gap] +- **Impact**: Any authenticated user can read any document by ID +- **Suggested Fix**: [Code that enforces authorization - NOT a comment] + +### Needs Manual Verification +[Issues where authorization exists but couldn't confirm effectiveness] + +### Areas Not Reviewed +[Endpoints or flows not covered in this review] +``` + +--- + +## Common Django Authorization Patterns + +These are patterns you might find - not a checklist to match against. + +### Query Scoping +```python +# Scoped to user +Document.objects.filter(owner=request.user) + +# Scoped to organization +Document.objects.filter(organization=request.user.organization) + +# Using a custom manager +Document.objects.for_user(request.user) # Investigate what this does +``` + +### Permission Enforcement +```python +# DRF permission classes +permission_classes = [IsAuthenticated, IsOwner] + +# Custom has_object_permission +def has_object_permission(self, request, view, obj): + return obj.owner == request.user + +# Django decorators +@permission_required('app.view_document') + +# Manual checks +if document.owner != request.user: + raise PermissionDenied() +``` + +### Ownership Assignment +```python +# Server-side (safe) +def perform_create(self, serializer): + serializer.save(owner=self.request.user) + +# From request (investigate) +serializer.save(**request.data) # Does request.data include owner? +``` + +--- + +## Investigation Checklist + +Use this to guide your review, not as a pass/fail checklist: + +``` +□ I understand how authorization is typically implemented in this codebase +□ I've identified the ownership model (user, org, tenant, etc.) +□ I've mapped the key endpoints that handle user data +□ For each sensitive endpoint, I've traced the flow and asked: + - Where does the ID come from? + - Where is data fetched? + - What checks exist between input and data access? +□ I've verified my findings by checking parent classes and middleware +□ I've only reported issues I've confirmed through investigation +``` diff --git a/django-access-review/references/django-orm.md b/django-access-review/references/django-orm.md new file mode 100644 index 0000000..8a72fc9 --- /dev/null +++ b/django-access-review/references/django-orm.md @@ -0,0 +1,71 @@ +# Django ORM - Context for Investigation + +How data access works in Django. Use this to understand query patterns you encounter. + +## Where Scoping Can Happen + +### Custom Managers + +Projects may have managers that auto-filter: + +```python +class TenantManager(models.Manager): + def get_queryset(self): + return super().get_queryset().filter(tenant=get_current_tenant()) + +class Document(models.Model): + objects = TenantManager() # All queries auto-scoped + unscoped = models.Manager() # Escape hatch for admin +``` + +**Key**: If you see `Model.objects.all()`, check if `objects` is a custom manager. + +### Middleware + +Some projects set context that managers use: + +```python +# Middleware might set thread-local tenant +_thread_locals.tenant = request.user.tenant + +# Manager reads it +def get_queryset(self): + return super().get_queryset().filter(tenant=_thread_locals.tenant) +``` + +## Query Patterns to Understand + +### Direct Fetch + +```python +# Fetches by ID only - no user scoping +Document.objects.get(pk=pk) + +# Includes user in query - scoped +Document.objects.get(pk=pk, owner=request.user) +``` + +### Filtered Fetch + +```python +# Unscoped - returns everything matching +Document.objects.filter(status='active') + +# Scoped - only user's documents +Document.objects.filter(status='active', owner=request.user) +``` + +### Related Objects + +```python +# If document is scoped but comments aren't... +document = Document.objects.get(pk=pk, owner=request.user) +comments = document.comments.all() # Are these comments scoped? +``` + +## Questions When Investigating Queries + +1. Is this using a custom manager that auto-scopes? +2. Is there middleware setting context the manager uses? +3. Does the query include the current user/tenant? +4. For related queries, is the parent object properly scoped? diff --git a/django-access-review/references/django-views.md b/django-access-review/references/django-views.md new file mode 100644 index 0000000..77d5bb8 --- /dev/null +++ b/django-access-review/references/django-views.md @@ -0,0 +1,76 @@ +# Django Views - Context for Investigation + +This is background context to help you understand Django authorization patterns when investigating. Not a checklist. + +## Where Authorization Can Happen + +When tracing a request, check these layers: + +``` +URL conf → Middleware → View decorators → View class → Method → Query +``` + +### URL-Level + +```python +# urls.py - decorators applied at routing +from django.contrib.admin.views.decorators import staff_member_required + +urlpatterns = [ + path('admin/', staff_member_required(admin_view)), +] +``` + +### Middleware + +```python +# settings.py MIDDLEWARE list +# Look for custom auth middleware that might set user context or enforce checks +``` + +### View Decorators + +```python +@login_required +@permission_required('app.view_document') +@user_passes_test(lambda u: u.is_staff) +``` + +### CBV Mixins + +```python +# Check the ENTIRE inheritance chain +class MyView(LoginRequiredMixin, PermissionRequiredMixin, DetailView): + ... + +# Also check for project-specific base classes +class MyView(BaseCompanyView, DetailView): + # What does BaseCompanyView do? +``` + +### View Methods + +```python +# get_queryset() - often where scoping happens +# get_object() - may have custom logic +# dispatch() - sometimes has permission checks +``` + +## DRF-Specific Layers + +```python +# Permission classes - check what they actually do +permission_classes = [IsAuthenticated, IsOwner] + +# get_queryset() - critical for scoping +def get_queryset(self): + return Model.objects.filter(...) + +# has_object_permission() - called by get_object() +def has_object_permission(self, request, view, obj): + return obj.owner == request.user +``` + +## Key Insight + +`has_object_permission()` is only called when `get_object()` is called. List views don't trigger it - they need `get_queryset()` scoping. diff --git a/django-access-review/references/drf-permissions.md b/django-access-review/references/drf-permissions.md new file mode 100644 index 0000000..55dbcc8 --- /dev/null +++ b/django-access-review/references/drf-permissions.md @@ -0,0 +1,79 @@ +# DRF Permissions - Context for Investigation + +Background on how DRF handles permissions. Use this to understand what you're seeing, not as patterns to match. + +## How DRF Permission Flow Works + +``` +Request → permission_classes.has_permission() → View method → get_object() → has_object_permission() +``` + +### has_permission() + +- Called on EVERY request +- Checked BEFORE the view method runs +- Good for "is user authenticated?" or "is user admin?" +- NOT good for "does user own this specific object?" + +### has_object_permission() + +- Only called when `self.get_object()` is called +- NOT called for list views (no specific object) +- This is where object-level checks can happen + +**Critical**: If a view does `Model.objects.get(pk=pk)` directly instead of `self.get_object()`, the `has_object_permission()` is NEVER called. + +## Things to Check When Investigating + +### 1. What's in DEFAULT_PERMISSION_CLASSES? + +```python +# settings.py +REST_FRAMEWORK = { + 'DEFAULT_PERMISSION_CLASSES': [...] +} +``` + +This applies to ALL views unless overridden. + +### 2. What does each permission class actually do? + +Don't assume from the name. Read the class: + +```python +# IsAuthenticated only checks login, not ownership +# DjangoModelPermissions checks model-level perms, not object-level +# Custom classes - read the implementation +``` + +### 3. How is data fetched? + +```python +# Uses get_object() - permissions apply +instance = self.get_object() + +# Direct query - permissions DON'T apply +instance = Model.objects.get(pk=pk) +``` + +### 4. What's in get_queryset()? + +This determines what objects are even reachable: + +```python +def get_queryset(self): + return Model.objects.all() # Everything + return Model.objects.filter(owner=self.request.user) # Scoped +``` + +## Serializer Considerations + +Serializers control what fields are readable/writable: + +```python +class Meta: + fields = '__all__' # What's included? + read_only_fields = [...] # What can't be set? +``` + +Key question: Can the client set the owner field, or is it server-controlled? diff --git a/django-access-review/references/tenant-isolation.md b/django-access-review/references/tenant-isolation.md new file mode 100644 index 0000000..c0b3275 --- /dev/null +++ b/django-access-review/references/tenant-isolation.md @@ -0,0 +1,67 @@ +# Multi-Tenant Isolation - Context for Investigation + +Background on multi-tenant architectures. Use this to understand the ownership model you're investigating. + +## Ownership Hierarchy + +Most apps have layered ownership: + +``` +Organization/Tenant + └── Team (optional) + └── User + └── Resource +``` + +**Key question**: At which level is authorization enforced? + +## Common Implementation Patterns + +### Tenant from Session + +```python +# User's current tenant stored in session/request +request.user.organization +request.user.current_tenant +``` + +### Tenant from URL + +```python +# URL: /orgs/{org_id}/projects/ +# Question: Is user verified as member of this org? +``` + +### Automatic Scoping + +```python +# Middleware sets tenant context +# Manager auto-filters by current tenant +# All queries implicitly scoped +``` + +## Questions When Investigating + +1. **How is tenant determined?** + - From authenticated user's profile? + - From URL parameter? + - From request header? + +2. **If from URL, is membership validated?** + - Can user access /orgs/999/ if they're not in org 999? + +3. **Are all queries scoped to tenant?** + - Check for auto-scoping managers + - Check for explicit tenant filters + +4. **Can user switch context to another tenant?** + - If yes, is that switch validated? + +## The Core Multi-Tenant Question + +**"Can a user in Organization A access data belonging to Organization B?"** + +Trace the code to answer this. Check: +- Where org context comes from +- Whether membership is validated +- Whether queries are scoped to that org