Security Issue on Update?

[dolbex]

Maybe I'm missing something here but removing 'unique' from email and username may be an issue in the updateRules for validation. Shouldn't we just be appending the exception id as noted in the Laravel docs?

Doesn't this allow anyone to edit their email / username with an existing username or email in the system?

    /**
     * Ardent validation rules
     *
     * @var array
     */
    public static $rules = array(
        'username' => 'required|alpha_dash|unique:users',
        'email' => 'required|email|unique:users',
        'password' => 'required|between:4,11|confirmed',
        'password_confirmation' => 'between:4,11',
    );

    /**
     * Rules for when updating a user.
     *
     * @var array
     */
    protected $updateRules = array(
        'username' => 'required|alpha_dash',
        'email' => 'required|email',
        'password' => 'between:4,11|confirmed',
        'password_confirmation' => 'between:4,11',
    );

[nightowl77]

Might be a good idea for out-the-box functionality, but the way I see it these rules have to be overridden in your own User model anyway. The users on the one site I currently run complained about the short max password length. They are a security conscious bunch.

Also, not all sites use usernames, some just use email for authentication, so just adding 'username' => 'alpha_dash|unique:users' without the "required" to the base ConfideUser might be an idea to look at.

Another suggestion would be to mentioned in the docs that if you use usernames, you should add the "unique" rule to your extended User model.

[dolbex]

I see what you're saying but it still allows the out-of-the-box configuration to be misused by folks that don't override the params. My thought is, if a security plugin includes rules, why not go ahead and set them in a way where users can't take someone else's username. Seems like a pretty big hole. All that needs to be done is add back 'unique' and then except for the current userid. Not sure how that confines anyone. Maybe I'm missing something?

[andrew13]

Please test with saving an update and having it set to unique. Let me know the results.

[andrew13]

In one of my apps I'm doing something like this for the update.

public function postUpdate($user)
{

       $rules = array (
            'username' => 'required|alpha_num|unique:users',
            'name' => 'required'
        );
        // Remove unique from update if it's the same.
        if(Input::get('username') == $user->username) {
            $rules['username'] = 'required|alpha_num';
        }

        // Validate & Update User
        [....]
}

Where $user is from the Route model binding http://laravel.com/docs/routing#route-model-binding

It's basically saying if the id that is passed in to get the object has the same username as what is from the Input then it doesn't need to be checked for uniqueness because it hasn't been updated.

[dolbex]

Andrew,

I'm sure that would work but you can do this directly in the validation rule ala the following:

'email' => 'unique:users,email_address,10'

You can see an example of this in the documentation here:
http://laravel.com/docs/validation#rule-unique

[andrew13]

That doesn't quite do what you want it to do. In that you're saying ignore the unique check for id 10. What if the user changes their email address? The new email address would not be checked that it is unique. You still need the check on whether the email address was updated. Unless I'm missing something in the documentation for unique.

[dolbex]

