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

login: Fix CSRF fail, add shake effect on authentication fail #3955

Open
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@greezybacon
Member

greezybacon commented Sep 12, 2017

It's possible I'm the only user annoyed by this issue. Everyday when I come into work and start working on tickets, I click somewhere in the interface and am presented with the login screen. After authenticating, I'm presented with the CSRF failure message and receive a database error email for DB Error 1062 (Duplicate entry '[blah]' for key 'PRIMARY').

This fixes an issue where the current session data is not retrieved from the database, because it is expired. However, the session-id is not reset either. Therefore, the session data with the new CSRF token is not updated in the database and the user may have trouble logging in. (This is where the db error email comes from).

This problem manifests itself as a session expires. If a user clicks somewhere in the software and their session is now expired, a redirect to the login page is triggered. However, the CSRF token sent in the login page is not saved in the database. So when the user logs in, they are greeted with the CSRF failure message.

This issue is addressed by retrieving the session data from the database, but clearing the content if it is expired. Therefore, the session data appears to the software as invalid and is properly reset and saved to the database, thereby avoiding the errors.

This also changes the staff authentication process to use AJAX to authenticate (with a failsafe of using traditional page requests). If the authentication fails on the server because of CSRF problems, the page is refreshed in the browser fetching a new CSRF token. If the authentication fails because of password issues, the login box shakes and the user can retry. As before, the forgot password link shows up after the first failure. Upon success, a redirect is issued into the staff control panel.

This should not affect external authentication plugins such as LDAP or OAUTH, but I have not yet tested them.

greezybacon added some commits Sep 12, 2017

login: Clear session data on expiration
This fixes an issue where the current session data is not retrieved from the
database, because it is expired. However, the session-id is not reset either.
Therefore, the session data with the new CSRF token is not updated in the
database and the user may have trouble logging in.

This problem manifests itself as a session expires. If a user clicks somewhere
in the software and their session is now expired, a redirect to the login page
is triggered. However, the CSRF token sent in the login page is not saved in
the database. So when the user logs in, they are greeted with the CSRF failure
message.

This issue is addressed by retrieving the session data from the database, but
clearing the content. Therefore it appears to the software as invalid and is
properly reset and saved to the database, thereby avoiding the errors.
login: Use AJAX to authenticate, add a shake effect
This changes the staff authentication process to use AJAX to authentication
(with a failsafe of using traditional page requests). If the authentication
fails on the server because of CSRF problems, the page is refreshed in the
browser fetching a new CSRF token. If the authentication fails because of
password issues, the login box shakes and the user can retry. As before, the
`forgot password` link shows up after the first failure. Upon success, a
redirect is issued into the staff control panel.

JediKev referenced this pull request Dec 20, 2017

fix #3546
DbSessionBackend->read() should not return null but empty string.

aydreeihn added a commit to aydreeihn/osTicket that referenced this pull request Mar 9, 2018

@kevinoconnor7

This comment has been minimized.

Show comment
Hide comment
@kevinoconnor7

kevinoconnor7 May 28, 2018

Contributor

This patch appears to have introduced a bug to the session code which breaks my CAS Authentication plugin.

Your change to the read method in DbSessionBackend adds an annotated field, which in turn gives us back an AnnotatedModel.

The problem is that in the update method reads the field __new__ on the model. However, this now goes through the AnnotatedModel magic getter which tries to read the field from the database result, rather the field on VerySimpleModel.

My quick-and-dirty fix was to update AnnotatedModel to first check if the field exists on the underlying object by updating the getter to as:

    function get($what) {
        if (isset($this->annotations[$what]))
            return $this->annotations[$what];
        if (property_exists($this->model, $what))
            return $this->model->{$what};
        return $this->model->get($what, null);
    }

This seems a bit messy though. Instead VerySimpleModel should probably have a isNew method and it shouold be used in the DbSessionBackend.

Contributor

kevinoconnor7 commented May 28, 2018

This patch appears to have introduced a bug to the session code which breaks my CAS Authentication plugin.

Your change to the read method in DbSessionBackend adds an annotated field, which in turn gives us back an AnnotatedModel.

The problem is that in the update method reads the field __new__ on the model. However, this now goes through the AnnotatedModel magic getter which tries to read the field from the database result, rather the field on VerySimpleModel.

My quick-and-dirty fix was to update AnnotatedModel to first check if the field exists on the underlying object by updating the getter to as:

    function get($what) {
        if (isset($this->annotations[$what]))
            return $this->annotations[$what];
        if (property_exists($this->model, $what))
            return $this->model->{$what};
        return $this->model->get($what, null);
    }

This seems a bit messy though. Instead VerySimpleModel should probably have a isNew method and it shouold be used in the DbSessionBackend.

@JediKev JediKev referenced this pull request Aug 14, 2018

Open

More issues with Valid CSRF #4446

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment