Non-special URLs should have an opaque origin

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Non-special URLs should have an opaque origin

[Alex Christensen]

Created attachment 289315 [details]
Patch

[Build Bot]

Comment on attachment 289315 [details]
Patch

Attachment 289315 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2110036

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289322 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289315 [details]
Patch

Attachment 289315 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2110155

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289323 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289315 [details]
Patch

Attachment 289315 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2110160

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289324 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Sam Weinig]

Comment on attachment 289315 [details]
Patch

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

> Source/WebCore/page/SecurityOrigin.cpp:182
> +    if (url.protocolIsInHTTPFamily()

This could use a either a comment, or a helper predicate that explains why this set. I would prefer a static shouldUseUniqueOrigin(url) or something.

[Alex Christensen]

Created attachment 289345 [details]
Patch

[Sam Weinig]

Comment on attachment 289345 [details]
Patch

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

I'm a little concerned that this is changing the behavior of file: URLs here, but I am honestly not sure what the old behavior was.  Also, how does this differ from SchemeRegistry::shouldTreatURLSchemeAsNoAccess()?

> Source/WebCore/page/SecurityOrigin.cpp:105
> +        && !url.protocolIs("gopher")

GOPHER!

[Build Bot]

Comment on attachment 289345 [details]
Patch

Attachment 289345 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2112734

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289351 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289345 [details]
Patch

Attachment 289345 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2112742

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289352 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289345 [details]
Patch

Attachment 289345 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2112740

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289353 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Alex Christensen]

Created attachment 289356 [details]
Patch

[Alex Christensen]

(In reply to comment #10)
> Comment on attachment 289345 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289345&action=review
> 
> I'm a little concerned that this is changing the behavior of file: URLs
> here, but I am honestly not sure what the old behavior was.  Also, how does
> this differ from SchemeRegistry::shouldTreatURLSchemeAsNoAccess()?
File URLs are in the list of unchanged schemes.  This makes it so all non-special schemes have opaque security origin.  I guess SchemeRegistry::shouldTreatURLSchemeAsNoAccess could be used to make a scheme like http have opaque security origins, but I'm not sure if it's ever used for that.  Maybe SchemeRegistry::shouldTreatURLSchemeAsNoAccess should be removed because of this.
> 
> > Source/WebCore/page/SecurityOrigin.cpp:105
> > +        && !url.protocolIs("gopher")
> 
> GOPHER!
It's in the spec.

[Build Bot]

Comment on attachment 289356 [details]
Patch

Attachment 289356 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2113011

New failing tests:
http/tests/security/postMessage/invalid-origin-throws-exception.html

[Build Bot]

Created attachment 289362 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289356 [details]
Patch

Attachment 289356 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2113030

New failing tests:
http/tests/security/postMessage/invalid-origin-throws-exception.html

[Build Bot]

Created attachment 289366 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289356 [details]
Patch

Attachment 289356 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2113027

New failing tests:
http/tests/security/postMessage/invalid-origin-throws-exception.html

[Build Bot]

Created attachment 289368 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289356 [details]
Patch

Attachment 289356 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2113040

New failing tests:
http/tests/security/postMessage/invalid-origin-throws-exception.html

[Build Bot]

Created attachment 289370 [details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Alex Christensen]

Created attachment 289384 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/changeset/206165

[WebKit Commit Bot]

Re-opened since this is blocked by bug 162492

[Alex Christensen]

Comment on attachment 289384 [details]
Patch

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

> Source/WebCore/page/SecurityOrigin.cpp:106
> +    return !innerURL.protocolIsInHTTPFamily()
> +        && !innerURL.protocolIs("file")
> +        && !innerURL.protocolIs("ftp")
> +        && !innerURL.protocolIs("gopher")
> +        && !innerURL.protocolIs("ws")
> +        && !innerURL.protocolIs("wss");

When I reland this, I plan to add this:
#if PLATFORM(GTK)
    // Comment explaining why this is needed...
    && !innerURL.protocolIs("resource")
#endif

[Carlos Garcia Campos]

(In reply to comment #30)
> Comment on attachment 289384 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289384&action=review
> 
> > Source/WebCore/page/SecurityOrigin.cpp:106
> > +    return !innerURL.protocolIsInHTTPFamily()
> > +        && !innerURL.protocolIs("file")
> > +        && !innerURL.protocolIs("ftp")
> > +        && !innerURL.protocolIs("gopher")
> > +        && !innerURL.protocolIs("ws")
> > +        && !innerURL.protocolIs("wss");
> 
> When I reland this, I plan to add this:
> #if PLATFORM(GTK)
>     // Comment explaining why this is needed...
>     && !innerURL.protocolIs("resource")
> #endif

We would still need the new api to mark custom uri schemes as non-unique when registered.

[Alex Christensen]

(In reply to comment #31)
> (In reply to comment #30)
> > Comment on attachment 289384 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=289384&action=review
> > 
> > > Source/WebCore/page/SecurityOrigin.cpp:106
> > > +    return !innerURL.protocolIsInHTTPFamily()
> > > +        && !innerURL.protocolIs("file")
> > > +        && !innerURL.protocolIs("ftp")
> > > +        && !innerURL.protocolIs("gopher")
> > > +        && !innerURL.protocolIs("ws")
> > > +        && !innerURL.protocolIs("wss");
> > 
> > When I reland this, I plan to add this:
> > #if PLATFORM(GTK)
> >     // Comment explaining why this is needed...
> >     && !innerURL.protocolIs("resource")
> > #endif
> 
> We would still need the new api to mark custom uri schemes as non-unique
> when registered.
Yes, and I think that api should be automatically called when making a new GTK WebProcess or when opening the Web Inspector instead of putting this layering violation here.

[Alex Christensen]

Created attachment 291698 [details]
Patch

[WebKit Commit Bot]

Attachment 291698 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
ERROR: Source/WebKit/mac/WebView/WebViewPrivate.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
Total errors found: 2 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Carlos or Michael, could you deprecate the old GTK API and add new GTK API like I tried to do in https://bugs.webkit.org/show_bug.cgi?id=162475 and use it to register the "resource" scheme when opening the web inspector so we don't have another https://bugs.webkit.org/show_bug.cgi?id=162387 ?

[Carlos Garcia Campos]

(In reply to comment #35)
> Carlos or Michael, could you deprecate the old GTK API and add new GTK API
> like I tried to do in https://bugs.webkit.org/show_bug.cgi?id=162475 and use
> it to register the "resource" scheme when opening the web inspector so we
> don't have another https://bugs.webkit.org/show_bug.cgi?id=162387 ?

Yes, I'll do it, but to not break anything until I write the patch, could you please do a couple of things?

 1- In Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp remove the uses of removed API from registerSecurityPolicyForURIScheme() and checkSecurityPolicyForURIScheme() just to fix the build. Do nothing when registering, and return false when checking, adding a FIXME in both cases.

 2- In SchemeRegistry.cpp add "resource" unconditionally to schemesForNonNullOrigins map inside a #if USE(SOUP) block. No need for FIXME here, we always want this.

Maybe that still breaks a unit tests, but that's not a problem. At least it will build and the inspector will be usable.

[Darin Adler]

Comment on attachment 291698 [details]
Patch

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

I’d like someone else, Anders maybe, to review the new SPI parts of this. I reviewed the rest.

> Source/WebCore/page/SecurityOrigin.cpp:107
> +    if (innerURL.protocolIsInHTTPFamily()
> +        || innerURL.protocolIs("file")
> +        || innerURL.protocolIs("ftp")
> +        || innerURL.protocolIs("gopher")
> +        || innerURL.protocolIs("ws")
> +        || innerURL.protocolIs("wss"))

Do we have this list anywhere else? I would have expected isSpecialProtocol instead of this list written out like this.

> Source/WebCore/platform/SchemeRegistry.cpp:77
> +static URLSchemesMap& schemesForNonNullOrigins()

Why call this NonNullOrigins when the code below calls it "TupleSecurityOrigin"?

> Source/WebCore/platform/SchemeRegistry.h:52
> -    WEBCORE_EXPORT static void registerURLSchemeAsNoAccess(const String&);
> -    static bool shouldTreatURLSchemeAsNoAccess(const String&);
> +    WEBCORE_EXPORT static void registerURLSchemeForTupleSecurityOrigin(const String&);
> +    static bool schemeShouldReceiveTupleSecurityOrigin(const String&);

GTK port was using this, needs to be fixed.

> LayoutTests/http/tests/security/postMessage/invalid-origin-throws-exception.html:32
> +    // URLs without an origin should fail witha syntax error.

Missing space between "with" and "a".

[Alex Christensen]

(In reply to comment #37)
> Comment on attachment 291698 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291698&action=review
> 
> I’d like someone else, Anders maybe, to review the new SPI parts of this. I
> reviewed the rest.
Anders said SchemeRegistry should be per Page instead of global.  Then we could put this new API on WKWebView.  I'm not sure if we can get away with a change like this without doing that.
> 
> > Source/WebCore/page/SecurityOrigin.cpp:107
> > +    if (innerURL.protocolIsInHTTPFamily()
> > +        || innerURL.protocolIs("file")
> > +        || innerURL.protocolIs("ftp")
> > +        || innerURL.protocolIs("gopher")
> > +        || innerURL.protocolIs("ws")
> > +        || innerURL.protocolIs("wss"))
> 
> Do we have this list anywhere else? I would have expected isSpecialProtocol
> instead of this list written out like this.
We do!  We should make URL::protocol return a StringView then use the same scheme function from URLParser.cpp.
> 
> > Source/WebCore/platform/SchemeRegistry.cpp:77
> > +static URLSchemesMap& schemesForNonNullOrigins()
> 
> Why call this NonNullOrigins when the code below calls it
> "TupleSecurityOrigin"?
Oh, this was from before I decided on a name.  I decided on "tuple" based on the spec, but I'm not sure if that's a good name.
> 
> > Source/WebCore/platform/SchemeRegistry.h:52
> > -    WEBCORE_EXPORT static void registerURLSchemeAsNoAccess(const String&);
> > -    static bool shouldTreatURLSchemeAsNoAccess(const String&);
> > +    WEBCORE_EXPORT static void registerURLSchemeForTupleSecurityOrigin(const String&);
> > +    static bool schemeShouldReceiveTupleSecurityOrigin(const String&);
> 
> GTK port was using this, needs to be fixed.
> 
> > LayoutTests/http/tests/security/postMessage/invalid-origin-throws-exception.html:32
> > +    // URLs without an origin should fail witha syntax error.
> 
> Missing space between "with" and "a".

[Alex Christensen]

Created attachment 413444 [details]
Patch

[Alex Christensen]

Comment on attachment 413444 [details]
Patch

Looks like I do need to organize this per-page and inform the network process by adding to NetworkSchemeRegistry

[Alex Christensen]

Just kidding.  I need to add blob to the list according to https://url.spec.whatwg.org/#origin

[Alex Christensen]

Created attachment 413460 [details]
Patch

[Alex Christensen]

Created attachment 413473 [details]
Patch

[Alex Christensen]

Created attachment 413505 [details]
Patch

[Alex Christensen]

Created attachment 413696 [details]
Patch

[Alex Christensen]

Created attachment 413721 [details]
Patch

[Devin Rousso]

Comment on attachment 413721 [details]
Patch

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

> LayoutTests/inspector/unit-tests/url-utilities.html:202
> -                origin: "app-specific://example.com",
> +                origin: "null",

I think this will modify the behavior of Web Inspector.  Web Inspector should always show the origin of URLs.  As-is, I think this change would cause all custom-origin URLs to be shown at the top-level of the navigation sidebar in the Sources Tab instead of being organized underneath a tree element for that custom-origin.

This would likely require minor adjustments to `parseURL` in `Source/WebInspectorUI/UserInterface/Base/URLUtilities.js`.

[Alex Christensen]

Devin, could you look into what problems that would cause?  I made it so if there are WKURLSchemeHandlers registered, it allows non-null origins.  I believe that should prevent such problems in practice.  If someone has a URL with an unrecognized scheme that has no scheme handler, then the origin should change to become null.  I'm open to learning what problems that may cause.

[Devin Rousso]

Created attachment 413731 [details]
[Image] screenshot of current behavior of custom origin in Web Inspector

(In reply to Alex Christensen from comment #48)
> Devin, could you look into what problems that would cause?  I made it so if there are WKURLSchemeHandlers registered, it allows non-null origins.  I believe that should prevent such problems in practice.  If someone has a URL with an unrecognized scheme that has no scheme handler, then the origin should change to become null.  I'm open to learning what problems that may cause.

Attached an example screenshot of what a custom origin looks like right now in Web Inspector.  You can reproduce this yourself by
1. inspecting <https://webkit.org/>
2. switching to "By Path" in the Sources Tab
3. evaluating `(new Image).src = "app-specific://example.com"` in the Web Inspector console

If the `origin` of the object returned by `parseURL` is `null`, then I think that would instead appear at the top-level instead of nested underneath a tree element for "app-specific://example.com".

FWIW, I'm not sure how often this will actually happen and/or be an issue, because as you said this only affects custom origins that do not have a scheme handler.  And even then, I think the only problem it would cause is that the resource is not placed in the same spot it was before.

It may be worth leaving the test as-is and just changing the expected file to `FAIL` with a followup bug for adjusting Web Inspector so that it can still recognize the origin.

[Alex Christensen]

I think that's an intentional change in behavior, and I don't think any followup is necessary with the web inspector.  I think the test correctly reflects that if you need the origin of an unrecognized scheme it will now be null.  I have added an API test that verifies that if a scheme handler is registered in the app, the origin will continue to be non-null.  If you feel strongly that I should leave the inspector test as failing and file another bug I will, but I don't think it's necessary.

[Devin Rousso]

(In reply to Alex Christensen from comment #50)
> I think that's an intentional change in behavior, and I don't think any followup is necessary with the web inspector.  I think the test correctly reflects that if you need the origin of an unrecognized scheme it will now be null.
I agree that as far as JavaScript is concerned, this is an intentional change in behavior, but for the purposes of Web Inspector as a developer tool, we should always show the origin even when it's hidden from JavaScript.  As an example, if a page tries to load `a://example.com/foo.js` and `b://example.com/foo.js`, they should not appear under the same top-level item when "By Path".  Web Inspector intentionally displays data that is hidden from JavaScript/native APIs all the time, as that's one of the reasons why it exists in the first place.

> I have added an API test that verifies that if a scheme handler is registered in the app, the origin will continue to be non-null.
Awesome!  That's I think why this is likely to be an extremely rare issue in production.

> If you feel strongly that I should leave the inspector test as failing and file another bug I will, but I don't think it's necessary.
Eh, I don't have a strong opinion one way or the other how you leave the test, but I do think this warrants a followup bug (and maybe a FIXME comment in the test) to ensure that this change in behavior does not negatively affect Web Inspector.  Maybe something like "Web Inspector: ensure that `parseURL` is able to derive an origin for non-registered schemes"?

[Alex Christensen]

The whole point of this is that neither Web Inspector nor web content should be able to derive an origin for non-registered schemes.  Only registered schemes should be able to be an origin.

[Devin Rousso]

(In reply to Alex Christensen from comment #52)
> The whole point of this is that neither Web Inspector nor web content should be able to derive an origin for non-registered schemes.  Only registered schemes should be able to be an origin.

To clarify something, I'm not suggesting that you modify `URL` when in Web Inspector to allow the `origin` for non-registered schemes.  Web Inspector should not be given any special functionality unless absolutely necessary.  I'm suggesting that the `parseURL` function that Web Inspector has defined be modified to derive the origin on its own.  I agree that as far as the JavaScript `URL` is concerned, the origin should not be shown for non-registered schemes.

The purpose of Web Inspector is to expose information to the developer that aids them in introspecting/debugging their web content.  This often takes the form of exposing information that is not normally available via web APIs.  If a developer has written code for `app-specific://example.com`, Web Inspector should show the information as it was actually used in the engine.

Also, even with your patch I believe Web Inspector will still show the custom scheme in other places like the table in the Network Tab (right click on the header and check Scheme).  Those places follow different code paths, so they are likely unaffected by your changes to `URL`.  I'm just asking that Web Inspector be consistent in what information it shows regardless of where it gets the data (which usually involves Web Inspector doing extra work on top of what is exposed via web APIs, which is one of the reasons why the Web Inspector protocol exists).  And to be clear, this is not something that needs to be changed in WebCore/WebKit/etc., it's purely something that Web Inspector will need to do on its own (`parseURL` inside `Source/WebInspectorUI/UserInterface/Base/URLUtilities.js`).

[Alex Christensen]

I filed https://bugs.webkit.org/show_bug.cgi?id=218780

[Alex Christensen]

Created attachment 413940 [details]
Patch

[Alex Christensen]

Comment on attachment 413940 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:136
> +    if (parsed.origin && parsed.origin != "null")

I did this tweak to parseURL instead, which preserves the WebInspector behavior.

[Devin Rousso]

Comment on attachment 413940 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:136
>> +    if (parsed.origin && parsed.origin != "null")
> 
> I did this tweak to parseURL instead, which preserves the WebInspector behavior.

NIT: this should be `!==`

Thanks for fixing this! :)

[Alex Christensen]

(In reply to Devin Rousso from comment #57)
> NIT: this should be `!==`
Will tweak when landing.

[Geoffrey Garen]

Comment on attachment 413940 [details]
Patch

r=me with the !== change

[Alex Christensen]

Created attachment 419525 [details]
Patch

[EWS]

Committed r272469: <https://commits.webkit.org/r272469>

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

[Radar WebKit Bug Importer]

<rdar://problem/74068207>

[Michael Catanzaro]

Hm, I think this caused Epiphany to crash when attempting to download any PDF. webkit_security_origin_new_for_uri() returns NULL for ephy-pdf: URIs, but the return value is not nullable, so Epiphany correctly dereferences it and crashes.

"""
In order to allow things like web extensions to continue to work, we allow non-null origins for schemes for which a WKURLSchemeHandler has been registered. We learned this lesson 4 years ago when we tried this change. This also makes sense conceptually because those schemes will be handled by the containing application, so they can be an "origin" for a page.
"""

That sounds good to me, Epiphany registers ephy-pdf: using webkit_web_context_register_uri_scheme(), so in theory it ought to work, but it doesn't. I'll look closer on Monday.

[Michael Catanzaro]

Bug #222943