1297361 - (CVE-2017-5390) JSON Viewer: use listeners instead of exporting postChromeMessage

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Follow up for:
https://bugzilla.mozilla.org/show_bug.cgi?id=1295309#c26

JSON viewer uses `Cu.exportFunction` to export `postChromeMessage` method into content scope to create a communication channel (used for, copying & saving JSON data and HTTP headers from the content).

This should be replaced by window listeners.

Honza

Comment 1

[Jerri Rice (rehash pending)]

Can we maybe go ahead a throw a sec-bounty? flag on here for the powers that be?

Comment 2

[Andrew McCreight [:mccr8]]

Jan, can you work on fixing this, or should I find somebody else? It is almost two months old. Thanks.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> Jan, can you work on fixing this, or should I find somebody else? It is
> almost two months old. Thanks.

I've got a lot on my plate these days so, if you can find someone that would be great!

Honza

Comment 4

[Andrew McCreight [:mccr8]]

Joe, do you know who else might be able to work on this sec-high issue? I'm not sure who is familiar with this code. Thanks.

Comment 5

[Joe Walker [:jwalker]]

Dan - is Honza's description of what we should be doing about this in Comment 0 correct and sufficient?

Comment 6

[Daniel Veditz [:dveditz]]

That seems correct.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Daniel Veditz [:dveditz] from comment #6)
> That seems correct.

Thanks Daniel, I'll look at this.

Honza

Comment 8

[Jerri Rice (rehash pending)]

Any movement here?  I'm going to throw something together and see how lacking it is.

Comment 9

[Jerri Rice (rehash pending)]

Ok I've identified the key points to replace, what would in fact be the proper way to pass objects through event listeners, or does someone else have an idea here?  I'll leave it this for now, with the rest of the work relating to this in place I'm somewhat hesitant to change anything yet.

Comment 10

[Jerri Rice (rehash pending)]

I guess I'll try to implement JSON.stringify for passing objects.  I'll contact you on IRC Honza, just to see if you have anything already going in this direction.

I did test the other defense in depth patches regarding this and they seem solid as of right now.  Let's hope no regressions break things =)

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks Jerri!

Honza

Comment 12

[Jerri Rice (rehash pending)]

I haven't gotten to this yet, having some problems with hg and mc cloning for some reason.  I'll grab a bundle and get to it soon though.

Comment 13

[:Gijs (he/him)]

FWIW, it should be possible to use a custom event (new CustomEvent("myEvent", {detail: obj}) ), I think.

Comment 14

[Jerri Rice (rehash pending)]

Attachment: Untested patch

Here is what I had so far, currently waiting to get another system configured to do the test build there.

Can you guys look this over / pick this up from here?

Thanks

Comment 15

[:Gijs (he/him)]

Do we think we need the JSON-ification for safety? Otherwise, I would have thought we could just pass the object directly on the event's detail property - is there some reason that doesn't work?

Comment 16

[Jerri Rice (rehash pending)]

I guess it would, I discussed this with Honza and now that I'm thinking about it that is probably the better solution.  I was stuck on how to have JSON parse the received object back without security issues anyway.

I'm still trying to get a source building setup right, any chance you can give that a try Gijs?

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8812986 [details] [diff] [review]
Untested patch

Review of attachment 8812986 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Jerri for working on this! I believe that this is going the right direction.

I know this is just a testing patch, but I put made some comments inline...

And, yeah, agree with Gijs, passing direct object would be better if safe.

Honza

::: devtools/client/jsonview/converter-child.js
@@ +140,4 @@
>      JsonViewUtils.exportIntoContentScope(win, Locale, "Locale");
>  
>      Events.once(win, "DOMContentLoaded", event => {
> +      win.addEventListener("contentMessage", getContentMessage, false, true);

nit: What about renaming getContentMessage to onContentMessage?

getContentMessage => this.getContentMessage.bind(this) or any other way to properly deal with |this| inside getContentMessage method.

@@ +144,2 @@
>        });
>      });

Remove extra pair of brackets.

@@ +262,5 @@
>  
>    // Chrome <-> Content communication
>  
> +  getContentMessage:  function(e) {
> +    if (typeof e.detail.val != 'string') {

Nit: what about renaming 'val' field to (a bit more clear) 'value'?

::: devtools/client/jsonview/json-viewer.js
@@ +43,5 @@
>    input.actions = {
>      onCopyJson: function () {
>        let value = input.prettified ? input.jsonPretty : input.jsonText;
> +      value = JSON.stringify(value);
> +      let contentMessageEvent = new CustomEvent('contentMessage', [detail:{type:"copy",val:value}]);

The second argument is supposed to be an object with field 'detail', right?
Not an array...

@@ +56,4 @@
>      },
>  
>      onCopyHeaders: function () {
> +      let value = JSON.stringify(value);

You should use `input.headers` to get the headers.

@@ +56,5 @@
>      },
>  
>      onCopyHeaders: function () {
> +      let value = JSON.stringify(value);
> +      let contentMessageEvent = new CustomEvent('contentMessage', [detail:{type:"copy-headers,val:value}]);

missing quotation mark.


Also, the source line is too long.

Comment 18

[Jerri Rice (rehash pending)]

Attachment: jsonview-WIP-2

Something like this Honza?

Comment 19

[Jerri Rice (rehash pending)]

Alright locally this looks broken but I'll wait on someone else.  I have the source building now, but I believe not properly setup.

*stares down local javascript files*

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8813274 [details] [diff] [review]
jsonview-WIP-2

Review of attachment 8813274 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jerri Rice (rehash pending) from comment #19)
> Alright locally this looks broken but I'll wait on someone else.  I have the
> source building now, but I believe not properly setup.
Check out my inline comments to see why it's broken.

This patch seems to be almost ready.
As soon as you resolve my comments we can ask for sec review.

Thanks for working on this!

Honza

::: devtools/client/jsonview/converter-child.js
@@ +140,4 @@
>      JsonViewUtils.exportIntoContentScope(win, Locale, "Locale");
>  
>      Events.once(win, "DOMContentLoaded", event => {
> +      win.addEventListener("contentMessage", this.onContentMessage.bind(this), false, true);

The line is too long. Maximum length of a line is 90 characters.

@@ +261,5 @@
>  
>    // Chrome <-> Content communication
>  
> +  onContentMessage:  function(e) {
> +   

Remove an empty line

::: devtools/client/jsonview/json-viewer.js
@@ +43,5 @@
>    input.actions = {
>      onCopyJson: function () {
>        let value = input.prettified ? input.jsonPretty : input.jsonText;
> +      let contentMessageEvent = new CustomEvent('contentMessage', {detail:{type:"copy",value:value}});
> +      window.dispatchEvent(contentMessageEvent);

The line is too long. Maximum length of a line is 90 characters.

Also use proper spacing:

new CustomEvent('contentMessage', {detail: {type: "copy", value: value}});

Or you can do:

let data = {
  detail: {
    type: "copy",
    value: value
  }
}

new CustomEvent('contentMessage', data);

@@ +48,5 @@
>      },
>  
>      onSaveJson: function () {
>        let value = input.prettified ? input.jsonPretty : input.jsonText;
> +      let contentMessageEvent = new CustomEvent('contentMessage', {detail:{type:"save",value:value}});

The line is too long. Maximum length of a line is 90 characters.

@@ +54,5 @@
>      },
>  
>      onCopyHeaders: function () {
> +      let value = input.headers;
> +      let contentMessageEvent = new CustomEvent('contentMessage', \

Remove the '\' characters at the end of the line.

This is breaking the JSON view entirely.

Comment 21

[Jerri Rice (rehash pending)]

Attachment: json-viewer-3.patch

Here try this.

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8813786 [details] [diff] [review]
json-viewer-3.patch

Review of attachment 8813786 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my inline comments

Note that some review comments from my previous review were not resolved.

Also, please set properly the review flag for me so, I can see it in my review queue
(and it isn't forgotten)

Thanks!

Honza

::: devtools/client/jsonview/converter-child.js
@@ +140,4 @@
>      JsonViewUtils.exportIntoContentScope(win, Locale, "Locale");
>  
>      Events.once(win, "DOMContentLoaded", event => {
> +      win.addEventListener("contentMessage", this.onContentMessage.bind(this), false, true);

This line is too long (over 90 chars) please split.

@@ +261,5 @@
>  
>    // Chrome <-> Content communication
>  
> +  onContentMessage:  function(e) {
> +   

Please remove this empty line

::: devtools/client/jsonview/json-viewer.js
@@ +43,4 @@
>    input.actions = {
>      onCopyJson: function () {
>        let value = input.prettified ? input.jsonPretty : input.jsonText;
> +      let contentMessageEvent = new CustomEvent('contentMessage', {detail:{type:"copy",value:value}});

This line is too long, please split.

@@ +48,5 @@
>      },
>  
>      onSaveJson: function () {
>        let value = input.prettified ? input.jsonPretty : input.jsonText;
> +      let contentMessageEvent = new CustomEvent('contentMessage', {detail:{type:"save",value:value}});

This line is too long, please split.

Also, see my previous review comment about spacing.

@@ +54,5 @@
>      },
>  
>      onCopyHeaders: function () {
> +      let value = input.headers;
> +      let contentMessageEvent = new CustomEvent('contentMessage', 

Remove empty space at the end of the line

Comment 23

[Jerri Rice (rehash pending)]

Attachment: json-view-4.patch

Here, I think that's it.  I tested the functionality a few minutes ago.  With those small changes of spacing I think it's ready.

Thanks Honza

Comment 24

[Jerri Rice (rehash pending)]

After this patch, running debug mode when the json-viewer content renders on screen the console window lights up with this error.

[Child 3176] ###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file c:
/Users/[redacted]/mozilla-central/dom/events/EventDispatcher.cpp, line 673

I'm going to file a follow up bug and leave the number here so we can discuss if this is actually a security issue or if the assertion is misleading considering the security measures here.

Maybe the event listener needs to check the event source, but I feel that too can be done in follow up bug.  Will post the follow up bug here in a second and leave the bug number here and we can go from there.

Comment 25

[Jerri Rice (rehash pending)]

That bug to follow up bug is bug 1320228.  I know it's Thanksgiving, but I haven't had much of a chance to just get work done recently so I actually took tonight to touch on things some.  You guys enjoy the holiday time though :)

Comment 26

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8814197 [details] [diff] [review]
json-view-4.patch

Review of attachment 8814197 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good to me, there are just some
minor coding-style issues that should be fixed
before r+.

Thanks,
Honza

::: devtools/client/jsonview/converter-child.js
@@ +140,5 @@
>      JsonViewUtils.exportIntoContentScope(win, Locale, "Locale");
>  
>      Events.once(win, "DOMContentLoaded", event => {
> +      win.addEventListener("contentMessage",
> +      this.onContentMessage.bind(this), false, true);

Please use indentation as follows:

Events.once(win, "DOMContentLoaded", event => {
  win.addEventListener("contentMessage",
    this.onContentMessage.bind(this), false, true);
});


You might also see our code-style guidelines:
https://wiki.mozilla.org/DevTools/CodingStandards

::: devtools/client/jsonview/json-viewer.js
@@ +44,5 @@
>      onCopyJson: function () {
>        let value = input.prettified ? input.jsonPretty : input.jsonText;
> +      let contentMessageEvent = new CustomEvent('contentMessage',
> +        {detail:{type:"copy",value:value}});
> +      window.dispatchEvent(contentMessageEvent);

The spacing is still not resolved, can you please do it?
(see my previous review comments)

Honza

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39c4622c74c4b0351850778aba2f8a51e43b5d5f

Honza

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Jerri, can you please CC me on the bug 1320228? I don't have an access to it.
Thanks
Honza

Comment 29

[Jerri Rice (rehash pending)]

Done very strange since I was sure I added you at first.  Also I used to have the spacing guidelines down, but I usually don't contribute code here, just break it :)

Comment 30

[Jerri Rice (rehash pending)]

Jan, could you possibly fix the spacing problems?  I'm not going to have the bandwidth or the system it would take to do it properly since I'm kind of with family for right now.  I also don't think this should be held back any longer due to some standards like ISO coding standards holding it back.

Thanks I hope, if not I can do it soon.  The functionality is there though :-)

Comment 31

[Jerri Rice (rehash pending)]

I'm on very limited data right now, but if I have to I'll get the proper text editors and such setup.  I've been doing this with the rawest of tools so far =/

Comment 32

[Jerri Rice (rehash pending)]

Attachment: jsonview-proper.patch

I didn't go back over the guidelines, but IIRC something like this is more what you're wanting right.

Sorry, I tend to be a little rough around the edges, but I think this should serve you well for review.

Comment 33

[Jerri Rice (rehash pending)]

Attachment: jsonview-proper-2.patch

Here, I'm working from two bases for this patch and that one still had an unneeded blank line.

I also have an alternate version of this patch with spacing the only other way I think it could make sense if this one isn't right.  Just let me know, killing my data setting these tools up though =/

Comment 34

[Jerri Rice (rehash pending)]

Attachment: jsonview-5.patch

Here I believe the spacing is pretty spot on.  I'm a little low on data now though and kind of in the middle of nowhere lol.

Thank for looking this over Honza.  I think it takes care of all of our concerns about exporting postChromeMessage.

Happy Holidays and cheers all.

Comment 35

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: jsonview-6.patch

Patch updated, spacing and Eslint warnings fixed.

Gijs, can you please review this, thanks!

Honza

Comment 36

[:Gijs (he/him)]

Comment on attachment 8815683 [details] [diff] [review]
jsonview-6.patch

Review of attachment 8815683 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/jsonview/converter-child.js
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const {Cc, Ci, components} = require("chrome");

Personally, I'd leave the "Cu" in, but if eslint complains I guess we can do this...

@@ +261,5 @@
>    },
>  
>    // Chrome <-> Content communication
>  
> +  onContentMessage: function (e) {

Should we verify that we're still on a JSON viewer document somehow, to be very safe? Note also e.g. bug 1044586 comment 13 and further. We should be very sure that if you e.g.:

myWin = window.open("some json", "_blank")
myWin.location.href = "my evil code that fires these custom events so that the converter child will then do stuff with chrome"

wouldn't work.

::: devtools/client/jsonview/json-viewer.js
@@ +70,5 @@
> +          type: "copy-headers",
> +          value: value
> +        }
> +      });
> +      window.dispatchEvent(contentMessageEvent);

We're duplicating this 3 times - we could create a helper to make this cleaner?

Either in the helper or all 3 times, please leave a trailing comma after the value prop, and just use the ES6 shorthand instead of duplicating "value: value"

Comment 37

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1297361.patch

(In reply to :Gijs Kruitbosch from comment #36)
> Comment on attachment 8815683 [details] [diff] [review]
> jsonview-6.patch
> 
> Review of attachment 8815683 [details] [diff] [review]:
> Personally, I'd leave the "Cu" in, but if eslint complains I guess we can do
> this...
Yeah, eslint was complaining.

> 
> @@ +261,5 @@
> >    },
> >  
> >    // Chrome <-> Content communication
> >  
> > +  onContentMessage: function (e) {
> 
> Should we verify that we're still on a JSON viewer document somehow, to be
> very safe? Note also e.g. bug 1044586 comment 13 and further. We should be
> very sure that if you e.g.:
Done

> We're duplicating this 3 times - we could create a helper to make this
> cleaner?
Done

> Either in the helper or all 3 times, please leave a trailing comma after the
> value prop, and just use the ES6 shorthand instead of duplicating "value:
> value"
Fixed


Thanks!

Honza

Comment 38

[:Gijs (he/him)]

Comment on attachment 8815695 [details] [diff] [review]
bug1297361.patch

Review of attachment 8815695 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/jsonview/json-viewer.js
@@ +54,5 @@
> +      dispatchEvent({
> +        detail: {
> +          type: "save",
> +          value: input.prettified ? input.jsonPretty : input.jsonText,
> +        }

I'd push this into the helper, passing the helper values for "type" and "value", and then just calling it with 2 args to make 'type' and 'value' happen.

Comment 39

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1297361.patch

(In reply to :Gijs Kruitbosch from comment #38)
> Comment on attachment 8815695 [details] [diff] [review]
> bug1297361.patch
> 
> Review of attachment 8815695 [details] [diff] [review]:
> -----------------------------------------------------------------
> I'd push this into the helper, passing the helper values for "type" and
> "value", and then just calling it with 2 args to make 'type' and 'value'
> happen.
Done

Thanks for the review!
Reusing R+ 

Honza

Comment 40

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8815779 [details] [diff] [review]
bug1297361.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
This is hard to estimate, but this comment can help:
https://bugzilla.mozilla.org/show_bug.cgi?id=1295309#c24

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No

Which older supported branches are affected by this flaw?
Target Milestone for JSON viewer (introducing the original implementation) was Firefox 44. I am not sure about affected branches.

If not all supported branches, which bug introduced the flaw?
The JSON Viewer has been introduced in Bug 1132203

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
They are no backports. They could be probably the same and representing low risk.

How likely is this patch to cause regressions; how much testing does it need?
Low risk. All affected JSON Viewer features have been manually tested and they are also covered by automated tests.

Honza

Comment 41

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8815779 [details] [diff] [review]
bug1297361.patch

sec-approval+ for trunk.

Once it is on trunk, please nominate branch patches for Aurora, Beta, and ESR45 as well so we can fix it in affected branches.

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbbe3c47bd0b

Comment 43

[Carsten Book [:Tomcat]]

backed out for es lint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=40306741&repo=mozilla-inbound

Comment 44

[Jerri Rice (rehash pending)]

I'll try to fix these failures, getting a nice text editor with an es lint plugin setup in windows right now.  Don't let me step on your toes here though Honza.

Will be in touch, although I'm not sure I have try server access ;-)

Comment 45

[Jerri Rice (rehash pending)]

Attachment: bug1297361-2.patch

Here is a patch to apply after the above patch which should take care of es lint warnings.  At least I hope, tried to redo the whole thing only to realize the previous patch has landed =/

Hope this helps guys, sorry if this makes it more of a mess.

Comment 46

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0d7431e24546049ee2bd0fb1e05a5b124ed01f81

Comment 47

[Jerri Rice (rehash pending)]

It looks as though the es lint problems were solved, but for right now I do see one mochi devtools chrome test failing.

Can someone else take a look and see what they think here?  hmm

Comment 48

[:Gijs (he/him)]

(In reply to Jerri Rice (rehash pending) from comment #47)
> It looks as though the es lint problems were solved, but for right now I do
> see one mochi devtools chrome test failing.
> 
> Can someone else take a look and see what they think here?  hmm

The retriggers were green and the test seems unrelated, so I don't expect it's an issue to do with this patch.

Comment 49

[Jerri Rice (rehash pending)]

That sounds good to me, Honza can you verify we're good here and mark resolved?  Thanks.

Comment 50

[:Gijs (he/him)]

(In reply to Jerri Rice (rehash pending) from comment #49)
> That sounds good to me, Honza can you verify we're good here and mark
> resolved?  Thanks.

Bugs are only marked resolved when they get merged to m-c.

Comment 51

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1297361.patch

The original patch updated (including changes from Jerri - fixing eslint warnings).

Let's try to land again.

Thanks Jerri & Gijs!

Honza

Comment 52

[:Gijs (he/him)]

This landed already (comment 46).

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d7431e24546
https://hg.mozilla.org/mozilla-central/rev/26025c7f0d29

this should have needed sec-approval right ?

Comment 54

[Jerri Rice (rehash pending)]

I can't speak for Honza and I know that there is a protocol, but nothing in the patch was modified in any real way.

A comment was removed, and a space before a parentheses was removed.

I'll let him follow up.

Comment 55

[Carsten Book [:Tomcat]]

ah right seems just the flag was lost :)

Comment 56

[:Gijs (he/him)]

Leaving the ni for Honza to file the uplift requests.

Comment 57

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

Re-requesting sec-approval (already approved in comment #41).

@abillings: the patch has been updated to fix eslint warnings. A comment was removed, and a space before a parentheses was removed.

Honza

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
This is hard to estimate, but this comment can help:
https://bugzilla.mozilla.org/show_bug.cgi?id=1295309#c24

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No

Which older supported branches are affected by this flaw?
Target Milestone for JSON viewer (introducing the original implementation) was Firefox 44. I am not sure about affected branches.

If not all supported branches, which bug introduced the flaw?
The JSON Viewer has been introduced in Bug 1132203

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
They are no backports. They could be probably the same and representing low risk.

How likely is this patch to cause regressions; how much testing does it need?
Low risk. All affected JSON Viewer features have been manually tested and they are also covered by automated tests.

Honza

Comment 58

[:Gijs (he/him)]

Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

This doesn't need new sec approval - it already landed and made it to nightly. It needs uplift requests for aurora and beta. Honza, can you fill those out?

Comment 59

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1132203
[User impact if declined]: No UI impact
[Is this code covered by automated tests? Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: Very low risk
[Why is the change risky/not risky?]: Relate UI features covered by tests 
[String changes made/needed]: n/a

Comment 60

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1132203
[User impact if declined]: No UI impact
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: Very low risk
[Why is the change risky/not risky?]: Related UI features covered by tests 
[String changes made/needed]: n/a

Comment 61

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: No UI changes
Fix Landed on Version: Nightly
Risk to taking this patch (and alternatives if risky): Very low risk
String or UUID changes made by this patch: n/a

Comment 62

[Liz Henry (:lizzard) (relman/hg->git project)]

Can this wait for 51 and 45.7.0esr in January?

Comment 63

[Liz Henry (:lizzard) (relman/hg->git project)]

I think this should wait. I'd like to start the ESR build tonight.

Comment 64

[:Gijs (he/him)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #62)
> Can this wait for 51 and 45.7.0esr in January?

Yes.

Comment 65

[Gerry Chang [:gchang]]

Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

Fix a sec-high. Beta51+ and Aurora52+. Should be in 51 beta 7.

Comment 66

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3a761e5cc19c38782cfc0da0bba68954bd06cd27
https://hg.mozilla.org/releases/mozilla-aurora/rev/3a761e5cc19c38782cfc0da0bba68954bd06cd27

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b6dada73f7d8955bc29673e54fc641137a89d3c (ignore the one before, different bug

Comment 68

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/af327d55ca0b

Comment 69

[Jerri Rice (rehash pending)]

I'm glad to see this one about wrapped up.  Bugs setting for multiple months is kind of new, I'll try to start bringing the patch with the work ;-)

Comment 70

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

let's take that in esr too.

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for ESR45.

Comment 72

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1297361-esr45.patch

(In reply to Ryan VanderMeulen [:RyanVM] from comment #71)
> Needs rebasing for ESR45.

Patch for ESR45 attached.

Honza

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/297c675ddadc

Comment 74

[Jerri Rice (rehash pending)]

I believe there may be a bit of a blunder here.  While trying to test for a stacktrace for bug 1330173 I thought wouldn't it be interesting to try to load the json viewer in a tab.

This still works for top level windows, and a point I believe was missed originally was that if you load it into a top level window, that window remains fully accessible from the content window that opened it.

I'll probably make another bug to handle that, but in everything that happened here I did forget to continue to stress that one point.

So if the patches here don't check for trusted events(will look it all over again in a second) the fact that the window is accessible could still be a hole.

Man this is kind of a mess, because I'm on limited time and I keep finding more things to dig into.  I don't want to leave any of them hanging.  Will follow up after dedicated a few hours to bug 1330173.

Comment 75

[Jerri Rice (rehash pending)]

Ok yeah, the patch here is lacking.  If someone with the proper bugzilla rights can strike my previous comment and possibly even this one, this can go ahead and go public and we can handle the window being accessible in another bug.

Sorry guys I slipped a little on this one.  I remember mentioning it in the bug that this bug was spawned from but I forgot to continue to stress that key point.

Now I know how Gijs has felt about some of those regressions.  Sorry man, but when we all work together we can find things like this much easier :-)

Comment 76

[Jerri Rice (rehash pending)]

See bug 1333210 for the follow up from here.