Bug 182579 – Disallow cross-origin subresources from asking for credentials

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

[Daniel Bates]

Disallow cross-origin subresources from asking for credentials

[Daniel Bates]

<rdar://problem/36162271>

[Daniel Bates]

Created attachment 333316 [details]
Patch and layout tests

[Daniel Bates]

Created attachment 333318 [details]
Patch and layout tests

[Daniel Bates]

Comment on attachment 333318 [details]
Patch and layout tests

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

> LayoutTests/http/tests/security/mixedContent/secure-redirect-to-secure-redirect-to-basic-auth-secure-image.https-expected.txt:7
> -PASS did load image.
> +FAIL did not load image.

This is expected since the image is cross origin with the origin of the page and hence it will be blocked from loading. Will change test to print PASS instead of FAIL.

[Daniel Bates]

Created attachment 333321 [details]
Patch and layout tests

[EWS Watchlist]

Comment on attachment 333321 [details]
Patch and layout tests

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

New failing tests:
http/tests/security/mixedContent/secure-redirect-to-secure-redirect-to-basic-auth-secure-image.https.html

[EWS Watchlist]

Created attachment 333336 [details]
Archive of layout-test-results from ews101 for mac-sierra

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

[Daniel Bates]

Created attachment 333337 [details]
Patch and layout tests

[EWS Watchlist]

Comment on attachment 333337 [details]
Patch and layout tests

Attachment 333337 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6407424

New failing tests:
http/tests/security/basic-auth-subresource.html
http/tests/security/credentials-iframes-allowCrossOriginSubresourcesToAskForCredentials.html

[EWS Watchlist]

Created attachment 333350 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Daniel Bates]

Created attachment 333396 [details]
Patch and layout tests

Add Windows-specific results for http/tests/security/basic-auth-subresource.html due to <https://bugs.webkit.org/show_bug.cgi?id=182609>. Skip allowCrossOriginSubresourcesToAskForCredentials-suffixed tests on Windows since DumpRenderTree does not support parsing test options. See <https://bugs.webkit.org/show_bug.cgi?id=17328> for more details.

[Daniel Bates]

(In reply to Daniel Bates from comment #11)
> [...] See <https://bugs.webkit.org/show_bug.cgi?id=17328> for more details.

*bug #173281

[Daniel Bates]

Created attachment 333398 [details]
Patch and layout tests

[Daniel Bates]

Comment on attachment 333398 [details]
Patch and layout tests

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

> Source/WebCore/ChangeLog:18
> +

Before landing I will add a remark of the form:

Since r224134 WebKit has disallowed mixed content subresources from asking for credentials. Let's go further and forbid all cross-origin subresources from asking for credentials.

[Andy Estes]

Comment on attachment 333398 [details]
Patch and layout tests

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

> Source/WebCore/loader/ResourceLoader.h:190
> +    bool canRequestAskUserForCredentials() const;

Can this just be called canAskUserForCredentials()? The first time I read this, I interpreted request as a verb and got confused. I get that we're basing the decision on the request URL, but maybe that fact doesn't need to be in the function signature. r=me either way.

[Daniel Bates]

(In reply to Andy Estes from comment #15)
> Comment on attachment 333398 [details]
> Patch and layout tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333398&action=review
> 
> > Source/WebCore/loader/ResourceLoader.h:190
> > +    bool canRequestAskUserForCredentials() const;
> 
> Can this just be called canAskUserForCredentials()? The first time I read
> this, I interpreted request as a verb and got confused. I get that we're
> basing the decision on the request URL, but maybe that fact doesn't need to
> be in the function signature. r=me either way.

I was planning to rename ResourceLoader::isAllowedToAskUserForCredentials() to canAskUserForCredentials() in a subsequent patch. I hope you do not mind that I rename canRequestAskUserForCredentials() to shouldBlockCrossOriginAuthenticationChallenge() before landing.

[Daniel Bates]

(In reply to Daniel Bates from comment #16)
> (In reply to Andy Estes from comment #15)
> > Comment on attachment 333398 [details]
> > Patch and layout tests
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=333398&action=review
> > 
> > > Source/WebCore/loader/ResourceLoader.h:190
> > > +    bool canRequestAskUserForCredentials() const;
> > 
> > Can this just be called canAskUserForCredentials()? The first time I read
> > this, I interpreted request as a verb and got confused. I get that we're
> > basing the decision on the request URL, but maybe that fact doesn't need to
> > be in the function signature. r=me either way.
> 
> I was planning to rename ResourceLoader::isAllowedToAskUserForCredentials()
> to canAskUserForCredentials() in a subsequent patch. I hope you do not mind
> that I rename canRequestAskUserForCredentials() to
> shouldBlockCrossOriginAuthenticationChallenge() before landing.

Or maybe shouldAllowResourceToAskForCredentials?

[Daniel Bates]

Committed r228486: <https://trac.webkit.org/changeset/228486>