Enforce SMART compartment authorization on _include/_revinclude - #5683
Open
Jared Erwin (feordin) wants to merge 14 commits into
Open
Enforce SMART compartment authorization on _include/_revinclude#5683Jared Erwin (feordin) wants to merge 14 commits into
Jared Erwin (feordin) wants to merge 14 commits into
Conversation
MSRC: SMART patient-scoped tokens could retrieve resources outside their
patient compartment via _include/_revinclude expansion (PHI disclosure).
Reuse the existing SmartCompartmentSearchExpression -> UNION-of-CTEs and
intersect it with the produced _include/_revinclude rows in the SQL query
generator, instead of the deprecated CompartmentAssignment table. Covers
SMART v1 and v2 granular scopes.
- SqlQueryGenerator: capture the compartment union, regenerate before the
include table expansion, and EXISTS-intersect produced include/revinclude
rows against the union.
- SqlCompartmentSearchRewriter (SMART-gated): additively enumerate every
materialized type-specific reference param targeting the compartment root,
because the compartment definition maps Encounter/Condition/Procedure/
ImagingStudy to the non-materialized clinical-patient (resolve()-based)
param. Every union member stays constrained to ReferenceResourceId =
compartmentId, matching the SMART "any resource that refers to them" model.
- SearchOptionsFactory: map the reversed wildcard _revinclude=*:* produced
"*" sentinel to DomainResource ("all compartment types") so every
revincluded type is covered by the union.
- Add two integration repro tests (_include and _revinclude) plus the
SmartCompartmentLeak.json fixture.
- ADR-2607 documents the issue, repro, and chosen union-intersection design.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Authorize include and revinclude candidates with immutable compartment membership rules while preserving includes paging and formal FHIR compartment semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…quivalents Tests: iterate and wildcard include/revinclude leaks, SMART v2 granular scopes with includes, positive root/universal inclusion, multi-parameter dedup, sort (_lastUpdated) with includes, and compartment-definition guard tests for Patient and Practitioner. New leak-enforcement tests skip on Cosmos DB (fix is SQL-only; Cosmos is deprecated) and the compartment count assertion is now datastore-dependent. Equivalence map: cover every resolve()-based (unmaterialized) compartment parameter found in the R4 definitions - Patient adds AuditEvent-patient (agent, entity), Basic/Invoice/MeasureReport-patient (subject), Person-patient (link), Provenance-patient (target); Practitioner adds Encounter-practitioner (participant) and Person-practitioner (link). EpisodeOfCare-care-manager has no materialized equivalent and is documented as a known gap. Fixture: enable CoreFeatureConfiguration.SupportsIncludes in the SQL integration fixture (production SQL configuration); without it the includes continuation token is never created and the includes-paging test could never pass. The sort-by-search-parameter variant is blocked by a pre-existing defect (#5672) and noted in the test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Remove orphaned sentinel comment in SearchOptionsFactory left from an earlier iteration. - Remove unused skipJoinFromPreviousUnions parameter from AppendNewSetOfUnionAllTableExpressions and the no-op save/restore of _unionAggregateCTEIndex around the SMART v2 include regeneration. - Add a fail-open detector in AttachSmartCompartmentMembership: if the request context is compartment-bound (fhirUser-derived CompartmentResourceType is set) and the expression contains include CTEs but no membership context could be constructed, log critical. System scopes never set CompartmentResourceType, so they cannot trip the detector. - Add a canary unit test exercising the factory -> attach -> generate chain with the SearchOptionsFactory-shaped expression tree, so any regression that silently drops include authorization fails CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Jared Erwin (feordin)
changed the title
Jared Erwin (feordin)
added
Bug-Security
Contributor
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Request the maximum search page size so the system-scope assertion still covers the full fixture after it grows beyond 100 resources. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c588fb8-d85b-445d-9d03-8bcdd8d3008b
Reconcile the branch's SMART _include/_revinclude candidate authorization with main's Device compartment limits. Under the restriction Device is no longer universally shared: own devices (Device.patient references the compartment root) and unassigned devices (no Device.patient reference) are authorized as include/revinclude candidates, while devices assigned to another patient are denied - closing the candidate device leak. Introduce a single declarative source of truth (SmartCompartmentConditionalRule + SmartCompartmentSearchRewriter.GetConditionalCompartmentRules) consumed by BOTH the compartment union and the SQL candidate predicate, so the two paths cannot drift. SqlQueryGenerator now lowers these rules generically with no resource-type-specific logic. Validated: 16 SqlQueryGenerator + 6 rewriter unit tests, 107/107 SqlServer SmartSearchTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6a1f0772-5429-4159-bc7d-657f9b64bbce
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5683 +/- ##
==========================================
+ Coverage 77.09% 78.60% +1.51%
==========================================
Files 1007 1016 +9
Lines 37171 37539 +368
Branches 5658 5714 +56
==========================================
+ Hits 28657 29508 +851
+ Misses 7147 6639 -508
- Partials 1367 1392 +25 🚀 New features to boost your workflow:
|
Contributor
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Adds tools/IncludePerf, a harness for measuring _include/_revinclude latency at realistic data volumes under SMART scopes. It complements tools/ABTestRunner, which deploys both services with security disabled and therefore cannot exercise the SMART compartment path this PR changes. - FhirPerfDataGenerator produces ~3.9M deterministic FHIR resources with heavy reference fan-out plus deliberate cross-compartment links, so the compartment predicate has something to exclude. - FhirIncludeBenchmark acquires real SMART tokens from the development identity provider and reports p50/p90/p95/p99 per query shape. - FhirPerfSqlOps reads Query Store statistics using Entra ID auth. - Both services are pointed at one shared database, which is safe because this change alters no SQL schema, and which removes data, statistics and fragmentation as sources of false deltas. Also adds two SMART integration tests for include shapes that were not covered: - Typed wildcard (_include=Coverage:*). ExpressionParser.ParseInclude builds a third distinct IncludeExpression for this form: "*" leaves SourceResourceType null, "*:*" sets it to "*", and "Type:*" sets it to the concrete type. Only the first two were exercised. - Device reached as an include candidate from another resource type (_include=Observation:device). Device is the only type with conditional compartment visibility, and every existing Device test approaches it as a match or through _include=Device:patient, never as an include candidate referenced by an in-compartment resource. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7747d4ce-019c-4255-a929-fe8459f4b89a
Adding Observation/smart-observation-A-outside-device to SmartPatientA.json put a third Observation (subject = smart-patient-A) into Patient A's compartment, breaking 12 pre-existing SQL integration tests that pin exact Observation counts for that compartment (expected 2 got 3, expected 43 got 44, etc.). The Device include-candidate scenario now lives in the purpose-built SmartCompartmentLeak.json bundle instead: a Device assigned to smart-leak-parent and an Observation in smart-leak-child's compartment that references it. SmartPatientA.json returns to its original resource set, so the pre-existing compartment count expectations hold again. Verified: all SqlServer-flavored SmartSearchTests pass (previous 12 failures resolved; remaining failures are CosmosDb emulator connection noise in local runs). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e subtraction Review follow-ups for the SMART include/revinclude compartment enforcement: 1. AttachSmartCompartmentMembership now throws instead of only logging when a compartment-bound request with include CTEs yields no membership context. System scopes never set AccessControlContext.CompartmentResourceType, so they cannot enter this branch; SearchOptionsFactory always adds the smart compartment expression when it is set, so the guard fires only when a rewrite step hides or drops the compartment expression. 2. SqlQueryGenerator.VisitSqlRoot re-checks (via SqlSearchOptions. IsSmartCompartmentSearch) that an attached membership context is still present at generation time, so a future rewrite step that reconstructs SqlRootExpression after the attach fails loudly instead of silently generating unauthorized include CTEs. 3. The "universal types minus conditionally-visible types" subtraction now lives in one place (SmartCompartmentSearchRewriter.GetSharedResourceTypes) consumed by both the compartment union and the membership context factory. Verified: SqlServer unit tests 1053/1053, Core SmartCompartment unit tests 6/6, SqlServer SmartSearchTests integration 109/109 (remaining local failures are Cosmos emulator connection noise). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on gaps Review follow-ups #4 and #5 for the SMART include/revinclude enforcement: 1. Partial-equivalence over-restriction fix: when an unmaterialized combined parameter (e.g. AuditEvent-patient spanning agent.who and entity.what) resolves materialized equivalents, the formal parameter is now retained alongside them. Retention is harmless while the parameter is unmaterialized (it matches no ReferenceSearchParam rows) and guarantees membership is never narrower than the formal compartment definition if only part of the equivalents validate on a given definition set. 2. Enumeration-gap observability: SqlCompartmentSearchRewriter now logs a warning (once per resource type and parameter) when a membership parameter is single-branch resolve()-based with no materialized equivalent - the state in which in-compartment resources of that type are silently absent from include results (documented example: EpisodeOfCare-care-manager in the Practitioner compartment). 3. New integration guard test pins the documented gap list exactly (Patient: none; Practitioner: EpisodeOfCare only), so a compartment definition or search parameter change that introduces a new silent gap fails the build instead of shipping. Verified: SqlServer unit tests 1053/1053, Core SmartCompartment unit tests 6/6, SqlServer SmartSearchTests integration 110/110 including the new guard test (remaining local failures are Cosmos emulator connection noise). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| rewriter.GetMaterializedCompartmentSearchParameters(compartmentType, filteredResourceTypes: null); | ||
|
|
||
| Assert.True( | ||
| membership.ContainsKey(resourceType), |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes the SMART-on-FHIR compartment leak where
_include/_revinclude(and the$includesoperation) returned referenced/referencing resources without compartment enforcement in the SQL provider.Each include candidate row is now authorized in the include CTE itself: the candidate must be the compartment root, a shared/universal resource type, or provably reference the root via a formal compartment-definition search parameter ('EXISTS' probe on 'ReferenceSearchParam').
See 'docs/arch/adr-2607-smart-include-compartment-scoping.md' for design details.
Related issues
Addresses [issue AB#196779].
Testing
SqlQueryGeneratorTests(predicate emission, membership factory rules, canary for the enforcement chain):iterate, wildcards,$includespaging, Practitioner compartment, and SMART v2 granular scopes.FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)