Bug 112999 – Added a setting for whether JavaScript markup is enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Added a setting for whether JavaScript markup is enabled

[Geoffrey Garen]

Created attachment 194437 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 194437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194437&action=review

> Source/WebCore/ChangeLog:9
> +        This setting is useful for clients that want protection from script
> +        injection attacks.

Is this setting used anywhere now? It seems valuable to merge the patch that uses this setting.

[Geoffrey Garen]

> Is this setting used anywhere now? It seems valuable to merge the patch that uses this setting.

Not yet -- just trying to split up the patch into pieces.

[Adam Barth]

Comment on attachment 194437 [details]
Patch

I don't think we should have divergent ways of disabling JavaScript in WebCore.

[Adam Barth]

I'm about to leave on vacation.  I would like a chance to discuss this feature before you land it.

[Adam Barth]

I hope you weren't trying to sneak this into the codebase while I wasn't paying attention.  We've discussed this topic on webkit-dev, and I made it clear that I objected to this feature.

[Ryosuke Niwa]

(In reply to comment #4)
> (From update of attachment 194437 [details])
> I don't think we should have divergent ways of disabling JavaScript in WebCore.

I talked with Geoff about this in person, and I don't think the intent of this feature is to disable JavaScript.  It's about stripping scripting contents as they're parsed.

In fact, we're not even interested in exposing anywhere on the Web.

[Adam Barth]

I would like to discuss this feature before it lands.  Please wait until I return from vacation.

Please don't make me police bugs.webkit.org during my vacation.

[Maciej Stachowiak]

Is this feature meant to be used by the browser or by non-browser clients of WebKit?

[Geoffrey Garen]

> Is this feature meant to be used by the browser or by non-browser clients of WebKit?

The setting is for Mail.

The feature -- stripping script markup in editing contexts -- has existed in WebKit since 2010. The setting is a way to expose the feature to a client app.

[Geoffrey Garen]

> I'm about to leave on vacation.  I would like a chance to discuss this feature before you land it.

> I hope you weren't trying to sneak this into the codebase while I wasn't paying attention.  We've discussed this topic on webkit-dev, and I made it clear that I objected to this feature.

> I would like to discuss this feature before it lands.  Please wait until I return from vacation.

> Please don't make me police bugs.webkit.org during my vacation.

Frankly, that's ridiculous. We're not going to halt all development at Apple just because you're going on vacation.

I'm not "sneaking" anything. I'm posting a patch to a public review forum. I've duly noted your objection to using this feature as the default way to disable JavaScript in WebKit -- and I'm not doing that.

Your objection to having this feature at all is ridiculous: We've had it since 2010.

[Maciej Stachowiak]

(In reply to comment #10)
> > Is this feature meant to be used by the browser or by non-browser clients of WebKit?
> 
> The setting is for Mail.
> 
> The feature -- stripping script markup in editing contexts -- has existed in WebKit since 2010. The setting is a way to expose the feature to a client app.

Adam, do you actually object to offering the markup stripping functionality for third-party WebKit clients?

I would hope the applicability to Mail seems obvious, since for example webmail products like GMail do similar processing of mail message markup on the server side.

I understand that you don't want to be bypassed, but since the purpose of the patch is not what you apparently thought it was, it's not really reasonable to ask us to sit on it for two weeks.

[Maciej Stachowiak]

Adam, I've consulted with some of your Google colleagues. Because your objection to the patch seems to be based on a misunderstanding, they thought it was ok to go ahead and review, with a clear explanation of what we think the difference in understanding was, and an offer to reopen the discussion once you are back.

Specifically:
- It seems you were concerned about this patch because you thought it was a way for browsers to change how they disable JavaScript, even though there was not webkit-dev consensus to do that.
- However, that's not actually the purpose.
- The purpose is to offer the separate feature of JavaScript markup stripping to third-party clients, such as Mac OS X Mail. For a mail client this is clearly appropriate behavior - webmail clients do this sort of thing, for instance, though typically on the server side.
- We have no plans to bypass the webkit-dev discussion or divide the way WebKit-based browsers disable JavaScript.
- The feature of script-related markup stripping actually already exists in WebKit, for use in paste operations. So this isn't even new fundamental functionality, just a way for other clients to do the same thing.

I expect if you'd been aware of the above, you would not be objecting. Therefore I'm going to go ahead and review. If my assumption is wrong, we'd be happy to reopen the discussion.

[Maciej Stachowiak]

Comment on attachment 194437 [details]
Patch

I agree with Ryosuke's feedback that it would be slightly better to combine this patch with the one that actually uses the setting to do something. But I also think this would be ok to land as-is.

r=me

Adam, if you want to object to this r+, please read the previous comment first.

[Geoffrey Garen]

Committed r146664: <http://trac.webkit.org/changeset/146664>

[Maciej Stachowiak]

Comment on attachment 194437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194437&action=review

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:214
> +    macro(WebKitJavaScriptMarkupEnabled, ScriptEnabled, javaScriptEnabled) \

Dan Bates pointed out that this seems wrong - should probably be javaScriptMarkupEnabled, not javaScriptEnabled.

[Geoffrey Garen]

Reopening to attach new patch.

[Geoffrey Garen]

Created attachment 194660 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 194660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194660&action=review

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:213
> -    macro(WebKitJavaScriptEnabled, ScriptEnabled, javaScriptEnabled) \
>      macro(WebKitJavaScriptMarkupEnabled, ScriptEnabled, javaScriptEnabled) \

Did you really mean to delete line 213 rather than 214?

[Geoffrey Garen]

Created attachment 194669 [details]
Patch

[Geoffrey Garen]

> Did you really mean to delete line 213 rather than 214?

No. Man, that line of code is my nemesis. :(

[Geoffrey Garen]

> Is this setting used anywhere now?

Here's the use code: https://bugs.webkit.org/show_bug.cgi?id=113122.

[Geoffrey Garen]

Committed r146722: <http://trac.webkit.org/changeset/146722>

[Adam Barth]

This patch was landed over my explicit object and without the consensus of the WebKit project.  I don't think the way this patch was handled is consistent with the way the WebKit project should operate.  It is, unfortunately, consistent with the growing trend of unilateral action by Apple in this project.  Given that there were 37 comments on bug 113122 (even without my participating), it's clear that this feature was not nearly as uncontroversial as you claim.

[Geoffrey Garen]

> It is, unfortunately, consistent with the growing trend of unilateral action by Apple in this project.

I guess you didn't read Maciej's comment, which stated, "Adam, I've consulted with some of your Google colleagues."