1729916 - Using the "edit as html" feature on a chrome page hits an assertion

Status: RESOLVED WONTFIX

(DevTools :: Inspector, defect, P3)

Description

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

STR:

1. open the toolbox on any page, open the performance panel
2. open the remote toolbox
3. inspect the performance panel
4. right click then edit as html, edit something and save

=> we hit an assertion in debug builds.

This is this assertion:

https://searchfox.org/mozilla-central/rev/aa46c2dcccbc6fd4265edca05d3d00cccdfc97b9/dom/security/DOMSecurityMonitor.cpp#110

Is that expected?

Comment 1

[Julian Descottes [:jdescottes]]

Chrome page only, low priority.

Comment 2

[Julian Descottes [:jdescottes]]

The assertion prevents editing HTML in chrome pages, we hit it when using "edit as HTML" which is handled by the Walker actor on the server: https://searchfox.org/mozilla-central/rev/4d90ff4537330d6b17cb956c0fadf759086d9bb7/devtools/server/actors/inspector/walker.js#1665

The Walker actor might be used to set innerHTML for both content pages or chrome pages.

One option is to add devtools/server/actors/inspector/walker.js to the allowlist at https://searchfox.org/mozilla-central/rev/aa46c2dcccbc6fd4265edca05d3d00cccdfc97b9/dom/security/DOMSecurityMonitor.cpp#55

Or we can disable the Edit as HTML on chrome pages (keeping in mind today the feature works fine non-debug builds).

I'm not sure about the security implications of doing one or the other, let's ask.
Christoph: what do you think we should do here?

Comment 3

[Christoph Kerschbaumer [:ckerschb}]

I think Freddy is the best person to answer. Generally we should avoid adding new files to the allowlist there.

Comment 4

[Frederik Braun [:freddy]]

This is expected behavior in the context of preventing that chrome code accidentally XSSes itself. But I can totally see this as an annoying stopgap for our developers.

I also want to point out that this a debug assertion (MOZ_ASSERT instead of MOZ_RELEASE_ASSERT), so that you should not experience this in opt builds. Does that help?

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Frederik Braun [:freddy] from comment #4)

This is expected behavior in the context of preventing that chrome code accidentally XSSes itself. But I can totally see this as an annoying stopgap for our developers.

I also want to point out that this a debug assertion (MOZ_ASSERT instead of MOZ_RELEASE_ASSERT), so that you should not experience this in opt builds. Does that help?

Thanks for the answer! You are right, this works fine in opt builds. I also agree that it's nice to preserve the feature, even if it breaks in DEBUG builds.

Freddy, sorry for sending the ni? back to you, but I want to explicitly check before I close the bug as WONTFIX: the only way to fix this for DEBUG builds would be to add the walker.js file to the allowlist. Do I understand correctly that we don't want to do that?

Comment 6

[Frederik Braun [:freddy]]

The only way to fix this would be to add the file to allow-list, that's correct.

We're hesitant to add files to that list as we are trying to get it down to zero. The currently allowed files are a mixed grab bag of "this was secure, when we last looked" and "this was hard to remove, but definitely an issue". Historically, we have also observed that these files become the "path of least resistance" for new code, which might then create security bugs in the future.

Comment 7

[Julian Descottes [:jdescottes]]

Thanks for the feedback, I'm going to mark this as wontfix then.