The way I understood it: This will check if it is unique but not for id 10. So, if the user saves without changing their email address (and the email address was present in the input array) it would ignore the uniqueness only against the id you specify (the user's id).

It's not saying "Don't check if it's unique for id 10 against everyone" it's saying "check if it's unique against everyone BUT id 10"

[nightowl77]

I have been working on this issue and manged to get it working with my own version of Confide. It can be see here

https://github.com/nightowl77/confide/tree/ardentv2

My version makes saving a user super easy.

First - I've thrown out UpdateRules. This is not what a developer would expect after reading the Eloquent docs. I wanted something very simple. As @Zizaco said in his first post about Confide on forums.laravel.io - we have Sentry2 for Laravel, but it is not very Laravel-ish. That's why he set out to create Confide

I wanted Confide to be a bit more Laravel-ish. If I want to save a user, I want to call "save" and it should be done. No amend/clone and or Updaterules. As a consumer of Confide I don't want to handle password and uniques and all that. Granted, the functions I now use in the my version of Confide I use was not available in Ardent, so a workaround was required. That workaround was UpdateRules/amend/etc.

Also, I wanted to use the power of Eloquent to decide if a user exists. I've thrown out the UserExists function that was in Confide. I now leave that up to the developer to decide. For example: some clients are ok with a user registering multiple accounts under the same email address. Some clients want only one email per user. Instead of having a config option for Confide to handle that, the developer can now handle that himself - simply by adding "unique" to his static $rules. Basically Eloquent will handle it, not Confide.

With the version of Confide I'm working on, saving a user is now as simple as:

 public function store($data, $id = null)
    {

        // Following is adapted from Confide generated controller.

        if (isset($id))
          $user = $this->findById($id);
        else
          $user = $this->instance();

        if (array_key_exists('username', $data))  $user->username = $data['username'];

        $user->email = $data['email'];
        $user->password = $data['password'];

        // The password confirmation will be removed from model
        // before saving. This field will be used in Ardent's
        // auto validation.
        $user->password_confirmation = $data['password_confirmation'];

        if (array_key_exists('confirmed', $data)) {
            $user->confirmed = $data['confirmed'];
        }

        // Save if valid. Password field will be hashed before save.
        $user->save();

        if ($user->id) {
            if (array_key_exists('roles', $data)) {
                $user->roles()->sync($data['roles']);
            } else {
                //$user->roles()->sync(array(1)); // Assign default role.
            }
        }
        return $user;
    }

Sidenote: @andrew13 - you might know that function. It is from your development branch - repositories/EloquentUserRepository.php. I however do not return an array on error, and rather check for $user->errors()->all() in my controller.

I only have "public static $rules" looking like this

  public static $rules = array(
     //'username' => 'required|unique:users',
     'email' => 'required|email|unique:users,email',
     'password' => 'required|between:4,20|confirmed',
     'password_confirmation' => 'between:4,20',
  );

I commented out "username" as the site I use this for only use email and not usernames.

The important bit here is that I use ONLY $user->save for both updates and creation of new users.

Currently this version is live in production on www.mybitalert.com and has been thoroughly tested for my use case. It might not be backwards compatible and it might now work for other use cases. I am working on backward compatibility here. laravel-ardent/ardent#52 (comment)

Oh, another big change I made is that this version of Confide also does not return the validation error "duplicate". This did not play nicely with the awesome error handling of Former. If a user had a duplicated email, I wanted the error next to the email. If a user had a duplicated username, I wanted the error next to that. The current version of Confide does not tell you which of the 2 is in duplicate, email or username? This leaves the user to wonder why it won't accept his registration. You can see it in action by going to the link above, click sign up, and try to register with the email "nightowl77@github.com".

If you want a pull request, please shout.

[nightowl77]

Oh, just one negative thing about the version of Confide I'm running. This THOROUGHLY marries Confide to Ardent and Eloquent. Switching out the underlying Data Abstraction Layer for something other than Eloquent will basically be impossible. Functions like "UserExists" will have to come back.

I do however not think this is a bad thing. Confide's original mission was to be a Laravel lib which means at least Eloquent will always be available. Making a purposefully "built for Eloquent" Auth Lib allows us to build the absolute best Eloquent Auth lib without being bogged down by all sorts of other issues.

@Zizaco knew this and I presume that's why he built a seperate https://github.com/Zizaco/confide-mongo for mongo based authentication.

[andrew13]

@dolbex I hope it works that way. Can you confirm that it does. If so that'll simplify the rules. I will verify as well but won't be able to check until this weekend.

[andrew13]

@nightowl77 The functions you removed should certainly be removed. When they were added it was due to a bug in Ardent at the time. If you'd like to make a pull request I would love to review it. Getting the code base "Cleaner" would be huge.

[andrew13]

@nightowl77 "Oh, another big change I made is that this version of Confide also does not return the validation error "duplicate". This did not play nicely with the awesome error handling of Former."
^ Really like to have that fixed

Get that pull request up :) 👍

[nightowl77]

@andrew13

I would not recommend sending a PR to the master branch. I forked this from cossou/confide and some of the small changes aren't mine. I also pull my version of ardent, but I checked - my version of Ardent is not required. It was mainly for backwards compatibility where the unsetting of usernames and passwords where handled in the controllers which would then pass their own rules to real_saves($rules). With this update it should not be required for the programmer to do that work.

You can however see the major updates here.

Most important is this

nightowl77/confide@Zizaco:master...ardentv2#L2L171

and this

nightowl77/confide@Zizaco:master...ardentv2#L2L273

I again screwed with the password update in real_save. It just makes more sense to me this way. Maybe I'm missing something, but it works quite nice the way it currently is.

If you give me some time I can do a proper PR. But if you maybe start cherry picking parts of the code you like, please please let me know. For me to do this PR right I need to figure out how I'm going to consolidate a fork of a fork. It might take a little while. I'm not quite a git ninja yet, but getting there.

[andrew13]

I would rather do this right. Take the time to do a proper PR. I'll start taking a look just to get familiar with the code changes so I'll be ready when it comes. When do you think you'll be able to get to it? 3 days / 1 week / 1 month? Just to get some sort of idea when I should free up time to review it and merge :)

[nadimtuhin]

learning a lot :)

[nightowl77]

The biggest issue here is backwards compatibility. A LOT of programmers still call validation before saving the user. :(

Case in point: https://github.com/andrew13/Laravel-4-Bootstrap-Starter-Site/blob/master/app/controllers/user/UserController.php#L92

I want to kill all that stuff, prepareRules, amend, it must all go. Updating a user should be as straight forward as updating any Eloquent model, and confide should hide all that complexities on the programmer's behalf.

The big question is: is that something you'd like to put in the master branch of Confide. We are GUARANTEED to break a lot of sites because Confide isn't tagged on a regular basis (last tag was 2 months ago) and most people use "dev-master" in their composer.json (the docs actually request that)

On the other hand, do we really want to be "held hostage" by backwards compatibility? I really believe that we could make some real progress here and make Confide real easy to use. Getting great authentication for a new site will be a snap, but we're going to have to break a 100 eggs to make this omelette. Someone is going to shout at us.

If you have developer access to confide it might be a good idea to tag this version of Confide v1 and warn people that version 2 with a major code cleanup is coming. Also change the docs under "Required Setup" to say "zizaco/confide": "1.0.x"

Let me know what you think. Must go to bed now, already 2 am but will check this thread again first thing in the morning.

[nadimtuhin]

I spent my last two days looking at ammend, updateRules, getupdaterules and what not... I think nightowl77 is right. They needs to be removed 👍

[nightowl77]

@dolbex - that is exactly how it works. Since ConfideUser extends Ardent you can call the function buildUniqueExclusionRules()

https://github.com/laravelbook/ardent/blob/master/src/LaravelBook/Ardent/Ardent.php#L750

It basically goes through your rules and will append the user id for you at the end of your unique rule.

Otherwise you can try as a quick fix to pull in my version of Confide by adding this to your composer.json file

Under "require"

"zizaco/confide": "v2.0.0-alpha1",

And then in the "root"

"repositories": [
      { 
        "type" : "vcs",
        "url" : "https://github.com/nightowl77/confide"
      }
    ],

You can see how it all fits together here https://gist.github.com/nightowl77/6144540

Your rules would then simply look like this

public static $rules = array(
    'username' => 'required|unique:users,username',
     'email' => 'required|email|unique:users,email',
     'password' => 'required|between:4,20|confirmed',
     'password_confirmation' => 'between:4,20',
  );

You would also then save your user the way you save any Ardent model. Don't do any validation before saving, do it the ardent way and check $user->errors()->all after your save call.

[andrew13]

I can tag it as v1.0.x. I don't like breaking people's applications but at some point it doesn't matter. Let's get v2 up and running.

[andrew13]

Tagged version v1.0.1

[andrew13]

@dolbex of note for buildUniqueExclusionRules you'll just want to call update instead of save.
@nightowl77 Send the PR for a new branch. when it's ready we'll merge it to master and tag it as version 2.0.0

[nightowl77]

You can send a PR to a new branch? That makes me a lot more comfortable. Every day I learn something new! Thank you Andrew.

To answer your question regarding "when", I'm working on it right now, and hopefully I can have it done some time today. It would be days rather thank weeks.

[andrew13]

@nightowl77 I made a develop branch since that's where these type of things usually go. PR to that. Great news on a couple days.

[nightowl77]

@andrew13 Thank you very much for that! :)

