Restrict RemoveLabelsFromLabelsetMutation to creator-only

The resolver previously fetched the labelset with `Q(creator=user) | Q(is_public=True)`
and removed the requested labels with no further authorization check, allowing any
authenticated user to mutate another user's public labelset. is_public exposes a
labelset for use, not for mutation, and the companion CreateLabelForLabelsetMutation
is already creator-only -- so the asymmetry was a real bug, not a design choice.

Tightened the lookup to pk + creator, dropped the now-unused Q import, and inverted
the regression test that previously pinned the permissive behavior.

Author: claude

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Security
 
+- **`RemoveLabelsFromLabelsetMutation` allowed any authenticated user to mutate public labelsets** (`config/graphql/label_mutations.py:283-303`): The resolver fetched the target labelset with `Q(creator=user) | Q(is_public=True)` and then unconditionally called `labelset.annotation_labels.remove(*label_pks)`. `is_public` is intended to expose a labelset for **use**, not for mutation, and the companion `CreateLabelForLabelsetMutation` (line 234) was already creator-only — so any logged-in user could strip labels from another user's public labelset, breaking shared-labelset workflows. Restricted the lookup to `pk=..., creator=user`, removed the now-unused `Q` import, and inverted the regression test (`opencontractserver/tests/test_label_mutations.py:174` — renamed to `test_remove_labels_rejects_non_owner_even_for_public_labelset`) so the rejection path is pinned going forward.
 - **Cross-corpus structural-annotation leak in `CoreAnnotationVectorStore`** (`opencontractserver/llms/vector_stores/core_vector_stores.py:296-326,371-413`): The corpus-wide retrieval path (`corpus_id` set, `document_id=None`) returned every structural annotation in the database regardless of corpus. Two collaborating defects caused the leak:
   1. `Q(structural=True)` in the corpus-only branch had **no corpus constraint** — parser-produced structural annotations have `Annotation.document_id = corpus_id = NULL`, so corpus membership is only knowable through `structural_set → Document.structural_annotation_set (reverse FK) → DocumentPath.corpus_id`, a join the previous code did not perform.
   2. The `check_corpus_deletion` block (default `True`) added `Q(document_id__in=active_doc_ids)`, and `__in` lookups never match `NULL`, so structural annotations were silently dropped on the production-default path. Bypassing this filter with `check_corpus_deletion=False` exposed defect #1 directly.

config/graphql/label_mutations.py

@@ -8,7 +8,6 @@
 import graphene
 from django.conf import settings
 from django.core.files.base import ContentFile
-from django.db.models import Q
 from graphql_jwt.decorators import login_required
 from graphql_relay import from_global_id, to_global_id
 
@@ -290,8 +289,7 @@ def mutate(root, info, label_ids, labelset_id):
                 map(lambda graphene_id: from_global_id(graphene_id)[1], label_ids)
             )
             labelset = LabelSet.objects.get(
-                Q(pk=from_global_id(labelset_id)[1])
-                & (Q(creator=user) | Q(is_public=True))
+                pk=from_global_id(labelset_id)[1], creator=user
             )
             labelset.annotation_labels.remove(*label_pks)
             ok = True

opencontractserver/tests/test_label_mutations.py

@@ -171,12 +171,12 @@ def test_remove_labels_empty_list_is_noop(self) -> None:
         remaining = set(self.labelset.annotation_labels.values_list("pk", flat=True))
         self.assertEqual(remaining, {self.label_a.pk, self.label_b.pk, self.label_c.pk})
 
-    def test_remove_labels_allows_public_labelset(self) -> None:
-        """A public labelset is editable via this mutation by any authed user.
+    def test_remove_labels_rejects_non_owner_even_for_public_labelset(self) -> None:
+        """Public labelsets are read-only via this mutation for non-creators.
 
-        This pins the current resolver behaviour (``Q(creator=user) | Q(is_public=True)``)
-        so that future permission hardening is an intentional change rather than an
-        accidental regression.
+        ``is_public`` exposes a labelset for use, not for mutation. Only the
+        creator may remove labels from it; matches the creator-only lookup in
+        ``CreateLabelForLabelsetMutation``.
         """
 
         self.labelset.is_public = True
@@ -191,6 +191,9 @@ def test_remove_labels_allows_public_labelset(self) -> None:
 
         self.assertIsNone(result.get("errors"))
         data = result["data"]["removeAnnotationLabelsFromLabelset"]
-        self.assertTrue(data["ok"])
+        self.assertFalse(data["ok"])
+        self.assertIn("Error removing label(s) from labelset", data["message"])
+
+        # Nothing should have changed
         remaining = set(self.labelset.annotation_labels.values_list("pk", flat=True))
-        self.assertEqual(remaining, {self.label_b.pk, self.label_c.pk})
+        self.assertEqual(remaining, {self.label_a.pk, self.label_b.pk, self.label_c.pk})