Bug 205157 – Ignore URL host for schemes that are not using host information

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

[youenn fablet]

Ignore URL host for schemes that are not using host information, like file, data or about

[youenn fablet]

Created attachment 385489 [details]
Patch

[youenn fablet]

<rdar://problem/57825963>

[Brent Fulgham]

Comment on attachment 385489 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        The resource loading will still be done based on the initial URL with host, but the document creation, including its location will do as if the host is empty.

"will ACT as if the host is empty"?

> Source/WebCore/loader/DocumentLoader.cpp:1631
> +        url.setHost({ });

This seems to be the only functional change in this patch. Is 'documentURL' used during the creation of the Document object (which your ChangeLog indicates), or do we need to modify Document::Document() to ensure we don't get a Document with a host that should be ignored?

For example, could JS create a Document object with a data URL that might bypass your change?

[youenn fablet]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 385489 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385489&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        The resource loading will still be done based on the initial URL with host, but the document creation, including its location will do as if the host is empty.
> 
> "will ACT as if the host is empty"?

Sounds good.
If a user is loading file://example.org/XX, the address bar will still have that URL, but 


> > Source/WebCore/loader/DocumentLoader.cpp:1631
> > +        url.setHost({ });
> 
> This seems to be the only functional change in this patch. Is 'documentURL'
> used during the creation of the Document object (which your ChangeLog
> indicates), or do we need to modify Document::Document() to ensure we don't
> get a Document with a host that should be ignored?

This is used for documents created by DocumentLoader/DocumentWriter, this change will basically apply whenever navigation happens.
This is the case when opening a link/iframe or using window.location.

If a user loads file://example.org/XX, location.host will be empty instead of "example.org" as done currently.

> For example, could JS create a Document object with a data URL that might
> bypass your change?

This is mostly targeting file URLs. Hosts in data URLs interact with parsing of the mime type.

JS could try to create a Document with XHR but in that case, this would be considered a cross origin request and would normally fail.
There is the case of SVG documents but they should also be same-origin.

The stronger way is to do the same modification within Document constructor.
The impact is wider though, for instance it impacts HTMLDocument::createSynthesizedDocument.
Maybe we should try this approach.

[Alex Christensen]

Comment on attachment 385489 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=205157

Please include radar.

[youenn fablet]

Created attachment 386390 [details]
Removing host inside Document class

[Darin Adler]

Comment on attachment 386390 [details]
Removing host inside Document class

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

> Source/WebCore/dom/Document.cpp:3233
> +    if (SecurityOrigin::shouldIgnoreHost(m_url) && !m_url.host().isEmpty())
> +        m_url.setHost({ });

I think this should do something more like setHostAndPort instead of setHost. I don’t think it makes sense to leave a port number behind. But setHostAndPort doesn’t seem to handle the empty string correctly. So might need to change it in the URL class or even add a new removeHostAndPort function. If we did add a new removeHostAndPort function, we could optimize it to quickly do nothing when it’s already empty, and then we wouldn’t need the host().isEmpty() check here.

> Source/WebCore/page/DOMWindow.h:179
> +    WEBCORE_EXPORT Location& location();

We also have WEBCORE_TESTSUPPORT_EXPORT. Not sure if we are using it consistently.

> Source/WebCore/page/Location.h:55
> +    WEBCORE_EXPORT String host() const;

Ditto.

> Source/WebCore/page/SecurityOrigin.cpp:61
> +    return url.protocolIs("data") || url.protocolIs("about") || url.protocolIs("file");

How did you determine this set of protocols?

Could use protocolIsAbout() and protocolIsData() rather than writing those two strings out.

Could consider using isLocalFile() for the "file" one, but not sure if that’s the correct abstraction.

I don’t understand how we determined this list. What about "blob"? What about "javascript"? How are they different from "data"?

[youenn fablet]

Comment on attachment 386390 [details]
Removing host inside Document class

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

>> Source/WebCore/dom/Document.cpp:3233
>> +        m_url.setHost({ });
> 
> I think this should do something more like setHostAndPort instead of setHost. I don’t think it makes sense to leave a port number behind. But setHostAndPort doesn’t seem to handle the empty string correctly. So might need to change it in the URL class or even add a new removeHostAndPort function. If we did add a new removeHostAndPort function, we could optimize it to quickly do nothing when it’s already empty, and then we wouldn’t need the host().isEmpty() check here.

OK, will add removeHostAndPort

>> Source/WebCore/page/SecurityOrigin.cpp:61
>> +    return url.protocolIs("data") || url.protocolIs("about") || url.protocolIs("file");
> 
> How did you determine this set of protocols?
> 
> Could use protocolIsAbout() and protocolIsData() rather than writing those two strings out.
> 
> Could consider using isLocalFile() for the "file" one, but not sure if that’s the correct abstraction.
> 
> I don’t understand how we determined this list. What about "blob"? What about "javascript"? How are they different from "data"?

We can only remove host information for the schemes we know hosts are not useful, custom protocols might rely on host information for instance.
From the known schemes, http/ftp cannot be in the list. "blob" is tied to the origin that created the blob URL so cannot be in the list.
I will add "javascript" in the list since it is similar to data URL.

[youenn fablet]

Created attachment 386519 [details]
Updated according review

[youenn fablet]

Created attachment 386531 [details]
Fix Win build

[WebKit Commit Bot]

Comment on attachment 386531 [details]
Fix Win build

Rejecting attachment 386531 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 386531, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13296948

[youenn fablet]

Created attachment 386534 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 386534 [details]:

inspector/debugger/breakpoint-scope.html bug 205646 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 386534 [details]
Patch

Clearing flags on attachment: 386534

Committed r253946: <https://trac.webkit.org/changeset/253946>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.