umbraco / UmbracoDocs

The official Umbraco Documentation
https://docs.umbraco.com
MIT License
269 stars 780 forks source link

v9: SurfaceController POST not allowed outside Umbraco due to missing antiforgery token #3242

Closed JoseMarcenaro closed 2 years ago

JoseMarcenaro commented 3 years ago

Which Umbraco version are you using?

9.0.0-beta003

Bug summary

Any [HttpPost] action in a SurfaceController fails with an HTTP 400 - Bad Request - at least when invoked outside Umbraco. The same controller class based on PluginController works Ok.

Specifics

Full code for the controller class:

using Umbraco.Cms.Core.Logging;
using Microsoft.AspNetCore.Mvc;
using Umbraco.Cms.Web.Website.Controllers;
using Umbraco.Cms.Core.Web;
using Umbraco.Cms.Infrastructure.Persistence;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Cache;
using Umbraco.Cms.Core.Routing;

namespace Website.Controllers
{
    public class ContactController : SurfaceController
    {
        public ContactController(IUmbracoContextAccessor umbracoContextAccessor, IUmbracoDatabaseFactory databaseFactory, ServiceContext services, AppCaches appCaches, IProfilingLogger profilingLogger, IPublishedUrlProvider publishedUrlProvider)
            : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger, publishedUrlProvider) { }

        [HttpGet]
        [Route("/api/contact/get")]
        public IActionResult Index()
        {
            return Ok(new ContactInfo { Name = "Mary" });
        }
        [HttpPost]
        [Route("/api/contact/send")]
        public IActionResult Send([FromBody] ContactInfo contact)
        {
            return Ok(contact);
        }
    }
    public class ContactInfo
    {
        public string Name { get; set; }
    }
}

GET request (succeeds):
https://localhost:6001/api/contact/get

AJAX POST request (fails with HTTP 400 when the controller is a SurfaceController)

$.ajax({
    url: '/api/contact/send',
    method: 'POST',
    contentType: 'application/json; charset=utf-8',
    data: '{"name": "William"}',
    dataType: 'json',
    success: function(result) { console.log(result);} ,
    error: function(err) { console.log(err.responseText); }
});

Steps to reproduce

Paste the controller class in the Umbraco website and invoke the GET url in the browser -> it succeeds From a js script in the site perform the AJAX POST request -> it fails

Modify the controller class to be based on PluginController (or Controller) -> both actions succeed.

Expected result / actual result

No response

nul800sebastiaan commented 3 years ago

I guess this should all work, but out of curiosity can you explain why you'd need a SurfaceController and not just a regular ApiController or UmbracoApiController (in case you need some Umbraco context)? SurfaceControllers are mostly for Form submissions on a MVC view, your example is more suitable for a lighter-weight UmbracoApiController.

nul800sebastiaan commented 3 years ago

Ps. I can confirm the sample gives me the same result.

JoseMarcenaro commented 3 years ago

Hi @nul800sebastiaan, thanks for the quick reply. I cannot explain why I need a SurfaceController, because I don't - in fact I already changed it.

The reason to report it is that I upgraded a v8 website that used a Surface controller ... and it failed. Thanks!

Shazwazza commented 3 years ago

Pretty sure it's because SurfaceControllers now by default have the anti-forgery check. This should also be in v8 but it would have been a breaking change which meant that anti-forgery checks had to be hacked into place for certain controllers. SurfaceControllers are primarily made for POSTing forms within Umbraco and that's about it.

nul800sebastiaan commented 3 years ago

I cannot explain why I need a SurfaceController, because I don't - in fact I already changed it.

That is a perfect explanation haha!

Might be good to document this, but yes, it feels like this is something that only accidentally worked in v8. I'll move this to the documentation tracker so we can update the docs. For v8 it should probably say: we highly recommend sending an antiforgery token with submissions. And then we can list it as a breaking change in v9.

mattbrailsford commented 3 years ago

Shoot! I think I have a problem related to this. I have a multi targeted package which shares a front end view on v8 and v9 and now that v9 adds the anti forgery token by default, my request is failing because I have 2 anti forgery tokens in the form.

JoseMarcenaro commented 3 years ago

Shoot! I think I have a problem related to this. I have a multi targeted package which shares a front end view on v8 and v9 and now that v9 adds the anti forgery token by default, my request is failing because I have 2 anti forgery tokens in the form.

Hi @mattbrailsford - Are you sure v9 adds an antiforgery token? As far as I understood, the Surface controller in v9 expects a token to come in the request, but placing the token is up to you. Maybe I'm wrong.

Shazwazza commented 3 years ago

The Html.BeginUmbracoForm adds it.

Previous to v9, this wasn't automatic but it should have been but we couldn't break things. For built in controllers there's hacks in v8 to ensure that the tokens are automatically added.

In v9 it adds them all the time for SurfaceControllers - which should have been in v8 but couldn't be done due to breaking changes.

mistyn8 commented 3 years ago

