Close review gaps in graphene→strawberry migration (FK-traversal IDOR, rate bucket, served-validation test)

[JSv4]

Stacks on top of the migration branch (claude/graphene-strawberry-migration-eyvj57, PR #2139) and closes the gaps found reviewing that migration. Base is the migration branch so this diff is only the fixes.

1. High — singular FK-object traversal lost its visibility filter (systematic)

graphene-django auto-converted a to-one FK whose target type overrode get_queryset into a permission-filtered resolver (convert_field_to_djangomodel → target.get_node), so an invisible FK target resolved to null. The strawberry port declared these as plain getattr fields, leaking the target row's fields across a permission boundary. Node/connection/list paths already funnel through apply_type_get_queryset; singular FK fields never did.

• Added config/graphql/core/relay.py::resolve_visible_fk, which applies the target type's registered get_node/get_queryset hook (returns None for an invisible/missing/malformed target).
• Routed the affected nullable FK fields through it across annotation/agent/conversation/corpus/document/extract/research/social/user types — including the confirmed-exploitable edges: AnnotationType.corpus, RelationshipType.corpus/document, CorpusReferenceType.targetDocument/targetCorpus/targetAnnotation, ConversationType.chatWithCorpus/chatWithDocument, MessageType.sourceDocument, DocumentType.parent/sourceDocument, CorpusType.parent/memoryDocument.
• Non-null DocumentPathType.document can't resolve to null, so its leak (DocumentPath membership is corpus-as-gate — a public corpus lists paths for its private documents too) is closed at the list level: _get_queryset_DocumentPathType now enforces MIN(document, corpus), matching CorpusType.documents (issue Reconcile CorpusObjsService corpus-as-gate vs GraphQL MIN-permission semantic #1682).
• FK edges whose source visibility already implies READ on the target (NoteType.*, DocumentRelationshipType.*, corpus-gated *.corpus, same-parent parent) are intentionally left as fast plain fields — filtering them is a no-op that only adds per-row queries.

2. Major — served-schema validation wiring was untested

Depth-limiting and prod introspection-blocking are enforced by the AddValidationRules extension in schema.py, but the existing tests only call graphql-core's validate(..., validation_rules) against the exported (decorative) list — they'd stay green if the extension were dropped. Added TestServedSchemaExecutesValidationRules, which drives a too-deep query and an introspection query through schema.execute_sync (the real extension path).

3. Minor

• drf_mutation/drf_deletion now pass group="mutate" — the ratelimit decorator was wrapping a lambda, so the ~9 DRF-routed mutations bucketed under "<lambda>" instead of the shared "mutate" write counter (roughly doubling combined write budget). No guard was dropped.
• get_node_from_global_id wraps the get_node hook path in the same malformed-pk guard as the default path, so int(pk) hooks return the unified not-found instead of a raw ValueError.
• Declared graphql-relay==3.2.0 explicitly (imported directly by ~20 modules, previously only transitive via django-graphql-jwt).
• Removed the dead ALLOW_GRAPHQL_DEBUG setting; corrected the stale MessageType relay docstring + migration doc, which described singular node resolution as unfiltered despite the shipped IDOR hook.

Verification

Run in-container against the strawberry schema:

• test_fk_visibility_traversal (new, 10 tests) — both resolve_visible_fk branches, the DocumentPath MIN filter, and an end-to-end CorpusType.parent → null for a non-owner.
• test_schema_parity — green (zero schema-shape change from the ~32 field conversions).
• test_singular_node_idor, served-validation classes — green.
• Regression: test_document_versioning_graphql, test_corpus_cards_structural_document_resolution, test_versioning_paths_audit, test_mentions — 95 tests green.
• black, isort, flake8 clean; no inline Tier-0 introduced (E001 unaffected).

Two changelog fragments added (2139-fk-traversal-idor.security.md, 2139-review-followups.fixed.md).

---

Generated by Claude Code

[claude]

Review

This PR does solid, well-tested follow-up work on the graphene→strawberry migration: it identifies a real and serious bug class (singular FK fields skipping the target type's permission hook), adds resolve_visible_fk as a clean, well-documented fix, covers the served-schema validation-wiring gap, and fixes a rate-limit bucketing bug. The new tests (test_fk_visibility_traversal.py) are well-targeted — they hit both branches of the helper (get_queryset and get_node), the null/malformed edge cases, the DocumentPath MIN-filter, and one end-to-end schema query. The writeup in the PR description and changelog fragments is excellent (exact repro reasoning, before/after semantics, issue references).

Major: the FK-traversal fix is incomplete — several fields with the same bug remain unfiltered

The PR is careful to call out which excluded fields are deliberate no-ops (NoteType.*, DocumentRelationshipType.*, "corpus-gated *.corpus", "same-parent parent") — and I spot-checked those and they check out (e.g. DocumentPathType.corpus/CorpusFolderType.corpus/CorpusFolderType.parent are genuinely no-ops because the row's own get_queryset already gates on corpus/tree visibility; MessageType.conversation is a no-op because MessageType has no get_queryset of its own — you only ever reach a message by first traversing a visible conversation).

But there's a large second group of plain FK fields that point at types with real, documented, per-object permission checks (get_node hooks explicitly labeled "IDOR guard" in their own docstrings: AnalysisService.check_analysis_permission, ExtractService.check_extract_permission, BaseService.get_or_none for Fieldset/Column/Datacell/AnnotationLabel/AgentConfiguration) and are not mentioned in either the "fixed" or "excluded" lists:

• config/graphql/annotation_types.py:375-391 — AnnotationType.analysis, .created_by_analysis, .created_by_extract (plain fields → AnalysisType/ExtractType, both gated by check_analysis_permission/check_extract_permission). This is exactly the traversal CLAUDE.md's own "Common Pitfalls" §2 calls out as privacy-sensitive ("annotation/relationship source privacy... dereferences created_by_analysis/created_by_extract").
• config/graphql/annotation_types.py:1986-1999 — same pattern on RelationshipType.analyzer/.analysis/.created_by_analysis/.created_by_extract.
• config/graphql/annotation_types.py:349 — AnnotationType.annotation_label / :1744 RelationshipType.relationship_label → AnnotationLabelType (its own get_node docstring literally says "IDOR guard").
• config/graphql/extract_types.py:1713-1714 — DatacellType.extract/.column → ExtractType/ColumnType. DatacellType's own get_node docstring says its purpose is so "extraction results no longer leak across corpora/documents the caller cannot access" — but .extract/.column on the datacell itself still leak exactly that.
• config/graphql/extract_types.py:728,740 — ExtractType.fieldset, .parent_extract; :1558 ColumnType.fieldset.
• config/graphql/agent_types.py:78-91 — CorpusActionType.corpus/.fieldset/.analyzer/.agent_config, and :647,654 CorpusActionExecutionType.extract/.analysis — notably in the same file where sibling fields (CorpusActionExecutionType.document/.conversation, AgentConfigurationType.corpus) were correctly converted in this very PR.
• config/graphql/document_types.py:2790,2793 — DocumentAnalysisRowType.analysis/.extract.
• config/graphql/annotation_types.py:2126,2195 — CorpusReferenceType.source_annotation/.created_by_analysis (note: CorpusReferenceType.target_annotation was fixed a few lines below in the diff — source_annotation sits right next to it, unconverted).

Lower-confidence/narrower cases also worth a look: ResearchReportType.corpus (research_types.py:112) — the type's docstring says visibility is "creator-only," but a creator can lose corpus access after the fact (the same "current permission ≠ permission-at-creation-time" drift the CorpusReferenceType fix in this PR explicitly targets for cross-corpus enrichment); and AssignmentType.document (user_types.py:4488) — the sibling .corpus field was fixed in this PR, but .document is non-nullable so it can't use resolve_visible_fk directly (same shape as DocumentPathType.document, which got a list-level MIN filter instead — AssignmentType has no equivalent).

None of this is a knock on the approach — resolve_visible_fk is the right fix — it's that the sweep looks like it was done by grepping for a subset of type names rather than exhaustively walking every plain FK field against the type registry's get_node/get_queryset hooks. Worth a follow-up pass (possibly a static check, similar in spirit to the existing test_singular_node_idor / opencontracts.E001 enforcement, that fails when a plain FK field's target type has a registered hook it doesn't route through).

Minor

• config/graphql/core/relay.py:259-267 — the new try/except (ValueError, TypeError, OverflowError) around entry.get_node(info, _pk) in get_node_from_global_id wraps the entire hook call, not just the int(pk) cast. If a hook's internal business logic (e.g. AnalysisService.check_analysis_permission) raises one of these for an unrelated reason, it now silently becomes "not found" instead of surfacing the real error. Probably fine given the hooks are simple today, but worth a narrower int(pk)-only guard if any hook grows more complex logic later.
• resolve_visible_fk rebuilds the full target get_queryset (e.g. CorpusType's, which does a select_related plus a per-row vote Subquery annotation) on every single FK dereference, with no caching/dataloader batching — so resolving .corpus across a list of N annotations issues N independently-built, non-trivial queries. This reproduces graphene's original per-row behavior (not a regression), and the PR already opts out of it where it's a pure no-op, but it's a real N+1 cost on hot paths (e.g. an annotations connection where clients select corpus) that might be worth a dataloader in a later pass.

Nice-to-haves confirmed correct

• The rate-limit group fix (group="mutate" on the two DRF-mutation lambdas) is a real, non-obvious bug fix — good catch.
• TestServedSchemaExecutesValidationRules closing the "asserts against the exported list, not the wired extension" gap is a good test-of-tests addition.
• graphql-relay==3.2.0 pin and ALLOW_GRAPHQL_DEBUG removal are safe, low-risk cleanup.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!