From 18bc2a1fde8ebc324bffc71e240d85d35ae2fd9c Mon Sep 17 00:00:00 2001 From: cmanu Date: Tue, 9 Jan 2018 16:59:13 -0800 Subject: [PATCH] Fix issue 1043: Obfuscate AI data on request redirect (#5236) Fix for issue 1043 - obfuscate the AI data on request redirect. --- src/NuGetGallery/App_Start/Routes.cs | 24 ++-- .../Extensions/RouteExtensions.cs | 49 +++++++ src/NuGetGallery/NuGetGallery.csproj | 1 + .../Telemetry/ClientTelemetryPIIProcessor.cs | 42 +++--- .../Extensions/RouteExtensionsFacts.cs | 58 ++++++++ .../NuGetGallery.Facts.csproj | 1 + .../ClientTelemetryPIIProcessorTests.cs | 132 ++++++++++-------- 7 files changed, 216 insertions(+), 91 deletions(-) create mode 100644 src/NuGetGallery/Extensions/RouteExtensions.cs create mode 100644 tests/NuGetGallery.Facts/Extensions/RouteExtensionsFacts.cs diff --git a/src/NuGetGallery/App_Start/Routes.cs b/src/NuGetGallery/App_Start/Routes.cs index e578f98971..2423a35f59 100644 --- a/src/NuGetGallery/App_Start/Routes.cs +++ b/src/NuGetGallery/App_Start/Routes.cs @@ -147,17 +147,20 @@ public static void RegisterUIRoutes(RouteCollection routes) routes.MapRoute( RouteName.PackageOwnerConfirmation, "packages/{id}/owners/{username}/confirm/{token}", - new { controller = "Packages", action = "ConfirmPendingOwnershipRequest" }); + new { controller = "Packages", action = "ConfirmPendingOwnershipRequest" }, + new RouteExtensions.ObfuscatedMetadata(3, Obfuscator.DefaultTelemetryUserName)); routes.MapRoute( RouteName.PackageOwnerRejection, "packages/{id}/owners/{username}/reject/{token}", - new { controller = "Packages", action = "RejectPendingOwnershipRequest" }); + new { controller = "Packages", action = "RejectPendingOwnershipRequest" }, + new RouteExtensions.ObfuscatedMetadata(3, Obfuscator.DefaultTelemetryUserName)); routes.MapRoute( RouteName.PackageOwnerCancellation, "packages/{id}/owners/{username}/cancel/{token}", - new { controller = "Packages", action = "CancelPendingOwnershipRequest" }); + new { controller = "Packages", action = "CancelPendingOwnershipRequest" }, + new RouteExtensions.ObfuscatedMetadata(3, Obfuscator.DefaultTelemetryUserName)); // We need the following two routes (rather than just one) due to Routing's // Consecutive Optional Parameter bug. :( @@ -242,7 +245,8 @@ public static void RegisterUIRoutes(RouteCollection routes) routes.MapRoute( RouteName.Profile, "profiles/{username}", - new { controller = "Users", action = "Profiles" }); + new { controller = "Users", action = "Profiles" }, + new RouteExtensions.ObfuscatedMetadata(1, Obfuscator.DefaultTelemetryUserName)); routes.MapRoute( RouteName.RemovePassword, @@ -257,17 +261,20 @@ public static void RegisterUIRoutes(RouteCollection routes) routes.MapRoute( RouteName.PasswordReset, "account/forgotpassword/{username}/{token}", - new { controller = "Users", action = "ResetPassword", forgot = true }); + new { controller = "Users", action = "ResetPassword", forgot = true }, + new RouteExtensions.ObfuscatedMetadata(2, Obfuscator.DefaultTelemetryUserName)); routes.MapRoute( RouteName.PasswordSet, "account/setpassword/{username}/{token}", - new { controller = "Users", action = "ResetPassword", forgot = false }); + new { controller = "Users", action = "ResetPassword", forgot = false }, + new RouteExtensions.ObfuscatedMetadata(2, Obfuscator.DefaultTelemetryUserName)); routes.MapRoute( RouteName.ConfirmAccount, "account/confirm/{username}/{token}", - new { controller = "Users", action = "Confirm" }); + new { controller = "Users", action = "Confirm" }, + new RouteExtensions.ObfuscatedMetadata(2, Obfuscator.DefaultTelemetryUserName)); routes.MapRoute( RouteName.ChangeEmailSubscription, @@ -277,7 +284,8 @@ public static void RegisterUIRoutes(RouteCollection routes) routes.MapRoute( RouteName.AdminDeleteAccount, "account/delete/{accountName}", - new { controller = "Users", action = "Delete" }); + new { controller = "Users", action = "Delete" }, + new RouteExtensions.ObfuscatedMetadata(2, Obfuscator.DefaultTelemetryUserName)); routes.MapRoute( RouteName.UserDeleteAccount, diff --git a/src/NuGetGallery/Extensions/RouteExtensions.cs b/src/NuGetGallery/Extensions/RouteExtensions.cs new file mode 100644 index 0000000000..30d44fdad3 --- /dev/null +++ b/src/NuGetGallery/Extensions/RouteExtensions.cs @@ -0,0 +1,49 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Generic; +using System.Linq; +using System.Web.Mvc; +using System.Web.Routing; + +namespace NuGetGallery +{ + public static class RouteExtensions + { + public struct ObfuscatedMetadata + { + public int ObfuscatedSegment + { get; } + + public string ObfuscateValue + { get; } + + public ObfuscatedMetadata(int obfuscatedSegment, string obfuscateValue) + { + ObfuscatedSegment = obfuscatedSegment; + ObfuscateValue = obfuscateValue; + } + } + + internal static Dictionary ObfuscatedRouteMap = new Dictionary(); + + public static void MapRoute(this RouteCollection routes, string name, string url, object defaults, ObfuscatedMetadata obfuscationMetadata) + { + routes.MapRoute(name, url, defaults); + if (!ObfuscatedRouteMap.ContainsKey(url)) { ObfuscatedRouteMap.Add(url, obfuscationMetadata); } + } + + public static string ObfuscateUrlPath(this Route route, string urlPath) + { + var path = route.Url; + if (!ObfuscatedRouteMap.ContainsKey(path)) + { + return null; + } + var metadata = ObfuscatedRouteMap[path]; + string[] segments = urlPath.Split('/'); + segments[metadata.ObfuscatedSegment] = metadata.ObfuscateValue; + return string.Join("/", segments); + } + } +} \ No newline at end of file diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index e7ca9e0490..6e671932d0 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -784,6 +784,7 @@ + diff --git a/src/NuGetGallery/Telemetry/ClientTelemetryPIIProcessor.cs b/src/NuGetGallery/Telemetry/ClientTelemetryPIIProcessor.cs index 2d8c461564..0e626d0020 100644 --- a/src/NuGetGallery/Telemetry/ClientTelemetryPIIProcessor.cs +++ b/src/NuGetGallery/Telemetry/ClientTelemetryPIIProcessor.cs @@ -2,8 +2,8 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Generic; -using System.Linq; +using System.Web; +using System.Web.Routing; using Microsoft.ApplicationInsights.Channel; using Microsoft.ApplicationInsights.DataContracts; using Microsoft.ApplicationInsights.Extensibility; @@ -30,29 +30,29 @@ private void ModifyItem(ITelemetry item) var requestTelemetryItem = item as RequestTelemetry; if(requestTelemetryItem != null) { - requestTelemetryItem.Url = ObfuscateUri(requestTelemetryItem); + var route = GetCurrentRoute(); + if(route == null) + { + return; + } + // Removes the first / + var requestPath = requestTelemetryItem.Url.AbsolutePath.TrimStart('/'); + var obfuscatedPath = route.ObfuscateUrlPath(requestPath); + if(obfuscatedPath != null) + { + requestTelemetryItem.Url = new Uri(requestTelemetryItem.Url.ToString().Replace(requestPath, obfuscatedPath)); + requestTelemetryItem.Name = requestTelemetryItem.Name.Replace(requestPath, obfuscatedPath); + if(requestTelemetryItem.Context.Operation?.Name != null) + { + requestTelemetryItem.Context.Operation.Name = requestTelemetryItem.Context.Operation.Name.Replace(requestPath, obfuscatedPath); + } + } } } - private Uri ObfuscateUri(RequestTelemetry telemetryItem) + public virtual Route GetCurrentRoute() { - if(IsPIIOperation(telemetryItem.Context.Operation.Name)) - { - // The new url form will be: https://nuget.org/ObfuscatedUserName - return new Uri(Obfuscator.DefaultObfuscatedUrl(telemetryItem.Url)); - } - return telemetryItem.Url; - } - - protected virtual bool IsPIIOperation(string operationName) - { - if(string.IsNullOrEmpty(operationName)) - { - return false; - } - // Remove the verb from the operation name. - // An example of operationName : GET Users/Profiles - return Obfuscator.ObfuscatedActions.Contains(operationName.Split(' ').Last()); + return RouteTable.Routes.GetRouteData(new HttpContextWrapper(HttpContext.Current)).Route as Route; } } } \ No newline at end of file diff --git a/tests/NuGetGallery.Facts/Extensions/RouteExtensionsFacts.cs b/tests/NuGetGallery.Facts/Extensions/RouteExtensionsFacts.cs new file mode 100644 index 0000000000..149d35f792 --- /dev/null +++ b/tests/NuGetGallery.Facts/Extensions/RouteExtensionsFacts.cs @@ -0,0 +1,58 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Web.Routing; +using Xunit; + +namespace NuGetGallery.Extensions +{ + public class RouteExtensionsFacts + { + private static string _routeUrl = "test/{user}"; + private static string _url = "test/user1"; + private static int _segment = 1; + private static string _obfuscatedValue = "obfuscatedData"; + + [Fact] + public void MapRouteAddObfuscation() + { + // Arrange + var routes = new RouteCollection(); + routes.MapRoute("test", _routeUrl, null, new RouteExtensions.ObfuscatedMetadata(_segment, _obfuscatedValue)); + + // Act + Assert + Assert.True(RouteExtensions.ObfuscatedRouteMap.ContainsKey(_routeUrl)); + Assert.Equal(_segment, RouteExtensions.ObfuscatedRouteMap[_routeUrl].ObfuscatedSegment); + Assert.Equal(_obfuscatedValue, RouteExtensions.ObfuscatedRouteMap[_routeUrl].ObfuscateValue); + } + + [Fact] + public void ObfuscateRoutePath_ReturnsNullWhenNotObfuscated() + { + //Arrange + var urlInput = "newtest/{user}"; + var route = new Route(url: urlInput, routeHandler:null); + + // Act + var obfuscated = route.ObfuscateUrlPath("newtest/user1"); + + //Assert + Assert.Null(obfuscated); + } + + [Fact] + public void ObfuscateRoutePath_CorrectObfuscation() + { + //Arrange + var routes = new RouteCollection(); + routes.MapRoute("test", _routeUrl, null, new RouteExtensions.ObfuscatedMetadata(_segment, _obfuscatedValue)); + var route = new Route(url: _routeUrl, routeHandler: null); + + // Act + var obfuscated = route.ObfuscateUrlPath(_url); + + //Assert + Assert.Equal($"test/{_obfuscatedValue}", obfuscated); + } + } +} diff --git a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj index b6db32d82c..3bea323405 100644 --- a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj +++ b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj @@ -401,6 +401,7 @@ + diff --git a/tests/NuGetGallery.Facts/Telemetry/ClientTelemetryPIIProcessorTests.cs b/tests/NuGetGallery.Facts/Telemetry/ClientTelemetryPIIProcessorTests.cs index b53b9ddb48..c0977b9fd7 100644 --- a/tests/NuGetGallery.Facts/Telemetry/ClientTelemetryPIIProcessorTests.cs +++ b/tests/NuGetGallery.Facts/Telemetry/ClientTelemetryPIIProcessorTests.cs @@ -2,91 +2,87 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections; using System.Collections.Generic; using System.Linq; -using System.IO; -using System.Security.Principal; -using System.Web; using System.Web.Routing; using Microsoft.ApplicationInsights.Channel; using Microsoft.ApplicationInsights.DataContracts; using Microsoft.ApplicationInsights.Extensibility; -using Microsoft.Owin; -using Microsoft.Owin.Security; using Moq; using Xunit; namespace NuGetGallery.Telemetry { - public class ClientTelemetryPIIProcessorTests + public class ClientTelemetryPIIProcessorTests { + private RouteCollection _currentRoutes ; + + public ClientTelemetryPIIProcessorTests() + { + if (_currentRoutes == null) + { + _currentRoutes = new RouteCollection(); + Routes.RegisterApiV2Routes(_currentRoutes); + Routes.RegisterUIRoutes(_currentRoutes); + } + } + [Fact] public void NullTelemetryItemDoesNotThorw() { // Arange - string userName = "user1"; - var piiProcessor = CreatePIIProcessor(false, userName); + var piiProcessor = CreatePIIProcessor(); // Act piiProcessor.Process(null); } [Theory] - [InlineData(false)] - [InlineData(true)] - public void UrlIsUpdatedOnPIIAction(bool actionIsPII) + [MemberData(nameof(PIIUrlDataGenerator))] + public void UrlIsUpdatedOnPIIAction(string routePath, string inputUrl, string expectedOutputUrl) { // Arange - string userName = "user1"; - var piiProcessor = CreatePIIProcessor(actionIsPII, userName); + var piiProcessor = CreatePIIProcessor(routePath); RequestTelemetry telemetryItem = new RequestTelemetry(); - telemetryItem.Url = new Uri("https://localhost/user1"); + telemetryItem.Url = new Uri(inputUrl); + telemetryItem.Name = $"GET {telemetryItem.Url.AbsolutePath}"; // Act piiProcessor.Process(telemetryItem); // Assert - string expected = actionIsPII ? $"https://localhost/{Obfuscator.DefaultTelemetryUserName}" : telemetryItem.Url.ToString(); - Assert.Equal(expected, telemetryItem.Url.ToString()); + Assert.Equal(expectedOutputUrl, telemetryItem.Url.ToString()); + Assert.Equal($"GET {(new Uri(expectedOutputUrl)).AbsolutePath}", $"GET {telemetryItem.Url.AbsolutePath}"); } - [Theory] - [MemberData(nameof(PIIOperationDataGenerator))] - public void TestValidPIIOperations(string operation) - { - // Arange - var piiProcessor = (TestClientTelemetryPIIProcessor)CreatePIIProcessor(false, "user"); - - // Act and Assert - Assert.True(piiProcessor.IsPIIOperationBase(operation)); - } - - [Theory] - [MemberData(nameof(InvalidPIIOPerationDataGenerator))] - public void TestInvalidPIIOperations(string operation) + [Fact] + public void ValidatePIIActions() { // Arange - var piiProcessor = (TestClientTelemetryPIIProcessor)CreatePIIProcessor(false, "user"); + HashSet existentPIIOperations = Obfuscator.ObfuscatedActions; + List piiOperationsFromRoutes = GetPIIOperationsFromRoute(); // Act and Assert - Assert.False(piiProcessor.IsPIIOperationBase(operation)); + Assert.True(existentPIIOperations.SetEquals(piiOperationsFromRoutes)); } [Fact] public void ValidatePIIRoutes() { // Arange - HashSet existentPIIOperations = Obfuscator.ObfuscatedActions; - List piiOperationsFromRoutes = GetPIIOperationsFromRoute(); + List piiUrlRoutes = GetPIIRoutesUrls(); // Act and Assert - Assert.True(existentPIIOperations.SetEquals(piiOperationsFromRoutes)); + foreach (var route in piiUrlRoutes) + { + var expectedTrue = RouteExtensions.ObfuscatedRouteMap.ContainsKey(route); + Assert.True(expectedTrue, $"Route was not added to the obfuscated routeMap."); + } } - private ClientTelemetryPIIProcessor CreatePIIProcessor(bool isPIIOperation, string userName) + private ClientTelemetryPIIProcessor CreatePIIProcessor(string url = "") { - return new TestClientTelemetryPIIProcessor(new TestProcessorNext(), isPIIOperation, userName); + return new TestClientTelemetryPIIProcessor(new TestProcessorNext(), url ); } private class TestProcessorNext : ITelemetryProcessor @@ -98,37 +94,28 @@ public void Process(ITelemetry item) private class TestClientTelemetryPIIProcessor : ClientTelemetryPIIProcessor { - private User _testUser; - private bool _isPIIOperation; + private string _url = string.Empty; - public TestClientTelemetryPIIProcessor(ITelemetryProcessor next, bool isPIIOperation, string userName) : base(next) + public TestClientTelemetryPIIProcessor(ITelemetryProcessor next, string url) : base (next) { - _isPIIOperation = isPIIOperation; - _testUser = new User(userName); + _url = url; } - protected override bool IsPIIOperation(string operationName) + public override Route GetCurrentRoute() { - return _isPIIOperation; + var handler = new Mock(); + return new Route(_url, handler.Object); } - public bool IsPIIOperationBase(string operationName) - { - return base.IsPIIOperation(operationName); - } } - private static List GetPIIOperationsFromRoute() + private List GetPIIOperationsFromRoute() { - var currentRoutes = new RouteCollection(); - NuGetGallery.Routes.RegisterApiV2Routes(currentRoutes); - NuGetGallery.Routes.RegisterUIRoutes(currentRoutes); - - var piiRoutes = currentRoutes.Where((r) => + var piiRoutes = _currentRoutes.Where((r) => { Route webRoute = r as Route; return webRoute != null ? IsPIIUrl(webRoute.Url.ToString()) : false; - }).Select((r)=> { + }).Select((r) => { var dd = ((Route)r).Defaults; return $"{dd["controller"]}/{dd["action"]}"; }).Distinct().ToList(); @@ -136,21 +123,42 @@ private static List GetPIIOperationsFromRoute() return piiRoutes; } + private List GetPIIRoutesUrls() + { + var piiRoutes = _currentRoutes.Where((r) => + { + Route webRoute = r as Route; + return webRoute != null ? IsPIIUrl(webRoute.Url.ToString()) : false; + }).Select((r) => ((Route)r).Url).Distinct().ToList(); + + return piiRoutes; + } + private static bool IsPIIUrl(string url) { return url.ToLower().Contains("username") || url.ToLower().Contains("accountname"); } - public static IEnumerable PIIOperationDataGenerator() + public static IEnumerable PIIUrlDataGenerator() { - return GetPIIOperationsFromRoute().Select(o => new string[]{$"GET {o}"}); + foreach (var user in GenerateUserNames()) + { + yield return new string[] { "packages/{id}/owners/{username}/confirm/{token}", $"https://localhost/packages/pack1/owners/user1/confirm/sometoken", $"https://localhost/packages/pack1/owners/ObfuscatedUserName/confirm/sometoken" }; + yield return new string[] { "packages/{id}/owners/{username}/reject/{token}", $"https://localhost/packages/pack1/owners/user1/reject/sometoken", $"https://localhost/packages/pack1/owners/ObfuscatedUserName/reject/sometoken" }; + yield return new string[] { "packages/{id}/owners/{username}/cancel/{token}", $"https://localhost/packages/pack1/owners/user1/cancel/sometoken", $"https://localhost/packages/pack1/owners/ObfuscatedUserName/cancel/sometoken" }; + + yield return new string[] { "account/confirm/{username}/{token}", $"https://localhost/account/confirm/user1/sometoken", $"https://localhost/account/confirm/ObfuscatedUserName/sometoken" }; + yield return new string[] { "account/delete/{accountName}", "https://localhost/account/delete/user1", $"https://localhost/account/delete/ObfuscatedUserName" }; + + yield return new string[] { "profiles/{username}", $"https://localhost/profiles/user1", $"https://localhost/profiles/ObfuscatedUserName" }; + yield return new string[] { "account/setpassword/{username}/{token}", $"https://localhost/account/setpassword/user1/sometoken", $"https://localhost/account/setpassword/ObfuscatedUserName/sometoken" }; + yield return new string[] { "account/forgotpassword/{username}/{token}", $"https://localhost/account/forgotpassword/user1/sometoken", $"https://localhost/account/forgotpassword/ObfuscatedUserName/sometoken" }; + } } - public static IEnumerable InvalidPIIOPerationDataGenerator() + public static List GenerateUserNames() { - yield return new string[]{ null }; - yield return new string[]{ string.Empty }; - yield return new string[]{"Some random data" }; + return new List{ "user1", "user.1", "user_1", "user-1"}; } } }