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

ASSIGNED
Assigned to

Status

DevTools
Storage Inspector
P2
normal
ASSIGNED
a year ago
a month ago

People

(Reporter: x22RWEN, Assigned: miker, NeedInfo)

Tracking

54 Branch

Firefox Tracking Flags

(Not tracked)

Details

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

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a year 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

a year 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.
Created attachment 8876110 [details]
Example-parsing-errors.png

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 hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
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 11

a year ago
mozreview-review
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

a year 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

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.