Deal with CSRF errors in a user friendly way #2876

Open
sunnysideup opened this Issue Feb 19, 2014 · 27 comments

Comments

Projects
None yet
7 participants
Contributor

sunnysideup commented Feb 19, 2014

When you submit a form and then copy the URL and send it to someone ... they get this horrible message:

There seems to have been a technical problem. Please click the back button, refresh your browser, and try again.

Which is wrong and useless.

I wonder if we can add something like this to the controller (PSEUDO CODE)


        if($this->request->param("Action") == $nameOfForm && !isset($_POST["SecurityID"])) {
            return $this->redirect($this->Link());
        }
Contributor

tractorcow commented Feb 20, 2014

Security ID can be disabled, so you can't rely on this behaviour in all requests, unfortunately.

In some cases it might not be appropriate either to return to the current link. I think redirectBack might be better in this context?

I would wonder if preventing the form name appearing in any copyable URL might be a better goal?

Contributor

sunnysideup commented Feb 21, 2014

I think this may need some wider discussion. The first thing is establish the exact problem... In my opinion, we need to fix:

  1. url with form-name (method / action) in it can not be copied
  2. missing Security ID message looks like the site is broken
Owner

wilr commented Feb 21, 2014

At a higher level, accessing a POST only form via GET should throw an error page right? So you should see that before you see a Security ID warning.

Member

micmania1 commented Feb 22, 2014

Instead of the current message could we not throw a relevant HTTP error then let the framework/cms deal with it?

For example:
405 Method Not Allowed
A request was made of a resource using a request method not supported by that resource;[2] for example, using GET on a form which requires data to be presented via POST, or using PUT on a read-only resource.

Where the CMS is present, this could be handled just like a 500 or 404 where the user can set their own content.

Contributor

tractorcow commented Feb 23, 2014

I think a 405 is likely the correct method for handing this situation; Whatever error generation we implement should be consistent with our current error handling system.

Contributor

sunnysideup commented Feb 23, 2014

I reckon we should solve this more elegantly than showing an error page, even if it is customised. We need to look at this from the user's perspective rather than what is theoretically correct (even though I agree with that). The key is to consider carefully on how these errors come about and what would be the most helpful response for the user.

Contributor

tractorcow commented Feb 23, 2014

How does the below sound as a draft solution?

  • If the method is POST and the form is GET then 405 error.
  • If the method is GET and the Form is a POST form then the default behaviour could redirectBack. However, the system needs to be configurable so that a 405 could be an potential response (whether using config or an instance variable). This may be necessary in strict environments. The 'Form::strictFormCheck' property probably should be replaced here (necessitating an API change).
  • If the method is of the correct type (POST/GET) and SecurityID is invalid, use a 403 error.
  • Otherwise, as per normal.
  • Also, documentation for Form should be updated to encourage Form usage to redirect users to appropriate action urls on submit (/thanks for success, for instance) to avoid copy/pasteable urls.

I think the problem with strictFormCheck being off, in its current incarnation, is that it means that more requests are now accepted by the form, but then subsequently fail due to missing SecurityID.The current behaviour should probably be deprecated in 3.2 in favour of the above reworked logic.

How does this sound?

Contributor

simonwelsh commented Feb 23, 2014

Before getting too carried away with this, remember there is currently no notion of a GET/POST only form.

Contributor

tractorcow commented Feb 23, 2014

@simonwelsh True, but that's only because strictFormMethodCheck is false by default.

At the moment it's quite easy to have a form that uses GET by default, but still requires a SecurityID. I don't think there's currently any catch all solution we can implement. There probably should be either some convention or in-code mechanism to prevent problematic scenarios such as this, which is why I suggested enforcing more strict form method checking in 3.2.

Contributor

tractorcow commented Feb 23, 2014

I think that whatever the solution, we should at the very least break up the form method and securityid checking components of Form::httpSubmission into their own methods. That way if a specific form requires customised access logic, it can be more easily overridden in user code.

