Fix missing logging of connections by disallowed IPs (#14011)

Jellyfin.Api/Middleware/IpBasedAccessValidationMiddleware.cs

@@ -1,8 +1,10 @@
 using System.Net;
 using System.Threading.Tasks;
+using System.Web;
 using MediaBrowser.Common.Extensions;
 using MediaBrowser.Common.Net;
 using Microsoft.AspNetCore.Http;
+using Microsoft.Extensions.Logging;
 
 namespace Jellyfin.Api.Middleware;
 
@@ -12,14 +14,17 @@ namespace Jellyfin.Api.Middleware;
 public class IPBasedAccessValidationMiddleware
 {
     private readonly RequestDelegate _next;
+    private readonly ILogger<IPBasedAccessValidationMiddleware> _logger;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="IPBasedAccessValidationMiddleware"/> class.
     /// </summary>
     /// <param name="next">The next delegate in the pipeline.</param>
-    public IPBasedAccessValidationMiddleware(RequestDelegate next)
+    /// <param name="logger">The logger to log to.</param>
+    public IPBasedAccessValidationMiddleware(RequestDelegate next, ILogger<IPBasedAccessValidationMiddleware> logger)
     {
         _next = next;
+        _logger = logger;
     }
 
     /// <summary>
@@ -32,16 +37,23 @@ public class IPBasedAccessValidationMiddleware
     {
         if (httpContext.IsLocal())
         {
-            // Running locally.
+            // Accessing from the same machine as the server.
             await _next(httpContext).ConfigureAwait(false);
             return;
         }
 
-        var remoteIP = httpContext.Connection.RemoteIpAddress ?? IPAddress.Loopback;
+        var remoteIP = httpContext.GetNormalizedRemoteIP();
 
-        if (!networkManager.HasRemoteAccess(remoteIP))
+        var result = networkManager.ShouldAllowServerAccess(remoteIP);
+        if (result != RemoteAccessPolicyResult.Allow)
         {
             // No access from network, respond with 503 instead of 200.
+            _logger.LogWarning(
+                "Blocking request to {Path} by {RemoteIP} due to IP filtering rule, reason: {Reason}",
+                // url-encode to block log injection
+                HttpUtility.UrlEncode(httpContext.Request.Path),
+                remoteIP,
+                result);
             httpContext.Response.StatusCode = StatusCodes.Status503ServiceUnavailable;
             return;
         }

Jellyfin.Api/Middleware/LanFilteringMiddleware.cs

@@ -1,51 +0,0 @@
-using System.Net;
-using System.Threading.Tasks;
-using MediaBrowser.Common.Extensions;
-using MediaBrowser.Common.Net;
-using MediaBrowser.Controller.Configuration;
-using Microsoft.AspNetCore.Http;
-
-namespace Jellyfin.Api.Middleware;
-
-/// <summary>
-/// Validates the LAN host IP based on application configuration.
-/// </summary>
-public class LanFilteringMiddleware
-{
-    private readonly RequestDelegate _next;
-
-    /// <summary>
-    /// Initializes a new instance of the <see cref="LanFilteringMiddleware"/> class.
-    /// </summary>
-    /// <param name="next">The next delegate in the pipeline.</param>
-    public LanFilteringMiddleware(RequestDelegate next)
-    {
-        _next = next;
-    }
-
-    /// <summary>
-    /// Executes the middleware action.
-    /// </summary>
-    /// <param name="httpContext">The current HTTP context.</param>
-    /// <param name="networkManager">The network manager.</param>
-    /// <param name="serverConfigurationManager">The server configuration manager.</param>
-    /// <returns>The async task.</returns>
-    public async Task Invoke(HttpContext httpContext, INetworkManager networkManager, IServerConfigurationManager serverConfigurationManager)
-    {
-        if (serverConfigurationManager.GetNetworkConfiguration().EnableRemoteAccess)
-        {
-            await _next(httpContext).ConfigureAwait(false);
-            return;
-        }
-
-        var host = httpContext.GetNormalizedRemoteIP();
-        if (!networkManager.IsInLocalNetwork(host))
-        {
-            // No access from network, respond with 503 instead of 200.
-            httpContext.Response.StatusCode = StatusCodes.Status503ServiceUnavailable;
-            return;
-        }
-
-        await _next(httpContext).ConfigureAwait(false);
-    }
-}

Jellyfin.Server/Extensions/ApiApplicationBuilderExtensions.cs

@@ -68,16 +68,6 @@ namespace Jellyfin.Server.Extensions
             return appBuilder.UseMiddleware<IPBasedAccessValidationMiddleware>();
         }
 
-        /// <summary>
-        /// Adds LAN based access filtering to the application pipeline.
-        /// </summary>
-        /// <param name="appBuilder">The application builder.</param>
-        /// <returns>The updated application builder.</returns>
-        public static IApplicationBuilder UseLanFiltering(this IApplicationBuilder appBuilder)
-        {
-            return appBuilder.UseMiddleware<LanFilteringMiddleware>();
-        }
-
         /// <summary>
         /// Enables url decoding before binding to the application pipeline.
         /// </summary>

Jellyfin.Server/Startup.cs

@@ -208,7 +208,6 @@ namespace Jellyfin.Server
                 mainApp.UseRouting();
                 mainApp.UseAuthorization();
 
-                mainApp.UseLanFiltering();
                 mainApp.UseIPBasedAccessValidation();
                 mainApp.UseWebSocketHandler();
                 mainApp.UseServerStartupMessage();

MediaBrowser.Common/Extensions/HttpContextExtensions.cs

@@ -12,7 +12,7 @@ namespace MediaBrowser.Common.Extensions
         /// Checks the origin of the HTTP context.
         /// </summary>
         /// <param name="context">The incoming HTTP context.</param>
-        /// <returns><c>true</c> if the request is coming from LAN, <c>false</c> otherwise.</returns>
+        /// <returns><c>true</c> if the request is coming from the same machine as is running the server, <c>false</c> otherwise.</returns>
         public static bool IsLocal(this HttpContext context)
         {
             return (context.Connection.LocalIpAddress is null

MediaBrowser.Common/Net/INetworkManager.cs

@@ -127,7 +127,7 @@ namespace MediaBrowser.Common.Net
         /// Checks if <paramref name="remoteIP"/> has access to the server.
         /// </summary>
         /// <param name="remoteIP">IP address of the client.</param>
-        /// <returns><b>True</b> if it has access, otherwise <b>false</b>.</returns>
+        /// <returns>The result of evaluating the access policy, <c>Allow</c> if it should be allowed.</returns>
-        bool HasRemoteAccess(IPAddress remoteIP);
+        RemoteAccessPolicyResult ShouldAllowServerAccess(IPAddress remoteIP);
     }
 }

MediaBrowser.Common/Net/RemoteAccessPolicyResult.cs

@@ -0,0 +1,29 @@
+using System;
+
+namespace MediaBrowser.Common.Net;
+
+/// <summary>
+/// Result of <see cref="INetworkManager.ShouldAllowServerAccess" />.
+/// </summary>
+public enum RemoteAccessPolicyResult
+{
+    /// <summary>
+    /// The connection should be allowed.
+    /// </summary>
+    Allow,
+
+    /// <summary>
+    /// The connection should be rejected since it is not from a local IP and remote access is disabled.
+    /// </summary>
+    RejectDueToRemoteAccessDisabled,
+
+    /// <summary>
+    /// The connection should be rejected since it is from a blocklisted IP.
+    /// </summary>
+    RejectDueToIPBlocklist,
+
+    /// <summary>
+    /// The connection should be rejected since it is from a remote IP that is not in the allowlist.
+    /// </summary>
+    RejectDueToNotAllowlistedRemoteIP,
+}

src/Jellyfin.Networking/Manager/NetworkManager.cs

@@ -690,33 +690,42 @@ public class NetworkManager : INetworkManager, IDisposable
     }
 
     /// <inheritdoc/>
-    public bool HasRemoteAccess(IPAddress remoteIP)
+    public RemoteAccessPolicyResult ShouldAllowServerAccess(IPAddress remoteIP)
     {
         var config = _configurationManager.GetNetworkConfiguration();
-        if (config.EnableRemoteAccess)
+        if (IsInLocalNetwork(remoteIP))
         {
-            // Comma separated list of IP addresses or IP/netmask entries for networks that will be allowed to connect remotely.
+            return RemoteAccessPolicyResult.Allow;
-            // If left blank, all remote addresses will be allowed.
-            if (_remoteAddressFilter.Any() && !IsInLocalNetwork(remoteIP))
-            {
-                // remoteAddressFilter is a whitelist or blacklist.
-                var matches = _remoteAddressFilter.Count(remoteNetwork => NetworkUtils.SubnetContainsAddress(remoteNetwork, remoteIP));
-                if ((!config.IsRemoteIPFilterBlacklist && matches > 0)
-                    || (config.IsRemoteIPFilterBlacklist && matches == 0))
-                {
-                    return true;
-                }
-
-                return false;
-            }
         }
-        else if (!IsInLocalNetwork(remoteIP))
+
+        if (!config.EnableRemoteAccess)
         {
             // Remote not enabled. So everyone should be LAN.
-            return false;
+            return RemoteAccessPolicyResult.RejectDueToRemoteAccessDisabled;
         }
 
-        return true;
+        if (!_remoteAddressFilter.Any())
+        {
+            // No filter on remote addresses, allow any of them.
+            return RemoteAccessPolicyResult.Allow;
+        }
+
+        // Comma separated list of IP addresses or IP/netmask entries for networks that will be allowed to connect remotely.
+        // If left blank, all remote addresses will be allowed.
+
+        // remoteAddressFilter is a whitelist or blacklist.
+        var anyMatches = _remoteAddressFilter.Any(remoteNetwork => NetworkUtils.SubnetContainsAddress(remoteNetwork, remoteIP));
+        if (config.IsRemoteIPFilterBlacklist)
+        {
+            return anyMatches
+                ? RemoteAccessPolicyResult.RejectDueToIPBlocklist
+                : RemoteAccessPolicyResult.Allow;
+        }
+
+        // Allow-list
+        return anyMatches
+            ? RemoteAccessPolicyResult.Allow
+            : RemoteAccessPolicyResult.RejectDueToNotAllowlistedRemoteIP;
     }
 
     /// <inheritdoc/>

tests/Jellyfin.Networking.Tests/NetworkParseTests.cs

@@ -281,11 +281,11 @@ namespace Jellyfin.Networking.Tests
         }
 
         [Theory]
