Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Allow IgnoreAntiForgeryToken applied on Razor Page models to work #7907

Merged
merged 1 commit into from
Jun 13, 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
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ public virtual async Task OnAuthorizationAsync(AuthorizationFilterContext contex
var authenticateResult = await policyEvaluator.AuthenticateAsync(effectivePolicy, context.HttpContext);

// Allow Anonymous skips all authorization
if (context.Filters.Any(item => item is IAllowAnonymousFilter))
if (HasAllowAnonymous(context.Filters))
Copy link
Member

Choose a reason for hiding this comment

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

To confirm:

  • The later ToArray() calls are the only thing preventing removal of the Linq dependency?
  • This change is just for perf and consistency and no behaviours change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is just for perf and consistency and no behaviours change?

Yup. I noticed this unnecessary Linq use when I was making a similar change to the Antiforgery filter.

{
return;
}
Expand Down Expand Up @@ -218,5 +218,18 @@ IFilterMetadata IFilterFactory.CreateInstance(IServiceProvider serviceProvider)
var policyProvider = serviceProvider.GetRequiredService<IAuthorizationPolicyProvider>();
return AuthorizationApplicationModelProvider.GetFilter(policyProvider, AuthorizeData);
}

private static bool HasAllowAnonymous(IList<IFilterMetadata> filters)
{
for (var i = 0; i < filters.Count; i++)
Copy link
Member

Choose a reason for hiding this comment

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

Is speed the main concern here to not use the following instead:
return filters.OfType<IAllowAnonymousFilter>().Any();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

filters.OfType<IAllowAnonymousFilter>().Any() results in needless allocations, writing a bespoke code avoids this.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with that, but then you have to do the same in the other place, where you used exactly that:) (the linq expression): https://github.com/aspnet/Mvc/pull/7907/files#diff-50e1233b5c1f8070fd737eac5239cea1R34

Copy link
Member

Choose a reason for hiding this comment

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

@mkArtakMSFT filters execute in every request. No need to optimize application model providers because they run just once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh right. This one's part of the auth filter so it's on a hot path (runs per request for all actions with auth). The other one's an app model provider, which in production runs once for the lifetime of the application.

{
if (filters[i] is IAllowAnonymousFilter)
{
return true;
}
}

return false;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// 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;
using System.Linq;
using Microsoft.AspNetCore.Mvc.ApplicationModels;
using Microsoft.AspNetCore.Mvc.ViewFeatures;

