Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions dotnet/src/InternalUtilities/src/Http/HttpClientProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ internal static class HttpClientProvider
/// <returns>An instance of HttpClient.</returns>
public static HttpClient GetHttpClient() => new(NonDisposableHttpClientHandler.Instance, disposeHandler: false);

/// <summary>
/// Retrieves an instance of HttpClient that does not automatically follow HTTP redirects.
/// </summary>
/// <returns>An instance of HttpClient that does not follow redirects.</returns>
public static HttpClient GetNonRedirectingHttpClient() => new(NonDisposableHttpClientHandler.NonRedirectingInstance, disposeHandler: false);

/// <summary>
/// Retrieves an instance of HttpClient.
/// </summary>
Expand All @@ -52,14 +58,19 @@ private sealed class NonDisposableHttpClientHandler : DelegatingHandler
/// <summary>
/// Private constructor to prevent direct instantiation of the class.
/// </summary>
private NonDisposableHttpClientHandler() : base(CreateHandler())
private NonDisposableHttpClientHandler(bool allowRedirect) : base(CreateHandler(allowRedirect))
{
}

/// <summary>
/// Gets the singleton instance of <see cref="NonDisposableHttpClientHandler"/>.
/// Gets the singleton instance of <see cref="NonDisposableHttpClientHandler"/> that follows HTTP redirects.
/// </summary>
public static NonDisposableHttpClientHandler Instance { get; } = new(allowRedirect: true);

/// <summary>
/// Gets the singleton instance of <see cref="NonDisposableHttpClientHandler"/> that does not follow HTTP redirects.
/// </summary>
public static NonDisposableHttpClientHandler Instance { get; } = new();
public static NonDisposableHttpClientHandler NonRedirectingInstance { get; } = new(allowRedirect: false);

/// <summary>
/// Disposes the underlying resources held by the <see cref="NonDisposableHttpClientHandler"/>.
Expand All @@ -74,7 +85,7 @@ protected override void Dispose(bool disposing)
}

#if NET
private static SocketsHttpHandler CreateHandler()
private static SocketsHttpHandler CreateHandler(bool allowRedirect)
{
return new SocketsHttpHandler()
{
Expand All @@ -86,12 +97,13 @@ private static SocketsHttpHandler CreateHandler()
{
CertificateRevocationCheckMode = X509RevocationMode.Online,
},
AllowAutoRedirect = allowRedirect,
};
}
#elif NETSTANDARD2_0_OR_GREATER
private static HttpClientHandler CreateHandler()
private static HttpClientHandler CreateHandler(bool allowRedirect)
{
var handler = new HttpClientHandler();
var handler = new HttpClientHandler() { AllowAutoRedirect = allowRedirect };
try
{
handler.CheckCertificateRevocationList = true;
Expand All @@ -100,8 +112,8 @@ private static HttpClientHandler CreateHandler()
return handler;
}
#elif NETFRAMEWORK
private static HttpClientHandler CreateHandler()
=> new();
private static HttpClientHandler CreateHandler(bool allowRedirect)
=> new() { AllowAutoRedirect = allowRedirect };
#endif
}
}
7 changes: 6 additions & 1 deletion dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ namespace Microsoft.SemanticKernel.Plugins.Core;
/// When exposing this plugin to an LLM via auto function calling, ensure that
/// <see cref="AllowedDomains"/> is restricted to trusted values only.
/// </para>
/// <para>
/// The default HTTP client does not follow redirects to prevent bypassing the allow-list.
/// </para>
/// </remarks>
[System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1054:URI-like parameters should not be strings",
Justification = "Semantic Kernel operates on strings")]
Expand All @@ -43,17 +46,19 @@ public HttpPlugin() : this(null)
/// <param name="client">The HTTP client to use.</param>
/// <remarks>
/// <see cref="HttpPlugin"/> assumes ownership of the <see cref="HttpClient"/> instance and will dispose it when the plugin is disposed.
/// When providing a custom client, configure it with <c>AllowAutoRedirect = false</c> to preserve the <see cref="AllowedDomains"/> guarantee.
/// </remarks>
[ActivatorUtilitiesConstructor]
public HttpPlugin(HttpClient? client = null) =>
this._client = client ?? HttpClientProvider.GetHttpClient();
this._client = client ?? HttpClientProvider.GetNonRedirectingHttpClient();

/// <summary>
/// List of allowed domains to send requests to.
/// </summary>
/// <remarks>
/// Defaults to an empty collection (no domains allowed). Must be explicitly populated
/// with trusted domains before any requests will succeed.
/// HTTP redirects are not followed to prevent bypassing the allow-list.
/// </remarks>
public IEnumerable<string>? AllowedDomains
{
Expand Down
47 changes: 47 additions & 0 deletions dotnet/src/Plugins/Plugins.UnitTests/Core/HttpPluginTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,53 @@ public async Task ItThrowsInvalidOperationExceptionForInvalidDomainAsync()
await Assert.ThrowsAsync<InvalidOperationException>(async () => await plugin.DeleteAsync(invalidUri));
}