Are we able to decorate a surface controller action with [IgnoreAntiforgeryToken] to perform a Safe cross site post?

https://docs.microsoft.com/en-us/aspnet/core/security/anti-request-forgery?view=aspnetcore-5.0#override-global-or-controller-antiforgery-attributes

and/or remove the token with @removeTagHelper Microsoft.AspNetCore.Mvc.TagHelpers.FormTagHelper, Microsoft.AspNetCore.Mvc.TagHelpers

https://docs.microsoft.com/en-us/aspnet/core/security/anti-request-forgery?view=aspnetcore-5.0#aspnet-core-antiforgery-configuration

ps would be nice to have a taghelper for umbracoForm.. so could also use <!Form /> to opt out?

maartenmensink commented 3 years ago

The Html.BeginUmbracoForm adds it.

Previous to v9, this wasn't automatic but it should have been but we couldn't break things. For built in controllers there's hacks in v8 to ensure that the tokens are automatically added.

In v9 it adds them all the time for SurfaceControllers - which should have been in v8 but couldn't be done due to breaking changes.

This needs to be in upgrade documentation Took me 45 minutes debugging and googling until i stumbled upon this thread ๐Ÿ˜ž

cheeseytoastie commented 3 years ago

I've just hit this and the [IgnoreAntiforgeryToken] (thanks @mistyn8!) decoration makes this work.

So if you're updating legacy Ajax calls that hit a surface controller this seems to be the way to go.

bjarnef commented 3 years ago

I will reference to this issue I created here: https://github.com/umbraco/UmbracoDocs/issues/3560 and to document AutoValidateAntiforgeryToken now is default behaviour.

I was able to make a Ajax POST request to a surface controller like in v8 by using the beforeSend function:

For example:

var form = document.getElementById("search");

var requestData = {
      term: term
};

$.ajax({
      type: "POST",
      url: "/Umbraco/Surface/Search/Search",
      data: JSON.stringify(requestData),
      contentType: "application/json; charset=utf-8",
      beforeSend: function (xhr) {
           xhr.setRequestHeader("RequestVerificationToken",
           form.find('input:hidden[name="__RequestVerificationToken"]').val());
      },
      success: function (data) {
      },
      error: function () {
      }
});
sofietoft commented 2 years ago

Hi all ๐Ÿ‘‹

Sounds to me like we can close this one down, as a new Issue (https://github.com/umbraco/UmbracoDocs/issues/3560) was created. Is that right, @bjarnef ? ๐Ÿคž

techpara commented 2 years ago

1) Add @Html.AntiForgeryToken() in your CSHTML page. 2) [ValidateAntiForgeryToken] on your action method. 3) If calling from ajax, add following code: beforeSend: function (xhr) { xhr.setRequestHeader("RequestVerificationToken", $('input:hidden[name="__RequestVerificationToken"]').val()); },

eshanrnh commented 2 years ago

Hey @techpara ๐Ÿ‘‹

Thanks for reverting with your findings. Would you be up for creating a PR for this? You can find more information at Contribute

pariashiri1984 commented 1 year ago

I tried all the mentioned steps with @Html.AntiForgeryToken() and [ValidateAntiForgeryToken] also with [IgnoreAntiforgeryToken] but I still have 400 in AJAX call.

sofietoft commented 1 year ago

Hi @pariashiri1984 ! Sorry to hear you're having some issues with this one.

Did you follow the docs outlined here: https://docs.umbraco.com/umbraco-cms/reference/routing/surface-controllers#preventing-cross-site-request-forgery-xsrf-csrf-attacks ? Also, what version of Umbraco are you using when running into these issues?

pariashiri1984 commented 1 year ago

Hi @pariashiri1984 ! Sorry to hear you're having some issues with this one.

Did you follow the docs outlined here: https://docs.umbraco.com/umbraco-cms/reference/routing/surface-controllers#preventing-cross-site-request-forgery-xsrf-csrf-attacks ? Also, what version of Umbraco are you using when running into these issues?

Thanks for the reply. I am using v10.2.1 and checked the link but there is no more information than this thread. Works fine with ApiController but not with the surface.

sofietoft commented 1 year ago

Thanks for sharing the details ๐Ÿ’ช

Could you please open a new Issue here on the Docs tracker, detailing what you're running into here? Then I'll run it by our developers for another checkup. Could be that something has changed between this article was updated and the latest Major releases.

I'd also recommend that you raise the issue on either the Forum or directly on the Umbraco CMS Issue tracker ๐Ÿ™Œ That might give you a slightly faster resolution time here!

ShlomitFxoro commented 11 months ago

Hello, one more late question :) I had a problem that if I submit a form after authentication cookie expired, my form returned: error 400. I Used [IgnoreAntiforgeryToken] - and it was solved!!! Thank you @mistyn8 !!!

But.. is it possible to use [ValidateAntiForgeryToken] but write an exception that if the antiforgery token is not validated, I will not get error 400 but will be redirected to another page?

Thank you!