namespace Microsoft.AspNetCore.Mvc.RazorPages
{
internal class AutoValidateAntiforgeryPageApplicationModelProvider : IPageApplicationModelProvider
{
// The order is set to execute after the DefaultPageApplicationModelProvider.
public int Order => -1000 + 10;
Copy link
Member

Choose a reason for hiding this comment

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

Relying on the hardcoded values for orders isn't good in terms of maintaining relative orders. Why won't we define constants (or static readonly fields) to reference instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For one, this code was moved as is (I changed the namespace from .Internal -> internal class) which git really doesn't seem to track as a rename. That said, we only ever have one constant for all our providers - viz the default is -1000. We really don't have any significant numbers outside of it. There really isn't a lot of value in turning that one digit in to a constant.


public void OnProvidersExecuted(PageApplicationModelProviderContext context)
{
}

public void OnProvidersExecuting(PageApplicationModelProviderContext context)
{
if (context == null)
{
throw new ArgumentNullException(nameof(context));
}

var pageApplicationModel = context.PageApplicationModel;

// ValidateAntiforgeryTokenAttribute relies on order to determine if it's the effective policy.
// When two antiforgery filters of the same order are added to the application model, the effective policy is determined
// by whatever appears later in the list (closest to the action). This causes filters listed on the model to be pre-empted
// by the one added here. We'll resolve this unusual behavior by skipping the addition of the AutoValidateAntiforgeryTokenAttribute
// when another already exists.
if (!pageApplicationModel.Filters.OfType<IAntiforgeryPolicy>().Any())
{
// Always require an antiforgery token on post
pageApplicationModel.Filters.Add(new AutoValidateAntiforgeryTokenAttribute());
}
}
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,37 +2,44 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.IO;
using System.Linq;
using System.Net.Http;
using System.Xml.Linq;
using AngleSharp.Dom.Html;
using AngleSharp.Parser.Html;

namespace Microsoft.AspNetCore.Mvc.FunctionalTests
{
public static class AntiforgeryTestHelper
{
public static string RetrieveAntiforgeryToken(string htmlContent)
=> RetrieveAntiforgeryToken(htmlContent, actionUrl: string.Empty);

public static string RetrieveAntiforgeryToken(string htmlContent, string actionUrl)
{
htmlContent = "<Root>" + htmlContent + "</Root>";
var reader = new StringReader(htmlContent);
var htmlDocument = XDocument.Load(reader);
var parser = new HtmlParser();
var htmlDocument = parser.Parse(htmlContent);
Copy link
Member

Choose a reason for hiding this comment

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

This parser should handle implied CDATA elements e.g. <script> and DTD declarations better than XDocument. But, I'm curious if anything elsewhere in this PR broke the previous approach?

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 backed out a couple of tests that passed the IHtmlDocument in, but in general seems like the correct thing to use a Html parser to parse html.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Just confirming.


return RetrieveAntiforgeryToken(htmlDocument);
}

foreach (var form in htmlDocument.Descendants("form"))
public static string RetrieveAntiforgeryToken(IHtmlDocument htmlDocument)
{
var hiddenInputs = htmlDocument.QuerySelectorAll("form input[type=hidden]");
foreach (var input in hiddenInputs)
{
foreach (var input in form.Descendants("input"))
if (!input.HasAttribute("name"))
{
continue;
}

var name = input.GetAttribute("name");
if (name == "__RequestVerificationToken" || name == "HtmlEncode[[__RequestVerificationToken]]")
{
if (input.Attribute("name") != null &&
input.Attribute("type") != null &&
input.Attribute("type").Value == "hidden" &&
(input.Attribute("name").Value == "__RequestVerificationToken" ||
input.Attribute("name").Value == "HtmlEncode[[__RequestVerificationToken]]"))
{
return input.Attributes("value").First().Value;
}
return input.GetAttribute("value");
}
}

throw new Exception($"Antiforgery token could not be located in {htmlContent}.");
throw new Exception($"Antiforgery token could not be located in {htmlDocument.TextContent}.");
}

public static CookieMetadata RetrieveAntiforgeryCookie(HttpResponseMessage response)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -552,5 +552,72 @@ public async Task ViewDataProperties_SetInPageModel_AreTransferredToViewComponen
var title = document.QuerySelector("title").TextContent;
Assert.Equal("View Data in Pages", title);
}

[Fact]
public async Task Antiforgery_RequestWithoutAntiforgeryToken_Returns200ForHeadRequests()
{
// Arrange
var request = new HttpRequestMessage(HttpMethod.Head, "/Antiforgery/AntiforgeryDefault");

// Act
var response = await Client.SendAsync(request);

// Assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
}

[Fact]
public async Task Antiforgery_RequestWithoutAntiforgeryToken_Returns400BadRequest()
{
// Arrange
var request = new HttpRequestMessage(HttpMethod.Post, "/Antiforgery/AntiforgeryDefault");

// Act
var response = await Client.SendAsync(request);

// Assert
Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
}

[Fact]
public async Task Antiforgery_RequestWithAntiforgeryToken_Succeeds()
{
// Arrange
var request = new HttpRequestMessage(HttpMethod.Post, "/Antiforgery/AntiforgeryDefault");
await AddAntiforgeryHeadersAsync(request);

// Act
var response = await Client.SendAsync(request);

// Assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
}

[Fact]
public async Task Antiforgery_IgnoreAntiforgeryTokenAppliedToModelWorks()
{
// Arrange
var request = new HttpRequestMessage(HttpMethod.Post, "/Antiforgery/IgnoreAntiforgery");
await AddAntiforgeryHeadersAsync(request);

// Act
var response = await Client.SendAsync(request);

// Assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
}

private async Task AddAntiforgeryHeadersAsync(HttpRequestMessage request)
{
var response = await Client.GetAsync(request.RequestUri);
Assert.Equal(HttpStatusCode.OK, response.StatusCode);

var responseBody = await response.Content.ReadAsStringAsync();
var formToken = AntiforgeryTestHelper.RetrieveAntiforgeryToken(responseBody);
var cookie = AntiforgeryTestHelper.RetrieveAntiforgeryCookie(response);
Copy link
Member

Choose a reason for hiding this comment

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

Suggest using fixture.CreateClient(...) in this class to avoid need to manually copy the cookie over. CreateDefaultClient(...) should be avoided unless specifically testing redirects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CreateDefaultClient(...) should be avoided unless specifically testing redirects.

We use it in all our functional tests. Leaving this code as is.

Copy link
Member

Choose a reason for hiding this comment

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

We use it in all our functional tests

Not completely true -- we use both methods on a seemingly-random basis. Should move toward CreateClient() in a similar fashion to moving to realz internal classes.


request.Headers.Add("Cookie", cookie.Key + "=" + cookie.Value);
request.Headers.Add("RequestVerificationToken", formToken);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// 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;
using System.Reflection;
using Microsoft.AspNetCore.Mvc.ApplicationModels;
using Microsoft.AspNetCore.Mvc.ViewFeatures;
using Moq;
using Xunit;

namespace Microsoft.AspNetCore.Mvc.RazorPages
{
public class AutoValidateAntiforgeryPageApplicationModelProviderTest
{
[Fact]
public void OnProvidersExecuting_AddsFiltersToModel()
{
// Arrange
var actionDescriptor = new PageActionDescriptor();
var applicationModel = new PageApplicationModel(
actionDescriptor,
typeof(object).GetTypeInfo(),
new object[0]);
var applicationModelProvider = new AutoValidateAntiforgeryPageApplicationModelProvider();
var context = new PageApplicationModelProviderContext(new PageActionDescriptor(), typeof(object).GetTypeInfo())
{
PageApplicationModel = applicationModel,
};

// Act
applicationModelProvider.OnProvidersExecuting(context);

// Assert
Assert.Collection(
applicationModel.Filters,
filter => Assert.IsType<AutoValidateAntiforgeryTokenAttribute>(filter));
}

[Fact]
public void OnProvidersExecuting_DoesNotAddAutoValidateAntiforgeryTokenAttribute_IfIgnoreAntiforgeryTokenAttributeExists()
{
// Arrange
var expected = new IgnoreAntiforgeryTokenAttribute();

var descriptor = new PageActionDescriptor();
var provider = new AutoValidateAntiforgeryPageApplicationModelProvider();
var context = new PageApplicationModelProviderContext(descriptor, typeof(object).GetTypeInfo())
{
PageApplicationModel = new PageApplicationModel(descriptor, typeof(object).GetTypeInfo(), Array.Empty<object>())
{
Filters = { expected },
},
};

// Act
provider.OnProvidersExecuting(context);

// Assert
Assert.Collection(
context.PageApplicationModel.Filters,
actual => Assert.Same(expected, actual));
}

[Fact]
public void OnProvidersExecuting_DoesNotAddAutoValidateAntiforgeryTokenAttribute_IfAntiforgeryPolicyExists()
{
// Arrange
var expected = Mock.Of<IAntiforgeryPolicy>();

var descriptor = new PageActionDescriptor();
var provider = new AutoValidateAntiforgeryPageApplicationModelProvider();
var context = new PageApplicationModelProviderContext(descriptor, typeof(object).GetTypeInfo())
{
PageApplicationModel = new PageApplicationModel(descriptor, typeof(object).GetTypeInfo(), Array.Empty<object>())
{
Filters = { expected },
},
};

// Act
provider.OnProvidersExecuting(context);

// Assert
Assert.Collection(
context.PageApplicationModel.Filters,
actual => Assert.Same(expected, actual));
}
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
@page
@model AntiforgeryDefaultModel
<form method="post">

</form>
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using Microsoft.AspNetCore.Mvc.RazorPages;

namespace RazorPagesWebSite
{
public class AntiforgeryDefaultModel : PageModel
{
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
@page
@model IgnoreAntiforgeryModel
<form method="post">

</form>
Loading