[Fact]
public async Task ItDoesNotFollowRedirectsAsync()
{
// Arrange - start a local server that always returns a 302 redirect
using var listener = new System.Net.HttpListener();
var port = new Random().Next(49152, 65535);
listener.Prefixes.Add($"http://localhost:{port}/");
listener.Start();
bool redirectTargetContacted = false;

_ = Task.Run(async () =>
{
while (listener.IsListening)
{
try
{
var ctx = await listener.GetContextAsync();
if (ctx.Request.Url!.AbsolutePath == "/start")
{
ctx.Response.StatusCode = 302;
ctx.Response.RedirectLocation = $"http://localhost:{port}/secret";
ctx.Response.Close();
}
else if (ctx.Request.Url.AbsolutePath == "/secret")
{
redirectTargetContacted = true;
ctx.Response.StatusCode = 200;
ctx.Response.Close();
}
}
catch (ObjectDisposedException) { break; }
catch (System.Net.HttpListenerException) { break; }
}
});

var plugin = new HttpPlugin()
{
AllowedDomains = ["localhost"]
};

// Act & Assert - the plugin should throw because 302 is a non-success status
await Assert.ThrowsAsync<HttpOperationException>(() => plugin.GetAsync($"http://localhost:{port}/start"));
Assert.False(redirectTargetContacted, "The redirect target should not have been contacted.");

listener.Stop();
}
Comment thread
SergeyMenshykh marked this conversation as resolved.

private Mock<HttpMessageHandler> CreateMock()
{
var mockHandler = new Mock<HttpMessageHandler>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Net.Http;
using System.Runtime.InteropServices;
using System.Threading.Tasks;
using Microsoft.SemanticKernel;
using Microsoft.SemanticKernel.Plugins.Web;
using Xunit;

Expand Down Expand Up @@ -77,6 +78,70 @@ public async Task DownloadToFileFailsForInvalidDomainAsync()
await Assert.ThrowsAsync<InvalidOperationException>(async () => await webFileDownload.DownloadToFileAsync(uri, filePath));
}

[Fact]
public async Task DownloadToFileDoesNotFollowRedirectsAsync()
{
// Arrange - start a local server that returns a 302 redirect
using var listener = new System.Net.HttpListener();
var port = new Random().Next(49152, 65535);
listener.Prefixes.Add($"http://localhost:{port}/");
listener.Start();
bool redirectTargetContacted = false;

_ = Task.Run(async () =>
{
while (listener.IsListening)
{
try
{
var ctx = await listener.GetContextAsync();
if (ctx.Request.Url!.AbsolutePath == "/start")
{
ctx.Response.StatusCode = 302;
ctx.Response.RedirectLocation = $"http://localhost:{port}/secret.png";
ctx.Response.Close();
}
else if (ctx.Request.Url.AbsolutePath == "/secret.png")
{
redirectTargetContacted = true;
ctx.Response.StatusCode = 200;
ctx.Response.ContentType = "image/png";
ctx.Response.OutputStream.Write(new byte[] { 0x89, 0x50, 0x4E, 0x47 });
ctx.Response.Close();
}
}
catch (ObjectDisposedException) { break; }
catch (System.Net.HttpListenerException) { break; }
}
});

var folderPath = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
var filePath = Path.Combine(folderPath, "file.png");
Directory.CreateDirectory(folderPath);

var webFileDownload = new WebFileDownloadPlugin()
{
AllowedDomains = ["localhost"],
AllowedFolders = [folderPath]
};

try
{
// Act & Assert - the plugin should throw because 302 is a non-success status
await Assert.ThrowsAsync<HttpOperationException>(() => webFileDownload.DownloadToFileAsync(new Uri($"http://localhost:{port}/start"), filePath));
Assert.False(redirectTargetContacted, "The redirect target should not have been contacted.");
Assert.False(Path.Exists(filePath));
}
Comment thread
SergeyMenshykh marked this conversation as resolved.
finally
{
listener.Stop();
if (Path.Exists(folderPath))
{
Directory.Delete(folderPath, true);
}
}
}

[Fact]
public async Task DownloadToFileDeniesAllWithDefaultConfigAsync()
{
Expand Down
9 changes: 8 additions & 1 deletion dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ namespace Microsoft.SemanticKernel.Plugins.Web;
/// values only. Unrestricted configuration may allow unintended downloads to
/// local paths.
/// </para>
/// <para>
/// The default HTTP client does not follow redirects to prevent bypassing the allow-list.
/// </para>
/// </remarks>
public sealed class WebFileDownloadPlugin
{
Expand All @@ -42,7 +45,7 @@ public sealed class WebFileDownloadPlugin
/// </summary>
/// <param name="loggerFactory">The <see cref="ILoggerFactory"/> to use for logging. If null, no logging will be performed.</param>
public WebFileDownloadPlugin(ILoggerFactory? loggerFactory = null) :
this(HttpClientProvider.GetHttpClient(), loggerFactory)
this(HttpClientProvider.GetNonRedirectingHttpClient(), loggerFactory)
{
}

Expand All @@ -51,6 +54,9 @@ public WebFileDownloadPlugin(ILoggerFactory? loggerFactory = null) :
/// </summary>
/// <param name="httpClient">The HTTP client to use for making requests.</param>
/// <param name="loggerFactory">The <see cref="ILoggerFactory"/> to use for logging. If null, no logging will be performed.</param>
/// <remarks>
/// When providing a custom client, configure it with <c>AllowAutoRedirect = false</c> to preserve the <see cref="AllowedDomains"/> guarantee.
/// </remarks>
public WebFileDownloadPlugin(HttpClient httpClient, ILoggerFactory? loggerFactory = null)
{
this._httpClient = httpClient;
Expand All @@ -63,6 +69,7 @@ public WebFileDownloadPlugin(HttpClient httpClient, ILoggerFactory? loggerFactor
/// <remarks>
/// Defaults to an empty collection (no domains allowed). Must be explicitly populated
/// with trusted domains before any downloads will succeed.
/// HTTP redirects are not followed to prevent bypassing the allow-list.
/// </remarks>
public IEnumerable<string>? AllowedDomains
{
Expand Down
Loading