Fix SMART compartment leak on _include\_revinclude#5668
Draft
feordin wants to merge 6 commits into
Draft
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>
…al gap
Add two Consequences bullets to ADR-2607:
- The additive reference-parameter enumeration is gated to
SmartCompartmentSearchExpression, so the regular (non-SMART)
Patient/{id}/* compartment search path is deliberately unchanged and
cannot be regressed by this security fix.
- Document the known residual under-match: Immunization's only
Patient-targeting reference parameter is the combined, unmaterialized
clinical-patient parameter (no type-specific materialized param), so it
is not admitted by the enumeration. This is a completeness gap, never a
disclosure, and is left as a follow-up.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Capture the actual parameterized SQL emitted before and after the SMART compartment include/revinclude fix and embed it in the ADR: - side-by-side cte6 predicate showing the added compartment EXISTS intersection - full BEFORE/AFTER _include statements (collapsible) - metrics: single _include 104->170 lines (+63%); wildcard *:* 89->8814 lines - analysis of OPTION(RECOMPILE) plan-caching trade-off, the correlated EXISTS semi-join execution cost, union-CTE duplication, and the wildcard worst case flagged as a future optimization Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…osion For a SMART compartment spanning every resource type (an unrestricted reversed wildcard such as _revinclude=*:*, surfaced as the DomainResource sentinel), the per-(resource type, reference parameter) membership enumeration produced thousands of OR clauses. Because the compartment union is re-generated wherever it is re-applied (the _include / _revinclude re-generation path), the enumeration was emitted twice and dominated the query text (_revinclude=*:* generated 8,814 lines). Every enumerated member is ultimately constrained to ReferenceResourceId = compartmentId, so the union is exactly 'any resource that references the compartment root'. For the all-types case, emit that single predicate directly instead of enumerating it. It is equivalent (a superset only over reference parameters that were previously unmapped, all still strictly bound to the compartment root, consistent with the SMART 'any resource which refers to them' model), cannot disclose another compartment's data, and collapses the generated SQL from 8,814 to 126 lines. Specific-type compartments are unchanged. A bare reference-field StringExpression (no reference SearchParameterExpression wrapper) now resolves to ReferenceQueryGenerator so the collapsed membership predicate targets the ReferenceSearchParam table. Validation (SQL 2022): full SMART SqlServer suite 80/80 (incl. both leak repro tests and the Encounter-restricted wildcard that must not collapse), SqlServer generator/compartment/reference unit tests 253/253, non-SMART SQL search integration 22/22. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add two R4/R4B integration tests confirming the $includes paging operation (SearchIncludeImpl + IncludesOperationRewriter) re-applies the SMART patient compartment, so a client cannot page out-of-compartment _include/_revinclude results even though the first page is scoped. The tests drive SearchIncludeImpl directly via a synthetic IncludesContinuationToken and assert differentially: an out-of-compartment resource is reachable with no compartment (control) but filtered once the compartment is applied (secure). SQL Server only. Refresh adr-2607 to document the Option A all-types wildcard collapse (enumeration 8,814 -> collapsed 126 lines), add a \-coverage consequence, and explain why the compartment union is re-generated inline rather than materialized once (the #5209 / #5224 table-variable lesson). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The all-types wildcard section showed only the collapsed cte0 snippet plus a metrics table. Add the complete generated statements captured from the current implementation: BEFORE fix (89 lines, revinclude with no compartment check) and AFTER fix (126 lines, all-types membership collapsed to a single ReferenceSearchParam predicate plus the revinclude compartment EXISTS intersection), as collapsible blocks mirroring the single-_include case. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
1 participant
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
Describe the changes in this PR.
Related issues
Addresses [issue #].
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)