Bug 209174 – Add routines to check about:blank and about:srcdoc URLs

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[youenn fablet]

Add routines to check about:blank and about:srcdoc URLs

[youenn fablet]

Created attachment 393738 [details]
Patch

[youenn fablet]

Created attachment 393957 [details]
Patch

[youenn fablet]

Two tests are failing:

imported/w3c/web-platform-tests/html/dom/usvstring-reflection.https.html gets some more PASS. I believe the issue was that we were comparing "about:blank" with "about:blank#xxx" and we are now no longer checking the hash. Seems like an improvement.

http/tests/dom/window-open-about-uppercase-blank-and-access-document.html is now failing and is specifically trying to do window.open("about:BLANK").
Both Firefox and Chrome pass the test but with different behaviours. They treat about:BLANK as somehow different from about:blank though. For instance Chrome will end-up loading "about:blank#blocked". Probably the navigation starts with "about:blank", tries to go to "about:BLANK" which is blocked.

I guess we could make a DOMWindow::open specific fix here, or preserve for now SecurityPolicy::shouldInheritSecurityOriginFromOwner behavior.

[youenn fablet]

Created attachment 393971 [details]
Patch

[youenn fablet]

Created attachment 393976 [details]
Patch

[youenn fablet]

Created attachment 394076 [details]
Patch

[Alex Christensen]

Comment on attachment 394076 [details]
Patch

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

> Source/WTF/wtf/URL.h:262
>  WTF_EXPORT_PRIVATE const URL& blankURL();

I think we should rename this.

[youenn fablet]

Comment on attachment 394076 [details]
Patch

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

>> Source/WTF/wtf/URL.h:262
>>  WTF_EXPORT_PRIVATE const URL& blankURL();
> 
> I think we should rename this.

Agreed, it should be aboutBlankURL probably.

[youenn fablet]

https://bugs.webkit.org/show_bug.cgi?id=209344

[EWS]

Committed r258769: <https://trac.webkit.org/changeset/258769>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394076 [details].

[Radar WebKit Bug Importer]

<rdar://problem/60692601>

[Darin Adler]

Comment on attachment 394076 [details]
Patch

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

> Source/WTF/wtf/URL.cpp:1026
> +    return protocolIsAbout() && path() == "blank";

Case sensitive. On purpose? I see FIXME that implies it is. But also, I would like to see test coverage.

> Source/WTF/wtf/URL.cpp:1031
> +    return protocolIsAbout() && path() == "srcdoc";

Ditto.

[Darin Adler]

Comment on attachment 394076 [details]
Patch

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

> Source/WTF/wtf/URL.cpp:878
> +    static NeverDestroyed<URL> staticAboutSrcDocURL(URL(), "about:srcdoc");

I wonder if we should include const for things like this.

    static const NeverDestroyed<URL> ...

Not sure if this has enough value to be worth including the extra keyword.

> Source/WebCore/html/HTMLFrameElementBase.cpp:156
> +        return WTF::aboutSrcDocURL();

Should not need a WTF prefix. Normally for WTF we are putting "using" in the header itself. I would like to stay consistent one way or the other.

> Source/WebCore/loader/FrameLoader.cpp:3714
> -    if (!equalLettersIgnoringASCIICase(url.string(), "about:srcdoc"))
> +    if (!url.isAboutSrcDoc())

Here we changed behavior. Do we have test coverage?

> Source/WebCore/page/SecurityPolicy.cpp:170
> +    // FIXME: We also allow some URLs like "about:BLANK". We should probably block navigation to these URLs, see rdar://problem/57966056.
> +    return url.isEmpty() || url.isAboutBlank() || url.isAboutSrcDoc() || equalIgnoringASCIICase(url.string(), WTF::blankURL());

Here we preserved this old behavior, and the FIXME suggests changing bahvior. Do we have test coverage either way?

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:82
> -        && !equalIgnoringASCIICase(openerUrl.string(), WTF::blankURL())) {
> +        && !openerUrl.isAboutBlank()) {

Here we changed behavior. Do we have test coverage?

[youenn fablet]

> > Source/WebCore/html/HTMLFrameElementBase.cpp:156
> > +        return WTF::aboutSrcDocURL();
> 
> Should not need a WTF prefix. Normally for WTF we are putting "using" in the
> header itself. I would like to stay consistent one way or the other.

Will fix it.

> > Source/WebCore/loader/FrameLoader.cpp:3714
> > -    if (!equalLettersIgnoringASCIICase(url.string(), "about:srcdoc"))
> > +    if (!url.isAboutSrcDoc())
> 
> Here we changed behavior. Do we have test coverage?

I do not think we are changing behavior since this is an early return.
We are early returning more often but, in the cases where we were not, ownerElement->hasAttributeWithoutSynchronization(srcdocAttr) would catch the other cases.

> > Source/WebCore/page/SecurityPolicy.cpp:170
> > +    // FIXME: We also allow some URLs like "about:BLANK". We should probably block navigation to these URLs, see rdar://problem/57966056.
> > +    return url.isEmpty() || url.isAboutBlank() || url.isAboutSrcDoc() || equalIgnoringASCIICase(url.string(), WTF::blankURL());
> 
> Here we preserved this old behavior, and the FIXME suggests changing
> bahvior. Do we have test coverage either way?

We have test coverage here: http/tests/dom/window-open-about-uppercase-blank-and-access-document.html.

> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:82
> > -        && !equalIgnoringASCIICase(openerUrl.string(), WTF::blankURL())) {
> > +        && !openerUrl.isAboutBlank()) {
> 
> Here we changed behavior. Do we have test coverage?

Right.
John, do you know how we can exercise that code path?