1687303 - Make nsHTMLContentSerializer handle noscript depending on the scripting enabled state of the document.

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect, P3)

Description

[Henri Sivonen (:hsivonen)]

See bug 1683627: Make nsHTMLContentSerializer handle noscript depending on the scripting enabled state of the document.

Comment 1

[Daniel Veditz [:dveditz]]

Is this a security bug also, or just hidden because bug 1683627 currently is hidden?

Comment 2

[Henri Sivonen (:hsivonen)]

(In reply to Daniel Veditz [:dveditz] from comment #1)

Is this a security bug also, or just hidden because bug 1683627 currently is hidden?

This was originally hidden because bug 1683627 is currently hidden, but now I think this has security bug potential in the Save As… case (to the extent opening Saved As pages isn't considered an open-ended security problem in general) and possibly even in the copy-paste case. Per spec, it seems that <iframe sandbox> should be a way to materialize a renderable scripting-disabled doc that one could copy to clipboard from, but I didn't test it just now.

Comment 3

[Andrew McCreight [:mccr8]]

Henri, could you try and see if this is actually an issue? Thanks.

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1687303 - Make nsHTMLContentSerializer handle noscript depending on the scripting enabled state of the document.

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to Andrew McCreight [:mccr8] from comment #3)

Henri, could you try and see if this is actually an issue? Thanks.

It seems easier to fix this than to prove whether it needs fixing.

Comment 6

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d822cabc052fa2e79089959d8ce69be5eb6f4060

Comment 7

[Andrew McCreight [:mccr8]]

Mirko, did you mean to mark the patch as needs revision? Thanks.

Comment 8

[Mirko Brodesser (:mbrodesser-Igalia), offline until April 25h]

Well, the review contains open questions, the patch itself might be OK. Henri: can you please have a look at the open questions?

Comment 9

[Andrew McCreight [:mccr8]]

Any updates here? It looks like this bug stalled out about 8 months ago. Thanks.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D124892	Bug 1687303 - Make nsHTMLContentSerializer handle noscript depending on the scripting enabled state of the document.	hsivonen	mbrodesser: Disabled

:hsivonen, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.

Comment 11

[Frederik Braun [:freddy]]

Edgar, can you find a new reviewer and help make sure the existing review feedback gets prioritized? It looks like the patch is almost ready and it would be nice to get our number of security bugs down.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Edgar, can you find a new reviewer and help make sure the existing review feedback gets prioritized? It looks like the patch is almost ready and it would be nice to get our number of security bugs down.

Let me take this action and review the priorities. :)

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai (she/her) [:hsinyi] from comment #12)

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

Edgar, can you find a new reviewer and help make sure the existing review feedback gets prioritized? It looks like the patch is almost ready and it would be nice to get our number of security bugs down.

Let me take this action and review the priorities. :)

Henri submitted a revision and asked review from Edgar.

Comment 14

[Henri Sivonen (:hsivonen)]

Added to Lando queue since sec-moderate.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Make nsHTMLContentSerializer handle noscript depending on the scripting enabled state of the document. r=edgar
https://hg.mozilla.org/integration/autoland/rev/6a46ccc50745ad60fac15bd5279824136588026a
https://hg.mozilla.org/mozilla-central/rev/6a46ccc50745

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:hsivonen, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox109 to wontfix.

For more information, please visit auto_nag documentation.

Comment 17

[Henri Sivonen (:hsivonen)]

Comment on attachment 9240005 [details]
Bug 1687303 - Make nsHTMLContentSerializer handle noscript depending on the scripting enabled state of the document.

Beta/Release Uplift Approval Request

• User impact if declined: Unknown. This patch fixes what's obviously a bug, but I'm unsure of attack vectors. However, the patch to central pretty clearly highlights the bug for an attacker to figure out the vector to reach the bug. Not sure if we should uplift for that reason.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed: None
• Is Android affected?: Yes

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9240005 [details]
Bug 1687303 - Make nsHTMLContentSerializer handle noscript depending on the scripting enabled state of the document.

Approved for 109.0b6 and 102.7esr.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ca50cbc861da
https://hg.mozilla.org/releases/mozilla-esr102/rev/a6a77bf24308