Bug 209049 – Unique origins should not be Potentially Trustworthy

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

[youenn fablet]

As per https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy

[youenn fablet]

Created attachment 393474 [details]
Patch

[youenn fablet]

<rdar://problem/57827160>

[youenn fablet]

Created attachment 393488 [details]
Patch

[youenn fablet]

Created attachment 393489 [details]
Patch

[youenn fablet]

Created attachment 393501 [details]
Patch

[Darin Adler]

Comment on attachment 393501 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:6025
> +static inline bool isURLPotentiallyTrustworthy(const URL& url)

Seems overkill to write inline here.

> Source/WebCore/dom/Document.cpp:6027
> +    if (url.protocol() == "about"_s)

Should use URL::protocolIsAbout.

If not, then no need to use ASCIILiteral (the _s suffix) with ==; in fact I think it currently makes the == operator less efficient, although we could fix that. In general, to check protocols, I think we should use URL::protocolIs rather than URL::protocol with ==.

> Source/WebCore/dom/Document.cpp:6028
> +        return url.path() == "blank"_s || url.path() == "srcdoc"_s;

For about:blank we already have URL::isBlankURL. I suggest consider adding something similar for "srcdoc" instead of writing out like this.

Is it correct for this to be case-sensitive or do we want case folding? If we want case folding we should use equalLettersIgnoringASCIICase.

Same comment about using ASCIILiteral (the _s suffix).

Also, should change URL::path to return a StringView for efficiency.

> Source/WebCore/dom/Document.cpp:6029
> +    if (url.protocol() == "data"_s)

URL::protocolIsData

[youenn fablet]

Created attachment 393636 [details]
Patch for landing

[youenn fablet]

Created attachment 393647 [details]
Patch for landing - fixing test

[youenn fablet]

Right, this makes sense.
I updated the patch accordingly.

(In reply to Darin Adler from comment #6)
> Comment on attachment 393501 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393501&action=review
> 
> > Source/WebCore/dom/Document.cpp:6025
> > +static inline bool isURLPotentiallyTrustworthy(const URL& url)
> 
> Seems overkill to write inline here.
> 
> > Source/WebCore/dom/Document.cpp:6027
> > +    if (url.protocol() == "about"_s)
> 
> Should use URL::protocolIsAbout.
> 
> If not, then no need to use ASCIILiteral (the _s suffix) with ==; in fact I
> think it currently makes the == operator less efficient, although we could
> fix that. In general, to check protocols, I think we should use
> URL::protocolIs rather than URL::protocol with ==.
> 
> > Source/WebCore/dom/Document.cpp:6028
> > +        return url.path() == "blank"_s || url.path() == "srcdoc"_s;
> 
> For about:blank we already have URL::isBlankURL. I suggest consider adding
> something similar for "srcdoc" instead of writing out like this.
> 
> Is it correct for this to be case-sensitive or do we want case folding? If
> we want case folding we should use equalLettersIgnoringASCIICase.
> 
> Same comment about using ASCIILiteral (the _s suffix).
> 
> Also, should change URL::path to return a StringView for efficiency.
> 
> > Source/WebCore/dom/Document.cpp:6029
> > +    if (url.protocol() == "data"_s)
> 
> URL::protocolIsData

[WebKit Commit Bot]

Comment on attachment 393647 [details]
Patch for landing - fixing test

Clearing flags on attachment: 393647

Committed r258494: <https://trac.webkit.org/changeset/258494>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 393647 [details]
Patch for landing - fixing test

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

> Source/WebCore/dom/Document.cpp:6044
> +        return equalIgnoringASCIICase(url.string(), WTF::blankURL()) || equalLettersIgnoringASCIICase(url.string(), "about:srcdoc");

Should be using:

    url.isBlankURL()

instead of comparing string with WTF::blankURL. But then I discovered that URL::isBlankURL is declared in URL.h but not defined! Would you be willing to either delete it, or implement and use it?

[youenn fablet]

(In reply to Darin Adler from comment #12)
> Comment on attachment 393647 [details]
> Patch for landing - fixing test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393647&action=review
> 
> > Source/WebCore/dom/Document.cpp:6044
> > +        return equalIgnoringASCIICase(url.string(), WTF::blankURL()) || equalLettersIgnoringASCIICase(url.string(), "about:srcdoc");
> 
> Should be using:
> 
>     url.isBlankURL()
> 
> instead of comparing string with WTF::blankURL. But then I discovered that
> URL::isBlankURL is declared in URL.h but not defined! Would you be willing
> to either delete it, or implement and use it?

Right I saw that.
Will fix it tomorrow.

[Darin Adler]

Comment on attachment 393647 [details]
Patch for landing - fixing test

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

>>> Source/WebCore/dom/Document.cpp:6044
>>> +        return equalIgnoringASCIICase(url.string(), WTF::blankURL()) || equalLettersIgnoringASCIICase(url.string(), "about:srcdoc");
>> 
>> Should be using:
>> 
>>     url.isBlankURL()
>> 
>> instead of comparing string with WTF::blankURL. But then I discovered that URL::isBlankURL is declared in URL.h but not defined! Would you be willing to either delete it, or implement and use it?
> 
> Right I saw that.
> Will fix it tomorrow.

Might be better to delete it. Some places want to treat any "about:" URL the same, others specifically want to check for "about:blank". Not clear which of those isBlankURL means in a way. So maybe we should just delete it.

[youenn fablet]

Filed bug 209173 and bug 209174.
I think it makes sense to introduce isAboutBlank/isSrcDoc, since, at least in theory, I think we should check the scheme case insensitive and path case sensitive.