Merge remote-tracking branch 'origin/main' into pr-1239-clean

Author: JSv4

CHANGELOG.md

@@ -65,6 +65,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Security
 
+- **`RemoveAnnotationLabelsFromLabelsetMutation` allowed any user to strip labels from public labelsets** (`config/graphql/label_mutations.py`): The resolver used `Q(creator=user) | Q(is_public=True)` with no further check, so `is_public` (a read flag) effectively granted write access to non-owners. Replaced with `user_has_permission_for_obj(user, labelset, PermissionTypes.UPDATE, include_group_permissions=True)`. Denial raises `LabelSet.DoesNotExist` so the response is byte-identical to the not-found case (no IDOR information leak). Coverage: `test_remove_labels_rejects_non_owner_even_when_labelset_is_public`.
+- **`CreateLabelForLabelsetMutation` ignored guardian UPDATE grants** (`config/graphql/label_mutations.py`): The resolver was creator-only, so collaborators with explicit `update_labelset` guardian permission (or a group grant) could not add labels even though the consolidated permissioning guide says edit rights on a `LabelSet` should permit add/remove/edit of its labels. Now uses the same `PermissionTypes.UPDATE` gate as `Remove`, with the matching IDOR-safe deny path. Coverage: `test_non_owner_with_explicit_update_permission_can_create`. Both mutations now have a dedicated `LabelSet.DoesNotExist` handler that logs at WARNING (not ERROR) so auth probes don't pollute logs with stack traces.
 - **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
 
@@ -20,7 +19,10 @@
 from config.graphql.validation_utils import validate_color
 from opencontractserver.annotations.models import AnnotationLabel, LabelSet
 from opencontractserver.types.enums import PermissionTypes
-from opencontractserver.utils.permissioning import set_permissions_for_obj_to_user
+from opencontractserver.utils.permissioning import (
+    set_permissions_for_obj_to_user,
+    user_has_permission_for_obj,
+)
 
 logger = logging.getLogger(__name__)
 
@@ -221,32 +223,69 @@ class Arguments:
 
     @login_required
     def mutate(
-        root, info, labelset_id, text, description, color, icon, label_type
+        root,
+        info,
+        labelset_id,
+        text=None,
+        description=None,
+        color=None,
+        icon=None,
+        label_type=None,
     ) -> "CreateLabelForLabelsetMutation":
 
         ok = False
         obj = None
         obj_id = None
 
-        # Validate color format (defense in depth)
-        is_valid_color, color_error = validate_color(color)
-        if not is_valid_color:
-            return CreateLabelForLabelsetMutation(
-                obj=None, obj_id=None, message=color_error, ok=False
-            )
-
         try:
-            labelset = LabelSet.objects.get(
-                pk=from_global_id(labelset_id)[1], creator=info.context.user
-            )
+            # Permission check runs before validation so a non-owner cannot
+            # distinguish "reached validation" from "denied" via different
+            # error messages (IDOR mitigation — see
+            # docs/permissioning/consolidated_permissioning_guide.md).
+            labelset = LabelSet.objects.get(pk=from_global_id(labelset_id)[1])
+            if not user_has_permission_for_obj(
+                info.context.user,
+                labelset,
+                PermissionTypes.UPDATE,
+                include_group_permissions=True,
+            ):
+                raise LabelSet.DoesNotExist()
+
+            # Reject blank text explicitly: Django's ``blank=False`` is
+            # form-only and ``objects.create()`` would silently apply the
+            # "Text Label" model default.
+            if not (text and text.strip()):
+                return CreateLabelForLabelsetMutation(
+                    obj=None,
+                    obj_id=None,
+                    message="Label text is required and cannot be blank.",
+                    ok=False,
+                )
+
+            if color == "":
+                color = None
+            is_valid_color, color_error = validate_color(color)
+            if not is_valid_color:
+                return CreateLabelForLabelsetMutation(
+                    obj=None, obj_id=None, message=color_error, ok=False
+                )
+
             logger.debug("CreateLabelForLabelsetMutation - mutate / Labelset", labelset)
+            # Drop None/"" so model field defaults apply rather than
+            # writing blank values at the DB level.
+            create_kwargs = {
+                k: v
+                for k, v in {
+                    "text": text,
+                    "description": description,
+                    "color": color,
+                    "icon": icon,
+                    "label_type": label_type,
+                }.items()
+                if v is not None and v != ""
+            }
             obj = AnnotationLabel.objects.create(
-                text=text,
-                description=description,
-                color=color,
-                icon=icon,
-                label_type=label_type,
-                creator=info.context.user,
+                creator=info.context.user, **create_kwargs
             )
             obj_id = to_global_id("AnnotationLabelType", obj.id)
             logger.debug("CreateLabelForLabelsetMutation - mutate / Created label", obj)
@@ -263,7 +302,19 @@ def mutate(
             message = "SUCCESS"
             logger.debug("Done")
 
+        except LabelSet.DoesNotExist:
+            # Auth rejection or genuine 404 — warn without stack trace.
+            logger.warning(
+                "CreateLabelForLabelsetMutation: labelset not found or "
+                "permission denied (labelset_id=%s)",
+                labelset_id,
+            )
+            message = (
+                "Failed to create label for labelset due to error: "
+                "LabelSet matching query does not exist."
+            )
         except Exception as e:
+            logger.exception("CreateLabelForLabelsetMutation failed")
             message = f"Failed to create label for labelset due to error: {e}"
 
         return CreateLabelForLabelsetMutation(
@@ -297,14 +348,29 @@ def mutate(
             label_pks = list(
                 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))
-            )
+            labelset = LabelSet.objects.get(pk=from_global_id(labelset_id)[1])
+            if not user_has_permission_for_obj(
+                user,
+                labelset,
+                PermissionTypes.UPDATE,
+                include_group_permissions=True,
+            ):
+                raise LabelSet.DoesNotExist()
             labelset.annotation_labels.remove(*label_pks)
             ok = True
             message = "Success"
 
+        except LabelSet.DoesNotExist:
+            # Auth rejection or genuine 404 — warn without stack trace.
+            logger.warning(
+                "RemoveLabelsFromLabelsetMutation: labelset not found or "
+                "permission denied (labelset_id=%s)",
+                labelset_id,
+            )
+            message = (
+                "Error removing label(s) from labelset: "
+                "LabelSet matching query does not exist."
+            )
         except Exception as e:
             logger.exception("RemoveLabelsFromLabelsetMutation failed")
             message = f"Error removing label(s) from labelset: {e}"