As per https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy
Created attachment 393474 [details] Patch
<rdar://problem/57827160>
Created attachment 393488 [details] Patch
Created attachment 393489 [details] Patch
Created attachment 393501 [details] Patch
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
Created attachment 393636 [details] Patch for landing
Created attachment 393647 [details] Patch for landing - fixing test
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
Comment on attachment 393647 [details] Patch for landing - fixing test Clearing flags on attachment: 393647 Committed r258494: <https://trac.webkit.org/changeset/258494>
All reviewed patches have been landed. Closing bug.
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?
(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.
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.
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.