diff --git a/dotnet/src/InternalUtilities/src/Http/HttpClientProvider.cs b/dotnet/src/InternalUtilities/src/Http/HttpClientProvider.cs index 3a6b5b9fe111..e5f79b3bad21 100644 --- a/dotnet/src/InternalUtilities/src/Http/HttpClientProvider.cs +++ b/dotnet/src/InternalUtilities/src/Http/HttpClientProvider.cs @@ -26,6 +26,12 @@ internal static class HttpClientProvider /// An instance of HttpClient. public static HttpClient GetHttpClient() => new(NonDisposableHttpClientHandler.Instance, disposeHandler: false); + /// + /// Retrieves an instance of HttpClient that does not automatically follow HTTP redirects. + /// + /// An instance of HttpClient that does not follow redirects. + public static HttpClient GetNonRedirectingHttpClient() => new(NonDisposableHttpClientHandler.NonRedirectingInstance, disposeHandler: false); + /// /// Retrieves an instance of HttpClient. /// @@ -52,14 +58,19 @@ private sealed class NonDisposableHttpClientHandler : DelegatingHandler /// /// Private constructor to prevent direct instantiation of the class. /// - private NonDisposableHttpClientHandler() : base(CreateHandler()) + private NonDisposableHttpClientHandler(bool allowRedirect) : base(CreateHandler(allowRedirect)) { } /// - /// Gets the singleton instance of . + /// Gets the singleton instance of that follows HTTP redirects. + /// + public static NonDisposableHttpClientHandler Instance { get; } = new(allowRedirect: true); + + /// + /// Gets the singleton instance of that does not follow HTTP redirects. /// - public static NonDisposableHttpClientHandler Instance { get; } = new(); + public static NonDisposableHttpClientHandler NonRedirectingInstance { get; } = new(allowRedirect: false); /// /// Disposes the underlying resources held by the . @@ -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() { @@ -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; @@ -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 } } diff --git a/dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs b/dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs index 89f49d8d4705..ea11b17a258a 100644 --- a/dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs @@ -23,6 +23,9 @@ namespace Microsoft.SemanticKernel.Plugins.Core; /// When exposing this plugin to an LLM via auto function calling, ensure that /// is restricted to trusted values only. /// +/// +/// The default HTTP client does not follow redirects to prevent bypassing the allow-list. +/// /// [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1054:URI-like parameters should not be strings", Justification = "Semantic Kernel operates on strings")] @@ -43,10 +46,11 @@ public HttpPlugin() : this(null) /// The HTTP client to use. /// /// assumes ownership of the instance and will dispose it when the plugin is disposed. + /// When providing a custom client, configure it with AllowAutoRedirect = false to preserve the guarantee. /// [ActivatorUtilitiesConstructor] public HttpPlugin(HttpClient? client = null) => - this._client = client ?? HttpClientProvider.GetHttpClient(); + this._client = client ?? HttpClientProvider.GetNonRedirectingHttpClient(); /// /// List of allowed domains to send requests to. @@ -54,6 +58,7 @@ public HttpPlugin(HttpClient? client = null) => /// /// 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. /// public IEnumerable? AllowedDomains { diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Core/HttpPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Core/HttpPluginTests.cs index d867e56c0630..f9a63f84264d 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Core/HttpPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Core/HttpPluginTests.cs @@ -133,6 +133,53 @@ public async Task ItThrowsInvalidOperationExceptionForInvalidDomainAsync() await Assert.ThrowsAsync(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(() => plugin.GetAsync($"http://localhost:{port}/start")); + Assert.False(redirectTargetContacted, "The redirect target should not have been contacted."); + + listener.Stop(); + } + private Mock CreateMock() { var mockHandler = new Mock(); diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs index 6d0ce6f5aeb4..92a15be0c1df 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs @@ -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; @@ -77,6 +78,70 @@ public async Task DownloadToFileFailsForInvalidDomainAsync() await Assert.ThrowsAsync(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(() => 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)); + } + finally + { + listener.Stop(); + if (Path.Exists(folderPath)) + { + Directory.Delete(folderPath, true); + } + } + } + [Fact] public async Task DownloadToFileDeniesAllWithDefaultConfigAsync() { diff --git a/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs b/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs index d410bb8e22f0..acdcd9659fd6 100644 --- a/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs @@ -29,6 +29,9 @@ namespace Microsoft.SemanticKernel.Plugins.Web; /// values only. Unrestricted configuration may allow unintended downloads to /// local paths. /// +/// +/// The default HTTP client does not follow redirects to prevent bypassing the allow-list. +/// /// public sealed class WebFileDownloadPlugin { @@ -42,7 +45,7 @@ public sealed class WebFileDownloadPlugin /// /// The to use for logging. If null, no logging will be performed. public WebFileDownloadPlugin(ILoggerFactory? loggerFactory = null) : - this(HttpClientProvider.GetHttpClient(), loggerFactory) + this(HttpClientProvider.GetNonRedirectingHttpClient(), loggerFactory) { } @@ -51,6 +54,9 @@ public WebFileDownloadPlugin(ILoggerFactory? loggerFactory = null) : /// /// The HTTP client to use for making requests. /// The to use for logging. If null, no logging will be performed. + /// + /// When providing a custom client, configure it with AllowAutoRedirect = false to preserve the guarantee. + /// public WebFileDownloadPlugin(HttpClient httpClient, ILoggerFactory? loggerFactory = null) { this._httpClient = httpClient; @@ -63,6 +69,7 @@ public WebFileDownloadPlugin(HttpClient httpClient, ILoggerFactory? loggerFactor /// /// 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. /// public IEnumerable? AllowedDomains {