I'm just waiting on that pull request above for Ardent. It's not really needed since Confide will now handle all rules internally, but there could always be that 1 use-case where a user want to pass in rules. Plus I don't really want to break compatibility and change the function parameters of real_save.

With the current Ardent you cannot pass $rules to either updateUniques or buildUniqueExclusionRules. The moment that PR is accepted I'll push what I have.

[andrew13]

Looking forward to it!

[nightowl77]

That issue laravel-ardent/ardent#86 has been sitting in the queue for more than a week now. Can someone please help the maintainers of Ardent by pulling down my version and reviewing the code.

Can someone also confirm on that thread that

$rules = $class->getStaticPropertyValue('rules')

is the same as

$rules = static::$rules

I think that might be one of the issues holding them up.

[andrew13]

What is $class defined as?

[nightowl77]

Hi Andrew

To answer your question. In the original buildUniqueExclusionRules they used ReflectionClass($this).

protected function buildUniqueExclusionRules() {
        // Because Ardent uses statics (sigh), we need to do this to get the
        // model's rules.
        $class = new \ReflectionClass($this);
        $rules = $class->getStaticPropertyValue('rules');

I then changed it to

protected function buildUniqueExclusionRules(array $rules = array()) {

        if (!count($rules))
          $rules = static::$rules;

As far as I know, you don't have to use ReflectionClass($this) to get the static rules. The comment in the first piece of code suggests that it is required. Maybe that is what is holding up the developers from Ardent to accept my pull request. That's why I want someone to confirm static::$rules is the correct way to go and that it is essentially the same as ReflectionClass($this)

I made another commit on that pull request (laravel-ardent/ardent#86) this morning. I even updated the docs, so hopefully they will accept it this time.

[noguespi]

So I didn't understand, is this project going to still depend on ardent or not ? I'm using the fixed version of ardent of @nightowl77 because in my case I need to pass rules to updateUnique() (and it's bugged with the actual ardent version as you know).

In my case I need custom rules when a user edit his profile, he can edit all the field he wants but, if he edit his password, he must enter his previous password (for security reason because csrf can be bypassed if there is a XSS vulnerability).

The problem is when he doesn't edit his password i have to remove password rules otherwise it won't pass the validation (because all the form input are filled but password). I can fix the this doing $user->password_confirmation = $user->password; or in a more cleaner way using unset($rules['password']); (but only works with fixed ardent of @nightowl77 ).

    public function postEdit(){

        $user = Confide::user();
        $rules = User::$rules;

        // if changing the password, must enter the actual valid password
        if(!empty(Input::get( 'password_old' ))){
            if(!Auth::validate(array('email' => $user->email, 'password' => Input::get('password_old')))){
                return Redirect::action('UserController@getEdit')
                    ->with( 'flash_error', Lang::get('Invalid password'));
            }

            $user->password = Input::get( 'password' );
            $user->password_confirmation = Input::get( 'password_confirmation' );
        }else{
            unset($rules['password']);
            // works too, needed to bypass password confirmation
//            $user->password_confirmation = $user->password;
        }

        $user->email = Input::get( 'email' );
        $user->twitter = Input::get( 'twitter' );

        if($user->updateUniques($rules)

[andrew13]

How about we set this to @nightowl77 Ardent version?

[andrew13]

From the Ardent discussion:

#157

[andrew13]

@nightowl77 Ready for the pull request when you are. Ardent has finally updated their code. So we should be good to go now.