Enforce SMART compartment authorization on _include/_revinclude#5683
Open
feordin wants to merge 4 commits into
Open
Enforce SMART compartment authorization on _include/_revinclude#5683feordin wants to merge 4 commits into
feordin wants to merge 4 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>
feordin
changed the title
feordin
added
Bug-Security
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+185
to
+196
| foreach (string equivalentCode in equivalentCodes) | ||
| { | ||
| if (SearchParameterDefinitionManager.Value.TryGetSearchParameter(resourceType, equivalentCode, out SearchParameterInfo equivalent) | ||
| && !string.Equals(equivalent.Url.AbsoluteUri, parameter.Url.AbsoluteUri, StringComparison.Ordinal) | ||
| && equivalent.Type == SearchParamType.Reference | ||
| && equivalent.IsSupported | ||
| && equivalent.TargetResourceTypes?.Contains(compartmentType, StringComparer.Ordinal) == true) | ||
| { | ||
| parameters[equivalent.Url.AbsoluteUri] = equivalent; | ||
| equivalentFound = true; | ||
| } | ||
| } |
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
2 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/SmartIncludeCandidateAuthorization.md' and '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)