Fix SSRF via nullable-bool allowlist fail-open in SessionsPythonPlugin

[tmacam]

Summary

Replaces a confusing, easy-to-misread nullable-bool operator-precedence expression in SessionsPythonPlugin.SendAsync() that governed the domain allowlist, and hardens/clarifies the surrounding validation.

Problem

The original validation expression:

if (!this._settings.AllowedDomains?.Contains(uri.Host) ?? false)

parses as (!nullable_bool) ?? false due to C# operator precedence:

AllowedDomains	?.Contains()	! result	?? false	Outcome
null	null	null	false	No throw (endpoint host allowed)
["a.io"], host = "a.io"	true	false	false	Allowed
["a.io"], host = "b.io"	false	true	true	Blocked

The expression is correct for the explicit-allowlist rows but is opaque and relies on bool? null-propagation through ! and ??, which is a well-known footgun and hard to audit for a security-sensitive check.

Fix

Replace the expression with explicit, readable logic and make the host comparison case-insensitive (DNS semantics; matches HttpPlugin/WebFileDownloadPlugin):

var effectiveAllowedDomains = this._settings.AllowedDomains
    ?? new[] { this._poolManagementEndpoint.Host };

if (!effectiveAllowedDomains.Contains(uri.Host, StringComparer.OrdinalIgnoreCase))
{
    throw new InvalidOperationException("Sending requests to the provided location is not allowed.");
}

Threat model / scope (updated after review)

An earlier version of this description claimed the null-AllowedDomains default blocks IMDS. That is not accurate for this plugin and has been corrected:

• In SendAsync, uri = new Uri(_poolManagementEndpoint, relativePath) and every caller passes a hardcoded relative path ("executions", "files", …), so uri.Host is invariantly _poolManagementEndpoint.Host.
• Therefore the null default ({ endpoint.Host }) is a readability / backward-compatibility replacement for the old (!x) ?? false expression — it does not add a restriction and does not, by itself, block IMDS.
• The only path that actually restricts which host the plugin talks to is an explicitly configured AllowedDomains, which is now correct and case-insensitive.

Why this is safe

• Backward compatible: users who never set AllowedDomains see no behavior change — requests go to the endpoint they configured at construction time.
• Readable / auditable: no nullable-bool subtlety — the intent is obvious.
• Explicit allowlist works correctly: arbitrary hosts are blocked when AllowedDomains is configured, regardless of casing.

Tests

Test	Verifies
ItShouldRespectAllowedDomainsAsync (+ new FAKE-TEST-HOST.io case)	Explicit allowlist matching, including case-insensitive host comparison
ItShouldAllowConfiguredEndpointHostWhenAllowedDomainsIsNullAsync	Null AllowedDomains permits the configured endpoint host, and the request is actually sent there
ItShouldBlockEndpointHostNotInAllowedDomainsAsync	An explicit allowlist blocks IMDS even when it is the configured endpoint

Known follow-up (out of scope here)

Redirect-based SSRF is not closed by this change: HttpClient follows 3xx redirects by default and the allowlist is only checked against the initial URI. Because the client comes from the injected IHttpClientFactory, this plugin cannot force AllowAutoRedirect = false on an already-constructed client without changing the construction contract. This is pre-existing and best handled as a focused follow-up (named/typed client registration with a non-redirecting handler).

References

• MSRC Case 125929 / VULN-200115
• ICM 31000000656989

[Copilot]

Pull request overview

This PR updates the SessionsPythonPlugin domain allowlist validation to remove a nullable-bool precedence pitfall, aiming to prevent a fail-open scenario when AllowedDomains is left unconfigured, and adds unit tests to cover the intended behavior/regression.

Changes:

• Replaces the nullable/?? allowlist check in SendAsync with explicit “effective allowed domains” logic.
• Adds tests for AllowedDomains = null behavior and a regression test tied to the referenced MSRC case.
• Normalizes file headers (BOM removal) in the touched files.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonPlugin.cs	Reworks host allowlist validation logic in SendAsync to avoid nullable-bool precedence fail-open.
dotnet/src/Plugins/Plugins.UnitTests/Core/SessionsPythonPluginTests.cs	Adds new tests covering the null-AllowedDomains scenario and a regression case.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.