Base64-encoded, IP addresses and other values are parsed as an array in Dev Tools > Storage Inspector

RESOLVED FIXED in Firefox 67

Status

defect
P5
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: sum1abi, Assigned: miker)

Tracking

54 Branch
Firefox 67
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [todo-mr][t1])

Attachments

(3 attachments)

Reporter

Description

2 years ago
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170330203839

Steps to reproduce:

Steps to reproduce:
1 - Go to https://mozilla.org
2 - In the dev console, run `document.cookie='test=aGVsbG93b3JsZA==';`
3 - Open the Storage Inspector tab (you may have to enable the tab in the Dev Tools preferences)
4 - Notice that the 'Parsed Value' tree displays the value as an array with 3 elements
--> It'd be nice if the Base64-encoded value were treated as a string instead

Version: Firefox Developer Edition, 54.0a2 (2017-04-16) (64-bit)
Reporter

Updated

2 years ago
Component: Untriaged → Developer Tools: Storage Inspector
Here we are splitting the "=" assuming a name, value pair and getting it wrong.

We should fix this and add some extra smarts to our code... one obvious thing is encoding e.g. MD5, Base64 etc.
Whiteboard: [todo-mr]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Filter on HOTFROG.
Whiteboard: [todo-mr] → [todo-mr][t1]
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Has STR: --- → yes
Chriso's validator.js is great for this kind of thing and is under the MIT license:
https://github.com/chriso/validator.js

We don't need the whole validator, just the validation functions that we need.

We can at least check for the following and display them as appropriate:
  - Base64
  - dataURI
  - email
  - FQDN
  - IP
  - ISBN
  - ISIN
  - ISSN
  - Mac Address
  - MD5 & SHA1 (SHA1 has 40 chars and MD5 32)
  - Mobile Phone
  - URL
  - UUID
  - Credit card number

We should also include a few changes from pull requests e.g. adding Mobile number formats from Hong Kong.

These are things covered by the validator that we display as an object when they should be displayed as strings.

Objects that do not fit these criteria can still be displayed as a smartly split object.
The rest of the validation methods could be useful so I will leave them in the library.
To see how badly this bug affects the side panel take a look at some images from when the value is set to any of the affected types.
The following values can be used for testing:

"foo@bar.co.uk",
"www.google.co.uk",
"01:AB:03:04:05:06",
"192.168.1.1",
"xn--froschgrn-x9a.co.uk",
"1.0.4",
"#ff0034",
"9-1",
"10,123",
"true",
"2009-05-19 14:39:22-01",
"data:,Hello World!",
Everything else that uses the following separators should be displayed in a tree:
["=", ":", "~", "#", "&", "*", ",", "."]
Summary: Base64-encoded cookie value is parsed as an array in Dev Tools > Storage Inspector → Base64-encoded, IP addresses and other values are parsed as an array in Dev Tools > Storage Inspector
Comment on attachment 8876321 [details]
Bug 1356943 - Only show a tree in the storage sidebar when it is useful

https://reviewboard.mozilla.org/r/147732/#review153434

::: devtools/client/storage/ui.js:216
(Diff revision 1)
>  StorageUI.prototype = {
>  
>    storageTypes: null,
>    sidebarToggledOpen: null,
>    shouldLoadMoreItems: true,
> +  sidebarParseTreeHidden: true,

It seems that this new property is only used in tests. In this file it is only set, never read.
I feel a bit uncomfortable with adding test-only code to production code.
Also, checking the value of this property from the test doesn't assert that the tree is indeed visible or not in the sidebar. Couldn't the test check the tree component itself? Does it have a state we can check? Or some DOM objects?
Attachment #8876321 - Flags: review?(pbrosset)
Comment hidden (mozreview-request)
Comment on attachment 8876321 [details]
Bug 1356943 - Only show a tree in the storage sidebar when it is useful

https://reviewboard.mozilla.org/r/147732/#review154972
Attachment #8876321 - Flags: review?(pbrosset) → review+

Comment 12

2 years ago
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9647f9c56e13
Only show a tree in the storage sidebar when it is useful r=pbro
Backed out for failing browser_storage_dynamic_updates_cookies.js, browser_storage_sidebar_update.js and browser_storage_values.js:

https://hg.mozilla.org/integration/autoland/rev/8b9b0a91608ef9d11400bb1b20ae4dd06f34f22b

Push with failures (later push has them on Linux): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9647f9c56e137e114af6de86a5cf9383611975fc&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=108917543&repo=autoland

09:53:41     INFO - TEST-START | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js
09:53:41     INFO - TEST-INFO | started process screenshot
09:53:42     INFO - TEST-INFO | screenshot: exit 0
09:53:42     INFO - Buffered messages logged at 09:53:41
09:53:42     INFO - Entering test bound 
09:53:42     INFO - Adding a new tab with URL: http://test1.example.org/browser/devtools/client/storage/test/storage-updates.html
09:53:42     INFO - Tab added and finished loading
09:53:42     INFO - Found a window: complete
09:53:42     INFO - Opening the storage inspector
09:53:42     INFO - Opening the toolbox
09:53:42     INFO - Console message: [JavaScript Warning: "XUL box for vbox element contained an inline label child, forcing all its children to be wrapped in a block." {file: "chrome://devtools/content/storage/storage.xul" line: 0}]
09:53:42     INFO - Waiting for the stores to update
09:53:42     INFO - Making sure that the toolbox's frame is focused
09:53:42     INFO - TEST-PASS | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js | Sidebar is initially hidden - 
09:53:42     INFO - TEST-PASS | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js | row found with id "c1{9d414cc5-8319-0a04-0586-c0a6ae01670a}test1.example.org{9d414cc5-8319-0a04-0586-c0a6ae01670a}/browser" - 
09:53:42     INFO - selecting row "c1{9d414cc5-8319-0a04-0586-c0a6ae01670a}test1.example.org{9d414cc5-8319-0a04-0586-c0a6ae01670a}/browser"
09:53:42     INFO - TEST-PASS | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js | found property c1 - 
09:53:42     INFO - TEST-PASS | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js | rule c1 matched for property c1 - 
09:53:42     INFO - TEST-PASS | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js | rule c1.Path matched for property Path - 
09:53:42     INFO - Buffered messages finished
09:53:42     INFO - TEST-UNEXPECTED-FAIL | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js | rule .c1 did not match any property - 
09:53:42     INFO - Stack trace:
09:53:42     INFO - chrome://mochitests/content/browser/devtools/client/storage/test/head.js:onAllRulesMatched:450
09:53:42     INFO - resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:process:922
09:53:42     INFO - resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:walkerLoop:806
09:53:42     INFO - Not taking screenshot here: see the one that was previously logged
Flags: needinfo?(mratcliffe)

Updated

Last year
Product: Firefox → DevTools
Flags: needinfo?(mratcliffe)
OS: Unspecified → All
Priority: P2 → P5
Hardware: Unspecified → All
Attachment #9041225 - Attachment description: Bug 1356943 - Only show a tree in the storage sidebar when it is useful → Bug 1356943 - Only show a tree in the storage sidebar when it is useful r?pbro

Comment 16

5 months ago
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e17a2d88817
Only show a tree in the storage sidebar when it is useful r=pbro

Comment 17

5 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.