[#12306] - Cookies Review
- Fixed in Code Base
- 22 May 2017
- Critical
- Build: staging
- # 12306
- Diff
- andrepereiradasilva:cookies-37
- Success continuous-integration/drone the build was successful Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
- Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
Summary of Changes
This PR does a cookies review across the cms.
- Use epoch + 1 to delete the cookie. I think it's not necessary time() - xxxx. Epoch + 1 should be enough. Also we can have problems with timezones if not putting time() less a full day.
- Use always '' (empty) when deleting a cookie.
- Use the Api when deleting the session cookie.
- Always use HTTPOnly for cookies so they can't be accessed through JS.
- If HTTPS is forced in the client (Site/Admin) always use only secure cookies
- Don't use
app->isSSLConnection()for checking if the cookie should be secure. We could have problem setting a secure cookie in a dual HTTP/HTTPS site. Use the new$app->isHttpsForced()instead. - Don't use a new language cookie if the cookie language is defined in the language filter plugin to be only session. Use the session instead (we already have a cookie...).
And minor code and code style improvements.
Testing Instructions
Since this impacts the users/admin sessions the tests must be made with special attention.
- First do a code review.
- Apply this patch in latest 3.7.x branch
- Watch the cookies that are created in the developer console (Resources -> Cookies) in the following processes:
- site login/logout process
- backend login/logout process
- the remember me process in site authentication
- in a multilanguage site, the remember of the user language (aka the language cookie)
Observations
I checked all the cookies in joomla core (i think i'm not missing none), one i don't know what it does (banner tracks), but i also updated that one.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
|
||
| Category | ⇒ | Administration Components Libraries Front End Plugins |
| Labels |
Added:
?
|
||
Bakual
- | Labels |
Added:
?
Removed: ? |
||
| Title |
|
||||||
| Category | Administration Components Libraries Front End Plugins | ⇒ | Administration com_banners Libraries Front End Plugins Components |
@andrepereiradasilva can you fix the conflicts here?
is it solved now, or still needs testing?
This needs testing for sure
What we should test?
@andrepereiradasilva Can we get another conflicts fix so testing can proceed?
I have fixed conflicts
I have tested this item
Tested both the front end and backend, works great.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12306.
Hi.
I just tried to test this, but after I removed all patches and applied patch 12306 it logged me out from administrator and now when I try to login it gives me:
"The most recent request was denied because it contained an invalid security token. Please refresh the page and try again."
No matter if I refresh the page, I still get this. My current setup is Nginx on Mac with php 7. This was in FF. I also tried it in Chrome to be sure, getting the same error.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12306.
| Priority | Medium | ⇒ | Critical |
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12306.
| Status | Pending | ⇒ | Ready to Commit |
RTC after two successful tests.
I tested frontend and backend and force ssl for admin only and for the entire site, and shared sessions no and yes, and language filter cookie for session and for year. Works like a charm, this PR does not introduce new bugs but saves 1 cookie in case if language filter cookie is session only, and the language filter still remembers the previously selected site language as before.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12306.
@kalemanzi This PR requires to be tested on latest staging + this patch applied (which can be done with patchtester latest stable release after having installed a clean staging with multilingual option). If you applied the patch on a 3.6.5 or something else not being latest staging, it can have caused your problems because other patches might have been missing. Can you check and if possible test again with latest staging?
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12306.
rdeutz
- | Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-05-22 18:08:11 |
| Closed_By | ⇒ | rdeutz | |
| Labels |
Added:
?
Removed: ? |
||
Hi, this cookies changes in plugins/system/languagefilter/languagefilter.php cause problem with "com_tags" in single tag view, if tags option "language filter" is set to "Current".
In my case in tag view I have only EN articles, no matter what language I set in frontend...
If I replace these lines with code from j 3.7.2, there is no problem.
Thanks.
Ok
This is actually very important. Adding release blocker tag