Contributor

Zauberfisch commented Feb 28, 2014

uhm, am I missing something here? because to me this never ever has been a problem.
after submitting a form, isn't it best practice to redirect back or to a other page after form submission to avoid duplicate submissions?

Contributor

tractorcow commented Mar 2, 2014

Best practice and what happens in the wild can differ. :) In an ideal world it would be impossible to do it the wrong way, but in a semi-forgiving environment it's easy to end up with situations like the above; Urls that work on initial submit but can't be copied and pasted elsewhere.

Contributor

Zauberfisch commented Mar 2, 2014

ok, lets get a code example in here to make sure we are all on the same page:

class SomeController extends Controller {
    private static $allowed_actions = array(
        'MyForm',
        'afterMyForm',
    );
    public function index() {
        return $this->customise(array(
            'Form' => $this->MyForm(),
        ));
    }
    public function MyForm(){
        return Form::create(
            $this,
            __FUNCTION__,
            FieldList::create(TextField::create('Name')),
            FieldList::create(FormAction::create('doSend', 'Send')),
            RequiredFields::create('Name')
        );
    }
    public function doSend($data) {
        // the user does not see any output from here,
        // he also does not get the chance to copy paste an URL to this
        Session::set('MyFormSubmissionName', $data['Name']);
        $this->redirect($this->Link('afterMyForm'));
    }
    public function afterMyForm() {
        $name = Session::get('MyFormSubmissionName');
        if ($name) {
            return $this->customise(array(
                'Title' => "Hello $name!",
                'Content' => 'How nice to see you.'
            ));
        }
        // no name, lets redirect to the page with the form on it
        // alternatively we could of course also display a nice message
        $this->redirect($this->Link());
    }
}

in the above code example, I can see no way that the user could copy paste any URL that would get him to a wrong page.
And as far as I can see, this is the only sane way to use a normal form anyway, outputting something in the form action handler doSend just sounds like a terrible idea,

Contributor

tractorcow commented Mar 2, 2014

@Zauberfisch That is the correct convention, at least in my experience. It would be great if we could somehow show a deprecation notice to the user when and if an appropriate redirect isn't used, or maybe perhaps it should be an E_USER_WARNING, if we agree this has always been a misusage of the API?

I would worry a little bit about not making it over-reaching and covering situations where a redirect isn't absolutely necessary (ajax, GET requests, etc).

It's a little hard to deprecate the non-usage of something also, so perhaps instead of enforcing it via code we do so via documentation. https://github.com/silverstripe/silverstripe-framework/blob/3.1/docs/en/topics/forms.md seems to encourage the troublesome methodology of simply outputting content in the form method, and is due an update, so perhaps a potential fix should start there?

Contributor

Zauberfisch commented Mar 2, 2014

ah, haven't looked at the docs in ages.
that makes a lot of things clear: if we tell people to do it the wrong way, there obviously is a problem.

I see 2 things that need to be done:

  • fix the docs
  • make the error message nicer, so that in the case someone gets to see There seems to have been a technical problem. Please click the back button, refresh your browser, and try again. it at least doesn't look scary.
Contributor

sunnysideup commented Mar 8, 2014

@Zauberfisch ... love your example and I agree that this is best practice, but it also immediately makes me think if we can not do the step from doSend to afterMyForm automatically.... I wonder if the form could take care of that?

Contributor

Zauberfisch commented Mar 8, 2014

while doing this automaticlly would help in most cases, there is a number of edge cases where one would not want that redirect to occur or a custom redirect.
thus I am not sure if letting form do that by default is the best way.

also, Form is an essential part of the framework, and any API change on it has to be very very well thought through, and switching to auto redirect will most likely bring many backwards compatibility issues.

@simonwelsh simonwelsh added the master label Mar 15, 2014

Contributor

tractorcow commented Mar 17, 2014

I just had a thought about this in the weekend. Doesn't it seem obvious that the best way to handle this error is with a form validation notification?

Contributor

Zauberfisch commented Mar 29, 2014

that is actually a pretty good idea.

because there is one problem I have not considered before:

if a user submits a form that has a wrong security token for some reason
(lets say he had 2 tabs open and got a new security token, but has not reloaded the old tab with the form containing the old security token).
In this case, after submitting, he would loose all submitted data and see the nasty error message.

your idea @tractorcow is actually brilliantly solving that as well by keeping the entered content.

Member

micmania1 commented Mar 30, 2014

@tractorcow I don't think that would work for all cases. Take for example...

<?php

class ContactPage_Controller extends Page_Controller {

    private static $allowed_actions = array(
        ContactForm",
        "displayform", // Form only displays on this action
    );

    public function ContactForm() {
        return Form::create($this, "ContactForm", new FieldList(), new FieldList());
    }

}

Assuming the index (default action) doesn't display the form, but the displayform action does then how would the form know to render the correct template or redirect back to the correct URL where the error message/form could be shown? The form only knows which controller it belongs to but has no idea of its intended context.

$controller->redirectBack() wouldn't work here because these URLs can be accessed directly (ie mydomain.com/contact/ContactForm). The form wouldn't know where the user should be redirected to.

Contributor

tractorcow commented Mar 31, 2014

You're right, it would only handle the case where the security ID had expired, but a referrer was still available. In other situations, it would redirect the user to the home page. Perhaps there's no one solution that solves all of the problems.

Contributor

Zauberfisch commented Mar 31, 2014

well, thinking about it I acutally find the expired security ID to be much a bigger problem than someone accessing the form direct, as I always do a redirect after the form.

an idea I just had is to add a configuration option, lets say FailureReturnURL.
then if and error occurs, it will try to get back by trying this 3 options in this order:

  • if FailureReturnURL is set, redirect to it
  • if redirectBack() is possible, do that
  • if no url is found, redirect to $controller->Link()

In most cases the form is on the index() method. so even if no previous url is set, it would still work.
for the case that the form is displayed on an action, the developer has to set the return url to ensure the form returns if redirectBack() is not possible.

and while on that subject, we can also consider having a SuccessReturnURL or something, to which silverstripe will redirect after the form action has been executed.
this just removes the need of adding a redirect to your form action, and might also help promote the best practice of doing the redirect.


additionally, we need to accept the fact that we can not "fix" angels of this problem, some forms might be used on 2 different actions, and without redirectBack() it would be impossible to determinate where to redirect to on error.

Member

micmania1 commented Mar 31, 2014

I personally don't think there's a "problem" with the way it currently works as the form correctly throws an HTTP error when it should. The issue here is the user-experience which at the moment isn't very nice.

How about adding a callback before the HTTP error is thrown? This would allow the user (dev) to take control, handle it however they wish and we wouldn't be making assumptions about how the system should work (ie auto-redirect). When no callback is set the current HTTP error would be used as a fallback.

Contributor

sunnysideup commented Apr 6, 2014

How about adding a callback before the HTTP error is thrown? - sounds like a great idea.

The issue here is the user-experience which at the moment isn't very nice. The user-experience is the MOST important thing in all web sites and web applications. If we loose sight of the user then we loose the user ;-)

Contributor

camfindlay commented Oct 3, 2014

Anyone want to put forward a pull request based on this discussion? ;)

@tractorcow tractorcow changed the title from smarter way to deal with un-submitted forms being called in a pasted URL to Deal with CSRF errors in a user friendly way May 18, 2017

Contributor

tractorcow commented May 18, 2017

I've re-named this ticket and changed its focus to the core issue.

I still think that moving CSRF to form validation is the best way to address this, but it would be acceptable to address this some other way that improves UX.

Contributor

Zauberfisch commented May 18, 2017

+1 for doing it in Form->validate(), makes a lot of sense and would be most easy to integrate there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment