aspnet / Antiforgery

[Archived] AntiForgery token feature for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
78 stars 40 forks source link

Remove Antiforgery field from model before being handled by MVC #160

Closed stephtr closed 7 years ago

stephtr commented 7 years ago

When using an IDictionary as model in MVC, for example like:

[HttpPost]
public IActionResult Index(IDictionary<string, string> viewData)

viewData contains the __RequestVerificationToken field. This is particularly impractical if one instead uses an IDictionary<int, int>, which then produces a middleware exception. I know that it is possible to wrap the IDictionary in a class, but wouldn't it be more useful to automatically remove the field before it reaches the controller’s action?

Eilon commented 7 years ago

This is very odd to me... I wouldn't have expected an action method parameter such as IDictionary<string, string> viewData to bind anything at all because there's nothing posted named ViewData.

@rynowak - any idea on this? How would anything at all show up in that dictionary?

@stephtr - or, I don't suppose you have some custom model binder or formatter in operation here, do you? Something that reads headers or form fields?

stephtr commented 7 years ago

I reduced the code to a new mvc app (dotnet new mvc) with just those two additions:

public IActionResult Test()
{
    var model = new Dictionary<string, string>();
    model.Add("test", "abc");
    return View(model);
}

[HttpPost]
public IActionResult Test(IDictionary<string, string> parameters)
{
    return View(parameters);
}
@model IDictionary<string, string>

<table>
    <tr>
        <td>Key</td>
        <td>Value</td>
    </tr>
    @foreach (var data in Model)
    {
        <tr>
            <td>@data.Key</td>
            <td>@data.Value</td>
        </tr>
    }
</table>

<form method="post">
    <p>
        Test: <input asp-for="@Model["test"]" />
    </p>
    <p>
        <input type="submit" />
    </p>
</form>

Because the post data contains just [test]: abc (and the __RequestVerificationToken field) it seems like mvc tries to bind it to all arguments. If I for example change the controller function signature to public IActionResult Test(IDictionary<string, string> first, IDictionary<string, string> second) both arguments contain the submitted values.

I'm not sure if the input tag helper is supposed to work with just an IDictionary as a model, because for example @model string <input asp-for="@Model" /> fails with The name of an HTML field cannot be null or empty.. It therefore would make more sense for me if my snippet above would just throw an error while rendering the input element.

This is very odd to me... I wouldn't have expected an action method parameter such as IDictionary<string, string> viewData to bind anything at all because there's nothing posted named ViewData.

When posting a class the argument name isn't relevant, only its type (for example in first-mvc-app the movie argument name is only used in the controller function, but not within the view.

Eilon commented 7 years ago

@kichalla can you investigate?

stephtr commented 7 years ago

Just for information: I have additionally opened an issue at Mvc.

Eilon commented 7 years ago

I looked into this, and this specific aspect is by design - the antiforgery middleware shouldn't be "hiding" its post data from other middleware components.

stephtr commented 7 years ago

Thanks for looking into it!