fix(access-control): apply write filter to delete-search (XFV-120)

[yogsototh]

Summary

delete-search (ES _delete_by_query) built its query body with the read
access-control filter (find-restriction-query-part). Under the default
ctia.access-control.max-record-visibility=everyone, that filter also matches
any TLP white/green document regardless of owner or group. As a result a caller
could delete another group's TLP green/white records that it was only
permitted to read, not write — a cross-tenant authorization bypass on the
bulk delete endpoint.

Related: XFV-120

Fix

• Add find-write-restriction-query-part (src/ctia/stores/es/query.clj). It
  mirrors the disjuncts of ctia.domain.access-control/allow-write? (owner,
  authorized_users, authorized_groups, same-group TLP amber-or-below,
  same-group owner TLP red) and omits the everyone/public-TLP clause that
  the read filter adds. The shared disjuncts are factored into a private
  write-restriction-should-clauses, so the read filter is byte-identical to
  before (it still conses the public-TLP clause under everyone).
• Thread an :access-control option through make-search-query
  (src/ctia/stores/es/crud.clj); handle-delete-search now requests
  :write. The dry-run count is computed with the same write filter, so
  the preview matches what an actual delete would remove.
• The DELETE /search routes (generic crud + feed) always go through
  delete-search, passing :really-delete? derived from
  REALLY_DELETE_ALL_THESE_ENTITIES. The non-destructive request returns the
  write-filtered match count instead of the broader read count.

The new write filter is a strict subset of the old read filter, so the
change can only ever delete fewer documents — it cannot widen access.

Tests

• New handle-delete-search-write-access-control-test
  (test/ctia/stores/es/crud_test.clj): under everyone, a foreign group can
  READ another group's green records but its delete-search (dry-run and
  destructive) matches/deletes 0, the owner's docs survive, and the owner
  can still delete its own.
• search-access-control-test gains an optional player-2-expected-delete-list
  (defaults to the read list; diverges only under everyone), with the
  TLP-green case asserting a foreign group can read but not delete a third
  group's green record.

Local ES7 suite (all green):

• ctia.stores.es.crud-test — 664 assertions, 0 failures
• ctia.entity.sighting-test — 292 assertions, 0 failures
• ctia.entity.feed-test — 176 assertions, 0 failures
• ctia.domain.access-control-test — 120 assertions, 0 failures

🤖 Generated with Claude Code

[ereteog]

Verdict: COMMENTED — 0 CRITICAL, 0 HIGH, 3 MEDIUM, 4 LOW

Write-access-control filter for ES delete-by-query so a caller can only delete what it may write, not merely read (XFV-120). Reviewed description-blind across six specialty passes (code, tests, silent-failure, types, comments, simplify) with every finding re-verified against the PR head. The core fix is correct and well-built: I traced find-write-restriction-query-part against ctia.domain.access-control/allow-write? disjunct-for-disjunct, confirmed the write filter is a strict subset of the read filter (so the change can only ever delete fewer documents), confirmed the read filter output is byte-identical to before, and confirmed the regression test fails if the write filter is reverted to the read filter. Nothing blocks. The three MEDIUMs are hardening/coverage, not defects: (1) the :access-control dispatch fails open to the read filter on an unknown value; (2) the store delete-search default silently became a no-op; (3) the dry-run write-filter count is unguarded end-to-end.

MEDIUM

• [S1] — Access-control dispatch fails open to the read filter. make-search-query's case sends any non-:write value (typo, wrong type, nil) to the broader read filter (src/ctia/stores/es/crud.clj:609), so a future delete-path caller that mistypes the option would silently delete against read visibility, re-opening XFV-120. No current caller triggers it — handle-delete-search passes the literal :write — so this is fail-open hardening, not a live bug (inline fix on the line).
• [B3] — Store delete-search default silently became a no-op. Deletion now happens only when the caller passes :really-delete? true; otherwise handle-delete-search returns nb-matched (src/ctia/stores/es/crud.clj:672), so a downstream IQueryStringSearchableStore/delete-search caller relying on the previous always-deletes contract gets a plausible non-zero count with nothing deleted. All in-repo callers are updated, so this is latent. Suggested fix: document the :really-delete? contract on the protocol method (src/ctia/store.clj:35).
• [T1] — Dry-run write-filter count is unguarded end-to-end. The route change's stated purpose is that the REALLY_DELETE_ALL_THESE_ENTITIES=false count now uses the write filter, but the green search-access-control-test only issues the destructive delete and test-delete-search uses a single owner where read==write (test/ctia/test_helpers/access_control.clj:557); reverting the dry-run route to query-string-count would pass every test. Suggested fix: add a foreign-caller dry-run DELETE asserting the body equals the write-filtered count.

