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
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
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
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.
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!
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
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
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
(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.
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
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
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
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
(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.
(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.
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.
(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.
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".
(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".
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
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`.
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.
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.
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.
(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"?
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.
(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`).
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! :)
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.
2016-09-19 22:00 PDT, Alex Christensen
2016-09-19 22:19 PDT, Build Bot
2016-09-19 22:37 PDT, Build Bot
2016-09-19 22:37 PDT, Build Bot
2016-09-20 08:11 PDT, Alex Christensen
2016-09-20 08:51 PDT, Build Bot
2016-09-20 08:57 PDT, Build Bot
2016-09-20 08:59 PDT, Build Bot
2016-09-20 09:01 PDT, Alex Christensen
2016-09-20 09:50 PDT, Build Bot
2016-09-20 09:59 PDT, Build Bot
2016-09-20 10:04 PDT, Build Bot
2016-09-20 10:11 PDT, Build Bot
2016-09-20 10:59 PDT, Alex Christensen
2016-10-14 18:47 PDT, Alex Christensen
2020-11-06 10:32 PST, Alex Christensen
2020-11-06 12:07 PST, Alex Christensen
2020-11-06 13:19 PST, Alex Christensen
2020-11-06 19:35 PST, Alex Christensen
2020-11-10 07:51 PST, Alex Christensen
2020-11-10 12:09 PST, Alex Christensen
2020-11-10 13:34 PST, Devin Rousso
2020-11-12 09:11 PST, Alex Christensen
2021-02-06 20:56 PST, Alex Christensen