Skip to content
This repository was archived by the owner on Sep 3, 2025. It is now read-only.

Conversation

@mvilanova
Copy link
Contributor

Summary

I have successfully implemented the security fixes for the critical access control vulnerability identified. Here's a comprehensive overview of the changes made:

🔒 Critical Security Fixes Implemented

1. Fixed restricted_incident_filter Function

File: src/dispatch/database/service.py (lines 809-826)

Previous vulnerability: Only applied restrictions if role == UserRoles.member, allowing bypass for users with None roles or any unhandled roles.

Fix implemented:

  • Default Deny Approach: Now restricts access for ALL roles except explicit admin roles
  • Admin Bypass: UserRoles.admin, UserRoles.owner, and UserRoles.manager get unrestricted access
  • Secure Filtering: All other roles (including UserRoles.member, None, and any unhandled roles) now have restrictive filters applied
  • Proper Join Logic: Changed from join to outerjoin with explicit IndividualContact.id == Participant.individual_contact_id to handle cases where users aren't participants

2. Fixed restricted_case_filter Function

File: src/dispatch/database/service.py (lines 841-858)

Applied identical security fixes as the incident filter:

  • Default deny behavior for all non-admin roles
  • Proper access control enforcement
  • Secure participant-based filtering

3. Secured Individual User Endpoint

File: src/dispatch/auth/views.py (lines 125-140)

Previous vulnerability: GET /{organization}/users/{user_id} had no access control, allowing unauthorized user enumeration.

Fix implemented:

  • Added Permission Dependency: Now requires OrganizationMemberPermission
  • Consistent Authorization: Matches the security model of the user list endpoint (GET /{organization}/users)
  • Prevents User Enumeration: Unauthorized users can no longer iterate through user IDs to extract user information

🧪 Comprehensive Test Coverage

File: tests/database/test_service.py

Added security-focused tests to verify:

  • Admin roles (owner, manager, admin) have unrestricted access
  • Non-admin roles (including None roles) have restrictive filters applied
  • Both incident and case filtering work correctly across all role scenarios

🛡️ Security Impact

Before the fix:

  • Users with role: null (non-members) could access ALL restricted incidents/cases across ANY organization
  • User enumeration was possible via unprotected individual user endpoint
  • "Default allow" behavior created organization-wide privilege escalation

After the fix:

  • Default Deny: Only explicitly authorized admin roles can see restricted data
  • Multi-tenant Isolation: Proper organization-level access control enforced
  • No User Enumeration: Individual user endpoint now requires proper membership
  • Comprehensive Coverage: Fixes apply to both incidents and cases

🔍 Technical Details

Key Changes:

  1. Role-based Access Control: Explicit allow-list for admin roles (admin, owner, manager)
  2. Defensive Filtering: outerjoin with explicit foreign key relationships
  3. Visibility Logic: Proper handling of Visibility.open vs Visibility.restricted
  4. Participant-based Access: Users can only see restricted items they're directly involved in

The fixes maintain backward compatibility while closing the critical security vulnerability.

@mvilanova mvilanova self-assigned this Jul 25, 2025
@mvilanova mvilanova added bug Something isn't working security labels Jul 25, 2025

This comment was marked as outdated.

@mvilanova mvilanova requested a review from Copilot July 29, 2025 18:07
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses critical authorization bypass and information disclosure vulnerabilities in the Dispatch application. The fixes implement a "default deny" security approach to prevent unauthorized access to restricted incidents and cases, while also securing user enumeration endpoints.

  • Replaces vulnerable role-based filtering logic that allowed bypass for users with null or unhandled roles
  • Implements proper access control for individual user endpoints to prevent unauthorized user enumeration
  • Adds comprehensive test coverage to verify security fixes work correctly across all role scenarios

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/dispatch/database/service.py Core security fixes - implements default deny approach for incident/case filtering and removes debug output
src/dispatch/auth/views.py Secures individual user endpoint with proper permission dependency
tests/database/test_service.py Adds comprehensive security-focused test coverage for filtering functions
tests/static/e2e/pages/auth-page.ts Enhances E2E test reliability with better error handling and retry logic
pyproject.toml Updates Ruff configuration structure for newer versions
playwright.config.ts Improves CI environment navigation timeout handling

mvilanova and others added 3 commits July 29, 2025 11:10
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Marc Vilanova <39573146+mvilanova@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Marc Vilanova <39573146+mvilanova@users.noreply.github.com>
Signed-off-by: Marc Vilanova <39573146+mvilanova@users.noreply.github.com>
@mvilanova mvilanova merged commit eb238bb into main Jul 29, 2025
14 checks passed
@mvilanova mvilanova deleted the sec/multi-tenant-authz-bypass branch July 29, 2025 18:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants