Bug 207427 – Expose "allowsContentJavaScript" on WKWebpagePreferences

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Brady Eidson]

Expose "allowsContentJavaScript" on WKWebpagePreferences

This will allow content (ne "markup") JS to be disabled while still allowing script from API entry points.

<rdar://problem/51534967>

[Brady Eidson]

Created attachment 390173 [details]
Patch

[Brady Eidson]

Created attachment 390178 [details]
Patch

[Brady Eidson]

Created attachment 390181 [details]
Patch

[Sam Weinig]

Comment on attachment 390181 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.h:650
> +    Optional<bool> m_allowsScriptMarkup;

I find Optional<bool> to be hard to reason about and not all that self documenting. In other cases I have been recommending a three state enum instead.

[Brady Eidson]

Created attachment 390191 [details]
Patch

[Brady Eidson]

(In reply to Sam Weinig from comment #4)
> Comment on attachment 390181 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390181&action=review
> 
> > Source/WebCore/loader/DocumentLoader.h:650
> > +    Optional<bool> m_allowsScriptMarkup;
> 
> I find Optional<bool> to be hard to reason about and not all that self
> documenting. In other cases I have been recommending a three state enum
> instead.

Fair enough.

[Brady Eidson]

Created attachment 390195 [details]
Patch

[Darin Adler]

Comment on attachment 390195 [details]
Patch

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

Looks generally good. I have some questions so I didn’t review+ or review-.

> Source/WebCore/loader/DocumentLoader.cpp:1227
> +    if (m_scriptMarkupPolicy != ScriptMarkupPolicy::Default)
> +        m_frame->settings().setScriptMarkupEnabled(m_scriptMarkupPolicy == ScriptMarkupPolicy::Enable);

I’m concerned that applyPoliciesToSettings is not a maintainable approach long term. Things that are different-per-document-loader probably should not be stored in settings that hang off the frame.

Separate thought: The behavior of ScriptMarkupPolicy::Default seems to be "leave the settings behind from the previous document"? Is that safe?

> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.mm:347
> +    switch (_websitePolicies->scriptMarkupPolicy()) {
> +    case WebCore::ScriptMarkupPolicy::Default:
> +    case WebCore::ScriptMarkupPolicy::Enable:
> +        return true;
> +    case WebCore::ScriptMarkupPolicy::Disable:
> +        return false;
> +    }

If you never set this property, the markup policy is different from either NO or YES. But we do not document this difference. Nor offer a way to restore this behavior. I guess this third policy means "respect a preference with no API but that might be set on the command line"?

[Brady Eidson]

(In reply to Darin Adler from comment #8)
> Comment on attachment 390195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390195&action=review
> 
> Looks generally good. I have some questions so I didn’t review+ or review-.
> 
> > Source/WebCore/loader/DocumentLoader.cpp:1227
> > +    if (m_scriptMarkupPolicy != ScriptMarkupPolicy::Default)
> > +        m_frame->settings().setScriptMarkupEnabled(m_scriptMarkupPolicy == ScriptMarkupPolicy::Enable);
> 
> I’m concerned that applyPoliciesToSettings is not a maintainable approach
> long term. Things that are different-per-document-loader probably should not
> be stored in settings that hang off the frame.

In this case, "any document loader in the page getting this" should apply to the page global settings.

I 100% agree that this approach doesn't seem as general purpose as as some of the other users of it!

> Separate thought: The behavior of ScriptMarkupPolicy::Default seems to be
> "leave the settings behind from the previous document"? Is that safe?

The *intention* was "If WKPreferencesRef or WKBrowsingContextGroup users had set the markup-enabled flag, and nobody set it on the WKWebsitePreferences, respect the SPI setting"

But in those cases, you are 100% correct that this is what happens in effect.

The SPI override really gets in the way here.

I'll either determine the SPI support is no longer needed and greatly simplify this code, or I'll adjust to handle it.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.mm:347
> > +    switch (_websitePolicies->scriptMarkupPolicy()) {
> > +    case WebCore::ScriptMarkupPolicy::Default:
> > +    case WebCore::ScriptMarkupPolicy::Enable:
> > +        return true;
> > +    case WebCore::ScriptMarkupPolicy::Disable:
> > +        return false;
> > +    }
> 
> If you never set this property, the markup policy is different from either
> NO or YES. But we do not document this difference. Nor offer a way to
> restore this behavior. I guess this third policy means "respect a preference
> with no API but that might be set on the command line"?

Indeed - this trinary state came about because of a conflict with the SPI code paths to this setting.

As mentioned above, I'll figure out the way forward.

[Brady Eidson]

Well it's actually SPI on WKWebViewConfiguration, so that's potentially much more problematic.

[Brady Eidson]

Yup, definitely still have to support that SPI.

The "dangerous" thing is accidentally allowing markup JS when somebody has asked the view to not allow it.

I can do that with Settings + FrameLoaderClient instead of this ternary state.

[Brady Eidson]

Created attachment 390346 [details]
Patch

[Brady Eidson]

(In reply to Brady Eidson from comment #9)
> (In reply to Darin Adler from comment #8
> 
> In this case, "any document loader in the page getting this" should apply to
> the page global settings.

This got me thinking as to my approach, which is now to put the "did this navigation have the markup disabled policy?" flag on the Page itself, living side-by-side with the WKPreferences-driven Settings version.

Then we consult both at decision time.

[Brady Eidson]

Created attachment 390669 [details]
Patch

[Brady Eidson]

Created attachment 390673 [details]
Patch

[Brady Eidson]

Ouch. The key with these layout test failures is:

bool Document::scriptMarkupEnabled() const
{
    if (!settings().scriptMarkupEnabled())
        return false;

    if (!m_frame || m_frame->document() != this)
        return false;
    return m_frame->loader().client().scriptMarkupPolicyFromMostRecentNavigation() == ScriptMarkupPolicy::Enable;
}

The middle clause of no frame.

I'd figured "A frameless document should never be executing script, right?"

But I was totally wrong.

So now I have to find a (good) way to propagate the policy forward to frameless documents from whoever creates them.

[Brady Eidson]

Created attachment 390731 [details]
Patch

Trying this approach on the bots (fixed at least some tests locally)

[Brady Eidson]

Created attachment 390768 [details]
Patch

Trying this approach on the bots (fixed more tests locally)

[Brady Eidson]

Created attachment 390784 [details]
Patch

[Darin Adler]

Comment on attachment 390784 [details]
Patch

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

I like the public allowsContentJavaScript name so much better than the internal scriptMarkupPolicy name.

> Source/WebCore/dom/Document.cpp:6817
> +    if (!settings().scriptMarkupEnabled())

Seems slightly dangerous to still have this Settings::scriptMarkupEnabled function. If anyone calls it and it returns true, they will likely do the wrong thing. Maybe at some point we can rename it?

> Source/WebCore/dom/Document.cpp:6826
> +        if (m_contextDocument)
> +            return m_contextDocument->scriptMarkupEnabled();
> +
> +        // If this Document is frameless or in the wrong frame, *and* it has no context document,
> +        // then it has no business running markup script.
> +        return false;

I’d find this slightly easier to read as an && expression:

    return m_contextDocument && m_contextDocument->scriptMarkupEnabled();

Also slightly annoying that there’s recursion here, but I suppose it’s unlikely to recurse deeply.

> Source/WebCore/xml/XMLHttpRequest.cpp:194
>              m_responseDocument->setContextDocument(context);
> +            m_responseDocument->setContent(m_responseBuilder.toStringPreserveCapacity());
>              m_responseDocument->setSecurityOriginPolicy(context.securityOriginPolicy());
>              m_responseDocument->overrideMIMEType(mimeType);

Maybe we should also move up the setSecurityOriginPolicy and overrideMIMEType calls up before we call setContent too? Seems like we’d want all those things set before we do any parsing.

> Source/WebKit/WebProcess/WebPage/WebFrame.cpp:269
> +        ASSERT(page());

What guarantees page() is not null?

[Brady Eidson]

(In reply to Darin Adler from comment #20)
> Comment on attachment 390784 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390784&action=review
> 
> I like the public allowsContentJavaScript name so much better than the
> internal scriptMarkupPolicy name.

Me too. It was originally the tri-state enum Sam requested and this "FooPolicy" name was matching other tri-state style.

I'll rename.

> 
> > Source/WebCore/dom/Document.cpp:6817
> > +    if (!settings().scriptMarkupEnabled())
> 
> Seems slightly dangerous to still have this Settings::scriptMarkupEnabled
> function. If anyone calls it and it returns true, they will likely do the
> wrong thing. Maybe at some point we can rename it?

I agree! Unfortunately it is exposed as (multiple) SPIs that are still used.

Renaming it to something that discourages its use seems great.

> > Source/WebCore/dom/Document.cpp:6826
> > +        if (m_contextDocument)
> > +            return m_contextDocument->scriptMarkupEnabled();
> > +
> > +        // If this Document is frameless or in the wrong frame, *and* it has no context document,
> > +        // then it has no business running markup script.
> > +        return false;
> 
> I’d find this slightly easier to read as an && expression:
> 
>     return m_contextDocument && m_contextDocument->scriptMarkupEnabled();
> 
> Also slightly annoying that there’s recursion here, but I suppose it’s
> unlikely to recurse deeply.
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:194
> >              m_responseDocument->setContextDocument(context);
> > +            m_responseDocument->setContent(m_responseBuilder.toStringPreserveCapacity());
> >              m_responseDocument->setSecurityOriginPolicy(context.securityOriginPolicy());
> >              m_responseDocument->overrideMIMEType(mimeType);
> 
> Maybe we should also move up the setSecurityOriginPolicy and
> overrideMIMEType calls up before we call setContent too? Seems like we’d
> want all those things set before we do any parsing.

Sounds right!

> 
> > Source/WebKit/WebProcess/WebPage/WebFrame.cpp:269
> > +        ASSERT(page());
> 
> What guarantees page() is not null?

The coreFrame null check earlier almost always guarantees this.
It's not a 100% guarantee, I'll clean it up.

[Brady Eidson]

Created attachment 390855 [details]
Patch

[Brady Eidson]

(In reply to Brady Eidson from comment #22)
> Created attachment 390855 [details]
> Patch

This will be for landing later: Some clients need to handle the deprecation first

[WebKit Commit Bot]

Comment on attachment 390855 [details]
Patch

Clearing flags on attachment: 390855

Committed r256715: <https://trac.webkit.org/changeset/256715>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59498009>

[Alex Christensen]

Comment on attachment 390855 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEvaluateJavaScript.mm:261
> +TEST(WebKit, AllowsContentJavaScript)

This needs #if HAVE(NETWORK_FRAMEWORK)