fix(search): enforce ContextMemory visibility at index time, not per-query - #29541
Conversation
…query Private/Shared ContextMemory could leak through search paths the per-query applyContextMemoryVisibility filter (#29384) did not cover. Instead of filtering on every search path, only org-wide (ENTITY) memories are written to the index: ContextMemoryRepository.isSearchIndexable gates the live create/update/embedding paths and getReindexFilter applies the same rule to the bulk reindex reader and counts (so partition totals stay balanced and the staged index promotes). Removes the applyContextMemoryVisibility apparatus from both search managers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
backend
safe to test
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!Entity.getEntityRepository(entityType).isSearchIndexable(entity)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
SearchRepository.updateEntityIndex calls deleteEntityIndex to remove the entity document. Because vector embeddings are written via partialUpdateEntity as fields on that same document, the entity-doc deletion implicitly cleans up the embedding too. The VectorEmbeddingHandler returning early here is therefore safe today. However, the asymmetry vs. SearchRepository (which explicitly deletes) is easy to miss: if the embedding is ever moved to its own child documents (the chunkIndex/chunkCount/parentId fields already hint at chunking), this silent return would become a real leak with no compiler or test warning. A brief inline comment explaining the implicit-cleanup assumption would protect future readers.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
createEntitiesIndex and updateEntitiesIndex (the bulk live paths reached via SearchIndexHandler.onEntitiesCreated/onEntitiesUpdated) bypassed the isSearchIndexable gate, so a PRIVATE/SHARED ContextMemory created or updated through a bulk lifecycle dispatch (import/batch extraction) could still be indexed. Apply the same gate: skip non-indexable entities on bulk create; on bulk update drop them and delete their stale docs, mirroring updateEntityIndex's ENTITY->PRIVATE flip handling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Playwright Results — workflow succeededValidated commit ✅ 1012 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 5 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 50m 12s ⏱️ Max setup 2m 55s · max shard execution 16m 36s · max shard-job elapsed before upload 19m 51s · reporting 6s 🌐 189.09 requests/attempt · 2.22 app boots/UI scenario · 27.61% common-shard skew Optimization targets still in progress:
🟡 1 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
checkIfIndexingIsSupported gates on the index mapping, not repository existence, so an indexable but repository-less type (e.g. an index-only/time-series sub-entity) reaching createEntityIndex/updateEntityIndex would throw EntityNotFoundException at the getEntityRepository().isSearchIndexable call added in the prior commits. Route all live index-path gates through a new Entity.isSearchIndexable facade that defaults to indexable when no repository is registered, so the rule still lives in ContextMemoryRepository but repository-less types index exactly as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…_PARAM Warn that the filter param is set only by ContextMemoryRepository.getReindexFilter and must not be applied to the REST /contextCenter/memories listing (owners must still read their own PRIVATE/SHARED memories there). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| public static boolean isSearchIndexable(EntityInterface entity) { | ||
| String entityType = entity.getEntityReference().getType(); | ||
| EntityRepository<? extends EntityInterface> repository = ENTITY_REPOSITORY_MAP.get(entityType); | ||
| return repository == null || repository.isSearchIndexable(entity); | ||
| } |
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.openmetadata.schema.EntityInterface; | ||
| import org.openmetadata.schema.entity.context.ContextMemory; | ||
| import org.openmetadata.schema.entity.context.MemoryShareConfig; | ||
| import org.openmetadata.schema.entity.context.MemoryVisibility; | ||
| import org.openmetadata.schema.type.EntityReference; | ||
| import org.openmetadata.service.Entity; | ||
|
|
||
| /** | ||
| * The ContextMemory search-visibility rule lives entirely in {@link ContextMemoryRepository}: only | ||
| * org-wide ({@link MemoryVisibility#ENTITY}) memories are indexed. {@link | ||
| * ContextMemoryRepository#isSearchIndexable} gates the live create/update path and {@link | ||
| * ContextMemoryRepository#getReindexFilter} carries the same rule into the bulk reindex as a DB | ||
| * query. These tests pin both halves across every visibility state. | ||
| */ | ||
| class ContextMemoryRepositoryTest { | ||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (3)
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/search/ReindexStatsIT.java:39
- In this test’s invariants,
processStats.successRecordsis the count of successful docs built, but the sink balance issuccess + failed + warning. That equality only holds when failures/warnings are always zero, which makes the invariant misleading.
The invariant description should be processStats.totalRecords == sinkStats.successRecords + sinkStats.failedRecords + sinkStats.warningRecords (processed docs are fully accounted for at the sink).
* <li>{@code readerStats.totalRecords == processStats.totalRecords}
* (everything read is processed);
* <li>{@code processStats.successRecords == sinkStats.successRecords +
* sinkStats.failedRecords + sinkStats.warningRecords} (everything
* successfully processed is accounted for at the sink);
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/search/ReindexStatsIT.java:76
- The assertion should validate the same invariant described above:
processStats.totalRecords(success+failed+warning) should equalsinkStats.success + failed + warning. UsingprocessStats.successRecordsmakes the check pass only when there are no warnings/failures and can hide accounting regressions.
assertThat(sumOrZero(processed.getSuccessRecords()))
.as("successfully processed records must equal sum of sink success/failure/warning")
openmetadata-service/src/main/java/org/openmetadata/service/workflows/searchIndex/PaginatedEntitiesSource.java:74
- Minor: same as the other constructor — caching the repository avoids repeated lookups and keeps the filter derivation centralized.
this.filter = Entity.getEntityRepository(entityType).getReindexFilter();
this.cachedTotalCount = knownTotal;
| if (!Entity.isSearchIndexable(entity)) { | ||
| LOG.debug( | ||
| "Skipping search index create for non-indexable {} [{}]", | ||
| entity.getEntityReference().getType(), | ||
| entity.getId()); | ||
| return; | ||
| } |
| } | ||
| } | ||
| } | ||
| applyContextMemoryVisibility(subjectContext, requestBuilder); | ||
| } | ||
|
|
||
| /** | ||
| * Enforces ContextMemory shareConfig visibility on search results for non-admin subjects. Applied | ||
| * independently of {@code shouldApplyRbacConditions} because memory visibility is a per-memory | ||
| * privacy guarantee, not an RBAC policy — disabling RBAC search filtering must not expose private | ||
| * memories. Non-memory documents are left untouched. | ||
| */ | ||
| private void applyContextMemoryVisibility( | ||
| SubjectContext subjectContext, ElasticSearchRequestBuilder requestBuilder) { | ||
| OMQueryBuilder visibilityBuilder = | ||
| contextMemoryVisibility.buildVisibilityFilter(subjectContext); | ||
| if (visibilityBuilder != null) { | ||
| Query visibilityQuery = ((ElasticQueryBuilder) visibilityBuilder).buildV2(); | ||
| Query existingQuery = requestBuilder.query(); | ||
| if (existingQuery != null) { | ||
| Query combinedQuery = | ||
| Query.of( | ||
| qb -> | ||
| qb.bool( | ||
| b -> { | ||
| b.must(existingQuery); | ||
| b.filter(visibilityQuery); | ||
| return b; | ||
| })); | ||
| requestBuilder.query(combinedQuery); | ||
| } else { | ||
| requestBuilder.query(visibilityQuery); | ||
| } | ||
| } | ||
| } |
| } | ||
| } | ||
|
|
||
| applyContextMemoryVisibility(subjectContext, requestBuilder); | ||
|
|
||
| return doListWithOffset(limit, offset, index, searchSortFilter, requestBuilder); | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
openmetadata-service/src/main/java/org/openmetadata/service/Entity.java:758
Entity.isSearchIndexable(...)is used as a safety gate in several live indexing paths (e.g.,VectorEmbeddingHandler.onEntityUpdated), but the current implementation can still throw ifentity.getEntityReference()throws or if the type is blank. That makes the gate itself unsafe and can break indexing/embedding on malformed entities; it should fail closed (return false) instead of propagating exceptions.
EntityReference entityReference = entity.getEntityReference();
if (entityReference == null) {
return false;
}
String entityType = entityReference.getType();
| try { | ||
| if (!getSearchClient().isClientAvailable()) { |
| * <p>Memory visibility is driven by the per-memory shareConfig, not by the OSS RBAC/policy model, | ||
| * so it is applied for every non-admin subject regardless of the RBAC access-control toggle. | ||
| * Disabling RBAC search filtering must never expose another user's private memories. | ||
| * | ||
| * <p>Current live writes and reindexing exclude restricted memories at index time. This filter | ||
| * remains as defense-in-depth for documents written before that policy was introduced and for | ||
| * mixed-version deployments during an upgrade. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
openmetadata-service/src/main/java/org/openmetadata/service/search/security/ContextMemorySearchVisibility.java:42
- PR description says the per-query ContextMemory visibility apparatus was removed from Elasticsearch/OpenSearch managers, but the codebase still applies it (e.g., ElasticSearchSearchManager.applyContextMemoryVisibility at openmetadata-service/.../ElasticSearchSearchManager.java:398 and OpenSearchSearchManager.java:413). Either update the PR description to reflect that the query-time filter remains as defense-in-depth, or remove the manager wiring if the intent is to enforce visibility only at index time.
* <p>Current live writes and reindexing exclude restricted memories at index time. This filter
* remains as defense-in-depth for documents written before that policy was introduced and for
* mixed-version deployments during an upgrade.
| private long getRegularEntityCount(String entityType) { | ||
| EntityRepository<?> repository = Entity.getEntityRepository(entityType); | ||
| return repository.getDao().listCount(new ListFilter(Include.ALL)); | ||
| return repository.getDao().listCount(repository.getReindexFilter()); | ||
| } |
| EntityReference entityReference = entity.getEntityReference(); | ||
| if (entityReference == null) { | ||
| return false; | ||
| } | ||
| String entityType = entityReference.getType(); | ||
| if (entityType == null) { | ||
| return false; | ||
| } | ||
| EntityRepository<? extends EntityInterface> repository = ENTITY_REPOSITORY_MAP.get(entityType); | ||
| return repository == null || repository.isSearchIndexable(entity); | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/search/ReindexStatsIT.java:80
- The invariant being asserted is incorrect: sink stats should account for all processed successes and processed failures (warnings are derived from the delta between processed and sink outcomes). Comparing only
processStats.successRecordsto sink totals will fail (or mask issues) when the processor records failures.
assertThat(sumOrZero(processed.getSuccessRecords()))
.as("successfully processed records must equal sum of sink success/failure/warning")
.isEqualTo(
sumOrZero(sink.getSuccessRecords())
+ sumOrZero(sink.getFailedRecords())
+ sumOrZero(sink.getWarningRecords()));
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/search/ReindexStatsIT.java:39
- This Javadoc invariant no longer matches how process/sink stats are reconciled: sink totals correspond to
processStats.successRecords + processStats.failedRecords(not justsuccessRecords). Keeping the doc accurate helps future debugging when failures are present.
* <li>{@code processStats.successRecords == sinkStats.successRecords +
* sinkStats.failedRecords + sinkStats.warningRecords} (everything
* successfully processed is accounted for at the sink);
| for (Map.Entry<String, List<EntityInterface>> entry : entitiesByType.entrySet()) { | ||
| String entityType = entry.getKey(); | ||
| List<EntityInterface> typeEntities = entry.getValue(); | ||
| List<EntityInterface> typeEntities = new ArrayList<>(); | ||
| for (EntityInterface entity : entry.getValue()) { | ||
| if (Entity.isSearchIndexable(entity)) { | ||
| typeEntities.add(entity); | ||
| } else { | ||
| // Mirror updateEntityIndex: a now-non-indexable entity (e.g. a memory flipped to | ||
| // PRIVATE/SHARED) must have its stale document removed from the index. | ||
| deleteEntityIndex(entity); | ||
| } | ||
| } |
| if (nullOrEmpty(entityType)) { | ||
| return; | ||
| } |
|
picked to 2.0 |
Code Review 👍 Approved with suggestions 1 resolved / 2 findingsEnforces ContextMemory visibility at index time by gating search indexing to ENTITY-only records, resolving the risk of data leakage via uncovered search paths. Consider refining integration test assertions and adding documentation to the visibility parameter to prevent future misuse. 💡 Edge Case: Deterministic tiebreak now sorts on name.keyword/id.keyword for all queries📄 openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchSearchManager.java:1114-1122 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchSearchManager.java:1254-1268 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchSearchManager.java:1073-1081 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchSearchManager.java:1211-1225 In both OpenSearchSearchManager.buildSearchRequestBuilder (~lines 1254-1278) and ElasticSearchSearchManager.buildSearchRequestBuilder (~lines 1211-1235), the deterministic tiebreaker ( This is a deliberate determinism improvement, but it broadens the set of queries that sort on ✅ 1 resolved✅ Security: Bulk live-index paths bypass isSearchIndexable visibility gate
🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
Describe your changes:
This addresses a security finding from the review of #29384: Private/Shared
ContextMemorycould still leak through search paths that the per-queryapplyContextMemoryVisibilityfilter didn't cover. Instead of filtering on every search path (fragile — the review found three uncovered ones), the rule is enforced once at index time: only org-wide (ENTITY) memories are written to the search index.ContextMemoryRepository.isSearchIndexablegates the live create/update/embedding paths andContextMemoryRepository.getReindexFilter()applies the samevisibility == ENTITYrule (a cross-DB JSONListFiltercondition) to the bulk-reindex reader and its entity counts, so partition totals stay balanced and the staged index promotes. TheapplyContextMemoryVisibilityapparatus is removed from both the Elasticsearch and OpenSearch managers; the REST/contextCenter/memoriesvisibility checks are unchanged.Type of change:
High-level design:
Two indexing pipelines need the rule, so it lives entirely in
ContextMemoryRepository(overriding trivialEntityRepositorybase hooks):isSearchIndexable(entity)for the live single-entity path (SearchRepository.createEntityIndex/updateEntityIndex,VectorEmbeddingHandler), andgetReindexFilter()(ashareConfig.visibility = 'Entity'JSON condition onListFilter) for the bulk reindex — applied at the reader (PaginatedEntitiesSource) and every entity-count site (PartitionCalculator/ReindexingOrchestrator/DistributedIndexingStrategy). Count and read must share the same filter because reindex promotion compares reader-total vs sink-success (StatsReconciler/EntityPromotionContext); excluding from the read but not the count would collapse the success ratio and block index promotion. No data migration is needed — ContextMemory search indexing is unreleased (added post-1.13, only on2.0.0-SNAPSHOT), so the feature ships with this fix.Tests:
Use cases covered
Unit tests
ContextMemoryRepositoryTest—isSearchIndexableacross ENTITY/PRIVATE/SHARED/missing-shareConfig +getReindexFilter.ListFilterTest— the generatedmemorySearchVisibilitySQL condition (MySQL + Postgres forms).Backend integration tests
openmetadata-integration-tests/.ContextMemoryIT.privateAndSharedMemoriesAreExcludedFromSearchIndexandContextMemoryIT.memoryVisibilityFlipAddsAndRemovesFromSearchIndex.Ingestion integration tests
Playwright (UI) tests
Manual testing performed
mvn compile+spotless:checkonopenmetadata-serviceandopenmetadata-integration-tests: pass.ContextMemoryRepositoryTest,ListFilterTest,ReindexingUtilTest. The two ContextMemory ITs require the Docker stack and were not run locally.UI screen recording / screenshots:
Not applicable.
Checklist:
🤖 Generated with Claude Code
Greptile Summary
This PR enforces ContextMemory search visibility at index time rather than per-query, addressing a security finding from #29384 where PRIVATE/SHARED memories could leak through search paths not covered by the runtime filter. The fix is applied consistently across all five indexing pipelines: live create/update (via
Entity.isSearchIndexableguardingSearchRepositoryandVectorEmbeddingHandler), bulk reindex reader and all entity-count sites (viaEntityRepository.getReindexFilter()overridden inContextMemoryRepository), ensuring reader totals and count totals stay balanced so staged-index promotion succeeds.ContextMemoryRepository.isSearchIndexableblocks PRIVATE/SHARED memories from all live write paths;getReindexFilter()applies the samevisibility = ENTITYSQL condition to bulk reindex reads and every count call (PartitionCalculator,ReindexingOrchestrator,DistributedIndexingStrategy,DistributedSearchIndexCoordinator,PaginatedEntitiesSource).SearchRepository.createEntityIndex/updateEntityIndex/updateEntitiesBulkcalldeleteEntityIndexfor newly non-indexable entities; ES/OS delete-on-NotFound is downgraded to DEBUG to reduce noise;VectorEmbeddingHandler.onEntityUpdatedcallsdeleteChunkson the flip-to-PRIVATE path.ContextMemorySearchVisibilityquery-time filter is kept for documents written before this policy was introduced, with updated Javadoc explaining its residual role.Confidence Score: 5/5
Safe to merge. The security fix is applied consistently to all five indexing pipelines with no logic gaps found; all count and read sites share the same filter, so index promotion ratios remain balanced.
Every live and bulk-reindex path is correctly gated: createEntityIndex, updateEntityIndex, updateEntitiesBulk, VectorEmbeddingHandler, and all five entity-count sites now use the same getReindexFilter() or isSearchIndexable check. The deleteEntityIndex path handles the unavailable-client case via the existing retry queue. ES/OS NotFound handling is downgraded to DEBUG.
Files Needing Attention: No files require special attention.
Important Files Changed
isSearchIndexable(live-path gate) andgetReindexFilter(bulk reindex gate); correctly restricts both to ENTITY-visibility memories onlyMEMORY_SEARCH_VISIBILITY_PARAMconstant andgetMemorySearchVisibilityCondition()to global condition chain; parameter is no-op unless explicitly set, with clear documentation warning against misuse on non-ContextMemory filterscreateEntityIndex,updateEntityIndex,createEntitiesIndex, andupdateEntitiesBulkwithEntity.isSearchIndexable; non-indexable entities triggerdeleteEntityIndex(has internal retry queue) on all live paths; empty-docs guard prevents no-op bulk callsisSearchIndexableguard inonEntityUpdatedwith explicitdeleteChunksfor the ENTITY-to-PRIVATE flip;onEntityCreatedis protected by inner guard inupdateEmbedding, but lacks a dedicated test for non-indexable create pathEntity.isSearchIndexable(entity)facade; defaults totruefor types without a regular repository (index-only/time-series) to avoid NPE; null-safe throughoutgetReindexFilter()for both the list filter and the cached total count, keeping reader and counter in syncduring()) and ENTITY-PRIVATE flip adds/removes from index;createMinimalRequestupdated to use ENTITY visibility for generic base testsFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Entity Lifecycle Event] --> B{Entity Created or Updated?} B -->|Create| C[SearchRepository.createEntityIndex] B -->|Update| D[SearchRepository.updateEntityIndex] B -->|Bulk Update| E[SearchRepository.updateEntitiesBulk] C --> F{Entity.isSearchIndexable?} D --> F E --> G[Filter list: isSearchIndexable per entity] F -->|false| H[deleteEntityIndex ES/OS NotFound to DEBUG log] F -->|true| I[Write to search index] G -->|non-indexable| H G -->|indexable| I A --> J[VectorEmbeddingHandler.onEntityUpdated] J --> K{Entity.isSearchIndexable?} K -->|false| L[deleteChunks remove vector chunks] K -->|true| M[updateEmbedding write vector chunks] A --> N[VectorEmbeddingHandler.onEntityCreated] N --> O[updateEmbedding inner guard] O --> P{Entity.isSearchIndexable?} P -->|false| Q[return early no chunks written] P -->|true| M subgraph Bulk Reindex R[PaginatedEntitiesSource] --> S[getReindexFilter visibility=ENTITY SQL] T[PartitionCalculator] --> S U[ReindexingOrchestrator] --> S V[DistributedIndexingStrategy] --> S W[DistributedSearchIndexCoordinator] --> S S --> X[Only ENTITY memories read and counted] endReviews (22): Last reviewed commit: "Merge branch 'main' into harshach/contex..." | Re-trigger Greptile