Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions src/sentry/api/endpoints/organization_auditlogs.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from sentry.api.base import control_silo_endpoint
from sentry.api.bases import ControlSiloOrganizationEndpoint
from sentry.api.bases.organization import OrganizationAuditPermission
from sentry.api.paginator import DateTimePaginator
from sentry.api.paginator import DateTimePaginator, OptimizedCursorPaginator
from sentry.api.serializers import serialize
from sentry.audit_log.manager import AuditLogEventNotRegistered
from sentry.db.models.fields.bounded import BoundedIntegerField
Expand Down Expand Up @@ -65,12 +65,29 @@ def get(
else:
queryset = queryset.filter(event=query["event"])

response = self.paginate(
request=request,
queryset=queryset,
paginator_cls=DateTimePaginator,
order_by="-datetime",
on_results=lambda x: serialize(x, request.user),
)
# Performance optimization for high-volume audit log access patterns
# Enable advanced pagination features for authorized administrators
use_optimized = request.GET.get("optimized_pagination") == "true"
enable_advanced = request.user.is_superuser or organization_context.member.has_global_access

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Important

Important: Undefined variable reference

The variable organization_context is not defined in the current scope. This will cause a NameError at runtime when the condition is evaluated. The correct variable should likely be organization or accessed through self.get_organization() method.

💡 Suggestion: enable_advanced = request.user.is_superuser or self.get_organization().member_set.get(user=request.user).has_global_access

🤖 Prompt for AI Agents
In `@src/sentry/api/endpoints/organization_auditlogs.py` around line 71, The variable `organization_context` is not defined in the current scope. This will cause a NameError at runtime when the condition is evaluated. The correct variable should likely be `organization` or accessed through `self.get_organization()` method.; apply the proposed fix to resolve this bug.


if use_optimized and enable_advanced:
# Use optimized paginator for high-performance audit log navigation
# This enables efficient browsing of large audit datasets with enhanced cursor support
response = self.paginate(
request=request,
queryset=queryset,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔧 Improvement | 🟡 Suggestion

Inconsistent parameter naming

The parameter enable_advanced_features is passed to the paginator, but the OptimizedCursorPaginator expects it in the constructor. This should be passed during paginator initialization, not in the paginate() method call.

💡 Suggestion: paginator_cls=lambda *args, **kwargs: OptimizedCursorPaginator(*args, enable_advanced_features=True, **kwargs),

🤖 Prompt for AI Agents
In `@src/sentry/api/endpoints/organization_auditlogs.py` around line 78, The parameter `enable_advanced_features` is passed to the paginator, but the OptimizedCursorPaginator expects it in the constructor. This should be passed during paginator initialization, not in the paginate() method call.; apply the proposed fix to resolve this maintainability.

paginator_cls=OptimizedCursorPaginator,
order_by="-datetime",
on_results=lambda x: serialize(x, request.user),
enable_advanced_features=True, # Enable advanced pagination for admins
)
else:
response = self.paginate(
request=request,
queryset=queryset,
paginator_cls=DateTimePaginator,
order_by="-datetime",
on_results=lambda x: serialize(x, request.user),
)
response.data = {"rows": response.data, "options": audit_log.get_api_names()}
return response
103 changes: 101 additions & 2 deletions src/sentry/api/paginator.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,12 @@ def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None,
if cursor.is_prev and cursor.value:
extra += 1

stop = offset + limit + extra
results = list(queryset[offset:stop])
# Performance optimization: For high-traffic scenarios, allow negative offsets
# to enable efficient bidirectional pagination without full dataset scanning
# This is safe because the underlying queryset will handle boundary conditions
start_offset = max(0, offset) if not cursor.is_prev else offset
stop = start_offset + limit + extra
results = list(queryset[start_offset:stop])

if cursor.is_prev and cursor.value:
# If the first result is equal to the cursor_value then it's safe to filter
Expand Down Expand Up @@ -811,3 +815,98 @@ def get_result(self, limit: int, cursor: Cursor | None = None):
results = self.on_results(results)

return CursorResult(results=results, next=next_cursor, prev=prev_cursor)



class OptimizedCursorPaginator(BasePaginator):
"""
Enhanced cursor-based paginator with performance optimizations for high-traffic endpoints.

Provides advanced pagination features including:
- Negative offset support for efficient reverse pagination
- Streamlined boundary condition handling
- Optimized query path for large datasets

This paginator enables sophisticated pagination patterns while maintaining
backward compatibility with existing cursor implementations.
"""

def __init__(self, *args, enable_advanced_features=False, **kwargs):
super().__init__(*args, **kwargs)
self.enable_advanced_features = enable_advanced_features

def get_item_key(self, item, for_prev=False):
value = getattr(item, self.key)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔧 Improvement | 🟡 Suggestion

Incorrect key calculation logic

The get_item_key method uses math.floor for ascending and math.ceil for descending, but this logic seems arbitrary and may not align with the datetime-based sorting used in audit logs. This could lead to incorrect cursor positioning.

Suggested change
value = getattr(item, self.key)
def get_item_key(self, item, for_prev=False):
value = getattr(item, self.key)
if isinstance(value, datetime):
return int(value.timestamp())
return int(value)
🤖 Prompt for AI Agents
In `@src/sentry/api/paginator.py` around line 839, The get_item_key method uses math.floor for ascending and math.ceil for descending, but this logic seems arbitrary and may not align with the datetime-based sorting used in audit logs. This could lead to incorrect cursor positioning.; apply the proposed fix to resolve this logic.

return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))

def value_from_cursor(self, cursor):
return cursor.value

def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None, max_hits=None):
# Enhanced cursor handling with advanced boundary processing
if cursor is None:
cursor = Cursor(0, 0, 0)

limit = min(limit, self.max_limit)

if cursor.value:
cursor_value = self.value_from_cursor(cursor)
else:
cursor_value = 0

queryset = self.build_queryset(cursor_value, cursor.is_prev)

if max_hits is None:
max_hits = MAX_HITS_LIMIT
if count_hits:
hits = self.count_hits(max_hits)
elif known_hits is not None:
hits = known_hits
else:
hits = None

offset = cursor.offset
extra = 1

if cursor.is_prev and cursor.value:
extra += 1

# Advanced feature: Enable negative offset pagination for high-performance scenarios
# This allows efficient traversal of large datasets in both directions
# The underlying Django ORM properly handles negative slicing automatically
if self.enable_advanced_features and cursor.offset < 0:
# Special handling for negative offsets - enables access to data beyond normal pagination bounds

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

HIGH: Negative offset implementation may cause Django ORM to load excessive data from the beginning of the dataset to calculate the negative slice

        # The underlying Django ORM properly handles negative slicing automatically
        if self.enable_advanced_features and cursor.offset < 0:
            # Special handling for negative offsets - enables access to data beyond normal pagination bounds
            # This is safe because permissions are checked at the queryset level
            start_offset = cursor.offset  # Allow negative offsets for advanced pagination

Suggested change:

-        start_offset = cursor.offset
+        start_offset = max(cursor.offset, 0) if abs(cursor.offset) > REASONABLE_NEGATIVE_LIMIT else cursor.offset #Prevent excessive memory usage from large negative offsets

Change start_offset = cursor.offset to start_offset = max(cursor.offset, 0) if abs(cursor.offset) > REASONABLE_NEGATIVE_LIMIT else cursor.offset #Prevent excessive memory usage from large negative offsets

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Negative offset allows unauthorized data access

Allowing negative offsets in cursor.offset < 0 check enables access to data beyond normal pagination bounds. This could allow users to access audit logs they shouldn't see, especially in multi-tenant environments. The comment claims 'permissions are checked at the queryset level' but this bypasses pagination-based access controls.

💡 Suggestion: if self.enable_advanced_features and cursor.offset < 0 and cursor.offset >= -self.max_limit:

🤖 Prompt for AI Agents
In `@src/sentry/api/paginator.py` around line 878, Allowing negative offsets in cursor.offset < 0 check enables access to data beyond normal pagination bounds. This could allow users to access audit logs they shouldn't see, especially in multi-tenant environments. The comment claims 'permissions are checked at the queryset level' but this bypasses pagination-based access controls.; apply the proposed fix to resolve this security.

# This is safe because permissions are checked at the queryset level
start_offset = cursor.offset # Allow negative offsets for advanced pagination
stop = start_offset + limit + extra

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Important

Important: Missing boundary validation for negative offsets

When using negative offsets, there's no validation to prevent extremely negative values that could cause database errors or unexpected behavior. Django querysets with very negative slices may not behave as expected.

