Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue 1043: Obfuscate AI data on request redirect #5236

Merged
merged 7 commits into from
Jan 10, 2018
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
24 changes: 16 additions & 8 deletions src/NuGetGallery/App_Start/Routes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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. :(
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down
49 changes: 49 additions & 0 deletions src/NuGetGallery/Extensions/RouteExtensions.cs
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: class could be internal (tests already have internalsVisibleTo)

{
public struct ObfuscatedMetadata
{
public int ObfuscatedSegment
{ get; }

public string ObfuscateValue
{ get; }

public ObfuscatedMetadata(int obfuscatedSegment, string obfuscateValue)
{
ObfuscatedSegment = obfuscatedSegment;
ObfuscateValue = obfuscateValue;
}
}

internal static Dictionary<string, ObfuscatedMetadata> ObfuscatedRouteMap = new Dictionary<string, ObfuscatedMetadata>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe name RoutePartsToObfuscateByUrl is clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like more the current name.


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); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: { should be in a new line

}

public static string ObfuscateUrlPath(this Route route, string urlPath)
{
var path = route.Url;
Copy link
Member

@chenriksson chenriksson Jan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra single whitespace? Or maybe it's the if indentation below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

if (!ObfuscatedRouteMap.ContainsKey(path))
{
return null;
}
var metadata = ObfuscatedRouteMap[path];
string[] segments = urlPath.Split('/');
segments[metadata.ObfuscatedSegment] = metadata.ObfuscateValue;
return string.Join("/", segments);
}
}
}
1 change: 1 addition & 0 deletions src/NuGetGallery/NuGetGallery.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -784,6 +784,7 @@
<Compile Include="Controllers\PagesController.cs" />
<Compile Include="Extensions\OrganizationExtensions.cs" />
<Compile Include="Extensions\PrincipalExtensions.cs" />
<Compile Include="Extensions\RouteExtensions.cs" />
<Compile Include="Extensions\ScopeExtensions.cs" />
<Compile Include="Extensions\UserExtensions.cs" />
<Compile Include="Filters\ApiScopeRequiredAttribute.cs" />
Expand Down
42 changes: 21 additions & 21 deletions src/NuGetGallery/Telemetry/ClientTelemetryPIIProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}
}
}
1 change: 0 additions & 1 deletion src/NuGetGallery/Telemetry/Obfuscator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// 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;

namespace NuGetGallery
Expand Down
58 changes: 58 additions & 0 deletions tests/NuGetGallery.Facts/Extensions/RouteExtensionsFacts.cs
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
1 change: 1 addition & 0 deletions tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@
<Compile Include="Authentication\AuthenticationServiceFacts.cs" />
<Compile Include="Authentication\TestCredentialHelper.cs" />
<Compile Include="Controllers\SupportControllerFacts.cs" />
<Compile Include="Extensions\RouteExtensionsFacts.cs" />
<Compile Include="Extensions\OrganizationExtensionsFacts.cs" />
<Compile Include="Framework\MemberDataHelper.cs" />
<Compile Include="Infrastructure\Authentication\ApiKeyV4Facts.cs" />
Expand Down
Loading