-        [InlineData("185.10.10.10,200.200.200.200", "79.2.3.4", true)]
+        [InlineData("185.10.10.10,200.200.200.200", "79.2.3.4", RemoteAccessPolicyResult.RejectDueToNotAllowlistedRemoteIP)]
-        [InlineData("185.10.10.10", "185.10.10.10", false)]
+        [InlineData("185.10.10.10", "185.10.10.10", RemoteAccessPolicyResult.Allow)]
-        [InlineData("", "100.100.100.100", false)]
+        [InlineData("", "100.100.100.100", RemoteAccessPolicyResult.Allow)]
 
-        public void HasRemoteAccess_GivenWhitelist_AllowsOnlyIPsInWhitelist(string addresses, string remoteIP, bool denied)
+        public void HasRemoteAccess_GivenWhitelist_AllowsOnlyIPsInWhitelist(string addresses, string remoteIP, RemoteAccessPolicyResult expectedResult)
         {
             // Comma separated list of IP addresses or IP/netmask entries for networks that will be allowed to connect remotely.
             // If left blank, all remote addresses will be allowed.
@@ -299,15 +299,38 @@ namespace Jellyfin.Networking.Tests
             var startupConf = new Mock<IConfiguration>();
             using var nm = new NetworkManager(NetworkParseTests.GetMockConfig(conf), startupConf.Object, new NullLogger<NetworkManager>());
 
-            Assert.NotEqual(nm.HasRemoteAccess(IPAddress.Parse(remoteIP)), denied);
+            Assert.Equal(expectedResult, nm.ShouldAllowServerAccess(IPAddress.Parse(remoteIP)));
         }
 
         [Theory]
