Reject search modifiers in SMART v2 clinical scopes#5643
Merged
apurvabhaleMS merged 19 commits intoJun 30, 2026
Conversation
- Update ClinicalScopeRegEx to capture ':' in search parameter names - Detect FHIR search modifiers (:not, :missing, :exact, :contains, :in, :not-in, :above, :below, :text, :of-type, :identifier, :iterate) and reject with BadRequest - Detect FHIR search prefixes (eq, ne, gt, lt, ge, le, sa, eb, ap) in parameter values and reject with BadRequest - Add try-catch for invalid scope parameter parsing failures - Add resource strings: SmartScopeSearchParameterModifiersNotSupported, SmartScopeInvalidSearchParameters - Add unit tests for modifier and prefix rejection (22 test cases)
apurvabhaleMS
added
Azure API for FHIR
- Remove prefix rejection logic (SearchPrefixRegex) from middleware - Prefixes (gt, lt, ge, le, ne, eq, sa, eb, ap) are valid FHIR search syntax - Restore ge2000 prefix in existing unit test data - Remove prefix test cases from modifier rejection tests - Add 12 integration tests validating all FHIR prefixes work with SMART scopes - Add .http file with modifier examples for all search parameters
…round regex - Replace hardcoded KnownSearchModifiers HashSet with values from SearchModifierCode enum - Add 'of-type' and 'iterate' modifiers not present in enum - Wrap ClinicalScopeRegEx.Matches() in try-catch for RegexMatchTimeoutException - Throw BadHttpRequestException on regex timeout instead of unhandled 500
- Remove extra 'of-type' and 'iterate' (R5 codes, not R4) - R4 spec uses 'ofType' (camelCase), R5 changed to 'of-type' (kebab-case) - Update test to use ':ofType' matching R4 spec
- Revert docs/rest/SMARTv2ScopesWithSearchParametersExample.http to original
- Catch all exceptions (not just RegexMatchTimeoutException) around regex
- Use 'when' filter instead of redundant catch(BadHttpRequestException){throw;}
- Strengthen integration test assertions: Assert.Single + Assert.Equal for exact result validation
- Restore .http file from main (fix trailing newline added by Out-File) - Remove try-catch around search param parsing - all operations are safe string ops (Split, IndexOf, Substring, HashSet.Contains, SearchParams.Add) that cannot throw
- Remove stale 'and prefixes' from SmartScopeSearchParameterModifiersNotSupported message - Validate error message content in modifier rejection test (Assert.Contains) - Rename test method to reflect modifiers-only behavior
apurvabhaleMS
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to harden SMART v2 granular clinical scopes by rejecting unsupported FHIR search features (modifiers and prefixes) when used in scope search-parameter constraints, returning BadRequest instead of allowing potentially unexpected scope filtering behavior.
Changes:
- Updates SMART clinical scope parsing to recognize search-parameter modifiers in scope constraints and reject known modifiers with a
BadRequestmessage. - Adds new localized resource strings for modifier rejection and invalid search-parameter parsing.
- Adds/updates tests around SMART scope handling (but current test additions don’t align with the stated “reject prefixes” requirement).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs | Adds new integration tests that currently validate prefixes work, which conflicts with the PR’s stated intent to reject prefixes. |
| src/Microsoft.Health.Fhir.Shared.Api.UnitTests/Features/SMART/SmartClinicalScopesMiddlewareTests.cs | Adds a theory for rejecting modifier-based scopes; does not currently include prefix cases despite the PR intent. |
| src/Microsoft.Health.Fhir.Api/Resources.resx | Adds new resource strings for modifier rejection and invalid scope search-parameter parsing. |
| src/Microsoft.Health.Fhir.Api/Resources.Designer.cs | Generated designer updates for the new resource strings. |
| src/Microsoft.Health.Fhir.Api/Features/SMART/SmartClinicalScopesMiddleware.cs | Extends scope parsing to allow : in parameter names and rejects known search modifiers; adds broad try/catch around regex matching. |
Files not reviewed (1)
- src/Microsoft.Health.Fhir.Api/Resources.Designer.cs: Generated file
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Narrow regex catch to RegexMatchTimeoutException (avoid swallowing critical exceptions)
- Detect modifiers in any modifier position (after ':') via ContainsSearchModifier helper, handling chaining ('.'), type modifiers, and _has reverse-chaining
- Exclude base param name so 'identifier'/'type' params are not falsely rejected
- Allow '.' in searchParams regex key class so chained params are captured, not silently dropped
- Add modifier rejection cases (chained + _has) and positive cases (chained, type modifier, base-name collision)
feordin
reviewed
Jun 26, 2026
- Chained searches are not enforceable as scope constraints, so reject them instead of silently dropping the constraint (avoids scopes that appear to restrict but do not) - Detect forward chaining (key contains '.', e.g. subject.name, subject:Patient.name) and reverse chaining (_has: prefix) - Add SmartScopeSearchParameterChainedSearchNotSupported resource string - Add dedicated chained-search rejection test with message assertions; move chained+modifier cases there; remove chained positive cases
- Chained keys are rejected earlier by IsChainedSearchParameter, so the inner dot-split loop in ContainsSearchModifier is dead code; remove it - Eliminates the nested foreach CodeQL flagged (implicit filtering) and simplifies the modifier check to a single HashSet lookup per colon-separated token
- SearchModifierCode enum does not contain the _include/_revinclude modifiers, so iterate (R4) and recurse (STU3) are added explicitly to KnownSearchModifiers - Add unit tests for _include:iterate and _revinclude:recurse rejection - Addresses Copilot review feedback on PR #5643
- Test _include:iterate and _revinclude:recurse are rejected with the modifier message - Complements the iterate/recurse additions to KnownSearchModifiers
…ed UnionExpression in SMART scopes
Harden SmartClinicalScopesMiddleware scope parsing so malformed scopes are rejected rather than silently enforcing a truncated/partial scope (over-broad / fail-open): - Reject scopes whose token is only partially matched by the regex (non-whitespace immediately after a match), e.g. 'patient/Observation.read-only', 'patient/Observation.rs?active', 'patient/Observation.rsXYZ'. - Reject malformed search-parameter constraints containing an extra '=' that previously bypassed the modifier/chained checks and were dropped, e.g. 'code:exact=a=b', 'identifier=x?mrn=12345'. Pairs are now split on the first '=' only and validated. Adds GetMalformedScopes data provider and GivenMalformedSmartScope_WhenInvoked_ThenBadRequestIsThrown theory.
- Reject SMART clinical scopes whose resource type is not a known FHIR resource type (e.g. typo 'Observaton'); previously stored as a ScopeRestriction matching nothing, masking client misconfiguration. The 'all'/'*' wildcard is excluded from the check. - Wrap Uri.UnescapeDataString defensively so any decode failure returns BadRequest instead of an unhandled 500 (UnescapeDataString is lenient on .NET 8/9, so this is defense-in-depth). - Add SmartScopeUnknownResourceType resource string and unit tests for unknown resource types.
Result parameters _include and _revinclude pull in additional resources and can broaden a response to resource types the scope does not otherwise grant, so they are not enforceable as SMART clinical scope constraints. Reject them (plain and modifier forms) with BadRequest instead of accepting them. Adds SmartScopeSearchParameterIncludesNotSupported resource string, IsIncludeParameter helper, GetScopesWithIncludes data provider and rejection test. Moves the former positive _include/_revinclude cases to the rejection set.
The [cruds]+ regex accepts repeated letters (e.g. 'rrrrrs', 'rss'), which idempotently OR the same DataActions flag but are malformed. Reject a v2 access level whose letters are not all distinct so malformed scopes are not silently accepted. Adds rrrrrs/rss/crudss cases to GetMalformedScopes.
Covers additional duplicate-letter access levels (rrrrssssss, crudsc) via the distinct-letter check and a trailing-junk variant (rssszz) that exercises the partial-match rejection path.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5643 +/- ##
=======================================
Coverage ? 77.59%
=======================================
Files ? 1004
Lines ? 36856
Branches ? 5587
=======================================
Hits ? 28600
Misses ? 6899
Partials ? 1357 🚀 New features to boost your workflow:
|
feordin
previously approved these changes
Jun 30, 2026
…-regex-drops-search-modifiers # Conflicts: # src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/ScalarTemporalEqualityRewriter.cs
feordin
approved these changes
Jun 30, 2026
apurvabhaleMS
deleted the
personal/abhale/scope-regex-drops-search-modifiers
branch
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
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
Related issues
Addresses AB196347
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)