935694 - SecReview: CodeMirror in Devtools

Status: VERIFIED FIXED

(mozilla.org :: Security Assurance: Review Request, task)

Description

[Mark Goodwin [:mgoodwin]]

Who is/are the point of contact(s) for this review?
Anton Kovalyov

Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.):
Replacement of Orion with CodeMirror in devtools

Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description:
See bug 816756

Does this request block another bug? If so, please indicate the bug number:
Yes, bug 816756

This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review?
High - a bunch of this code is already landed

To help prioritize this work request, does this project support a goal specifically listed on this quarter's goal list? If so, which goal?
-

Please answer the following few questions: (Note: If you are asked to describe anything, 1-2 sentences shall suffice.)
Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users?

Yes. Firefox (desktop)

Will your application/service collect user data?
No.

Comment 1

[Jan Keromnes [:janx]]

For what it's worth, Google also did a security review to use CodeMirror in the Chrome devtools about a year ago. The big concern at the time was the use of `innerHTML`, which lead to [this patch](https://github.com/marijnh/CodeMirror/commit/0f44da049a11) by @repenaxa, making CodeMirror use DOM bindings and fragments instead of `innerHTML`.

Comment 2

[Mark Goodwin [:mgoodwin]]

(In reply to Jan Keromnes [:janx] from comment #1)

Thanks, Jan, that's useful information.

Comment 3

[Mark Goodwin [:mgoodwin]]

Reviewed as per: https://hg.mozilla.org/mozilla-central/rev/8db3d638ccd1

Looking at changes subsequent to: https://bugzilla.mozilla.org/show_bug.cgi?id=816756#c42

Comment 4

[Jan Keromnes [:janx]]

Excellent! Mark, do you know if updating CodeMirror to a newer major release will require an additional SecReview? (CodeMirror v4 is in the pipes)

Comment 5

[Mark Goodwin [:mgoodwin]]

(In reply to Jan Keromnes [:janx] from comment #4)
> Excellent! Mark, do you know if updating CodeMirror to a newer major release
> will require an additional SecReview? (CodeMirror v4 is in the pipes)

Just sec-review ? me when there's a bug for that and I'll take a look. Probably doesn't need full review - but I'll let you know if that changes.