chrome.zabbix.cookies.png
js_class.cookie.patch
Team B
[ZBX-14758] Cookie names and values should be URI-encoded Created: 2018 Aug 22 Updated: 2024 Apr 10 Resolved: 2018 Nov 05 |
|
| Status: | Closed |
| Project: | ZABBIX BUGS AND ISSUES |
| Component/s: | Frontend (F) |
| Affects Version/s: | 3.4.12, 4.0.0alpha9 |
| Fix Version/s: | 3.0.24rc1, 3.4.15rc1, 4.0.2rc1, 4.2.0alpha1, 4.2 (plan) |
| Type: | Incident report | Priority: | Minor |
| Reporter: | Fernando Schmitt | Assignee: | Ivo Kurzemnieks |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | cookie, encoding, frontend, javascript | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Environment: |
all |
||
| Attachments: |
chrome.zabbix.cookies.png
js_class.cookie.patch
|
| Team: | Team B |
| Sprint: | Sprint 41, Sprint 42, Sprint 43, Sprint 44, Sprint 45, Sprint 46, Nov 2018 |
| Story Points: | 0.5 |
| Description |
|
File js/class.cookie.js does not encode cookies' name or value. As a result, some cookies such as 'cb_items_10114_1' are set with values containing invalid characters such as commas: "31757,31827,31828". Accessing Zabbix directly, via NGINX + PHP, there is no problem. Some federation services, such as Microsoft ADFS, however, do check cookie compliance, and return a HTTP 500 error instead of allowing Zabbix frontend to load. There is a simple fix to that, only file js/class.cookie.js is affected. I've attached a patch as an exemple. Tested and working (see screenshot with cookies properly encoded). |
| Comments |
| Comment by dimir [ 2018 Aug 23 ] |
|
Nice catch and thank you for the patch! |
| Comment by Ivo Kurzemnieks [ 2018 Aug 30 ] |
|
RESOLVED in svn://svn.zabbix.com/branches/dev/ZBX-14758 |
| Comment by Fernando Schmitt [ 2018 Sep 07 ] |
|
I think I know why it failed the test. In line 109 of the code in SVN for this issue you have: else if (document.cookie.indexOf(name) != -1) { But you should have: else if (document.cookie.indexOf(encodeURIComponent(name)) != -1) { |
| Comment by Valdis Murzins [ 2018 Sep 07 ] |
|
Exactly. |
| Comment by Fernando Schmitt [ 2018 Sep 07 ] |
|
Line 59: var list = encodeURIComponent(value.join(',')); Line 74: tmp_index = list_part.lastIndexOf('%'); Lines 76-77 (move the '%' and whatever follows it to the next part): list = list_part.substring(tmp_index) + list; Line 80: result = this.create(name + '_' + part, decodeURIComponent(list_part), days); Function this.Create will then reencode the value... I know it's not the best solution |
| Comment by Valdis Murzins [ 2018 Oct 12 ] |
|
It was also decided to change separator from "," to "." when saving list of IDs. |
| Comment by Valdis Murzins [ 2018 Nov 05 ] |
|
Fixed in:
|
But there is also a second one: in cookie.createArray() part length is counted before encoding, resulting in too long cookie value.
I missed that one... If you ignore that the function enforces slicing the value string at commas (which is not required used by cookie.readArray), then you can fix like this (untested):