Fix the redirect auth issues#47265
Merged
Merged
Conversation
rads-1996
requested review from
JacksonWeber,
MSNev,
hectorhdzg,
jeremydvoss and
lzchen
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an ICM-tracked security/auth issue where a server-issued redirect could cause the exporter's auth policy to attach a freshly-signed Authorization header to an attacker-controlled host. The exporter now refuses redirects whose target is not under the "same registered domain" as the currently-configured ingestion endpoint, and updates the existing redirect tests to use realistic ingestion-style hosts.
Changes:
- Add
_is_same_registered_domainheuristic (same-host or leftmost-label-only difference with ≥3-label shared suffix and ≥4 total labels) and gate the recursive_transmitredirect path on it; refused redirects produceFAILED_NOT_RETRYABLE, log an error, and track dropped items via customer sdkstats. - Update
test_transmit_http_error_redirectandtest_redirect_does_not_double_consume_rate_limiterto redirect to a host that passes the new guard. - Add
test_transmit_http_error_redirect_refuses_cross_originandtest_is_same_registered_domainto cover the new behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/_base.py | Adds cross-origin redirect guard and helper. |
| sdk/monitor/azure-monitor-opentelemetry-exporter/tests/test_base_exporter.py | Updates existing redirect test and adds tests for refusal + helper. |
| sdk/monitor/azure-monitor-opentelemetry-exporter/tests/test_rate_limiter.py | Updates redirect target to a host that satisfies the new guard. |
rads-1996
force-pushed
the
fix-redirect-auth-issues
branch
from
309db26 to
f6f1216
Compare
JacksonWeber
approved these changes
Jun 1, 2026
Member
Author
|
BEFORE - AFTER - |
rads-1996
force-pushed
the
fix-redirect-auth-issues
branch
from
f6f1216 to
0e576ed
Compare
rads-1996
force-pushed
the
fix-redirect-auth-issues
branch
from
0e576ed to
13513f1
Compare
3 tasks
JacksonWeber
approved these changes
Jun 2, 2026
JacksonWeber
added a commit
to Azure/azure-sdk-for-js
that referenced
this pull request
Jun 2, 2026
# Description Fixes the [MSRC 119493](https://portal.microsofticm.com/imp/v5/incidents/details/31000000623459/summary) defect family in the Node.js exporter, mirroring the Python fix in [Azure/azure-sdk-for-python#47265](Azure/azure-sdk-for-python#47265). ## Defect `sdk/monitor/monitor-opentelemetry-exporter` disables `@azure/core-rest-pipeline`'s `redirectPolicy` (`httpSender.ts`) and handles 30x responses itself in `baseSender.ts`. On any 307/308 the previous `handlePermanentRedirect`: - accepted **any** `Location` host with no origin allowlist; - mutated `appInsightsClientOptions.host` and rebuilt the `ApplicationInsightsClient`, persisting the new host for the lifetime of the exporter; and - on the recursive `exportEnvelopes` call, the rebuilt pipeline still contained `bearerTokenAuthenticationPolicy` (when a `credential` is configured), so a freshly-signed AAD token for `https://monitor.azure.com/.default` plus the telemetry envelope were POSTed to the redirect target. This is the same code-shape MSRC paid out against the Python exporter (#119493), the .NET exporter (researcher Finding #12), and the Java exporter (researcher Finding #14). ## Fix - New `ALLOWED_REDIRECT_DOMAIN_SUFFIXES` constant (`Declarations/Constants.ts`) listing the canonical Azure Monitor / Application Insights ingestion suffixes across public, US Gov, and China clouds (identical list to the Python PR). - New `isSameRegisteredDomain(currentHost, redirectHost)` helper (`utils/redirectUtils.ts`) that returns `true` only when the redirect target is an **exact host match** for the configured ingestion host, or when **both** hosts live under the **same** trusted suffix. Customers with a custom ingestion host therefore still get the single-hop "remember the redirect" UX for an exact-host redirect, but server-issued cross-host redirects are refused. - `HttpSender.handlePermanentRedirect` now returns a `boolean`: `false` (without mutating any state) when the redirect would cross the trust boundary, `true` otherwise. The abstract signature on `BaseSender` is updated to match. - `BaseSender.exportEnvelopes` treats a `false` return as a non-retriable failure: it does **not** recurse into another `send`, surfaces an `ExportResultCode.FAILED` with a `Refused cross-origin redirect` error, and counts the exception via `networkStatsbeat.countException` / `customerSDKStatsMetrics.countDroppedItems(DropCode.CLIENT_EXCEPTION, ..., ExceptionType.CLIENT_EXCEPTION)`. - Diag log added at `error` level when a refusal occurs so operators can identify the misbehaving server / proxy. ## Tests - New `test/internal/redirectUtils.spec.ts` covering exact-match, same-suffix, cross-suffix, untrusted-parent (PoC scenario), trusted↔untrusted, empty inputs, user-info / port, and case / trailing-dot normalization. - New `BaseSender` test: `should refuse cross-origin redirects without retrying` — asserts `send` is invoked exactly once, the result is `FAILED` with `Refused cross-origin redirect`, and `networkStatsbeat.countException` is called. - New `HttpSender` test: `should refuse a cross-origin redirect and not leak telemetry to a foreign host` — uses `nock` to assert the exporter does **not** POST to the attacker host and that `appInsightsClient.host` remains the originally configured `DEFAULT_BREEZE_ENDPOINT` (no persistent host poisoning). - Existing `HttpSender` redirect tests that redirected from `dc.services.visualstudio.com` to `ukwest-0.in.applicationinsights.azure.com` (which crosses trusted suffixes and would now be refused) are retargeted to `westus.services.visualstudio.com`, matching the Python PR's analogous test update. - Existing `BaseSender` test mock returns `true` by default so legacy positive-path tests continue to exercise the success branch; the circular-redirect test is unaffected (refusal is gated by `numConsecutiveRedirects < 10`). ## Verification - `pnpm turbo build --filter @azure/monitor-opentelemetry-exporter... --token 1` — green. - `pnpm format` / `npm run lint` — green (only pre-existing warnings, none from new code). - Full vitest suite: **20 files, 347 tests, all passing.** ## Version Bumps `@azure/monitor-opentelemetry-exporter` from `1.0.0-beta.42` to `1.0.0-beta.43`. ## Checklist - [x] **The pull request does not introduce [breaking changes]** — the only public surface change is `handlePermanentRedirect`'s return type on `BaseSender`, which is `@internal`. - [x] CHANGELOG updated under `1.0.0-beta.43 (Unreleased)`. - [x] Tests added for the security-relevant behavior. Co-authored-by: Jackson Weber <jacwebe@microsoft.com> 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.
Description
This PR fixes the issues highlighted in the ICM
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines