Repository navigation
fix(replays): Don't match pending invites for replay access with no user - #129540
Merged
Merged
Conversation
github-actions
Bot
added
the
Scope: Backend
gricha
force-pushed
the
fix/none-lookup-replayperm
branch
from
November 2, 2026 21:25
bb1ea48 to
f510d1a
Compare
gricha
force-pushed
the
fix/none-lookup-replayperm
branch
from
November 2, 2026 21:31
f510d1a to
ed8030b
Compare
gricha
force-pushed
the
fix/none-lookup-replayperm
branch
from
November 2, 2026 21:44
ed8030b to
6a9b0a1
Compare
gricha
force-pushed
the
fix/none-lookup-replayperm
branch
from
November 2, 2026 22:08
6a9b0a1 to
a8a72b2
Compare
gricha
force-pushed
the
fix/none-lookup-replayperm
branch
from
November 2, 2026 22:16
a8a72b2 to
b526ca8
Compare
Contributor
Sentry Snapshot Testing
|
gricha
force-pushed
the
fix/none-lookup-replayperm
branch
2 times, most recently
from
November 2, 2026 23:08
dccc8bc to
4863db0
Compare
gricha
marked this pull request as ready for review
November 2, 2026 23:11
gricha
force-pushed
the
fix/none-lookup-replayperm
branch
3 times, most recently
from
November 2, 2026 23:42
2aa09ee to
2a01137
Compare
gricha
force-pushed
the
fix/none-lookup-replayperm
branch
from
November 3, 2026 20:00
e6b41f7 to
90b1ac9
Compare
gricha
force-pushed
the
fix/none-lookup-replayperm
branch
from
November 3, 2026 20:09
90b1ac9 to
6de4675
Compare
azulus
approved these changes
Nov 3, 2026
gricha
force-pushed
the
fix/none-lookup-replayperm
branch
from
November 3, 2026 20:51
6de4675 to
1793b6e
Compare
cvxluo
approved these changes
Nov 3, 2026
gricha
force-pushed
the
fix/none-lookup-replayperm
branch
from
November 3, 2026 21:10
1793b6e to
464e2d8
Compare
gricha
force-pushed
the
fix/none-lookup-replayperm
branch
from
November 3, 2026 23:19
464e2d8 to
79d8b20
Compare
get_org_member looked the caller up by user_id=request.user.id. For an API key that's None, which matched pending invites: a pending invite on the replay allowlist granted the key access, and several pending invites raised MultipleObjectsReturned.
gricha
force-pushed
the
fix/none-lookup-replayperm
branch
from
November 4, 2026 00:44
79d8b20 to
0507444
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.
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.
With granular replay permissions on, API keys got the replay access of a pending invite, or a 500.
get_org_memberlooks the caller up withuser_id=request.user.id. For an API key that'sNone, which matched pending invites (member rows with no user). It runs on every replay endpoint oncesentry:granular-replay-permissionsis on:.get()raisesMultipleObjectsReturned, so every replay request from an API key 500s.remove_user()nullsuser_id) and keep their allowlist row.The lookup now goes through
owned_by_acting_user, which matches nothing without a user, so a caller with no user is treated as not a member, as it already is when there are no pending invites. Org auth tokens and Sentry apps are handled earlier and are unaffected, as are users.Found by the
request.user.idNone-lookup mypy check in #129532.Evidence: the two new tests fail on master:
With the fix:
2 passed, andtest_replay_granular_permissions.py+test_project_replay_jobs_delete.py:53 passed.Production check (Sentry's own telemetry, last 90 days; spans are sampled, counts below are sampled spans):
replays/andreplay-count/: about 126k sampled requests. Across all replay routes, requests without a user: no successful responses and no 500s; 1 sampled 403 onreplays/, the rest 401.OrganizationMember.MultipleObjectsReturnederrors on any replay route.replays/) could miss rare API-key use.