Skip to content

fix(issues): Assert user.id is not None before ORM filter #107744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JoshFerge merged 1 commit into from
Feb 6, 2026
Merged

fix(issues): Assert user.id is not None before ORM filter #107744

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions src/sentry/issues/endpoints/organization_group_search_views_starred.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ def get(self, request: Request, organization: Organization) -> Response:
Retrieve a list of starred views for the current organization member.
"""

assert request.user.id is not None
Copy link
Member

@shellmayr shellmayr Feb 6, 2026

Choose a reason for hiding this comment

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

We should probably return a 4XX error here, instead of an assert which will produce a 500

Copy link
Member Author

@JoshFerge JoshFerge Feb 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW it is actually impossible for our code to reach here with user id being none as this is an authenticated endpoint. just an annoyance that our type checker can't handle. can see we have a lot of these peppered throughout our endpoints.

Copy link
Member

@shellmayr shellmayr Feb 6, 2026

Choose a reason for hiding this comment

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

Get your point - I think it's fine, but in general I don't love it. We should find a better way of making sure our typing works in these cases

Copy link
Member Author

@JoshFerge JoshFerge Feb 6, 2026

Choose a reason for hiding this comment

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

yeah it is not great. I think that it has to do with django/drf typing stubs, where request.user can be User or AnonymousUser. I don't think it's an easy fix.

Copy link
Contributor

@sentry sentry bot Feb 6, 2026

Choose a reason for hiding this comment

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

Bug: The assertion assert request.user.id is not None will fail for valid API token requests that don't have an authenticated user, causing an unhandled AssertionError.
Severity: MEDIUM

Suggested Fix

Replace the assertion assert request.user.id is not None with an explicit check like if not request.user.is_authenticated: return Response(status=400). This ensures proper error handling for unauthenticated API requests, consistent with other similar endpoints in the codebase.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/issues/endpoints/organization_group_search_views_starred.py#L33

Potential issue: The endpoint allows access via API tokens with the `member:read` scope,
which does not require an authenticated user. In such cases, `request.user` is an
`AnonymousUser` and `request.user.id` is `None`. The `assert request.user.id is not
None` statement will then raise an `AssertionError`, leading to an unhandled server
exception instead of a proper HTTP error response. This is inconsistent with similar
endpoints that explicitly check for an authenticated user and return a 400 Bad Request
status.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Member Author

@JoshFerge JoshFerge Feb 6, 2026

Choose a reason for hiding this comment

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

i'm pretty sure api tokens do get user ids.

starred_views = GroupSearchViewStarred.objects.filter(
organization=organization, user_id=request.user.id
).select_related("group_search_view")
Expand Down
Loading