feat: add backward-compatible AAD auth to Azure Search - #2591
Merged
ranadeepsingh merged 6 commits intoAug 4, 2026
Merged
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey @ranadeepsingh 👋! We use semantic commit messages to streamline the release process. Examples of commit messages with semantic prefixes:
To test your commit locally, please follow our guild on building from source. |
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a backward-compatible authentication abstraction for Azure Search in the cognitive module, enabling AAD bearer tokens and custom auth/headers while preserving legacy subscription-key APIs. The changes also centralize header-precedence logic with other cognitive services and improve HTTP response resource handling.
Changes:
- Introduces
AzureSearchAuth(subscription key / AAD token / custom auth header / custom headers) plus shared request builders for Azure Search index APIs. - Updates Azure Search index list/get/create/statistics call paths to accept auth-aware overloads while keeping existing key-based signatures source-compatible.
- Extracts shared header precedence logic into
ServiceAuthHeadersand adds a dedicated unit test suite for auth parsing/precedence/redaction/compatibility.
Show a summary per file
| File | Description |
|---|---|
| cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/search/AzureSearchAuthSuite.scala | Adds secret-free unit coverage for auth parsing, precedence, redaction, and legacy API compatibility. |
| cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/search/AzureSearchAuth.scala | Adds AzureSearchAuth + option parsing/validation/redaction and a shared AzureSearchRequests builder layer. |
| cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/search/AzureSearchAPI.scala | Refactors index APIs to use AzureSearchAuth overloads and ensures responses are always closed via try/finally. |
| cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/search/AzureSearch.scala | Updates writer option handling to support AAD/custom auth and applies auth consistently when constructing AddDocuments. |
| cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/CognitiveServiceBase.scala | Centralizes header precedence/merging into ServiceAuthHeaders and reuses it from HasCognitiveServiceInput.getHeaders. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2591 +/- ##
==========================================
- Coverage 84.79% 84.71% -0.09%
==========================================
Files 334 335 +1
Lines 17806 17882 +76
Branches 1623 1639 +16
==========================================
+ Hits 15099 15148 +49
- Misses 2707 2734 +27 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
## Summary Add shared Azure Search authentication configuration for subscription keys, AAD tokens, custom authorization, and custom headers across writer preparation and index list/get/create/statistics APIs. Preserve legacy key-based signatures and add secret-free unit coverage. ## Prompting Intent Recreate the intent of the stale Azure Search AAD PR on current master without breaking callers. Use existing service authentication behavior, retain subscription-key support, reject missing credentials, avoid live-secret tests, and validate security and API compatibility. ## Linked Sources - Original Azure Search AAD PR: microsoft#2285 - AAD cognitive-services foundation: microsoft#1778 ## Rationale Centralize header precedence in the existing cognitive-service path and delegate Azure Search requests to it rather than duplicating authentication logic. Use additive AzureSearchAuth overloads so legacy JVM and Scala signatures remain intact, while request-builder unit tests cover every management API without contacting Azure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ranadeepsingh
force-pushed
the
copilot/ancient-2285-search-aad
branch
from
August 1, 2026 13:13
2b52576 to
8cdc699
Compare
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
…ormed customHeaders error ## Summary Remediates two independent security-review findings on the Azure Search AAD auth change (PR microsoft#2591): 1. Credential/header precedence bypass and duplicate Authorization. Auth entries placed in the user-supplied customHeaders map could bypass the key/AAD/custom-auth precedence, and mixed-case names (e.g. "AUTHORIZATION") could emit a duplicate Authorization header. The Azure Search management index API also used HttpRequestBase.setHeader while the document writer used addHeader, so the two paths had divergent header semantics. ServiceAuthHeaders.build now resolves exactly one credential in a single, case-insensitive precedence step (subscriptionKey > aadToken > customAuthHeader > customHeaders-embedded credential), canonicalizes it to the context header name, and strips every case variant of the api-key / Authorization names from the generic-header pass so they can neither override nor duplicate the resolved credential. AzureSearchRequests.addHeaders now uses request.addHeader to mirror HasCognitiveServiceInput.addHeaders, so writer and management index requests apply identical semantics over the deduplicated canonical map. 2. Credential leak via malformed-JSON exception. Malformed customHeaders JSON previously chained the spray-json parser exception as the cause; that parser message echoes the full input, which can contain credential values, leaking them through logs/stack traces. parseCustomHeaders now throws a sanitized IllegalArgumentException with no parser cause. Adds focused, secret-free tests covering mixed casing, duplicate prevention, precedence, generic-vs-auth stripping, identical writer/management results, and a test asserting the entire rendered exception chain leaks no credential or malformed-input value. No public API changed; all existing signatures and the 10 prior tests are preserved (15/15 tests pass). ## Prompting Intent Engineer asked to fix two independent-security-review findings using TDD: (1) resolve every credential/header name case-insensitively in ONE precedence step, strip auth entries from generic headers, and apply identical semantics across the writer and management index APIs while preserving intended precedence and all public API compatibility; (2) throw a sanitized exception WITHOUT the spray-json parser cause for malformed customHeaders JSON. Add focused secret-free tests for both, keep unrelated work, and run targeted cognitive compile/tests + scalastyle + security + API-compat review following the repo code-review skill. ## Linked Sources - Pull request: microsoft#2591 - Independent security-review findings (customHeaders auth-bypass / duplicate Authorization; malformed-JSON parser-cause credential leak) - Prior feature commit 8cdc699 "feat: add backward-compatible AAD auth for Azure Search" ## Rationale Fixed the bypass/duplication in the shared ServiceAuthHeaders.build helper so every consumer (all cognitive services and Azure Search) gets one canonical, case-insensitive precedence step rather than patching each call site. Chose to change the Azure Search management path to addHeader (matching the shared writer) instead of altering the shared writer to setHeader, minimizing blast radius; because build now returns a unique-key canonical map, addHeader and setHeader are equivalent (asserted by test). For the leak, dropped the parser cause entirely rather than scrubbing its message, since the sanitized static message is sufficient and any retained cause risks re-exposing the raw input. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Collaborator
Author
🔒 Security re-review requestedFollow-up commit Finding 1 — credential/header precedence bypass & duplicate
Finding 2 — credential leak via malformed-JSON exception.
Validation. Added focused, secret-free tests covering mixed casing, duplicate prevention, precedence, generic-vs-auth stripping, identical writer/management results, and a test asserting the entire rendered exception chain (message + causes + stack) leaks no credential or malformed-input value. No public API changed; all prior tests preserved (15/15 pass locally). CI is green on Compile & Style Check, ADO Style, and UnitTests search1 / search2 / openai. Full rationale is in the commit message of |
…e it lazily, and ignore blank auth values ## Summary Fixes four medium findings from independent reviews of the Azure Search AAD auth work (PR microsoft#2591, commit 09e56b1 and its follow-ups). All live in CognitiveServiceBase's shared cognitive auth path, so they apply uniformly to the Azure Search document writer, the Search management-index APIs, and every non-Search HasCognitiveServiceInput consumer: 1. Writer/management auth divergence. The document writer's automatic Fabric fallback token was returned from getCustomAuthHeader and passed through the explicit custom-auth slot, so ServiceAuthHeaders.build ranked the synthesized Fabric token ABOVE a credential embedded in customHeaders, while the management-index path (no fallback) used the embedded credential. getCustomAuthHeader now returns only the explicit CustomAuthHeader param, getFabricFallbackAuthHeader supplies the synthesized token separately, and build takes it as a dedicated lowest-priority parameter. Precedence is subscriptionKey > explicit AAD > explicit custom-auth > embedded customHeaders credential (api-key then Authorization) > automatic Fabric fallback. Writer and management now resolve identically. 2. Blank auth value suppressed a valid credential. A blank/whitespace embedded api-key beat a valid Authorization in the same customHeaders map. Every credential source is now blank-filtered, and embedded-credential selection ignores blank values and resolves mixed-case duplicates deterministically (sorted by raw header name, canonicalized to the context header name). 3. Fabric-fallback gate ignored blank explicit credentials. getFabricFallbackAuthHeader decided eligibility with getValueOpt(row, CustomAuthHeader).isEmpty, so a Some(" ")/Some(null) CustomAuthHeader suppressed the fallback even though build then blank-filters it. Eligibility is now decided by a shared lacksExplicitAuthCredential seam that treats a null/blank/whitespace subscriptionKey, AADToken, OR CustomAuthHeader as absent (reusing ServiceAuthHeaders.nonBlank so the gate and build agree on "blank"). 4. Eager Fabric token acquisition could fail an otherwise-valid embedded credential (this follow-up). getHeaders eagerly evaluated getFabricFallbackAuthHeader(row), and lacksExplicitAuthCredential considered only subscriptionKey/AADToken/CustomAuthHeader -- never a non-blank api-key/Authorization embedded in customHeaders. On Fabric that eager call acquires a token via reflection and can throw, so a request whose ONLY credential is an embedded customHeaders api-key/Authorization could throw during writer header preparation before ServiceAuthHeaders.build discarded the fallback in favor of that embedded credential -- while the management-index path (which supplies no fallback) succeeded with the same credential. The synthesized fallback is now threaded BY-NAME through buildServiceAuthHeaders into ServiceAuthHeaders.build, where it is the lowest-priority source of the single precedence chain. Because Option.orElse takes its alternative by name, the fallback thunk -- and therefore getFabricFallbackAuthHeader and any live token acquisition -- is evaluated ONLY when every higher-priority source (subscription key, AAD token, explicit custom-auth header, and a non-blank embedded customHeaders credential) is absent. The gate is intentionally NOT taught to parse customHeaders; embedded-credential precedence over the fallback is enforced by the one build chain, so the parsing is not duplicated. Precedence, null/blank handling, mixed-case dedupe, and the malformed-JSON sanitization are unchanged; writer and management still resolve identically. No public API changed. ServiceAuthHeaders is private[ml] and both callers were updated; lacksExplicitAuthCredential/buildServiceAuthHeaders are private[ml] and getFabricFallbackAuthHeader is protected with an unchanged signature. Making buildServiceAuthHeaders and ServiceAuthHeaders.build take the fallback by-name is source-compatible for every caller (writers pass getFabricFallbackAuthHeader(row); the AzureSearch management path passes None); the whole cognitive module compiles and both scalastyle configs pass. ## Prompting Intent Prior asks fixed findings 1-3 (make the Fabric credential a true lowest-priority fallback below any non-blank embedded api-key/Authorization; distinguish an explicit customAuthHeader from a synthesized fallback on one shared case-insensitive path; stop blank/whitespace/null values from suppressing a valid credential or the fallback), preserving the malformed-JSON sanitization and API/binary/source compatibility with tests on the real writer header path and no FuzzingTest-discoverable stage. This follow-up ask: fix, with TDD, the final concrete reviewed issue -- lacksExplicitAuthCredential does not account for a case-insensitive non-blank api-key/Authorization embedded in customHeaders, and production getHeaders eagerly evaluates getFabricFallbackAuthHeader, so on Fabric token acquisition/reflection can throw before ServiceAuthHeaders.build discards the fallback in favor of the embedded credential; thus a valid embedded credential can fail writer preparation while management succeeds. Implement the smallest robust solution so automatic Fabric token acquisition is genuinely lazy and happens ONLY when all higher-priority sources (subscription key, AAD token, explicit custom auth, and embedded non-blank customHeaders credential) are absent; prefer centralizing this in the single ServiceAuthHeaders resolution chain rather than duplicating customHeaders parsing in the gate (a by-name/lazy fallback across the helper boundaries). Preserve exact precedence, null/blank behavior, sanitization, and API compatibility (helpers are private[ml]/protected). Add a RED/GREEN test where a valid embedded credential is present and the fallback supplier throws if evaluated, asserting writer header preparation succeeds and the fallback is never evaluated, and asserting the fallback IS evaluated when no higher credential exists; avoid adding a discoverable test Transformer. Run AzureSearchAuthSuite, compile as appropriate, cognitive main/test scalastyle, and inspect non-Search consumers. Amend only the current unpushed commit 564199ba (parent 09e56b must remain unchanged); do not push or trigger CI. ## Linked Sources - Pull request: microsoft#2591 - Independent reviews of commit 09e56b1 and its follow-ups (writer Fabric-fallback outranking an embedded customHeaders credential; blank api-key suppressing a valid Authorization; production Fabric-fallback gate suppressed by a blank/whitespace/null customAuthHeader; eager Fabric token acquisition failing an otherwise-valid embedded customHeaders credential) - Prior fix commit 09e56b1 "fix(search): dedupe auth headers case-insensitively and sanitize malformed customHeaders error" ## Rationale Chose a by-name (lazy) fallback threaded through buildServiceAuthHeaders and ServiceAuthHeaders.build over teaching lacksExplicitAuthCredential to parse customHeaders. The precedence order -- including "an embedded customHeaders credential outranks the Fabric fallback" -- already lives in build's single orElse chain; re-implementing the embedded api-key/Authorization scan inside the gate would duplicate that logic and risk the two drifting (the exact class of bug being fixed). Option.orElse evaluates its alternative by-name, so placing the fallback last in the chain makes its thunk -- hence getFabricFallbackAuthHeader and any live FabricClient token acquisition -- run only after every higher-priority source (including a non-blank embedded credential) is found absent; a Fabric token-acquisition/reflection failure can therefore no longer fail header preparation when any higher-priority credential exists. Kept lacksExplicitAuthCredential and its non-blank gate: getFabricFallbackAuthHeader is protected (extension surface) and may be called directly, so the gate still prevents a wasteful/erroring token fetch when subscriptionKey/AADToken/CustomAuthHeader is set, and the existing blank-credential test stays valid; inside the build chain the gate is a harmless redundant guard. getFabricFallbackAuthHeader's protected signature and getHeaders are unchanged; only the private[ml] seams became by-name, so the change is source- and binary-compatible for consumers. Verified no non-Search HasCognitiveServiceInput consumer (OpenAI, Vision, Speech, Text, Anomaly, Form, Face, Language, Translate) overrides getHeaders, buildServiceAuthHeaders, getFabricFallbackAuthHeader, or getCustomAuthHeader; all use the shared lazy chain, and the Fabric default-endpoint path (CognitiveServicesBaseNoHandler) sets no explicit credential so it still receives the fallback. TDD: added the failing test first -- a throwing fallback supplier plus an embedded api-key, asserting writer preparation succeeds and the supplier is never evaluated -- and confirmed RED (the eager by-value parameter evaluated the supplier and threw "Fabric fallback must not be evaluated when a credential is present"), then made the fallback by-name to turn it GREEN, with a companion assertion that the supplier IS evaluated exactly once when no higher-priority credential exists. The test drives the production buildServiceAuthHeaders seam (the same one getHeaders uses) rather than a live Fabric environment, and adds no Param/PipelineStage, so FuzzingTest's discovery set is unchanged. Validation: AzureSearchAuthSuite 23/23 passed; cognitive main scalastyle 0 errors/0 warnings (53 files); cognitive test scalastyle 0 errors/0 warnings (37 files). FuzzingTest was not run because no new stage, Param, or public signature was added. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/search/AzureSearchAuth.scala:16
nonBlankis not null-safe. IfAzureSearchAuthis constructed withSome(null)(e.g., from Java call sites) or acustomHeadersvalue is null,value.trimwill throw a NPE duringvalidated/normalized. This is inconsistent withServiceAuthHeaders.nonBlank, which explicitly treats null as absent.
private def nonBlank(value: String): Boolean = value.trim.nonEmpty
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
…credentials and custom headers
## Summary
AzureSearchAuth is public, so Java/direct callers can build the final case class with null in places the
Scala factories never produce. Two related NullPointerException hazards existed on the
construction -> normalization -> validation -> toString -> request path:
1. nonBlank called value.trim with no null guard, so Some(null) credentials or null custom-header values
threw NPE during normalized/validated (and toString and header assembly) -- inconsistent with the
shared ServiceAuthHeaders.nonBlank null-safe rule (value != null && value.trim.nonEmpty).
2. normalized called .filter directly on each of the three outer credential Option containers, so a null
*container* (e.g. AzureSearchAuth(null, Some(aad), None, Map.empty)) threw NPE before the valid
lower-priority credential could be selected.
This makes the entire path null-safe:
- nonBlank matches the shared rule and treats a null value as absent.
- normalized wraps each outer container with Option(_).flatten before filtering, so a null container
collapses to None (never NPEs) while Some(null) is preserved and then dropped by nonBlank.
- A private sanitizedCustomHeaders treats a null customHeaders map as empty and drops entries with a
null header name (never a valid HTTP header name), applied inside normalized so validated and header
assembly (ServiceAuthHeaders.build, which is not null-key-safe) never dereference a null container or key.
- toString renders sanitized header names and <redacted> credential literals (it never dereferences the
outer containers), so a null map/key/container no longer NPEs and no value leaks.
Audit of every access to the three outer Option containers: only normalized reads them raw; validated,
toString, and request construction (headers -> ServiceAuthHeaders.build) all consume the normalized copy,
so the single Option(_).flatten fix in normalized covers them.
Behavior: null credential option containers, Some(null) credential values, and null/blank custom-header
values are all treated as ABSENT -- a null/blank higher-priority credential never suppresses a valid
lower-priority one; when no credential remains, the existing sanitized IllegalArgumentException
("Azure Search authentication requires ...") is thrown, never an NPE and never leaking a value. Exact
precedence (subscriptionKey > AADToken > CustomAuthHeader > embedded api-key > embedded Authorization),
redaction, and the prior malformed-JSON no-cause guarantee are unchanged. No public API changed: only
private / private[search] internals and the toString override differ; the case-class signature, validated,
headers, and the companion factories are untouched, as is the shared ServiceAuthHeaders.
## Prompting Intent
Two sequential asks against the fresh GitHub Copilot re-review of final commit aa07b15:
1. AzureSearchAuth.nonBlank is not null-safe, so callers can construct Some(null) credentials or null
customHeaders values and NPE during validated/normalized; fix the whole direct-construction path for
adjacent null name/value/container hazards with the smallest backward-compatible behavior.
2. Remaining reviewed null-safety issue: because AzureSearchAuth is public, a caller can pass a null outer
Option container (e.g. AzureSearchAuth(null, Some(validAad), None, Map.empty)); normalized called
.filter on each Option and NPE'd before selecting the valid lower credential. Audit normalized,
validated, toString, and request construction for all accesses to the three outer Option containers;
normalize null containers safely (Option(container).flatten) before filtering while preserving
Some(null) handling, exact precedence, redaction, and fail-closed missing-credential behavior. Add
tests for null outer subscription-key/AAD/custom-auth Option containers (valid lower-priority credential
still works; all-null containers yield the existing sanitized missing-auth IllegalArgumentException; the
entire rendered chain contains no canary). Keep public signatures unchanged. Use TDD; never NPE and
never leak values. Run AzureSearchAuthSuite and cognitive main/test scalastyle, compile as needed; amend
only the current unpushed commit without pushing or triggering CI.
## Linked Sources
- Pull request: microsoft#2591
- Fresh GitHub Copilot re-review of final commit aa07b15 (findings: AzureSearchAuth.nonBlank not
null-safe; adjacent null customHeaders name/value/container hazards; remaining null outer Option
container hazard in normalized)
- Shared null-safe rule: ServiceAuthHeaders.nonBlank in
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/CognitiveServiceBase.scala
(value != null && value.trim.nonEmpty)
- Prior fix commit aa07b15 "fix(search): rank Fabric fallback below embedded credentials, evaluate it
lazily, and ignore blank auth values"
## Rationale
Centralized all null sanitization in normalized rather than scattering guards across validated, toString,
and headers: validated and headers both consume normalized, so one sanitization point makes the
embedded-credential scan, the require gate, and ServiceAuthHeaders.build (shared, private[ml], NOT
null-key-safe) all safe without modifying shared code or duplicating logic. For the outer containers, chose
Option(container).flatten.filter(nonBlank) because Option(null) -> None (no NPE), Option(Some(x)) ->
Some(x), Option(None) -> None, and Option(Some(null)) -> Some(null) which nonBlank then drops -- preserving
the established Some(null)=absent semantics and exact precedence with the smallest possible change and no
new branches. toString was left rendering <redacted> literals (it never reads the outer containers) and
sanitized header names, so it stays leak-free and null-safe. Dropped null-named custom-header entries
instead of guarding each dereference because a null header name is never a valid credential and can never
be emitted as an HTTP header (Apache BasicHeader rejects a null name); retained null non-auth-header VALUES
(Apache allows them and shared build keeps them) to avoid a behavior change. Left ServiceAuthHeaders and
parseCustomHeaders untouched (out of scope; preserves the malformed-JSON sanitized no-cause exception).
Scoped a single "// scalastyle:off null"/on around the null tests (repo convention: the test config's
NullChecker forbids null literals but allows == null / != null) because the tests must construct null
inputs. TDD: the commit adds eight secret-free tests -- five for Some(null) values, a null-valued api-key
custom header, a null custom-header name, a null customHeaders map, and a value-leak canary; plus three new
RED-first tests for the outer-container hazard (a null container not suppressing a valid lower credential,
all-null containers rejected as missing auth, and a rejection whose full rendered exception chain must not
contain a canary), confirmed RED (3 NPEs at normalized) then GREEN after Option(_).flatten. Validation:
AzureSearchAuthSuite 31/31 passed (28 prior + 3 new); cognitive main scalastyle 0 errors (53 files);
cognitive test scalastyle 0 errors (37 files); public API surface unchanged (diff touches only private
internals + toString + tests); no secrets introduced (only clearly-fake canary literals).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (3)
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/CognitiveServiceBase.scala:371
- ServiceAuthHeaders.build currently merges telemHeaders without sanitizing Some(null)/null keys and without protecting auth header names, so a telemetry map could NPE or override the resolved api-key/Authorization credential. Treat a null telemetry map as empty, drop null keys, and ignore auth header names here.
telemHeaders.foreach(_.foreach { case (headerName, headerValue) =>
headers += (headerName -> headerValue)
})
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/CognitiveServiceBase.scala:336
- ServiceAuthHeaders.build assumes customHeaders is never Some(null) and never contains null header names, but getValueOpt wraps scalar param values with Some(value) even when value is null, and setCustomHeaders(java.util.HashMap) can include a null key. That can lead to NPEs in embeddedCredential/isAuthHeaderName and while iterating. Sanitize to treat a null map as empty and drop null-named entries up front.
This issue also appears on line 369 of the same file.
val providedCustomHeaders = customHeaders.getOrElse(Map.empty[String, String])
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/search/AzureSearchAuth.scala:78
- AzureSearchAuth.optionValue treats blank strings as real values when checking for conflicting options. That can incorrectly throw "Conflicting Azure Search options" if one alias is set to whitespace while the other contains the real credential (elsewhere this PR consistently treats blank as absent). Filter out null/blank values before distinct/conflict checking.
private def optionValue(options: Map[String, String], names: Seq[String]): Option[String] = {
val values = names.flatMap(options.get).distinct
require(values.size <= 1, s"Conflicting Azure Search options: ${names.mkString(" and ")}")
values.headOption
}
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
…undary so every generic persistence path is NPE-safe, strip telemetry auth headers, and ignore blank auth aliases ## Summary Addresses suppressed findings from the GitHub Copilot re-review of PR microsoft#2591, all on the shared cognitive-services auth-header path and the Azure Search option parser. This revision adds a fifth, persistence-focused finding on top of the original four: 1. ServiceAuthHeaders.build merged telemHeaders verbatim after the resolved credential, so a telemetry entry named api-key/Authorization (any casing) could override or duplicate the one canonical auth header, and a null outer Option / Some(null) / null map / null name would NPE. A new shared sanitizeHeaderMap normalizes any optional header map (null outer Option, None, Some(null), or a null underlying map collapse to empty; null names are dropped), and telemetry is now filtered through it and stripped of any api-key/Authorization name before merge. Legitimate non-auth telemetry (e.g. the default x-ai-telemetry-properties) is preserved. 2. build assumed customHeaders was never Some(null) and had no null names, which the writer entry point setCustomHeaders(java.util.HashMap) (and setCustomHeaders(null: Map) -> Some(null)) can violate. customHeaders is now routed through the same sanitizeHeaderMap at the shared boundary before embeddedCredential / isAuthHeaderName / iteration, so null containers/names never NPE and a null or blank embedded auth value is never emitted as auth. Prior precedence and generic-header behavior are unchanged. 3. AzureSearchAuth.optionValue treated blank/null alias values as real, so AADToken=" " plus aadToken=<valid> threw a bogus conflict even though blank is treated as absent everywhere else. optionValue now filters null/blank alias values (via ServiceAuthHeaders.nonBlank) before the distinct/conflict check -- without trimming and without leaking values in the failure message -- and subscriptionKey is routed through optionValue so key, AAD, and custom-auth aliases behave identically. 4. setCustomHeaders sanitized only during header assembly and still stored raw null maps/keys/values as the Spark customHeaders ServiceParam. Because customHeaders is a JsonEncodableParam (not a ComplexParam), ComplexParamsWritable.getMetadataToSave calls Param.jsonEncode(value) on it during transformer save, so a stored null map, null key, or null value NPEs / trips spray-json require(x ne null) (JsString(null)) at persistence time; the Java/PySpark setCustomHeaders(HashMap) overload also dereferenced a null map immediately. Both setCustomHeaders overloads now normalize through the shared ServiceAuthHeaders.sanitizeHeaderMap BEFORE setScalarParam (null map -> empty; null-named or null-valued entries dropped), and the Java overload null-guards the HashMap (Option(v).map(_.asScala.toMap).getOrElse(empty)) so it is never dereferenced. sanitizeHeaderMap now also drops null values (previously only null names), so the shared boundary reused by build() and by AzureSearchAuth never emits a null generic value, and AzureSearchAuth.sanitizedCustomHeaders is consolidated onto the same normalizer so its null rules cannot drift. Public signatures and legitimate headers are unchanged; build() still sanitizes as defense in depth for any other mutation route. 5. Finding 4's setter-time normalization only ran through setCustomHeaders. The public generic ServiceParam paths -- the typed setScalarParam(customHeaders, value), the string-name setScalarParam("customHeaders", value), and the raw Params.set(customHeaders, Left(value)) -- bypass the setters and store the value verbatim, so a null map, null key, or null value again reached ComplexParamsWritable.getMetadataToSave -> Param.jsonEncode and NPE'd / tripped spray-json require(x ne null) at save time (and the mirror path on load via jsonDecode). customHeaders is now an anonymous ServiceParam[Map[String, String]] subclass whose jsonEncode/jsonDecode normalize through the shared ServiceAuthHeaders.sanitizeHeaderMap at the param's own JSON boundary, so every generic setting/persistence path is null-safe regardless of how the raw value was stored. The val keeps an explicit ServiceParam[Map[String, String]] type annotation so the public field/getter JVM descriptor (ServiceParam<Map<String,String>>) is unchanged despite the anonymous subclass, and the setCustomHeaders overloads and build() still normalize as defense in depth. ## Prompting Intent Engineer asked to comprehensively but narrowly fix the concrete suppressed findings from the latest GitHub Copilot re-review of PR microsoft#2591, using TDD, on branch copilot/ancient-2285-search-aad on top of 8d6e744. Constraints for the original three findings: sanitize telemHeaders centrally (null outer Option / Some(null) / null map empty; null names dropped; any api-key/Authorization under any casing stripped; preserve legitimate telemetry); sanitize customHeaders at the shared boundary with the same null rules and no emitted malformed auth; filter null/blank alias values before the conflict check for key, AAD, and custom-auth aliases without trimming or leaking the credential. Add focused, secret-free tests, asserting exactly one canonical auth header and no credential values in rendered failures; preserve all public signatures and earlier hardening; avoid new discoverable PipelineStage test classes. Engineer then asked to fix the remaining reviewed null-map persistence issue (finding 4) with TDD: normalize or intentionally reject null maps/keys/values BEFORE setScalarParam in both the Scala and Java setCustomHeaders setters while retaining defense-in-depth boundary sanitization; prefer consistent normalization (null map -> empty, null key/value entries dropped); update the shared boundary sanitizer to avoid emitting null generic values; audit other setters/callers of customHeaders; preserve public signatures, legitimate headers, and auth precedence/filtering. Add tests for the Scala setter and the Java HashMap setter with null map/key/value asserting stored params are safe and that JSON encoding (and an actual save/load round-trip) does not NPE, that valid headers survive, and that auth precedence and filtering remain correct; avoid discoverable test PipelineStage classes. Engineer then asked to fix the remaining generic-param persistence bypass with TDD: the finding-4 setter-time sanitization only ran through setCustomHeaders, so the public generic paths -- setScalarParam by param or by "customHeaders" name, and Params.set -- could still store null maps/keys/values that ComplexParamsWritable later fed to CustomHeaders.jsonEncode, NPE-ing persistence. Enforce safe normalization (or a sanitized rejection) in the customHeaders ServiceParam itself so every generic setting/persistence path is safe, preferring normalization at the param encode/decode boundary to avoid leaking values in validation errors, while retaining build-boundary and dedicated-setter defense in depth. Preserve the public customHeaders getter/field JVM descriptor and all existing signatures (explicitly type-annotate the specialized param), audit copy/default/JSON decode behavior, and never leak credentials in exceptions. Reproduce via each accessible generic path, then exercise a real AddDocuments save/load (and the exact ComplexParamsWritable serializer step) asserting no NPE, null entries removed after the round-trip, legitimate headers preserved, and auth precedence intact. ## Linked Sources - PR: microsoft#2591 - GitHub Copilot re-review of PR microsoft#2591 (three suppressed findings + follow-up null-map persistence finding + generic-param persistence bypass) - Prior hardening commits: 8d6e744 (null-safe direct construction), aa07b15 (Fabric fallback rank + lazy eval + blank auth), 09e56b1 (case-insensitive dedupe + malformed customHeaders sanitize) - No AB# work item: continuation of the ancient-PR remediation series, which carries none. ## Rationale A single shared sanitizeHeaderMap helper handles both customHeaders and telemHeaders so the null rules (null outer Option / None / Some(null) / null map -> empty; drop null names) live in one place and cannot drift between the two sources. Telemetry auth entries are stripped with the existing case-insensitive isAuthHeaderName predicate (reused, not duplicated) and applied after the credential is resolved, so telemetry can never become an auth source regardless of whether a credential was found. Values are preserved verbatim (never trimmed) so a legitimate credential is never mutated, and generic non-auth headers keep their prior override semantics. For finding 3, blank filtering reuses ServiceAuthHeaders.nonBlank (the same predicate the resolver uses) so "absent" means the same thing in the parser and the resolver; routing subscriptionKey through optionValue makes all aliases consistent with no signature change. For finding 4, normalizing at the setter (before setScalarParam) fixes the defect at its source: the stored param can no longer hold a null map/key/value, so ComplexParamsWritable/spray-json persistence and the Java HashMap path are safe by construction, while build() keeps sanitizing as defense in depth for any other mutation route. Consistent normalization (null map -> empty, drop null-named/null-valued entries) was chosen over a sanitized rejection because the surrounding conventions already treat blank/absent headers as droppable and null header names/values are never valid HTTP headers, so silently dropping them preserves every legitimate header and keeps the existing null-handling tests behavior-compatible; sanitizeHeaderMap was extended (rather than adding a second helper) and AzureSearchAuth routed through it to keep the null-key/null-value rules in one place. TDD: reverting only the two main-source files reproduced the earlier findings (8 tests RED) and the fix turned them GREEN (42/42); the finding-4 follow-up added 6 tests RED against the pre-fix setters (null value stored, null Map -> Left(null) NPE, null HashMap deref, null key/value entries, boundary null generic, precedence-under-normalization) that went GREEN with the setter/boundary normalization, taking the full AzureSearchAuthSuite to 48/48 with cognitive main+test scalastyle clean; an end-to-end AddDocuments save()/load() round-trip with a null key + null value confirmed no persistence NPE (only the legitimate header survives). For finding 5, the authoritative fix moved to the param's own JSON boundary: all persistence -- save via ComplexParamsWritable.getMetadataToSave and load via getAndSetParams -- dispatches through the virtual Param.jsonEncode/jsonDecode, so overriding them on the customHeaders ServiceParam normalizes the value no matter which generic path (typed or string-name setScalarParam, or Params.set) stored it, closing the bypass the finding-4 setter-only normalization left open; the setters, the Java HashMap overload, and build() remain as defense in depth. Encode/decode normalization was chosen over a Param isValid rejection because isValid renders the offending value in its failure message (leaking header values) and cannot express "drop the bad entries", whereas encode-time sanitization silently drops null names/values so no rejection path can leak a credential and every legitimate header survives. The val is explicitly annotated ServiceParam[Map[String, String]] so the anonymous subclass never widens the inferred type -- javap confirms the public field and getter still expose ServiceParam<Map<String,String>> (descriptor ()Lcom/microsoft/azure/synapse/ml/param/ServiceParam;), preserving binary/source compatibility and the generated Python/R wrappers. TDD: 6 new pure tests drove each generic path RED against the exact serializer step (persistCustomHeaders = jsonDecode(jsonEncode(stored))) -- typed setScalarParam, string-name setScalarParam, Params.set(Left(null map)), null key/value entries, embedded-credential precedence under normalization, and a canary value asserting no leak -- and a real AddDocuments save()/load() round-trip drove the public ComplexParamsWritable path RED (NPE / require-failed at save); all went GREEN with the param-boundary override. Full run: AzureSearchAuthSuite 48/48 unchanged, the new AzureSearchGenericParamPersistenceSuite 6/6, and AddDocumentsHeaderPersistenceSuite 1/1 (55 total), with cognitive compile and cognitive main+test scalastyle clean and the javap descriptor verified unchanged. The generic-path tests live in a dedicated suite rather than appended to AzureSearchAuthSuite (which would exceed the 800-line scalastyle limit) and add no discoverable PipelineStage test classes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/CognitiveServiceBase.scala:226
- Typo in comment: "compatability" should be "compatibility".
// For Pyspark compatability accept Java HashMap as input to parameter
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Collaborator
Author
|
@copilot fix the typos |
…overload comments ## Summary Correct two misspellings of "compatability" to "compatibility" in the Pyspark-compatibility comments above the Java HashMap overloads of setCustomHeaders (HasCustomHeaders) and setTelemHeaders (HasTelemHeaders) in CognitiveServiceBase.scala. Comment-only change; no code, API, or behavior is modified. ## Prompting Intent Address the remaining actionable PR microsoft#2591 review feedback: in CognitiveServiceBase.scala replace both misspellings "compatability" with "compatibility", after verifying there are exactly two intended occurrences, making no unrelated source edits. ## Linked Sources - PR review feedback: microsoft#2591 ## Rationale Scoped strictly to the two flagged comment typos to keep the diff minimal and reviewable. Validated with cognitive compile, cognitive main/test scalastyle (0 errors), and the AzureSearchAuthSuite (48/48 passing) to confirm the comment change does not disturb the module. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
This was referenced Aug 4, 2026
Collaborator
Author
|
Follow-up to this PR is now open as #2604: it migrates the Azure AI Search data-plane API off the deprecated Split out from this PR deliberately so the auth work stayed reviewable on its own. The auth support merged here is independent of the api-version — Entra bearer tokens work on any supported version — so nothing here was blocked on it. |
ranadeepsingh
added a commit
to ranadeepsingh/SynapseML
that referenced
this pull request
Aug 4, 2026
## Summary Merge the latest master commits into microsoft#2581 after restoring the stacked CI changes and extending the Docker validation budget. ## Prompting Intent The engineer asked to refresh the parent PR against master, preserve all merged stack changes, and continue full validation until the current merge candidate is ready. ## Linked Sources - Parent PR: microsoft#2581 - CDN migration merged to master: microsoft#2589 - Azure Search AAD authentication merged to master: microsoft#2591 ## Rationale Merging master preserves the existing reviewed branch history and the official stack recovery commit without rewriting the fork. The merge is conflict-free, keeps the Docker timeout regression fix intact, and ensures the next Azure run tests the actual current integration candidate rather than a stale base. Co-authored-by: Copilot <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
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.
Related Issues/PRs
What changes are proposed in this pull request?
AzureSearchAuthconfiguration for subscription keys, AAD bearer tokens, custom authorization values, and custom headers.try/finally.How is this patch tested?
AzureSearchAuthSuiteunit tests covering key, AAD, custom header, missing credential, writer preparation, all index API request paths, redaction, malformed headers, and legacy API source compatibility.cognitive/Test/compilecognitive/testOnly com.microsoft.azure.synapse.ml.services.search.AzureSearchAuthSuite(10 tests)cognitive/scalastylecognitive/Test/scalastyleLive Azure Search integration suites remain behind the repository's existing credential gates and were not run locally.
Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?