LOW / non-blocking follow-ups (4)

• [doc/O1] — the TLP-red should clause is fully subsumed by the owner clause (owner+groups already matches at any TLP), and the docstrings list "same-group owner records at TLP red" as an allow-write? disjunct, which has no red branch — its owner rule is TLP-agnostic (src/ctia/stores/es/query.clj:20, :35).
• [type] — handle-delete-search now serves both the count and the delete and is ok'd by both routes but has no return schema, unlike its sibling handle-query-string-count :- (s/pred nat-int?) (src/ctia/stores/es/crud.clj:654).
• [observability] — pre-existing: the destructive branch reads only :deleted, dropping _delete_by_query :failures/:version_conflicts, and in async mode (route default wait_for_completion=false) returns the pre-count as the deleted count (src/ctia/stores/es/crud.clj:678); the touched expression is a good place to log partial failures and document the async semantics.
• [tests] — coverage follow-ups: no owner (non-zero) store-level dry-run assertion; the feed DELETE /search route is untested (:delete-search-tests? false); no direct unit test for find-write-restriction-query-part (its authorized_users/authorized_groups disjuncts are never exercised on the write path); confirm no other entity test relies on cross-identity purge now that the write filter is enforced.

PASS

• Security (core) — find-write-restriction-query-part mirrors ctia.domain.access-control/allow-write? disjunct-for-disjunct (owner, authorized_users, authorized_groups, tlp ∈ {white,green,amber}+group; the red+owner clause is subsumed by owner), and is a strict subset of the read filter, so the change can only ever delete fewer documents. The read filter output is byte-identical to before the refactor.
• Correctness — the dry-run preview and the destructive delete run through one write-filtered query, so they cannot drift; every non-delete make-search-query caller (handle-query-string-search, handle-query-string-count, aggregation) keeps the :read default and is unaffected.
• Tests — the core regression is well-guarded: negative control (foreign group → 0, owner's docs survive) and positive control (owner → full count), and the read-first assertion (foreign reads all 60 under everyone) makes the zero-delete non-vacuous.
• Cross-cutting — purge-incidents! correctly migrated to a store-level owner-identity delete so the test suite keeps purging under the tighter filter.

Intent pass

Code matches the stated scope: the write filter is a strict subset of the read filter (verified), only delete-search opts into :write, and read/count/aggregate paths are untouched — no scope creep, and the "can only ever delete fewer documents" claim holds. The one inherent consequence is that a dry-run now returns the write-deletable count, so a caller who can read but not delete sees 0 (see the LOW observability note); that is the intended security tradeoff, not a mismatch.

---

Reviewed via /review-contributor-pr — verify mode: high; agents: code-reviewer, silent-failure-hunter, pr-test-analyzer, type-design-analyzer, comment-analyzer, code-simplifier. Layer-0 findings dropped: 0. Short-PR heuristic: off. Rubric: RUBRIC.md @ ccf75440 (no upstream criteria file in ctia — drift check skipped). Not approved: --approve low requires max severity ≤ LOW, but 3 MEDIUM findings survived, so the event resolved to COMMENT.

[ereteog]

[S1] The case selects the broader read filter for any :access-control value other than the literal :write (a typo, a string, or nil), so a future delete-path caller that mistypes the option silently deletes against read visibility, re-opening the XFV-120 cross-group delete this PR closes. No current caller triggers it — handle-delete-search passes :write — so this is fail-open hardening, not a live bug. Suggested fix: make the dispatch fail closed so an unknown value throws instead of widening:

(case access-control
  :write (es.query/find-write-restriction-query-part ident)
  :read  (es.query/find-restriction-query-part ident get-in-config))

[ereteog]

[doc] This TLP-red should clause is fully subsumed by the owner clause above — owner+groups already matches the document at any TLP, including red — and the shared docstring lists "same-group owner records at TLP red" as a disjunct of allow-write?, which has no red branch (its owner rule is TLP-agnostic). Harmless: the effective match set still equals allow-write?. But a maintainer checking parity will look for a red branch and not find one. Suggested fix: drop the red clause and describe the owner rule as TLP-independent in the docstring.

[ereteog]

[type] handle-delete-search now serves both the dry-run count and the destructive delete and is ok'd by both routes, but unlike its sibling handle-query-string-count :- (s/pred nat-int?) just below it, it has no return schema. Suggested fix: annotate :- (s/pred nat-int?) to pin the "always a nat-int count" contract.