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

refactor: Session does not use cookies() #5656

Merged
merged 3 commits into from
Feb 15, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Feb 5, 2022

Description

  • The Response should send all cookies.

Checklist:

  • Securely signed commits
  • [] Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the refactor Pull requests that refactor code label Feb 5, 2022
@kenjis kenjis merged commit ea2366a into codeigniter4:develop Feb 15, 2022
@kenjis kenjis deleted the refactor-session branch February 15, 2022 06:30
@skuadron45
Copy link

hy kenji, i have problem after this commit. I am using myth auth and login with ajax. i see response for Set Cookie order was different before this commit. please you can see my repo this use case

https://github.com/skuadron45/ajaxmythauth

@skuadron45
Copy link

this problem i found when using version 4.2.0 and earlier, but there is no problem for version 4.1.9 and older

@kenjis
Copy link
Member Author

kenjis commented Jun 19, 2022

i see response for Set Cookie order was different before this commit.

Yes, it may change the order.
But why is it a problem? Did you send more than two cookies with the same name?

@skuadron45
Copy link

skuadron45 commented Jun 20, 2022

so it's confirm it will change the order of Set Cookie,

The problem is when i am using myth auth for authentication with ajax, the Set Cookie order was changed after version 4.2.0

In AuthenticationBase in myauth, after we successfully loggin, they regenerate the session, i don't know how it work, but when we regenerate session, there is will Set Cookie for new session, but in V 4.2.0, the problem was Set Cookie order was changed.
last Set Cookie order was old session and not new session after we regenerate session.

this problem didn't appear if we use redirrect response and not using ajax

maybe you can see my repo if you can help me with this commit issue.
https://github.com/skuadron45/ajaxmythauth

I already compare Set Cookie order for this project only success until version 4.1.9 before this commit

Thanks

@kenjis
Copy link
Member Author

kenjis commented Jun 20, 2022

You mean, after successful login, CI sends two session cookies?

@skuadron45
Copy link

yes if i use ajax method @kenjis

@skuadron45
Copy link

should i send a screenshoot ?

@skuadron45
Copy link

skuadron45 commented Jun 20, 2022

gambar

blue was old session, and red was new session after session regenerate was calling.
On 4.1.9 Set Cookie order is blue after that red, but in 4.2.0, Set Cookie order is like the picture above.

If i call this method before send the response, it will fix the problem because Set Cookie is only one for new session (red)
gambar

I hope you can help me to troubleshoot about this problem, i don't know if this issue for CI or myth auth, because in myth auth, after successfull login, they will regenerate the session

gambar

Thanks

@skuadron45
Copy link

i have a question, what is Set Cookie was send if we regenerate session by default ?
because i didn't see this on debug the code, so i can't found why there is Set Cookie if there was a regenerate session.

@kenjis
Copy link
Member Author

kenjis commented Jun 20, 2022

It seems this bug is related to Apache.
https://bugs.php.net/bug.php?id=75554

But in this case, it may be a bit different from the PHP bug.

@kenjis
Copy link
Member Author

kenjis commented Jun 20, 2022

this problem didn't appear if we use redirrect response and not using ajax

Did you redirect with withCookies()?

return redirect()->back()->withCookies();

https://codeigniter4.github.io/CodeIgniter4/general/common_functions.html#redirect

@kenjis
Copy link
Member Author

kenjis commented Jun 20, 2022

It seems this is a bug in the framework Session.

As a workaround, remove the old session cookie with CookieStore::remove().
If there is the old session cookie only, CookieStore::clear() as you did is fine.

@skuadron45
Copy link

this problem didn't appear if we use redirrect response and not using ajax

Did you redirect with withCookies()?

return redirect()->back()->withCookies();

https://codeigniter4.github.io/CodeIgniter4/general/common_functions.html#redirect

I don't have problem with page request, the problem only appear if i use ajax,

@skuadron45
Copy link

It seems this is a bug in the framework Session.

As a workaround, remove the old session cookie with CookieStore::remove(). If there is the old session cookie only, CookieStore::clear() as you did is fine.

Okay thanks, so that was i done for resolve this case, thanks kenjis

@kenjis
Copy link
Member Author

kenjis commented Jun 20, 2022

I don't know why this issue appears only on Ajax.

@skuadron45
Copy link

It seems this bug is related to Apache. https://bugs.php.net/bug.php?id=75554

But in this case, it may be a bit different from the PHP bug.

I think so, because at v 4.1.9, there are 2 Set Cookie, the different just only in ordering as you can see on picture above

@skuadron45
Copy link

But this was very clear for me

"Servers SHOULD NOT include more than one Set-Cookie header field in the same response with the same cookie-name. (See Section 5.2 for how user agents handle this case.)"

@kenjis
Copy link
Member Author

kenjis commented Jun 20, 2022

I created an issue for this: #6166

@skuadron45
Copy link

I created an issue for this: #6166

Thanks @kenjis

I hope you can reproduce this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants