Closed
Bug 1377668
Opened 7 years ago
Closed 7 years ago
JSON object with "type": "string" collapses to “Invalid object”
Categories
(DevTools :: JSON Viewer, defect, P3)
Tracking
(firefox57 fixed)
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: lucas.werkmeister, Assigned: Honza)
References
Details
Attachments
(2 files)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170629075044
Steps to reproduce:
Open a JSON file with the following content (e. g. locally, or https://lucaswerkmeister.de/x-type-string.json):
{ "x": { "type": "string" } }
Collapse the “x” member.
Alternatively, open https://www.wikidata.org/wiki/Special:EntityData/Q42.json and expand .entities.Q42.claims.P1417[0].mainsnak, and observe the collapsed datavalue member.
Actual results:
The collapsed value is summarized as “Invalid object”; the title attribute instructs the user to file a bug on bugzilla.mozilla.org.
In the Browser Console, the error message “<unavailable>” (without quotes, with angle brackets) is logged, with the location information reps.js:496 (pointing to resource://devtools/client/shared/components/reps/reps.js).
Expected results:
The collapsed value should be summarized as “Object”.
Similarly, a value with { "type": "undefined" } collapses to “undefined” (without quotes), and a value with { "type": "number" } collapses to “[Object object]” (without quotes, with brackets).
It looks like the JSON file can trick the JSON viewer into using a different renderer for the value, which sounds like it *might* be exploitable, so I’m marking this as “Security” (but feel free to make the bug public if you disagree).
Comment 1•7 years ago
|
||
Honza, one for you?
(In reply to Lucas Werkmeister from comment #0)
> It looks like the JSON file can trick the JSON viewer into using a different
> renderer for the value
(FWIW, I am not sure if this is actually true, Honza would know better)
> , which sounds like it *might* be exploitable, so I’m
> marking this as “Security” (but feel free to make the bug public if you
> disagree).
This would indeed be a serious problem if the document could communicate with anything else (ie you could run code outside of the context where it's meant to run, unlike running JS in normal webpages), but the JSON viewer document should be isolated (ie doesn't have any specific privileges at all besides communicating with the parent process for the 'copy' and 'save' functionality, which you could do in a normal webpage, too). You can verify this by running JS in the JSON viewer document with e.g. the web console (without even trying to exploit the former). Based on this, I think this can be unhidden, but I'll let Honza make the call here.
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: JSON Viewer
Ever confirmed: true
Flags: needinfo?(odvarko)
Assignee | ||
Comment 2•7 years ago
|
||
Thanks for the report!
The problem is related to `Reps` (DevTools rendering engine) and so, I reported an issue on github.
https://github.com/devtools-html/devtools-core/issues/480
This is not security issue and the flag can be removed.
@Gijs: Can you please unhide this report?
Thanks!
Honza
Flags: needinfo?(odvarko) → needinfo?(gijskruitbosch+bugs)
Priority: -- → P3
Updated•7 years ago
|
Group: firefox-core-security
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
Depends on https://github.com/devtools-html/devtools-core/pull/483
(need to wait till the new Reps version lands in m-c)
Honza
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8883927 [details]
Bug 1377668 - Avoid invalid object by using noGrip option;
https://reviewboard.mozilla.org/r/154906/#review159882
This is a small change and I don't see what could be wrong here :)
I know that the reps bundle didn't landed yet into m-c, but would it be possible to add a test to make sure the test case won't be a problem anymore ?
For now the test would fail, and when we land the reps bundle, after rebasing the test would be green.
This is not mandatory since there are tests on the Reps side, but it would prevent us some regressions in case someone remove/change the prop in Reps.
What do you think about it Honza ?
Attachment #8883927 -
Flags: review?(nchevobbe) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
Good point, done!
Honza
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8883927 [details]
Bug 1377668 - Avoid invalid object by using noGrip option;
https://reviewboard.mozilla.org/r/154906/#review159920
::: devtools/client/jsonview/test/browser_jsonview_object-type.js:17
(Diff revision 2)
> + yield addJsonViewTab(TEST_JSON_URL);
> +
> + let count = yield getElementCount(".jsonPanelBox .treeTable .treeRow");
> + is(count, 2, "There must be two rows");
> +
> + // Collapsed auto-expanded node.
nit: s/collapsed/collapse
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
Does this fix bug 1373102 too?
Comment 12•7 years ago
|
||
(In reply to Oriol [:Oriol] from comment #11)
> Does this fix bug 1373102 too?
Yes, it should
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #12)
> (In reply to Oriol [:Oriol] from comment #11)
> > Does this fix bug 1373102 too?
>
> Yes, it should
Yes, it'll, I've tested this.
Honza
Comment 15•7 years ago
|
||
Why not land this?
Flags: needinfo?(odvarko)
Assignee | ||
Comment 16•7 years ago
|
||
Ah, forgotten patch.
There is another Reps PR needed for this issue.
https://github.com/devtools-html/devtools-core/pull/663
(I am keeping the NI flag open for now)
Honza
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(odvarko)
Assignee | ||
Updated•7 years ago
|
status-firefox57:
--- → fix-optional
Flags: needinfo?(odvarko)
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52fa1b99d27c
Avoid invalid object by using noGrip option; r=nchevobbe
Comment 19•7 years ago
|
||
Backed out on request from Honza: https://hg.mozilla.org/integration/autoland/rev/b10415064b6626fc1fddceaa2e847f958d10e287
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
Try looks good now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc8fca6b120cbf41624c90d10651c32dc7800a79
Honza
Comment 22•7 years ago
|
||
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff7d6888f249
Avoid invalid object by using noGrip option; r=nchevobbe
Comment 23•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 24•7 years ago
|
||
Seeing a similar behaviour on the Web Console (Network Tab); the object does render (so it's not a major cause for alarm) but the tree displays an "Invalid Object" tag which, when hovered upon, shows a tooltip saying "This object could not be rendered, please file a bug on bugzilla.mozilla.org" (which is what led me here :) )
I have captured a screenshot using a public Google API Discovery Document URL (https://content.googleapis.com/discovery/v1/apis/storage/v1/rest?fields=kind%2Cname%2Cversion%2CrootUrl%2CservicePath%2Cresources%2Cparameters%2Cmethods%2CbatchPath%2Cid&pp=0), opened directly in a browser tab. Note that both JSON viewers (in-tab and Web Console) display the JSON tree correctly, but the in-tab one does not display the "Invalid Object" tag (e.g. for the `/parameters/alt` node).
(Cannot seem to find a way to attach the screenshot right here; will try to attach it as the next comment)
I'm on Nightly 61.0a1 (slightly old, "61.0a1 (2018-04-13) (64-bit)"; I can try upgrading within the next few days, but I'm fairly sure that won't make a difference :) )
Comment 25•7 years ago
|
||
Screenshot (Web Console vs JSON Viewer) as mentioned in comment #24
Updated•7 years ago
|
Comment 26•7 years ago
|
||
Yes, as said in https://github.com/devtools-html/devtools-core/pull/483#issuecomment-314118338, the netmonitor needs noGrip:true too, filed bug 1457701. And in fact reps should fail fast and loud if this parameter is not provided instead of seemingly work in most cases but absolutely fail in edge ones.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•