2024294 - Lets remove all the usages of dangerouslySetInnerHTML

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Hubert Boma Manilla (:bomsy)]

We have a couple of usages of the dangerouslySetInnerHTML in the debugger . See here https://searchfox.org/firefox-main/search?q=dangerouslySetInnerHTML&path=devtools%2Fclient%2Fdebugger&case=false&regexp=false.

Comment 1

[Tom Schuster [:tschuster]]

Probably not a good idea to point so directly to a security issue.

Comment 2

[Julian Descottes [:jdescottes]]

We quickly reviewed the two remaining call sites for dangerouslySetInnerHTML

1. Breakpoint.js

https://searchfox.org/firefox-main/rev/410a52656dd13aa2360a3e96788e41bf48e80c0f/devtools/client/debugger/src/components/SecondaryPanes/Breakpoints/Breakpoint.js#170

dangerouslySetInnerHTML: this.highlightText(breakpointText, editor),

This is setting html based on a breakpoint text, which we don't control completely. It's not clear if this text has been properly sanitized. We render the breakpoint text with highlighting via CodeMirror 6 and retrieve the innerHTML of the CodeMirror instance. I think it is unlikely to break but we should fix it.

2. DOMMutationBreakpoints.js

https://searchfox.org/firefox-main/rev/410a52656dd13aa2360a3e96788e41bf48e80c0f/devtools/client/debugger/src/components/SecondaryPanes/DOMMutationBreakpoints.js#115-118,125-127

const text = L10N.getFormatStr(
  "noDomMutationBreakpoints",
  `<a>${L10N.getStr("inspectorTool")}</a>`
);
...
    dangerouslySetInnerHTML: {
      __html: text,
    },

This one is simply setting a localized string, not harmful.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Attachment: (secure)

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Julian Descottes [:jdescottes] from comment #2)

2. DOMMutationBreakpoints.js

https://searchfox.org/firefox-main/rev/410a52656dd13aa2360a3e96788e41bf48e80c0f/devtools/client/debugger/src/components/SecondaryPanes/DOMMutationBreakpoints.js#115-118,125-127

const text = L10N.getFormatStr(
  "noDomMutationBreakpoints",
  `<a>${L10N.getStr("inspectorTool")}</a>`
);
...
    dangerouslySetInnerHTML: {
      __html: text,
    },

This one is simply setting a localized string, not harmful.

What's done is a bit odd:

• wrap the "Inspector" word inside an href-less link
• set the onClick event listener on the parent div

I would expect us to only have the click event on the "Inspector" word then
Or make the noDomMutationBreakpoints label to include the "Inspector" word, and have a button below with a "Open inspector" localized string.
Also, this doesn't seem to be covered by a test, would be nice to have one I guess

Comment 5

[Tom Schuster [:tschuster]]

Is there some sort of lint we could use to disallow dangerouslySetInnerHTML?

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Tom Schuster (Limited availability until May 5) from comment #5)

Is there some sort of lint we could use to disallow dangerouslySetInnerHTML?

there's already some, we're disabling it here https://searchfox.org/firefox-main/rev/410a52656dd13aa2360a3e96788e41bf48e80c0f/devtools/client/debugger/src/components/SecondaryPanes/DOMMutationBreakpoints.js#112-113,123,125-128

/* eslint-disable react/no-danger */
renderEmpty() {
...
    div({
...
      dangerouslySetInnerHTML: {
        __html: text,
      },
    })

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #6)

(In reply to Tom Schuster (Limited availability until May 5) from comment #5)

Is there some sort of lint we could use to disallow dangerouslySetInnerHTML?

there's already some, we're disabling it here https://searchfox.org/firefox-main/rev/410a52656dd13aa2360a3e96788e41bf48e80c0f/devtools/client/debugger/src/components/SecondaryPanes/DOMMutationBreakpoints.js#112-113,123,125-128

/* eslint-disable react/no-danger */
renderEmpty() {
...
    div({
...
      dangerouslySetInnerHTML: {
        __html: text,
      },
    })

mh not sure actually, if I remove the comment mach lint doesn't complain

Comment 8

[Tom Schuster [:tschuster]]

Oh ... I was actually gone ask why there is no annotation on the use in Breakpoint.js.

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Attachment: (secure)

Comment 10

[Tom Schuster [:tschuster]]

Nicolas, would you mind filing a follow-up security bug to make sure this lint (and maybe others?) are working.

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Tom Schuster (Limited availability until May 5) from comment #8)

Oh ... I was actually gone ask why there is no annotation on the use in Breakpoint.js.

I asked in #Lint And Formatting to see what's going on, I may end up filing a bug

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)

(In reply to Tom Schuster (Limited availability until May 5) from comment #8)

Oh ... I was actually gone ask why there is no annotation on the use in Breakpoint.js.

I asked in #Lint And Formatting to see what's going on, I may end up filing a bug

I investigated a bit and it looks like the rule only handles JSX, that we're not using anymore (we're using those React DOM factories, which rely on React.createElement)
I'll report upstream and will file a bug to see if we can do something about it in the meantime

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)

I asked in #Lint And Formatting to see what's going on, I may end up filing a bug

Filed Bug 2036699

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a7ab1273cf07
https://hg.mozilla.org/integration/autoland/rev/4b49342bc824
[devtools] Redesign empty dom mutation panel and add tests. r=devtools-reviewers,bomsy,jdescottes.
https://github.com/mozilla-firefox/firefox/commit/87424cb028a6
https://hg.mozilla.org/integration/autoland/rev/26d6e8e3880b
[devtools] Update Breakpoint text highlight. r=devtools-reviewers,jdescottes

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg-edge.mozilla.org/mozilla-central/rev/4b49342bc824
https://hg-edge.mozilla.org/mozilla-central/rev/26d6e8e3880b