-        [InlineData("185.10.10.10", "79.2.3.4", false)]
+        [InlineData("185.10.10.10,200.200.200.200", "79.2.3.4", RemoteAccessPolicyResult.RejectDueToRemoteAccessDisabled)]
-        [InlineData("185.10.10.10", "185.10.10.10", true)]
+        [InlineData("185.10.10.10", "127.0.0.1", RemoteAccessPolicyResult.Allow)]
-        [InlineData("", "100.100.100.100", false)]
+        [InlineData("", "100.100.100.100", RemoteAccessPolicyResult.RejectDueToRemoteAccessDisabled)]
 
-        public void HasRemoteAccess_GivenBlacklist_BlacklistTheIPs(string addresses, string remoteIP, bool denied)
+        public void HasRemoteAccess_GivenRemoteAccessDisabled_IgnoresAllowlist(string addresses, string remoteIP, RemoteAccessPolicyResult expectedResult)
+        {
+            // Comma separated list of IP addresses or IP/netmask entries for networks that will be allowed to connect remotely.
+            // If left blank, all remote addresses will be allowed.
+            var conf = new NetworkConfiguration()
+            {
+                EnableIPv4 = true,
+                EnableRemoteAccess = false,
+                RemoteIPFilter = addresses.Split(','),
+                IsRemoteIPFilterBlacklist = false
+            };
+
+            var startupConf = new Mock<IConfiguration>();
+            using var nm = new NetworkManager(NetworkParseTests.GetMockConfig(conf), startupConf.Object, new NullLogger<NetworkManager>());
+
+            Assert.Equal(expectedResult, nm.ShouldAllowServerAccess(IPAddress.Parse(remoteIP)));
+        }
+
+        [Theory]
+        [InlineData("185.10.10.10", "79.2.3.4", RemoteAccessPolicyResult.Allow)]
+        [InlineData("185.10.10.10", "185.10.10.10", RemoteAccessPolicyResult.RejectDueToIPBlocklist)]
+        [InlineData("", "100.100.100.100", RemoteAccessPolicyResult.Allow)]
+
+        public void HasRemoteAccess_GivenBlacklist_BlacklistTheIPs(string addresses, string remoteIP, RemoteAccessPolicyResult expectedResult)
         {
             // Comma separated list of IP addresses or IP/netmask entries for networks that will be allowed to connect remotely.
             // If left blank, all remote addresses will be allowed.
@@ -321,7 +344,7 @@ namespace Jellyfin.Networking.Tests
             var startupConf = new Mock<IConfiguration>();
             using var nm = new NetworkManager(NetworkParseTests.GetMockConfig(conf), startupConf.Object, new NullLogger<NetworkManager>());
 
-            Assert.NotEqual(nm.HasRemoteAccess(IPAddress.Parse(remoteIP)), denied);
+            Assert.Equal(expectedResult, nm.ShouldAllowServerAccess(IPAddress.Parse(remoteIP)));
         }
 
         [Theory]