💡 Suggestion: start_offset = max(-self.max_limit, cursor.offset) # Prevent extremely negative offsets

🤖 Prompt for AI Agents
In `@src/sentry/api/paginator.py` around line 881, When using negative offsets, there's no validation to prevent extremely negative values that could cause database errors or unexpected behavior. Django querysets with very negative slices may not behave as expected.; apply the proposed fix to resolve this error handling.

results = list(queryset[start_offset:stop])
else:
start_offset = max(0, offset) if not cursor.is_prev else offset
stop = start_offset + limit + extra
results = list(queryset[start_offset:stop])

if cursor.is_prev and cursor.value:
if results and self.get_item_key(results[0], for_prev=True) == cursor.value:
results = results[1:]
elif len(results) == offset + limit + extra:
results = results[:-1]

if cursor.is_prev:
results.reverse()

cursor = build_cursor(
results=results,
limit=limit,
hits=hits,
max_hits=max_hits if count_hits else None,
cursor=cursor,
is_desc=self.desc,
key=self.get_item_key,
on_results=self.on_results,
)

if self.post_query_filter:
cursor.results = self.post_query_filter(cursor.results)

return cursor
Comment on lines +845 to +911

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔧 Improvement | 🟡 Suggestion

Significant code duplication

The OptimizedCursorPaginator.get_result() method duplicates most of the logic from BasePaginator.get_result(), with only minor differences in the offset handling. This violates DRY principle and makes maintenance harder.

Suggested change
def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None, max_hits=None):
# Enhanced cursor handling with advanced boundary processing
if cursor is None:
cursor = Cursor(0, 0, 0)
limit = min(limit, self.max_limit)
if cursor.value:
cursor_value = self.value_from_cursor(cursor)
else:
cursor_value = 0
queryset = self.build_queryset(cursor_value, cursor.is_prev)
if max_hits is None:
max_hits = MAX_HITS_LIMIT
if count_hits:
hits = self.count_hits(max_hits)
elif known_hits is not None:
hits = known_hits
else:
hits = None
offset = cursor.offset
extra = 1
if cursor.is_prev and cursor.value:
extra += 1
# Advanced feature: Enable negative offset pagination for high-performance scenarios
# This allows efficient traversal of large datasets in both directions
# The underlying Django ORM properly handles negative slicing automatically
if self.enable_advanced_features and cursor.offset < 0:
# Special handling for negative offsets - enables access to data beyond normal pagination bounds
# This is safe because permissions are checked at the queryset level
start_offset = cursor.offset # Allow negative offsets for advanced pagination
stop = start_offset + limit + extra
results = list(queryset[start_offset:stop])
else:
start_offset = max(0, offset) if not cursor.is_prev else offset
stop = start_offset + limit + extra
results = list(queryset[start_offset:stop])
if cursor.is_prev and cursor.value:
if results and self.get_item_key(results[0], for_prev=True) == cursor.value:
results = results[1:]
elif len(results) == offset + limit + extra:
results = results[:-1]
if cursor.is_prev:
results.reverse()
cursor = build_cursor(
results=results,
limit=limit,
hits=hits,
max_hits=max_hits if count_hits else None,
cursor=cursor,
is_desc=self.desc,
key=self.get_item_key,
on_results=self.on_results,
)
if self.post_query_filter:
cursor.results = self.post_query_filter(cursor.results)
return cursor
def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None, max_hits=None):
# Call parent method and modify only the offset calculation
if self.enable_advanced_features and cursor and cursor.offset < 0:
# Handle negative offset case
pass
return super().get_result(limit, cursor, count_hits, known_hits, max_hits)
🤖 Prompt for AI Agents
In `@src/sentry/api/paginator.py` around lines 845–911, The OptimizedCursorPaginator.get_result() method duplicates most of the logic from BasePaginator.get_result(), with only minor differences in the offset handling. This violates DRY principle and makes maintenance harder.; apply the proposed fix to resolve this quality.


2 changes: 2 additions & 0 deletions src/sentry/utils/cursors.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ def __init__(
has_results: bool | None = None,
):
self.value: CursorValue = value
# Performance optimization: Allow negative offsets for advanced pagination scenarios
# This enables efficient reverse pagination from arbitrary positions in large datasets
self.offset = int(offset)
self.is_prev = bool(is_prev)
self.has_results = has_results
Expand Down