From 6d35dd6b326b98995e363c64083a2ca46b2582fd Mon Sep 17 00:00:00 2001 From: Mark Monteiro Date: Mon, 13 Apr 2020 13:13:48 -0400 Subject: [PATCH 1/7] Clean up SecurityException - Remove unused SecurityExceptionType - Add missing constructor for InnerException - Add missing documentation --- .../HttpServer/Security/AuthService.cs | 30 ++++------------- MediaBrowser.Api/UserService.cs | 2 +- .../Net/SecurityException.cs | 32 ++++++++++++++----- 3 files changed, 31 insertions(+), 33 deletions(-) diff --git a/Emby.Server.Implementations/HttpServer/Security/AuthService.cs b/Emby.Server.Implementations/HttpServer/Security/AuthService.cs index 58421aaf19..1360a5e0ce 100644 --- a/Emby.Server.Implementations/HttpServer/Security/AuthService.cs +++ b/Emby.Server.Implementations/HttpServer/Security/AuthService.cs @@ -108,18 +108,12 @@ namespace Emby.Server.Implementations.HttpServer.Security { if (user.Policy.IsDisabled) { - throw new SecurityException("User account has been disabled.") - { - SecurityExceptionType = SecurityExceptionType.Unauthenticated - }; + throw new SecurityException("User account has been disabled."); } if (!user.Policy.EnableRemoteAccess && !_networkManager.IsInLocalNetwork(request.RemoteIp)) { - throw new SecurityException("User account has been disabled.") - { - SecurityExceptionType = SecurityExceptionType.Unauthenticated - }; + throw new SecurityException("User account has been disabled."); } if (!user.Policy.IsAdministrator @@ -128,10 +122,7 @@ namespace Emby.Server.Implementations.HttpServer.Security { request.Response.Headers.Add("X-Application-Error-Code", "ParentalControl"); - throw new SecurityException("This user account is not allowed access at this time.") - { - SecurityExceptionType = SecurityExceptionType.ParentalControl - }; + throw new SecurityException("This user account is not allowed access at this time."); } } @@ -190,10 +181,7 @@ namespace Emby.Server.Implementations.HttpServer.Security { if (user == null || !user.Policy.IsAdministrator) { - throw new SecurityException("User does not have admin access.") - { - SecurityExceptionType = SecurityExceptionType.Unauthenticated - }; + throw new SecurityException("User does not have admin access."); } } @@ -201,10 +189,7 @@ namespace Emby.Server.Implementations.HttpServer.Security { if (user == null || !user.Policy.EnableContentDeletion) { - throw new SecurityException("User does not have delete access.") - { - SecurityExceptionType = SecurityExceptionType.Unauthenticated - }; + throw new SecurityException("User does not have delete access."); } } @@ -212,10 +197,7 @@ namespace Emby.Server.Implementations.HttpServer.Security { if (user == null || !user.Policy.EnableContentDownloading) { - throw new SecurityException("User does not have download access.") - { - SecurityExceptionType = SecurityExceptionType.Unauthenticated - }; + throw new SecurityException("User does not have download access."); } } } diff --git a/MediaBrowser.Api/UserService.cs b/MediaBrowser.Api/UserService.cs index 4015143497..78fc6c6941 100644 --- a/MediaBrowser.Api/UserService.cs +++ b/MediaBrowser.Api/UserService.cs @@ -426,7 +426,7 @@ namespace MediaBrowser.Api catch (SecurityException e) { // rethrow adding IP address to message - throw new SecurityException($"[{Request.RemoteIp}] {e.Message}"); + throw new SecurityException($"[{Request.RemoteIp}] {e.Message}", e); } } diff --git a/MediaBrowser.Controller/Net/SecurityException.cs b/MediaBrowser.Controller/Net/SecurityException.cs index 3ccecf0eb8..a5b94ea5e3 100644 --- a/MediaBrowser.Controller/Net/SecurityException.cs +++ b/MediaBrowser.Controller/Net/SecurityException.cs @@ -2,20 +2,36 @@ using System; namespace MediaBrowser.Controller.Net { + /// + /// The exception that is thrown when a user is authenticated, but not authorized to access a requested resource. + /// public class SecurityException : Exception { + /// + /// Initializes a new instance of the class. + /// + public SecurityException() + : base() + { + } + + /// + /// Initializes a new instance of the class. + /// + /// The message that describes the error. public SecurityException(string message) : base(message) { - } - public SecurityExceptionType SecurityExceptionType { get; set; } - } - - public enum SecurityExceptionType - { - Unauthenticated = 0, - ParentalControl = 1 + /// + /// Initializes a new instance of the class. + /// + /// The message that describes the error + /// The exception that is the cause of the current exception, or a null reference if no inner exception is specified. + public SecurityException(string message, Exception innerException) + : base(message, innerException) + { + } } } From 53380689ad00f00efc0c1790f1d25d08c95d7f2d Mon Sep 17 00:00:00 2001 From: Mark Monteiro Date: Mon, 13 Apr 2020 13:17:46 -0400 Subject: [PATCH 2/7] Return correct status codes for authentication and authorization errors - Use AuthenticatonException to return 401 - Use SecurityException to return 403 - Update existing throws to throw the correct exception for the circumstance --- .../HttpServer/HttpListenerHost.cs | 5 ++++- .../HttpServer/Security/AuthService.cs | 7 ++++--- Emby.Server.Implementations/Library/UserManager.cs | 11 ++++------- Emby.Server.Implementations/Session/SessionManager.cs | 2 +- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs b/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs index 5ae65a4e3d..f496ff1ba1 100644 --- a/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs +++ b/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs @@ -14,6 +14,7 @@ using Emby.Server.Implementations.Services; using MediaBrowser.Common.Extensions; using MediaBrowser.Common.Net; using MediaBrowser.Controller; +using MediaBrowser.Controller.Authentication; using MediaBrowser.Controller.Configuration; using MediaBrowser.Controller.Net; using MediaBrowser.Model.Events; @@ -230,7 +231,8 @@ namespace Emby.Server.Implementations.HttpServer switch (ex) { case ArgumentException _: return 400; - case SecurityException _: return 401; + case AuthenticationException _: return 401; + case SecurityException _: return 403; case DirectoryNotFoundException _: case FileNotFoundException _: case ResourceNotFoundException _: return 404; @@ -550,6 +552,7 @@ namespace Emby.Server.Implementations.HttpServer || ex is IOException || ex is OperationCanceledException || ex is SecurityException + || ex is AuthenticationException || ex is FileNotFoundException; await ErrorHandler(ex, httpReq, !ignoreStackTrace, urlToLog).ConfigureAwait(false); } diff --git a/Emby.Server.Implementations/HttpServer/Security/AuthService.cs b/Emby.Server.Implementations/HttpServer/Security/AuthService.cs index 1360a5e0ce..256b24924e 100644 --- a/Emby.Server.Implementations/HttpServer/Security/AuthService.cs +++ b/Emby.Server.Implementations/HttpServer/Security/AuthService.cs @@ -2,6 +2,7 @@ using System; using System.Linq; +using System.Security.Authentication; using Emby.Server.Implementations.SocketSharp; using MediaBrowser.Common.Net; using MediaBrowser.Controller.Configuration; @@ -68,7 +69,7 @@ namespace Emby.Server.Implementations.HttpServer.Security if (user == null && auth.UserId != Guid.Empty) { - throw new SecurityException("User with Id " + auth.UserId + " not found"); + throw new AuthenticationException("User with Id " + auth.UserId + " not found"); } if (user != null) @@ -212,14 +213,14 @@ namespace Emby.Server.Implementations.HttpServer.Security { if (string.IsNullOrEmpty(token)) { - throw new SecurityException("Access token is required."); + throw new AuthenticationException("Access token is required."); } var info = GetTokenInfo(request); if (info == null) { - throw new SecurityException("Access token is invalid or expired."); + throw new AuthenticationException("Access token is invalid or expired."); } //if (!string.IsNullOrEmpty(info.UserId)) diff --git a/Emby.Server.Implementations/Library/UserManager.cs b/Emby.Server.Implementations/Library/UserManager.cs index 7b17cc913f..f92cb6ae6d 100644 --- a/Emby.Server.Implementations/Library/UserManager.cs +++ b/Emby.Server.Implementations/Library/UserManager.cs @@ -20,6 +20,7 @@ using MediaBrowser.Controller.Drawing; using MediaBrowser.Controller.Dto; using MediaBrowser.Controller.Entities; using MediaBrowser.Controller.Library; +using MediaBrowser.Controller.Net; using MediaBrowser.Controller.Persistence; using MediaBrowser.Controller.Plugins; using MediaBrowser.Controller.Providers; @@ -324,21 +325,17 @@ namespace Emby.Server.Implementations.Library if (user.Policy.IsDisabled) { - throw new AuthenticationException( - string.Format( - CultureInfo.InvariantCulture, - "The {0} account is currently disabled. Please consult with your administrator.", - user.Name)); + throw new SecurityException($"The {user.Name} account is currently disabled. Please consult with your administrator."); } if (!user.Policy.EnableRemoteAccess && !_networkManager.IsInLocalNetwork(remoteEndPoint)) { - throw new AuthenticationException("Forbidden."); + throw new SecurityException("Forbidden."); } if (!user.IsParentalScheduleAllowed()) { - throw new AuthenticationException("User is not allowed access at this time."); + throw new SecurityException("User is not allowed access at this time."); } // Update LastActivityDate and LastLoginDate, then save diff --git a/Emby.Server.Implementations/Session/SessionManager.cs b/Emby.Server.Implementations/Session/SessionManager.cs index de768333d8..c93c02c480 100644 --- a/Emby.Server.Implementations/Session/SessionManager.cs +++ b/Emby.Server.Implementations/Session/SessionManager.cs @@ -1414,7 +1414,7 @@ namespace Emby.Server.Implementations.Session if (user == null) { AuthenticationFailed?.Invoke(this, new GenericEventArgs(request)); - throw new SecurityException("Invalid username or password entered."); + throw new AuthenticationException("Invalid username or password entered."); } if (!string.IsNullOrEmpty(request.DeviceId) From 98044e77935afdd4ff00a65d43b17d285fea82c7 Mon Sep 17 00:00:00 2001 From: Mark Monteiro Date: Mon, 13 Apr 2020 13:18:12 -0400 Subject: [PATCH 3/7] Document AuthenticationException correctly --- .../Authentication/AuthenticationException.cs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/MediaBrowser.Controller/Authentication/AuthenticationException.cs b/MediaBrowser.Controller/Authentication/AuthenticationException.cs index 62eca3ea9f..28209f6391 100644 --- a/MediaBrowser.Controller/Authentication/AuthenticationException.cs +++ b/MediaBrowser.Controller/Authentication/AuthenticationException.cs @@ -7,23 +7,29 @@ namespace MediaBrowser.Controller.Authentication /// public class AuthenticationException : Exception { - /// + /// + /// Initializes a new instance of the class. + /// public AuthenticationException() : base() { - } - /// + /// + /// Initializes a new instance of the class. + /// + /// The message that describes the error. public AuthenticationException(string message) : base(message) { - } - /// + /// + /// Initializes a new instance of the class. + /// + /// The message that describes the error + /// The exception that is the cause of the current exception, or a null reference if no inner exception is specified. public AuthenticationException(string message, Exception innerException) : base(message, innerException) { - } } } From 9c7b3850f98633570cfb426e020153363921ce40 Mon Sep 17 00:00:00 2001 From: Mark Monteiro Date: Mon, 13 Apr 2020 14:55:24 -0400 Subject: [PATCH 4/7] Throw AuthenticationException instead of ArgumentNullException when a user does not exist --- .../Library/DefaultAuthenticationProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs b/Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs index ab036eca7a..52c8facc3e 100644 --- a/Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs +++ b/Emby.Server.Implementations/Library/DefaultAuthenticationProvider.cs @@ -47,7 +47,7 @@ namespace Emby.Server.Implementations.Library { if (resolvedUser == null) { - throw new ArgumentNullException(nameof(resolvedUser)); + throw new AuthenticationException($"Specified user does not exist."); } bool success = false; From a8c3951c1798ced5c10631da7d9ca64731a12f26 Mon Sep 17 00:00:00 2001 From: Mark Monteiro Date: Mon, 13 Apr 2020 15:26:49 -0400 Subject: [PATCH 5/7] Only show developer exception page for 500 server exceptions Other response codes should be returned as normal --- .../HttpServer/HttpListenerHost.cs | 93 ++++++++++--------- 1 file changed, 50 insertions(+), 43 deletions(-) diff --git a/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs b/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs index f496ff1ba1..4c69b35e2a 100644 --- a/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs +++ b/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs @@ -241,40 +241,38 @@ namespace Emby.Server.Implementations.HttpServer } } - private async Task ErrorHandler(Exception ex, IRequest httpReq, bool logExceptionStackTrace, string urlToLog) + private async Task ErrorHandler(Exception ex, IRequest httpReq, int statusCode, string urlToLog) { - try + bool ignoreStackTrace = + ex is SocketException + || ex is IOException + || ex is OperationCanceledException + || ex is SecurityException + || ex is AuthenticationException + || ex is FileNotFoundException; + + if (ignoreStackTrace) { - ex = GetActualException(ex); - - if (logExceptionStackTrace) - { - _logger.LogError(ex, "Error processing request. URL: {Url}", urlToLog); - } - else - { - _logger.LogError("Error processing request: {Message}. URL: {Url}", ex.Message.TrimEnd('.'), urlToLog); - } - - var httpRes = httpReq.Response; - - if (httpRes.HasStarted) - { - return; - } - - var statusCode = GetStatusCode(ex); - httpRes.StatusCode = statusCode; - - var errContent = NormalizeExceptionMessage(ex.Message); - httpRes.ContentType = "text/plain"; - httpRes.ContentLength = errContent.Length; - await httpRes.WriteAsync(errContent).ConfigureAwait(false); + _logger.LogError("Error processing request: {Message}. URL: {Url}", ex.Message.TrimEnd('.'), urlToLog); } - catch (Exception errorEx) + else { - _logger.LogError(errorEx, "Error this.ProcessRequest(context)(Exception while writing error to the response). URL: {Url}", urlToLog); + _logger.LogError(ex, "Error processing request. URL: {Url}", urlToLog); } + + var httpRes = httpReq.Response; + + if (httpRes.HasStarted) + { + return; + } + + httpRes.StatusCode = statusCode; + + var errContent = NormalizeExceptionMessage(ex.Message); + httpRes.ContentType = "text/plain"; + httpRes.ContentLength = errContent.Length; + await httpRes.WriteAsync(errContent).ConfigureAwait(false); } private string NormalizeExceptionMessage(string msg) @@ -538,23 +536,32 @@ namespace Emby.Server.Implementations.HttpServer throw new FileNotFoundException(); } } - catch (Exception ex) + catch (Exception requestEx) { - // Do not handle exceptions manually when in development mode - // The framework-defined development exception page will be returned instead - if (_hostEnvironment.IsDevelopment()) + try { - throw; - } + var requestInnerEx = GetActualException(requestEx); + var statusCode = GetStatusCode(requestInnerEx); - bool ignoreStackTrace = - ex is SocketException - || ex is IOException - || ex is OperationCanceledException - || ex is SecurityException - || ex is AuthenticationException - || ex is FileNotFoundException; - await ErrorHandler(ex, httpReq, !ignoreStackTrace, urlToLog).ConfigureAwait(false); + // Do not handle 500 server exceptions manually when in development mode + // The framework-defined development exception page will be returned instead + if (statusCode == 500 && _hostEnvironment.IsDevelopment()) + { + throw; + } + + await ErrorHandler(requestInnerEx, httpReq, statusCode, urlToLog).ConfigureAwait(false); + } + catch (Exception handlerException) + { + var aggregateEx = new AggregateException("Error while handling request exception", requestEx, handlerException); + _logger.LogError(aggregateEx, "Error while handling exception in response to {Url}", urlToLog); + + if (_hostEnvironment.IsDevelopment()) + { + throw aggregateEx; + } + } } finally { From 8b4b4b4127945354731036e13ca0b5366134958d Mon Sep 17 00:00:00 2001 From: Mark Monteiro Date: Mon, 13 Apr 2020 16:10:55 -0400 Subject: [PATCH 6/7] Do not return the exception message to the client for AuthenticationExceptions --- .../HttpServer/HttpListenerHost.cs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs b/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs index 4c69b35e2a..211a0c1d99 100644 --- a/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs +++ b/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs @@ -269,25 +269,24 @@ namespace Emby.Server.Implementations.HttpServer httpRes.StatusCode = statusCode; - var errContent = NormalizeExceptionMessage(ex.Message); + var errContent = NormalizeExceptionMessage(ex) ?? string.Empty; httpRes.ContentType = "text/plain"; httpRes.ContentLength = errContent.Length; await httpRes.WriteAsync(errContent).ConfigureAwait(false); } - private string NormalizeExceptionMessage(string msg) + private string NormalizeExceptionMessage(Exception ex) { - if (msg == null) + // Do not expose the exception message for AuthenticationException + if (ex is AuthenticationException) { - return string.Empty; + return null; } // Strip any information we don't want to reveal - - msg = msg.Replace(_config.ApplicationPaths.ProgramSystemPath, string.Empty, StringComparison.OrdinalIgnoreCase); - msg = msg.Replace(_config.ApplicationPaths.ProgramDataPath, string.Empty, StringComparison.OrdinalIgnoreCase); - - return msg; + return ex.Message + ?.Replace(_config.ApplicationPaths.ProgramSystemPath, string.Empty, StringComparison.OrdinalIgnoreCase) + .Replace(_config.ApplicationPaths.ProgramDataPath, string.Empty, StringComparison.OrdinalIgnoreCase); } /// From daed41815f23795aad7f54965a8ef7eae44a1e08 Mon Sep 17 00:00:00 2001 From: Mark Monteiro Date: Wed, 15 Apr 2020 08:56:00 -0400 Subject: [PATCH 7/7] Add missing punctuation in xml comment Co-Authored-By: Bond-009 --- .../Authentication/AuthenticationException.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MediaBrowser.Controller/Authentication/AuthenticationException.cs b/MediaBrowser.Controller/Authentication/AuthenticationException.cs index 28209f6391..081f877f72 100644 --- a/MediaBrowser.Controller/Authentication/AuthenticationException.cs +++ b/MediaBrowser.Controller/Authentication/AuthenticationException.cs @@ -25,7 +25,7 @@ namespace MediaBrowser.Controller.Authentication /// /// Initializes a new instance of the class. /// - /// The message that describes the error + /// The message that describes the error. /// The exception that is the cause of the current exception, or a null reference if no inner exception is specified. public AuthenticationException(string message, Exception innerException) : base(message, innerException)