panique / huge

Simple user-authentication solution, embedded into a small framework.
2.14k stars 788 forks source link

[TODO] add CSRF token logic into every relevant POST request #725

Closed panique closed 8 years ago

panique commented 9 years ago

The CSRF logic (https://github.com/panique/huge/pull/693) is currently only implemented at login and registration (or so), but could be extended to all form fields handling sensitive data.

A documentation on CSRF logic would also be cool.

JFJanssen commented 9 years ago

This would bring it an important step closer to live use for me.

slaveek commented 8 years ago

Maybe this two pices of code can be useful to make it easier.

Add this method to core/Csrf.php

public static function invalidToken_action() {
    LoginModel::logout();
    Redirect::home();
    exit();
}

and then use:

// check if csrf token is valid
if (!Csrf::isTokenValid()) {
    Csrf::invalidToken_action();
}
panique commented 8 years ago

Hey, I've added a little documentation on the readme on how to use the CSRF feature on a form, and also copied a little bit of code from the csrf.php, it should now be super-easy for everybody to implement this whenever you need. So this ticket can be closed :)

slaveek commented 8 years ago

README instruction: https://github.com/panique/huge/tree/develop#how-to-use-the-csrf-feature-

Sorry to say but this:

if (!Csrf::isTokenValid()) {
    Login::logout();
}

doesn't work.

AFAIK there is no class Login in the project. Test give:

Fatal error: Class 'Login' not found
panique commented 8 years ago

Ah okay thanks, I'll have a look! ...

Hmm, in the loginController there's this:

        if (!Csrf::isTokenValid()) {
            self::logout();
        }

(which is basically LoginController:logout();), in the UserController there's

        if (!Csrf::isTokenValid()) {
            LoginModel::logout();
            Redirect::home();
            exit();
        }

The thing is LoginController:logout(); IS this:

    public function logout()
    {
        LoginModel::logout();
        Redirect::home();
        exit();
    }

Long story short, I've changed Login::logout(); to LoginController::logout(); in the documentations, so it should work fine now!

panique commented 8 years ago

(intense testing coming up, currently i just have some problems running the HUGE vagrant machine) :)

slaveek commented 8 years ago

Call LoginController::logout(); in static way probably give an error.

geozak commented 8 years ago

LoginController::logout(); would error because logout() is not a static method. However slaveek's earlier suggestion of making a method public static function invalidToken_action() in Csrf would work as a general use way to handle it and would make development less error prone.

panique commented 8 years ago

I think it's done for now... to keep it supersimple there's a little bit of duplicate code and a hard exit(), but i really don't have time to work on this right now.

geozak commented 8 years ago

I really like the idea of this project and I wish I would have discovered it sooner before I started descending into its "soft End Of Life". I was hope to contribute to it during the winter break. I still plan on contributing to it fixing up issues and minor improvement though as I am planing to use this as the starting framework for my capstone project next semester.

On an aside would you be okay if I continued the project and making it huge-r. I think it would be cool to implement some of the feature that are to big for huge, for example #691.

panique commented 8 years ago

Hey @geozak , definitly! You can take this project and fork it and do whatever you want to do, but it would be cool if you would do this on your own fork (and maybe calling it Huge 2 or huge remixed or so).

Personally I spend way to much time with this project (and others) and in the end it's just work work work and fixing and no real usage for me so it's tricky to do it permanently.

Ah yes and I will definitly promote your fork directly from the top of the readme if you like! :)

geozak commented 8 years ago

Awesome. I was thinking something like huger, hugeR, huge-r, huge-R. To kinda follow the size theme, Tiny, Mini, Huge and then huger. The word remixed is good too and would be nice wordplay with HUGE-R, HUGE...R

I think it would be better to clone it because of how the code searching mechanic of the GitHub and cloning still keep the commits. Although at first I will just fork it that way its easier to merge the work people submitted to huge, like #691, so that they are still credited.

Thanks, I'll probably start working on it in a few weeks after final exams times.