Skip to content

Commit bf1a93c

Browse files
committed
Address review: dedicated DoesNotExist handler, stable error msgs, CHANGELOG split
- CreateLabelForLabelsetMutation now matches Remove's exception handling: explicit LabelSet.DoesNotExist branch logs at WARNING (no stack trace) and falls through to a clean 'matching query does not exist' message, while the catch-all uses logger.exception() for genuine errors. - Both mutations now build the deny-path message from a stable string instead of instantiating a DoesNotExist purely to call its __str__ (Django version coupling). - Split the long CHANGELOG entry into two focused bullets, one per mutation, matching the per-fix style of the surrounding entries.
1 parent 4181b86 commit bf1a93c

2 files changed

Lines changed: 17 additions & 2 deletions

File tree

CHANGELOG.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1919

2020
### Security
2121

22-
- **Labelset-membership mutations now enforce the documented edit-rights model** (`config/graphql/label_mutations.py`, `CreateLabelForLabelsetMutation` and `RemoveLabelsFromLabelsetMutation`): Both resolvers previously made the wrong scope call. `Remove` used `Q(creator=user) | Q(is_public=True)` with no further check, so any logged-in user could strip labels from another user's *public* labelset. `Create` was creator-only, so a collaborator 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. Both now fetch the labelset by pk and gate mutation on `user_has_permission_for_obj(user, labelset, PermissionTypes.UPDATE, include_group_permissions=True)`, raising `LabelSet.DoesNotExist` on denial so the response message and code path are identical to the not-found case (no IDOR information leak). Test coverage in `opencontractserver/tests/test_label_mutations.py`: `test_remove_labels_rejects_non_owner_even_when_labelset_is_public` (replaces the prior permissive test) confirms `is_public` alone does not grant write; `test_remove_labels_allows_non_owner_with_explicit_update_permission` pins the edit-rights model. The unused `Q` import was removed.
22+
- **`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`.
23+
- **`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.
2324
- **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:
2425
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.
2526
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

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,20 @@ def mutate(
282282
message = "SUCCESS"
283283
logger.debug("Done")
284284

285+
except LabelSet.DoesNotExist:
286+
# Legitimate auth rejection or genuine 404 — log without a stack
287+
# trace to avoid polluting logs with what looks like real errors.
288+
logger.warning(
289+
"CreateLabelForLabelsetMutation: labelset not found or "
290+
"permission denied (labelset_id=%s)",
291+
labelset_id,
292+
)
293+
message = (
294+
"Failed to create label for labelset due to error: "
295+
"LabelSet matching query does not exist."
296+
)
285297
except Exception as e:
298+
logger.exception("CreateLabelForLabelsetMutation failed")
286299
message = f"Failed to create label for labelset due to error: {e}"
287300

288301
return CreateLabelForLabelsetMutation(
@@ -336,7 +349,8 @@ def mutate(root, info, label_ids, labelset_id):
336349
labelset_id,
337350
)
338351
message = (
339-
f"Error removing label(s) from labelset: {LabelSet.DoesNotExist()}"
352+
"Error removing label(s) from labelset: "
353+
"LabelSet matching query does not exist."
340354
)
341355
except Exception as e:
342356
logger.exception("RemoveLabelsFromLabelsetMutation failed")

0 commit comments

Comments
 (0)