@diosmosis opened this Pull Request on January 27th 2019 Member

Only done in the Login controller so should not be done on every request.

Tested w/ a PHP INI config that set the cookie path to a subdirectory and was able to verify this fixed the issue.

h/t to @GreenReaper for identifying the issue (at least it seems likely this is the case).

@tsteur commented on January 28th 2019 Member

@diosmosis need to investigate but for some reason I cannot log in anymore. Can you still log in on this PR?

@GreenReaper commented on January 28th 2019

Are you using OAuth, like a LiveJournal logon or suchlike? GitHub deprecated it; you will need to do a reset to your email address, or contact them.


From: Thomas Steur <notifications@github.com>
Sent: Monday, January 28, 2019 2:05:38 AM
To: matomo-org/matomo
Cc: GreenReaper; Mention
Subject: Re: [matomo-org/matomo] Make sure any precreated session cookies on the wrong path are deleted (#14026)

@diosmosishttps://github.com/diosmosis need to investigate but for some reason I cannot log in anymore. Can you still log in on this PR?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com/matomo-org/matomo/pull/14026#issuecomment-457977895, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAXYQ5cE-lK0mXujudFScFx5b99tBEfEks5vHlrygaJpZM4aUdWz.

@tsteur commented on January 28th 2019 Member

No custom login plugin or anything. I always get this error:

image

@tsteur commented on January 28th 2019 Member

Just double checked... I can log in with 3.x-dev but not this PR

@tsteur commented on January 28th 2019 Member

image

and it goes into that method

@diosmosis commented on January 28th 2019 Member

@tsteur yes I can login w/ it. Can you check what your ini_get('session.cookie_path') setting is and the INI config for login_cookie_path is?

The PR should just remove the cookie on the ini_get('session.cookie_path') path if it's different than login_cookie_path. Can you also check if there is a PIWIK_SESSID cookie? Would help to look at the Set-Cookie header from matomo too.

@tsteur commented on January 28th 2019 Member

image

image

image

@tsteur commented on January 28th 2019 Member

Maybe empty path is same as /?

@diosmosis commented on January 28th 2019 Member

It is essentially (if it's empty, it gets set to / in Session.php). Maybe there's some whitespace there? (Nevermind no whitespace in the image.)

@tsteur commented on January 28th 2019 Member

No whitespace anywhere...

@tsteur commented on January 28th 2019 Member

maybe don't clear existing cookie if form was posted? not sure though...

@diosmosis commented on January 28th 2019 Member

Can you check what the Set-Cookie is in the module=Login&action=login call? And if there is a PIWIK_SESSID on any path in the chrome dev tools?

@diosmosis commented on January 28th 2019 Member

Oh maybe that's it...

@tsteur commented on January 28th 2019 Member

image

image

@diosmosis commented on January 28th 2019 Member

Updated the PR to check for non-POST requests.

@diosmosis commented on January 28th 2019 Member

Oh nvm, this is a dumb mistake. Will push a fix now.

@diosmosis commented on January 28th 2019 Member

@tsteur can you pull and try now?

@tsteur commented on January 28th 2019 Member

Now I can log in @diosmosis

btw the POST check didn't really help... The first time I submit the form it would show the error message again, and only the second time I submit the form again it would work

@diosmosis commented on January 28th 2019 Member

btw the POST check didn't really help... The first time I submit the form it would show the error message again, and only the second time I submit the form again it would work

The bug was because the final value was '/', but the config option would be empty, so it always removed the cookie. The POST check is definitely helpful since it might've lead to some weird bugs (though not sure exactly what)...

@tsteur commented on January 28th 2019 Member

Just removed the POST check and still worked

just random thought... would have been an alternative been to rename the cookie name?

@tsteur commented on January 28th 2019 Member

The POST check is definitely helpful since it might've lead to some weird bugs (though not sure exactly what)...

👍

@diosmosis commented on January 28th 2019 Member

just random thought... would have been an alternative been to rename the cookie name?

You mean PIWIK_SESSID => MATOMO_SESSID or something? Think so... not tested though.

@tsteur commented on January 28th 2019 Member

Might be good to change it to Matomo* anyway... can do it in a next release though. Would have expected this should fix any already set cookie

@diosmosis commented on January 28th 2019 Member

Would have expected this should fix any already set cookie

Just in the case of session.cookie_path != login_cookie_path, not every case. MATOMO_SESSID would be simpler.

@tsteur commented on January 28th 2019 Member

I suppose it looks good to merge though. Hoping there are no side effects :)

This Pull Request was closed on January 28th 2019
Powered by GitHub Issue Mirror