Repository navigation
fix(members): Don't use a pending invite's roles for a caller with no user - #129538
Merged
Merged
Conversation
github-actions
Bot
added
the
Scope: Backend
gricha
force-pushed
the
fix/none-lookup-memutils
branch
from
November 2, 2026 21:44
3a2b80f to
121422a
Compare
gricha
changed the base branch from
master
to
ref/mypy-request-user-principal
November 2, 2026 21:45
Contributor
|
No findings on the latest commit. |
gricha
force-pushed
the
fix/none-lookup-memutils
branch
from
November 2, 2026 22:08
121422a to
68636e6
Compare
Contributor
Sentry Snapshot Testing
|
gricha
force-pushed
the
fix/none-lookup-memutils
branch
from
November 2, 2026 22:16
68636e6 to
c5c8720
Compare
gricha
force-pushed
the
fix/none-lookup-memutils
branch
2 times, most recently
from
November 2, 2026 23:08
9071031 to
abec4ac
Compare
gricha
marked this pull request as ready for review
November 2, 2026 23:11
gricha
force-pushed
the
fix/none-lookup-memutils
branch
from
November 2, 2026 23:18
abec4ac to
cacda72
Compare
gricha
force-pushed
the
fix/none-lookup-memutils
branch
from
November 2, 2026 23:31
cacda72 to
d065838
Compare
gricha
force-pushed
the
fix/none-lookup-memutils
branch
from
November 2, 2026 23:42
d065838 to
02a7506
Compare
gricha
force-pushed
the
fix/none-lookup-memutils
branch
from
November 3, 2026 19:53
b77de02 to
b595ba1
Compare
gricha
force-pushed
the
fix/none-lookup-memutils
branch
2 times, most recently
from
November 3, 2026 20:09
3ed9758 to
f72ac90
Compare
azulus
approved these changes
Nov 3, 2026
gricha
force-pushed
the
fix/none-lookup-memutils
branch
from
November 3, 2026 20:51
f72ac90 to
fa60c6d
Compare
cvxluo
approved these changes
Nov 3, 2026
… user get_allowed_org_roles looked up the caller's member row by user_id=request.user.id. For an API key that's None, which matched pending invites: with one, the key could invite with that invitee's role limits; with several, the lookup raised MultipleObjectsReturned.
gricha
force-pushed
the
fix/none-lookup-memutils
branch
from
November 3, 2026 21:10
fa60c6d to
2d76232
Compare
gricha
added a commit
that referenced
this pull request
Nov 4, 2026
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.
gricha
added a commit
that referenced
this pull request
Nov 4, 2026
Service accounts can't invite, edit, remove or provision members. They can still list members. Drops the four member files from the `request.user` allowlist (5 → 1). - **Member detail endpoints** (`OrganizationMemberEndpoint`: member details, member teams, invite request details, SCIM member details) set `requires_user`. They already returned 404 to API keys; a service account now gets 403 `user-required`, also when reading a single member (it used to get 200). - **SCIM** (`SCIMEndpoint`: users, teams, schemas) sets `requires_user`. A service account with `member:write` could provision members through SCIM on an SCIM-enabled org; it now gets 403. SCIM's real caller is the SCIM token's proxy user, which is unaffected. - **Invites and invite requests** (`POST members/`, `POST invite-requests/`) call `require_user()` first and record the inviter from it. - A service account gets 403. Before, it got a role validation 400 for invites, or reached the write and failed on its `ServiceAccountId`. - A legacy API key never had an org role to invite with (every role was a 400); it now gets 401. Integration tokens still work, since their caller is the integration's proxy user. Production had no invite or invite request without a user in the last 30 days (0 of 5,760 sampled invites). The new service-account tests fail without this change: ``` test_service_account_cannot_read_member assert 200 == 403 test_service_account_cannot_invite assert 400 == 403 (role validation) test_service_account_cannot_request_invite assert 400 == 403 (role validation) test_service_account_cannot_provision_users assert 201 == 403 (SCIM created the member) ``` (`test_service_account_cannot_remove_member` already got 403 before, from the `ServiceAccountId` guard.) With it, the member index, details, team-details, invite-request and SCIM suites pass, and the endpoint parity matrix moves the member and SCIM rows to user-required (318 passed). The two API-key tests from #129538 now expect 401, and still check that nothing is created.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An API key with
member:admincould invite owners when the org had a pending owner invite.get_allowed_org_rolesfinds the caller's member row withuser_id=request.user.id. For an API key that'sNone, 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()raisedMultipleObjectsReturnedand 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 onlymember:invitecould 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.idNone-lookup mypy check in #129532. Stacked on the request.user stack, soorganization_member_utils.pyalso leaves #129402's request.user allowlist (23 to 22).Evidence: the two new tests fail on master:
With the fix:
2 passed, andtest_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):
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.OrganizationMember.MultipleObjectsReturnederrors on any member or invite-request route.