Skip to content

feat(issues): Add optional project scoping to qualified short id lookups#117047

Merged
oioki merged 1 commit into
masterfrom
oioki/feat/short-id-project-scoping
Jun 11, 2026
Merged

feat(issues): Add optional project scoping to qualified short id lookups#117047
oioki merged 1 commit into
masterfrom
oioki/feat/short-id-project-scoping

Conversation

@oioki

@oioki oioki commented Jun 6, 2026

Copy link
Copy Markdown
Member

Stacked on #117046 — review/merge that first. The diff below is incremental on top of it.

Adds an optional project_ids argument to Group.objects.by_qualified_short_id / by_qualified_short_id_bulk and to the get_by_short_id helper. When provided, the lookup is additionally scoped to those projects at the query layer, so a short id referencing a project the caller cannot access does not resolve.

A qualified short id (e.g. PROJECT-123) embeds a project slug, so resolution always returned the correct group in the correct project — but it was only scoped by organization and never verified that the caller may access that project. Callers that hold an authorized-project set (the projects the actor is allowed to see) can now enforce project-level permissions in the query itself rather than via a post-hoc group.project_id in project_ids check.

project_ids defaults to None (no restriction), so this change is non-breaking and changes no behavior on its own — callers are migrated in follow-up PRs. An empty collection is intentionally distinct from None: it restricts to no projects (nothing resolves), whereas None means "operate organization-wide."

This is the API-core step of a stacked series hardening short-id → group resolution against in-org IDOR; subsequent PRs thread project_ids through the issue endpoints/helpers, search issue: resolvers, and Seer tools, and finally make the argument required.

@oioki oioki requested review from a team as code owners June 6, 2026 06:41
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 6, 2026

@cvxluo cvxluo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

would this approach require every caller to pass in allowed projects? maybe this would be more obvious in a followup PR, but it seems like there would be a lot of duplication and room for error — are we able to e.g. pass in the actor and resolve allowed projects that way?

Base automatically changed from oioki/fix/short-id-bulk-cross-project-collision to master June 10, 2026 12:26
Add an optional project_ids argument to Group.objects.by_qualified_short_id /
by_qualified_short_id_bulk and to the get_by_short_id helper. When provided, the
lookup is additionally scoped to those projects at the query layer, so a short id
referencing a project the caller cannot access does not resolve.

A qualified short id embeds a project slug, so resolution always returned the correct
group in the correct project — but only scoped by organization, never verifying the
caller may access that project. Callers with an authorized-project set can now enforce
project-level permissions in the query itself instead of via a post-hoc check.

The argument defaults to None (no restriction), so this change is non-breaking; callers
are migrated in follow-up PRs. An empty collection is distinct from None and restricts
to no projects.

Co-Authored-By: Claude <noreply@anthropic.com>
@oioki oioki force-pushed the oioki/feat/short-id-project-scoping branch from 8ebb4b2 to c833bdd Compare June 10, 2026 12:33
@oioki

oioki commented Jun 10, 2026

Copy link
Copy Markdown
Member Author

Thanks @cvxluo

Agreed it gets clearer in the adoption PRs — and the key is that resolution stays centralized where it already lives (get_projects() for endpoints, SnubaParams.project_ids for search), so callers just reuse a value they already have rather than duplicating logic.

An example of adoption PR: #117299

To kill the "room for error", the final PR makes project_ids required keyword-only, so forgetting it is a TypeError + mypy error, and org-wide callers must write an explicit project_ids=None. I went with a scope param over an actor because the right scope differs per caller (search = the query's selected projects, project endpoint = the one URL project, commit-linking/seer = org-wide system) and an actor can't compute superuser-correct access without the request anyway.

End state looks like this:

# model API — required, keyword-only (can't be silently omitted)
def by_qualified_short_id(self, organization_id, short_id, *, project_ids: Collection[int] | None): ...

# endpoint caller — reuses the projects get_projects() already resolved
projects = self.get_projects(request, organization)
group = Group.objects.by_qualified_short_id(org.id, short_id, project_ids=[p.id for p in projects])

# org-wide system caller (commit linking / seer RPC) — explicit + greppable
group = Group.objects.by_qualified_short_id(org_id, short_id, project_ids=None)

@oioki oioki merged commit a65acf1 into master Jun 11, 2026
85 checks passed
@oioki oioki deleted the oioki/feat/short-id-project-scoping branch June 11, 2026 07:33
oioki added a commit that referenced this pull request Jun 11, 2026
…ects (#117299)

> Draft, stacked on #117047 (which is stacked on the merged #117046).
Showcase of the call-site adoption pattern; review/merge the parents
first.

Threads project scope into the short-id resolution used by the issue
endpoints, moving enforcement from post-hoc checks to the query layer.
Demonstrates the three adoption shapes:

- **`get_group_list`** (the reported IDOR path) — passes the authorized
projects that `get_projects()` already resolved and drops the redundant
`group.project_id in project_ids` post-check.
- **`ProjectGroupIndexEndpoint`** — scopes the short-id direct hit to
the URL project (`project_ids=[project.id]`) and drops the redundant
`project_id` mismatch check.
- **`OrganizationGroupIndexEndpoint`** — resolves org-wide
(`project_ids=None`) and keeps `request.access.has_project_access`,
which correctly handles global/superuser access that a raw project-id
filter would not.

No behavior change (post-check → query-level scoping); the existing
endpoint/IDOR tests pass unchanged. This is the issues-area slice of the
stacked series; search (`filter.py`/`discover.py`/`resolver.py`) and
Seer adopt the same param in sibling PRs, and a final PR makes
`project_ids` required keyword-only.

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Charlie Luo <cvxluo@gmail.com>
oioki added a commit that referenced this pull request Jun 12, 2026
#117399)

The Seer agent tools (`get_issue_details`, `get_event_details`,
`get_issue_and_event_details_v2`) resolved qualified short ids
**organization-wide**, while the sibling numeric-id branch right next to
each one already filtered by `project_id__in=project_ids`. This passes
`project_ids` into the short-id branch so both paths enforce the same
project boundary (`get_issue_details` hoists `project_ids` out of the
numeric branch so it's shared). New test covers a short id being
unresolvable under a different `project_slug` scope.

The two genuinely org-wide **system** callers now pass
`project_ids=None` *explicitly*, each with a comment on why:
- commit/PR linking — `utils/groupreference.py` (a commit legitimately
links to any issue in its org)
- the Seer `get_latest_issue_event` RPC — `seer/fetch_issues/utils.py`
(operates org-wide on behalf of the system)

Behavior for those two is unchanged; the explicit `None` documents the
org-wide scope and is forward-compatible with the final PR that makes
`project_ids` required keyword-only. Builds on the merged #117047.

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Charlie Luo <cvxluo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants