Bug 159085 – Remove didFailRedirectCheck ThreadableLoaderClient callback

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

To implement fetch, it is convenient to add CORS checks below DocumentThreadableLoader, typically in Cached loaders and SubresourceLoader.
This loading code does only know didFail and not didFailRedirectCheck.

It would be convenient to remove didFailRedirectCheck and use didFailAccessControlCheck or didFail directly.

[youenn fablet]

Created attachment 281966 [details]
Patch

[Daniel Bates]

Comment on attachment 281966 [details]
Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:202
> +        m_client->didFailAccessControlCheck(ResourceError(errorDomainWebKitInternal, 0, redirectResponse.url(), "Failed CSP redirect check."));

If we choose to have a custom error message (*) then another way to express this error is "Cross-origin redirection denied by Content Security Policy." This wording is similar to the wording we use when we deny a cross-origin redirection in SubresourceLoader::checkCrossOriginAccessControl() (**).

(*) Notice that CSP will emit its own "Refused to ..." error message as can be seen in LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-eventsource-redirect-to-blocked-expected.txt.
(**) <http://trac.webkit.org/browser/trunk/Source/WebCore/loader/SubresourceLoader.cpp?rev=202431#L409>

> Source/WebCore/loader/DocumentThreadableLoader.cpp:248
> +    m_client->didFailAccessControlCheck(ResourceError(errorDomainWebKitInternal, 0, redirectResponse.url(), "Failed redirection access control check."));

Maybe we should use similar wording as we do in SubresourceLoader::checkCrossOriginAccessControl() (**), say "Cross-origin redirection denied by Cross-Origin Resource Sharing policy."?

> Source/WebCore/loader/DocumentThreadableLoader.cpp:385
> +        m_client->didFailAccessControlCheck(ResourceError(errorDomainWebKitInternal, 0, requestURL, "Failed redirection access control check."));

Another way to describe this error is "Cross-origin redirection denied." Alternatively we could emit independent error messages when !isAllowedByContentSecurityPolicy() evaluates to true (*) and !isAllowedRedirect() evaluates to true. If we did this, then I suggest that we use the same error message as we in DocumentThreadableLoader::redirectReceived() (commented above) when !isAllowedRedirect() evaluates to true and emit a similar worded error message for CSP when !isAllowedByContentSecurityPolicy() evaluates to true, say "Cross-origin redirection denied by Content Security Policy."

[youenn fablet]

Comment on attachment 281966 [details]
Patch

Thanks for the review.
See inline.

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

>> Source/WebCore/loader/DocumentThreadableLoader.cpp:202
>> +        m_client->didFailAccessControlCheck(ResourceError(errorDomainWebKitInternal, 0, redirectResponse.url(), "Failed CSP redirect check."));
> 
> If we choose to have a custom error message (*) then another way to express this error is "Cross-origin redirection denied by Content Security Policy." This wording is similar to the wording we use when we deny a cross-origin redirection in SubresourceLoader::checkCrossOriginAccessControl() (**).
> 
> (*) Notice that CSP will emit its own "Refused to ..." error message as can be seen in LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-eventsource-redirect-to-blocked-expected.txt.
> (**) <http://trac.webkit.org/browser/trunk/Source/WebCore/loader/SubresourceLoader.cpp?rev=202431#L409>

At first I thought that one message was good enough.
But having two allows to be more precise on which client (event source in the test case) fails, which might be useful for web developers.
Ideally, we might want one message that combines both information, like for cars check.

OK for changing the error string.

>> Source/WebCore/loader/DocumentThreadableLoader.cpp:248
>> +    m_client->didFailAccessControlCheck(ResourceError(errorDomainWebKitInternal, 0, redirectResponse.url(), "Failed redirection access control check."));
> 
> Maybe we should use similar wording as we do in SubresourceLoader::checkCrossOriginAccessControl() (**), say "Cross-origin redirection denied by Cross-Origin Resource Sharing policy."?

OK.

Note that the future plan is to remove those checks from DocumentThreadableLoader.
SubresourceLoader should be made responsible for those checks and DocumentThreadableLoader should just use regular ResourceLoader options to activate or not those checks.

>> Source/WebCore/loader/DocumentThreadableLoader.cpp:385
>> +        m_client->didFailAccessControlCheck(ResourceError(errorDomainWebKitInternal, 0, requestURL, "Failed redirection access control check."));
> 
> Another way to describe this error is "Cross-origin redirection denied." Alternatively we could emit independent error messages when !isAllowedByContentSecurityPolicy() evaluates to true (*) and !isAllowedRedirect() evaluates to true. If we did this, then I suggest that we use the same error message as we in DocumentThreadableLoader::redirectReceived() (commented above) when !isAllowedRedirect() evaluates to true and emit a similar worded error message for CSP when !isAllowedByContentSecurityPolicy() evaluates to true, say "Cross-origin redirection denied by Content Security Policy."

I thought of that but was not sure how useful that was.
I'll make that consistent with the async case then.

[youenn fablet]

Created attachment 282108 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 282108 [details]
Patch for landing

Clearing flags on attachment: 282108

Committed r202480: <http://trac.webkit.org/changeset/202480>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.