Skip to content

ref(lint): Flag ORM lookups that may match NULL rows - #129532

Merged
gricha merged 1 commit into
fix/none-lookup-replaypermfrom
ref/mypy-none-lookup
Nov 4, 2026
Merged

gricha merged 1 commit into
fix/none-lookup-replaypermfrom
ref/mypy-none-lookup

Conversation

@gricha

@gricha gricha commented Nov 2, 2026 •

Copy link
Copy Markdown
Member

Adds lookupMayMatchNullRows to the decision-model linter's security suite.

An exact ORM lookup whose value can be None, like filter(user_id=request.user.id) for an API key, compiles to user_id IS NULL and matches rows with no user: pending invites, deleted users' exports, token-created rows. That caused #129441, #129442 and #129443, and the two fixes below this PR (#129538, #129540). Flake8 rule S032 only catches a literal acting_user_id(...) keyword, so it missed all of them.

The rule runs on changed files only, at warn. Its report says how to fix it: decide whether None means no rows (check for None first, or owned_by_acting_user() for the caller's own rows), no filter, or NULL rows (pass None or __isnull=True).

An earlier version of this PR was a mypy plugin. On top of the request.user stack it found 147 such lookups in 74 files; that list is the cleanup worklist, handled in follow-up PRs instead of an allowlist here.

Checked locally with this config: the full security suite in diff mode, as CI runs it, on 12 diffs. Seven add one of this week's bugs and five add lookups that are fine. The threshold is 0.7.

Diff adds P(TRUE) Flagged
Data export lookup by request.user.id (#129442) 0.81 yes
is_member projects by request.user.id (#129443) 0.83 yes
Member roles by request.user.id (#129538) 0.80 yes
Replay access by request.user.id (#129540) 0.79 yes
Team unsubscribe by member.user_id (#129600) 0.72 yes
Dashboard edit check (#129441) 0.51 no
Sentry app token rescope (#129602) 0.10 no
An __isnull=False guard 0.27 no
An application_id is None guard 0.14 no
dist_id=dist.id if dist else None 0.13 no
Global user options (project_id=None) 0.46 no
Derived pipeline_hash (commented as meant) 0.35 no

The two misses: #129441's None comes through an untyped parameter and a ==, and #129602 depends on SentryApp.application being nullable, which the diff doesn't show. A diff that only deletes a guard isn't checked either, since the linter asks about added lines. An earlier, more abstract wording flagged none of the seven.

Reviewer cases (re-measured Nov 2, changeset mode, full security suite):

Case Expected P(TRUE)
UserEmail.objects.filter(user_id=authenticator.user_id), NOT NULL column no warning 0.19
filter(project_id=project_id) where None means the global user option no warning 0.49
organization_service.check_membership_by_id(user_id=request.user.id), lookup behind a service call warning 0.22 (missed)

The two false-positive risks stay under the 0.7 threshold. A rewording that limits the rule to nullable columns, exempts "None means global" and mentions service calls dropped two real bugs below it (#129540 to 0.63, #129600 to 0.55) and still missed the service call. A lighter one kept 5 of 8 but left #129540 and #129600 at 0.72 and 0.70. So the wording stays; lookups behind service calls are a known gap.

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 2, 2026
@gricha
gricha force-pushed the ref/mypy-none-lookup branch from 02857ab to ad91bd2 Compare November 2, 2026 21:31
@gricha
gricha changed the base branch from master to fix/none-lookup-replayperm November 2, 2026 21:32
@gricha
gricha force-pushed the ref/mypy-none-lookup branch from ad91bd2 to 01ee7e3 Compare November 2, 2026 21:32
@gricha
gricha added this pull request to stack #129570 November 2, 2026 21:33
@gricha
gricha force-pushed the ref/mypy-none-lookup branch from 01ee7e3 to df3c8c4 Compare November 2, 2026 21:44
@gricha
gricha removed this pull request from stack #129570 November 2, 2026 21:45
@gricha
gricha added this pull request to stack #129572 November 2, 2026 21:45
@sentry

sentry Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Sentry Snapshot Testing

Name Added Removed Changed Renamed Unchanged Skipped Status
sentry-frontend
sentry-frontend
0 0 0 0 780 0 ✅ Unchanged

⚙️ sentry-frontend Snapshot Settings

@gricha
gricha force-pushed the ref/mypy-none-lookup branch 2 times, most recently from 12a5c55 to cbb07a4 Compare November 2, 2026 22:16
@gricha
gricha force-pushed the ref/mypy-none-lookup branch 2 times, most recently from 69cf385 to 769300e Compare November 2, 2026 22:51
@gricha gricha changed the title ref(typing): Flag ORM lookups whose value may be None ref(lint): Flag ORM lookups that may match NULL rows Nov 2, 2026
@github-actions github-actions Bot added Scope: Frontend Automatically applied to PRs that change frontend components and removed Scope: Backend Automatically applied to PRs that change backend components labels Nov 2, 2026
@gricha
gricha force-pushed the ref/mypy-none-lookup branch 2 times, most recently from 5de3e7c to 22905eb Compare November 2, 2026 23:08
@gricha
gricha marked this pull request as ready for review November 2, 2026 23:11
@gricha
gricha requested review from azulus and skaasten November 2, 2026 23:11
@gricha
gricha force-pushed the ref/mypy-none-lookup branch from 22905eb to d65b589 Compare November 2, 2026 23:18
@gricha
gricha requested review from a team as code owners November 3, 2026 20:00
@gricha
gricha force-pushed the ref/mypy-none-lookup branch 2 times, most recently from 2f56c3b to f52b457 Compare November 3, 2026 20:09

@azulus azulus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:bufo-hype:

@gricha
gricha force-pushed the ref/mypy-none-lookup branch from f52b457 to eb624bf Compare November 3, 2026 20:51
@gricha
gricha force-pushed the ref/mypy-none-lookup branch from eb624bf to 0dae3f6 Compare November 3, 2026 21:10
gricha added a commit that referenced this pull request Nov 3, 2026
… user (#129538)

An API key with `member:admin` could invite owners when the org had a
pending owner invite.

`get_allowed_org_roles` finds the caller's member row with
`user_id=request.user.id`. For an API key that's `None`, which matched
pending invites (member rows with no user yet). With one invite, the key
got that invitee's role limits, so a pending owner invite let it invite
owners. Pending owner invites occur without anyone inviting an owner:
when an org starts requiring 2FA, non-compliant members, owners
included, are turned back into pending invites. With several invites,
`.get()` raised `MultipleObjectsReturned` and the invite request 500'd.
The same helper backs the member details and invite-request endpoints.

The lookup now goes through `owned_by_acting_user`, which matches
nothing without a user, so a caller with no user falls through to the
existing no-member case (no roles), the same as when there are no
pending invites. Users are unaffected.

This also closes a team-restriction bypass in the invite POST. Its
"members can only invite into their own teams" check looks up the
requester's teams with `user_id=request.user.id`, so with one pending
invite a key with only `member:invite` could invite into that invite's
teams. That check is only reached when the caller has roles to invite
with, and after this fix a caller with no user has none, so it is
rejected before the team check.

Found by the `request.user.id` None-lookup mypy check in #129532.
Stacked on the request.user stack, so `organization_member_utils.py`
also leaves #129402's request.user allowlist (23 to 22).

Evidence: the two new tests fail on master:

```
E   assert 201 == 400       (response: {"email":"jane@gmail.com",...,"orgRole":"owner","pending":true,...})
E   assert 500 == 400       (MultipleObjectsReturned: get() returned more than one OrganizationMember -- it returned 2!)
FAILED ...::OrganizationMemberListPostTest::test_api_key_does_not_get_a_pending_invites_roles
FAILED ...::OrganizationMemberListPostTest::test_api_key_with_several_pending_invites
2 failed, 62 deselected
```

With the fix: `2 passed`, and `test_organization_member_index.py`,
`test_organization_member_details.py`,
`tests/sentry/models/test_organizationmember.py`: `182 passed`.

Production check (Sentry's own telemetry, last 90 days; spans are
sampled, counts below are sampled spans):

- Invite POST (`members/`): 215 sampled requests, none without a user.
Invite-request POST/PUT: 9 sampled, none without a user. Member details
PUT already rejects callers with no user.
- No `OrganizationMember.MultipleObjectsReturned` errors on any member
or invite-request route.
- Nothing seen relying on the current behavior. Sampling is about 1 in
200 on the invite POST, so rare API-key invites could be missed.
@gricha
gricha force-pushed the ref/mypy-none-lookup branch from 0dae3f6 to beed6a9 Compare November 3, 2026 23:19
Adds lookupMayMatchNullRows to the decision-model linter's security suite. An
exact lookup like filter(user_id=x) with x None means user_id IS NULL, which
matched pending invites and orphaned rows for callers with no user. The rule
warns on changed code and says how to decide what None should mean.
@gricha
gricha force-pushed the ref/mypy-none-lookup branch from beed6a9 to 8e53441 Compare November 4, 2026 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants