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

Proposal: decrease cookies usage. #11688

Closed
netroby opened this Issue Nov 24, 2015 · 56 comments

Comments

Projects
None yet
7 participants
@netroby
Contributor

netroby commented Nov 24, 2015

Current phpmyadmin using cookies for auth type by default.
But phpmyadmin take lot's of cookies. it will use at least 10 cookies.

That's too much. because the per domain cookie limit only 20.

pma_lang
pma_iv-1
pma_console_mode
pma_console_height
pma_console_config
pma_collation_connection
pmaUser-1
pmaPass-1
pmaCookieVer
phpMyAdmin

We can decrease cookie by merge cookie into json style value or other ways to do this.

2 - 3 cookies will be good.

@netroby

This comment has been minimized.

Show comment
Hide comment
@netroby

netroby Nov 24, 2015

Contributor

For example: cookie named pma_data

{"pma_lang":"en","pma_iv-1":"xxxxx","pma_console_mode":"coolapse","pma_console_height":"92"}
Contributor

netroby commented Nov 24, 2015

For example: cookie named pma_data

{"pma_lang":"en","pma_iv-1":"xxxxx","pma_console_mode":"coolapse","pma_console_height":"92"}

@lem9 lem9 changed the title from Proposal: decrase cookies usage. to Proposal: decrease cookies usage. Nov 24, 2015

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Nov 24, 2015

Contributor

You are quoting an older cookie spec. The current one AFAIK (https://tools.ietf.org/html/rfc6265) mentions "At least 50 cookies per domain."

Contributor

lem9 commented Nov 24, 2015

You are quoting an older cookie spec. The current one AFAIK (https://tools.ietf.org/html/rfc6265) mentions "At least 50 cookies per domain."

@netroby

This comment has been minimized.

Show comment
Hide comment
@netroby

netroby Nov 25, 2015

Contributor

I am using a domain with other my php apps. and also using phpmyadmin (on a special port, private access only). I meet a problem with login with phpmyadmin. at last i figure out it was cookies problem. I cleared other cookies. Then i can normally login into phpmyadmin.

Contributor

netroby commented Nov 25, 2015

I am using a domain with other my php apps. and also using phpmyadmin (on a special port, private access only). I meet a problem with login with phpmyadmin. at last i figure out it was cookies problem. I cleared other cookies. Then i can normally login into phpmyadmin.

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Nov 25, 2015

Contributor

Starting with version 4.5.1, there is code to clear cookies on version change.

Contributor

lem9 commented Nov 25, 2015

Starting with version 4.5.1, there is code to clear cookies on version change.

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Nov 28, 2015

Contributor

@netroby Was it a case of updating phpMyAdmin? Which version are you using?

Contributor

lem9 commented Nov 28, 2015

@netroby Was it a case of updating phpMyAdmin? Which version are you using?

@nijel nijel added the enhancement label Dec 2, 2015

@nijel nijel added the newbie label Jan 13, 2016

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Jan 13, 2016

Member

Indeed the cookie usage could be reduced. I think it could go down to:

  • pmaUser-N (we want longer validity here than other cookies)
  • pmaAuth-N (session cookie containing IV and encrypted password)
  • pmaConfig containing rest of the configuration including it's version to handle upgrades
Member

nijel commented Jan 13, 2016

Indeed the cookie usage could be reduced. I think it could go down to:

  • pmaUser-N (we want longer validity here than other cookies)
  • pmaAuth-N (session cookie containing IV and encrypted password)
  • pmaConfig containing rest of the configuration including it's version to handle upgrades
@zaverichintan

This comment has been minimized.

Show comment
Hide comment
@zaverichintan

zaverichintan Jan 13, 2016

Contributor

@lem9 is there any need to create multiple cookies?
As it seems the creator of this logic might r have thought about merging it.

Contributor

zaverichintan commented Jan 13, 2016

@lem9 is there any need to create multiple cookies?
As it seems the creator of this logic might r have thought about merging it.

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Jan 13, 2016

Member

I think the reason is mostly historical - nobody really thought about cookie usage and when needed to store something in cookie, he added new one...

Member

nijel commented Jan 13, 2016

I think the reason is mostly historical - nobody really thought about cookie usage and when needed to store something in cookie, he added new one...

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jan 13, 2016

Contributor

@zaverichintan There might be a reason: not deleting all cookies at once. Try "git grep removeCookie" to get an idea about when which cookies are deleted and why.

Contributor

lem9 commented Jan 13, 2016

@zaverichintan There might be a reason: not deleting all cookies at once. Try "git grep removeCookie" to get an idea about when which cookies are deleted and why.

@zaverichintan

This comment has been minimized.

Show comment
Hide comment
@zaverichintan

zaverichintan Jan 14, 2016

Contributor

Where should I start working on this issue?
Kindly help me.

Contributor

zaverichintan commented Jan 14, 2016

Where should I start working on this issue?
Kindly help me.

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jan 14, 2016

Contributor

@zaverichintan At this point your contributions are to prove your skills for your possible GSoC participation, so please come back with a more specific question.

Contributor

lem9 commented Jan 14, 2016

@zaverichintan At this point your contributions are to prove your skills for your possible GSoC participation, so please come back with a more specific question.

@zaverichintan

This comment has been minimized.

Show comment
Hide comment
@zaverichintan

zaverichintan Jan 15, 2016

Contributor

@lem9

  • There are some cookies which are generated sometimes only like - db_operations.php , cookie is set to show if the version is to be upgraded or not , should i merge it ? (in my opinion, this cookie is created rarely only when version is to be upgraded)
  • Setting global variables and setting cookies later according to -

pmaUser-N (containing pmaUser , pmalang)
pmaAuth-N (pma_iv and encrypted pmaPass)
pmaConfig (cookieVer , collation_collection , console _config , console_mode, console_height)

  • I am planning to use json_encode to merge multiple values, but this will lead to decode ookie and then set it in global variable .

kindly approve this cookie structure

Contributor

zaverichintan commented Jan 15, 2016

@lem9

  • There are some cookies which are generated sometimes only like - db_operations.php , cookie is set to show if the version is to be upgraded or not , should i merge it ? (in my opinion, this cookie is created rarely only when version is to be upgraded)
  • Setting global variables and setting cookies later according to -

pmaUser-N (containing pmaUser , pmalang)
pmaAuth-N (pma_iv and encrypted pmaPass)
pmaConfig (cookieVer , collation_collection , console _config , console_mode, console_height)

  • I am planning to use json_encode to merge multiple values, but this will lead to decode ookie and then set it in global variable .

kindly approve this cookie structure

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Jan 17, 2016

Member

@zaverichintan We follow the discussion, there is no need to ping everybody! It's weekend, so you really can not expect fast responses...

As for db_operations use, IMHO this should be rather stored in the URL or session, than using cookies for such temporary data.

Using json is indeed okay.

Member

nijel commented Jan 17, 2016

@zaverichintan We follow the discussion, there is no need to ping everybody! It's weekend, so you really can not expect fast responses...

As for db_operations use, IMHO this should be rather stored in the URL or session, than using cookies for such temporary data.

Using json is indeed okay.

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jan 17, 2016

Contributor

@zaverichintan What would be the duration of each cookie?

Contributor

lem9 commented Jan 17, 2016

@zaverichintan What would be the duration of each cookie?

@zaverichintan

This comment has been minimized.

Show comment
Hide comment
@zaverichintan

zaverichintan Jan 17, 2016

Contributor

@lem9
pmaUser-N -1 month (which is currently used for pmaUser)
pmaAuth-N - As configured ( which is used by pmaPass)
pmaConfig - 1 month
If json_encode and Json_decode is used then the places where $_COOKIES[''] was used will have to be replaced by $cookes[''], is that fine ?

Contributor

zaverichintan commented Jan 17, 2016

@lem9
pmaUser-N -1 month (which is currently used for pmaUser)
pmaAuth-N - As configured ( which is used by pmaPass)
pmaConfig - 1 month
If json_encode and Json_decode is used then the places where $_COOKIES[''] was used will have to be replaced by $cookes[''], is that fine ?

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Jan 17, 2016

Member

Dne 17.1.2016 v 16:54 Chintan Zaveri napsal(a):

If json_encode and Json_decode is used then the places where
$_COOKIES[''] was used will have to be replaced by $cookes[''], is that
fine ?

I think there should rather be some code wrapping this, optionally with
caching...

Michal Čihař | http://cihar.com | http://blog.cihar.com
Member

nijel commented Jan 17, 2016

Dne 17.1.2016 v 16:54 Chintan Zaveri napsal(a):

If json_encode and Json_decode is used then the places where
$_COOKIES[''] was used will have to be replaced by $cookes[''], is that
fine ?

I think there should rather be some code wrapping this, optionally with
caching...

Michal Čihař | http://cihar.com | http://blog.cihar.com
@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jan 17, 2016

Contributor

We already have wrappers for setting and deleting cookies, they could be adapted for the JSON stuff.

Contributor

lem9 commented Jan 17, 2016

We already have wrappers for setting and deleting cookies, they could be adapted for the JSON stuff.

@zaverichintan

This comment has been minimized.

Show comment
Hide comment
@zaverichintan

zaverichintan Jan 17, 2016

Contributor

Where can I find documentation about it?
As at places $_COOKIE variable are directly used.

Contributor

zaverichintan commented Jan 17, 2016

Where can I find documentation about it?
As at places $_COOKIE variable are directly used.

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jan 17, 2016

Contributor

@zaverichintan I assume you are talking about wrappers? Well, 4 days ago in a comment above, I suggested a command, did you try it?

Contributor

lem9 commented Jan 17, 2016

@zaverichintan I assume you are talking about wrappers? Well, 4 days ago in a comment above, I suggested a command, did you try it?

@zaverichintan

This comment has been minimized.

Show comment
Hide comment
@zaverichintan

zaverichintan Jan 17, 2016

Contributor

Yea, I successfully did that, from that I concluded that cookies are removed in case like - pma update.
It showed me when all the cookies removal is called.
But I am unable to find wrappers used to retrieve data from cookies.

Contributor

zaverichintan commented Jan 17, 2016

Yea, I successfully did that, from that I concluded that cookies are removed in case like - pma update.
It showed me when all the cookies removal is called.
But I am unable to find wrappers used to retrieve data from cookies.

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jan 17, 2016

Contributor

We are using $_COOKIE as there was no reason to wrap this. By the way, access could be wrapped into something like getCookie(). Did you find setCookie() ?

Contributor

lem9 commented Jan 17, 2016

We are using $_COOKIE as there was no reason to wrap this. By the way, access could be wrapped into something like getCookie(). Did you find setCookie() ?

@zaverichintan

This comment has been minimized.

Show comment
Hide comment
@zaverichintan

zaverichintan Jan 18, 2016

Contributor

Yes, setCookie is a wrapper used, may I proceed to use getCookie?
This will have changes in multiple files, should I proceed?

Contributor

zaverichintan commented Jan 18, 2016

Yes, setCookie is a wrapper used, may I proceed to use getCookie?
This will have changes in multiple files, should I proceed?

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jan 18, 2016

Contributor

Please proceed.

Contributor

lem9 commented Jan 18, 2016

Please proceed.

@zaverichintan

This comment has been minimized.

Show comment
Hide comment
@zaverichintan

zaverichintan Jan 19, 2016

Contributor

Php also provides some function for getCookie like
http://php.net/manual/en/httprequest.getcookies.php
Shall I create pma wrapper function?

Contributor

zaverichintan commented Jan 19, 2016

Php also provides some function for getCookie like
http://php.net/manual/en/httprequest.getcookies.php
Shall I create pma wrapper function?

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Jan 19, 2016

Member

Given that that is extension not shipped with PHP, I don't think it's good idea to depend on it...

Member

nijel commented Jan 19, 2016

Given that that is extension not shipped with PHP, I don't think it's good idea to depend on it...

@zaverichintan

This comment has been minimized.

Show comment
Hide comment
@zaverichintan

zaverichintan Jan 20, 2016

Contributor

@lem9 Where can I find definition of setCookie ?
kindly help me with this .

Contributor

zaverichintan commented Jan 20, 2016

@lem9 Where can I find definition of setCookie ?
kindly help me with this .

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jan 20, 2016

Contributor

@zaverichintan

git grep setCookie | grep function
Contributor

lem9 commented Jan 20, 2016

@zaverichintan

git grep setCookie | grep function
@zaverichintan

This comment has been minimized.

Show comment
Hide comment
@zaverichintan

zaverichintan Mar 21, 2016

Contributor

What we can do is to separate the cookies array on the basis of usage to reduce time complexity

Contributor

zaverichintan commented Mar 21, 2016

What we can do is to separate the cookies array on the basis of usage to reduce time complexity

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Aug 27, 2016

Member

See also phpmyadmin/docker#32 why having too many cookies can cause problems. I've just removed one potentially big cookie for most browsers in ad51873.

Member

nijel commented Aug 27, 2016

See also phpmyadmin/docker#32 why having too many cookies can cause problems. I've just removed one potentially big cookie for most browsers in ad51873.

@zaverichintan

This comment has been minimized.

Show comment
Hide comment
@zaverichintan

zaverichintan Aug 27, 2016

Contributor

I would request to integrate JS part of the code. And will change Php part to make realtime

Contributor

zaverichintan commented Aug 27, 2016

I would request to integrate JS part of the code. And will change Php part to make realtime

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Jul 10, 2017

Member

I've taken a deeper look at persistent user settings and it really deserves cleanup regardless what the browser limits for cookies are, see #13466.

Member

nijel commented Jul 10, 2017

I've taken a deeper look at persistent user settings and it really deserves cleanup regardless what the browser limits for cookies are, see #13466.

@nijel nijel self-assigned this Nov 28, 2017

nijel added a commit that referenced this issue Nov 28, 2017

Store console mode in userconfig
Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Store console height in user preferences
Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Store console configuration in user preferences
Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Generalize code for console preferences
We can reuse and better cache locally the operations to avoid additional
roundtrip to server when changing settings.

Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Integrate debug console settings into user preferences
Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Remove special casing for font size
It is now stored in configuration in same way as any other setting.

Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Store navigation width in user preferences
There is no need to have this separetely.

Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Use session to indicate switching to new database
There is no reason to store this in cookies.

Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Remove cookie cleanup code
We no longer store any structured information in the cookies, so the
upgrade path is not really needed. Also this can cause problems with
third party integrations.

Issue #11688
Fixes #13480

Signed-off-by: Michal Čihař <michal@cihar.com>

@nijel nijel added this to the 4.8.0 milestone Nov 28, 2017

nijel added a commit to nijel/phpmyadmin that referenced this issue Nov 29, 2017

Move collation connection setting to user preferences
It is now handled same way as other user settings.

Issue phpmyadmin#11688
Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/phpmyadmin that referenced this issue Nov 29, 2017

Move collation connection setting to user preferences
It is now handled same way as other user settings.

Issue phpmyadmin#11688
Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/phpmyadmin that referenced this issue Nov 29, 2017

Move collation connection setting to user preferences
It is now handled same way as other user settings.

Issue phpmyadmin#11688
Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>
@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Nov 29, 2017

Member

After cleanup done in #13466, we're down to following cookies:

  • phpMyAdmin - session ID, session
  • pma_lang - language preference, persistent
  • pmaUser-N - per server username, persistent
  • pmaAuth-N - per server authentication, session
Member

nijel commented Nov 29, 2017

After cleanup done in #13466, we're down to following cookies:

  • phpMyAdmin - session ID, session
  • pma_lang - language preference, persistent
  • pmaUser-N - per server username, persistent
  • pmaAuth-N - per server authentication, session

@nijel nijel closed this Nov 29, 2017

nijel added a commit that referenced this issue Nov 29, 2017

Changelog entries for #11688 and #13466
Signed-off-by: Michal Čihař <michal@cihar.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment