Add api-key and Ocp-Apim-Subscription-Key to DEFAULT_SENSITIVE_HEADERS

[Devendra Kumar Sinha (Devendrakumarsinha)]

Hi Team,

While doing some security testing on azure-core's redirect handling,
I noticed that api-key and Ocp-Apim-Subscription-Key headers are not
being stripped on cross-origin redirects, unlike Authorization which
is correctly handled.

I tested this with a simple setup - two local servers on different
ports (which azure-core treats as different domains via get_domain()),
and confirmed that when a redirect happens across origins, the api-key
header travels to the new destination without being stripped.

Tested on all redirect status codes (301, 302, 303, 307, 308) - same
result on all of them.

The fix is straightforward - just adding these two headers to
DEFAULT_SENSITIVE_HEADERS in SensitiveHeaderCleanupPolicy, similar
to how Authorization is already handled there.

I also checked AzureKeyCredentialPolicy and it sets the api-key header
directly, so this cleanup policy is the right place to fix it.

Happy to add tests if needed. Let me know if you have any questions.

Thanks

[github-actions]

Thank you for your contribution Devendra Kumar Sinha (@Devendrakumarsinha)! We will review the pull request and get back to you soon.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
5 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

🟡 Changes recommended

The nested list causes an import-time TypeError, and regression coverage is missing.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds API-key headers to redirect cleanup to prevent credential leakage across origins.

Changes:

• Marks api-key and Ocp-Apim-Subscription-Key as sensitive headers.

File summaries

File	Description
_sensitive_header_cleanup_policy.py	Extends default redirect-sensitive headers.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Devendra Kumar Sinha (Devendrakumarsinha)]

Thanks for the review! Fixed the indentation issue.
The nested list has been corrected.

[Devendra Kumar Sinha (Devendrakumarsinha)]

Happy to add tests. Will add a regression test that verifies
api-key and Ocp-Apim-Subscription-Key are stripped on
cross-domain redirect. Let me know if you'd like me to proceed.

[Copilot]

🟡 Changes recommended

The previously requested cross-origin redirect regression tests remain absent.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Devendra Kumar Sinha (Devendrakumarsinha)]

@microsoft-github-policy-service agree

[Copilot]

🟡 Changes recommended

The regression test can pass without following the redirect and introduces flaky resource handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

sdk/core/azure-core/tests/test_pipeline.py:614

• The test can pass without ever reaching the redirect target: if no request populates captured, every lookup falls back to {} and all three absence assertions succeed. Assert that captured["headers"] exists first, then inspect that value so the regression test proves a redirected request was actually sent.

        # After cross-origin redirect, sensitive headers must be stripped
        assert "api-key" not in captured.get("headers", {}), \
            "api-key should be stripped on cross-origin redirect"
        assert "ocp-apim-subscription-key" not in captured.get("headers", {}), \
            "Ocp-Apim-Subscription-Key should be stripped on cross-origin redirect"
        assert "authorization" not in captured.get("headers", {}), \
            "Authorization should be stripped on cross-origin redirect"

sdk/core/azure-core/tests/test_pipeline.py:617

• shutdown() stops serve_forever() but does not close the listening socket, so this test leaks both server descriptors until garbage collection. Explicitly close both servers during teardown.

    finally:
        server_a.shutdown()
        server_b.shutdown()

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Balanced

[Devendra Kumar Sinha (Devendrakumarsinha)]

Added Bugs Fixed entry to CHANGELOG.md under 1.42.0 (Unreleased).

[Copilot]

🟡 Changes recommended

The regression test leaks its transport session and both server sockets.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The implementation, regression test, and changelog entry consistently address the reported header-leak issue.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Devendra Kumar Sinha (Devendrakumarsinha)]

Hi team, checked the test results - 0 test failures out of 86,696 tests.
The Build Analyze failure appears to be an environment issue (Black passes locally with v26.5.1).
The ib4 batch failures seem pre-existing.
Happy to help debug if needed.