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

Application: added CSRF protection for signals using method POST [Closes #469] #1385

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fabik
Copy link
Contributor

@fabik fabik commented Jan 30, 2014

The RFC 2616, section 9.1.1 states:

In particular, the convention has been established that the GET and HEAD methods SHOULD NOT have the significance of taking an action other than retrieval. These methods ought to be considered "safe". This allows user agents to represent other methods, such as POST, PUT and DELETE, in a special way, so that the user is made aware of the fact that a possibly unsafe action is being requested.

So we should use POST for secured signals (and ideally for all signals with side effects).

We can do this using a JavaScript, that sends links with parameter data-nette-post using POST. We can also easily create a fallback method for users without JavaScript - if an user attempts to access a protected page using GET method, we will show him a confirmation form which will execute the request with the CSRF token in POST param.

This commit allows you to protect signal by calling e.g. $this->addProtection("Do you really want to delete user '$username'?"); at the beggining of the signal. After that, if you access the page via GET or via POST but with an invalid token, a confirmation dialog will be displayed.

A secured link can be created using <a n:href="signal!" n:secured>, which adds the parameter data-nette-post. You can send links with this parameter by method POST using the following JavaScript:

jQuery(function($) {
    $(document).on("click", "a[data-nette-post]", function(event) {
        var form = document.createElement("form");
        form.method = "POST";
        form.action = this.href;
        form.style.display = "none";
        var post = $(this).data("nette-post");
        for (var key in post) {
            var input = document.createElement("input");
            input.setAttribute("type", "hidden");
            input.setAttribute("name", key);
            input.setAttribute("value", post[key]);
            form.appendChild(input);
        }
        $(form).submit();
        event.preventDefault();
    });
});

(Maybe we should also create some official JavaScript like netteForms.js.)

If you want to send signals using AJAX, you have to just modify your JavaScript, so that it will use method POST and send POST params contained in $control->getCsrfPost().

The confirmation dialog can be customized by overriding the method displayCsrfConfirmationDialog() in Presenter.

@Majkl578
Copy link
Contributor

As a concept, good. But I don't like the addProtection call, what about annotation?

@fabik
Copy link
Contributor Author

fabik commented Jan 31, 2014

I've got an idea: We can make all signals protected by default. And if I somebody doesn't want it, they could use an annotation @unprotected. And the same thing will be applied to links - <a n:href="signal!" will add the data-nette-post attribute by default and those, who don't want to use it, can suppress it by an attribute n:unprotected.

The advantage is, that you cannot create an unprotected signal by a mistake. If you forget (even if only once!) to add e.g. the @secured annotation, you have a security vulnerability in your application. This will protect you against CSRF, just like Latte protects you against XSS.

The disadvantage is, that all web sites will have to use the JavaScript, otherwise it would be quite uncomfortable for users.

@rostenkowski
Copy link
Contributor

@fabik 👍

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

Successfully merging this pull request may close these issues.

None yet

3 participants