Skip to content

Fixes #22470: Restrict TaggedItem API to visible target objects#22505

Open
pheus wants to merge 1 commit into
mainfrom
22470-tagged-objects-rest-api-should-respect-target-object-view
Open

Fixes #22470: Restrict TaggedItem API to visible target objects#22505
pheus wants to merge 1 commit into
mainfrom
22470-tagged-objects-rest-api-should-respect-target-object-view

Conversation

@pheus

@pheus pheus commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Closes: #22470

This updates the TaggedItem API to respect object-level view permissions for each tagged object.

Tagged items are now filtered against the permissions of their target objects, preventing users from discovering objects they are not allowed to view through the tags endpoint. It also handles missing or deleted tagged targets more safely during serialization.

@pheus

pheus commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @pheus's task in 4m 22s —— View job


PR Review: Restrict TaggedItem API to visible target objects

  • Read changed files
  • Analyze for potential concerns
  • Post review findings

Concerns

1. Silent exclusion of plugin-tagged objects — utilities/permissions.py:199-201

if not hasattr(target_queryset, 'restrict'):
    # Fail closed: target model has no object-level permission enforcement.
    continue

Plugin models that tag objects but whose manager doesn't use RestrictedQuerySet will have their tagged items silently excluded from API results. This is a behavioral change for any plugin that uses tagging. It's intentional ("fail closed"), but it's a breaking change for those plugins and should be noted in the release notes or upgrade guide, not only in a code comment. Plugin authors also need to know they must now extend RestrictedQuerySet if their models are tagable and they want them to appear in the tagged-objects API.


2. Unnecessary subquery for EXEMPT_VIEW_PERMISSIONS models — utilities/permissions.py:202

When a model is listed in EXEMPT_VIEW_PERMISSIONS, restrict() returns self (the full queryset). The resulting Q clause becomes:

content_type_id = X AND object_id IN (SELECT pk FROM <exempt_model_table>)

This is logically equivalent to content_type_id = X but incurs a full-table subquery scan. A pre-check of permission_is_exempt(get_permission_for_model(model, action)) before calling restrict() could replace it with a simpler Q(content_type_id=ct_id) condition. Not a bug, but in deployments with EXEMPT_VIEW_PERMISSIONS = ['*'] this generates a subquery per content type that serves no filtering purpose. Fix this →


3. content_type_ids evaluated separately from final filter — utilities/permissions.py:189

content_type_ids = queryset.order_by().values_list(content_type_id_field, flat=True).distinct()

This executes as a separate DB query (accounts for the +1 in query_counts.json). Under READ COMMITTED isolation (PostgreSQL default), a TaggedItem row inserted for a content type not present in content_type_ids's snapshot will be absent from the final filter's subquery conditions and so excluded from results. This is a benign TOCTOU — the user refreshing would fix it — but worth being aware of.


4. get_serializer_for_model fallback behavior changed — extras/api/serializers_/tags.py:61-64

Before this PR, if content_object resolved to None, get_serializer_for_model(None) would likely raise an unhandled exception. The new early-return is correct. However, the old fallback to obj.object_repr for a SerializerNotFound is preserved — that branch now only fires when content_object is not None but has no serializer. The logic is sound, just worth confirming the two code paths are still intentionally distinct.


Add permission filtering to TaggedItemViewSet to prevent disclosure of
objects outside user's view permissions. Filter queryset by target model
permissions using restrict_queryset_by_gfk utility, which honors
object-level constraints. Add None check in serializer for missing or
deleted targets.

Fixes #22470
@pheus pheus force-pushed the 22470-tagged-objects-rest-api-should-respect-target-object-view branch from e93f637 to bafca24 Compare June 22, 2026 14:05
@pheus

pheus commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

@claude /review

  1. Addressed with a docs note. I kept the fail-closed behavior because if a plugin model has no restrict(), we can’t safely check object-level permissions.
  2. Fixed. Exempt models now match by content_type_id directly, avoiding the no-op subquery. Added test coverage for specific and blanket exemptions.
  3. Left as-is. The race can only cause under-inclusion for one response, not data exposure.
  4. Left as-is. The fallback paths are intentional, and the new None guard avoids a possible 500.

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @pheus's task in 58s —— View job


PR Review (follow-up)


All four responses check out.

#1 — Docs note (docs/plugins/development/models.md:165-170): Well-placed directly under the TagsMixin API ref. Covers the behavior, root cause, workaround, and safe path. Sufficient.

#2 — Exempt-model optimization (utilities/permissions.py:199-203): The early-return correctly emits Q(content_type_id=ct_id) without a subquery for both the specific-model and blanket-* cases. Test coverage at lines 869-895 exercises both. The call chain (get_permission_for_modelpermission_is_exempt) is consistent with the rest of the codebase.

#3 — TOCTOU race: Under-inclusion only, not over-inclusion — correct call to leave it.

#4 — Serializer None guard: The three branches (None target → None, no serializer → object_repr, serializer found → serialized data) are distinct and intentional. The new guard prevents a potential 500 without changing the other paths.

No new concerns.

@pheus pheus marked this pull request as ready for review June 22, 2026 14:18
@pheus pheus requested review from a team and arthanson and removed request for a team June 22, 2026 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tagged-objects REST API should respect target object view permissions

1 participant