Bug 182579 - Disallow cross-origin subresources from asking for credentials
Summary: Disallow cross-origin subresources from asking for credentials
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-07 13:40 PST by Daniel Bates
Modified: 2018-02-14 14:28 PST (History)
9 users (show)

See Also:


Attachments
Patch and layout tests (91.46 KB, patch)
2018-02-07 14:53 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (91.69 KB, patch)
2018-02-07 15:06 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (92.59 KB, patch)
2018-02-07 15:17 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.19 MB, application/zip)
2018-02-07 16:37 PST, Build Bot
no flags Details
Patch and layout tests (92.57 KB, patch)
2018-02-07 16:41 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (11.49 MB, application/zip)
2018-02-07 18:57 PST, Build Bot
no flags Details
Patch and layout tests (94.33 KB, patch)
2018-02-08 11:43 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (94.33 KB, patch)
2018-02-08 11:45 PST, Daniel Bates
aestes: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-02-07 13:40:08 PST
Disallow cross-origin subresources from asking for credentials
Comment 1 Daniel Bates 2018-02-07 13:41:03 PST
<rdar://problem/36162271>
Comment 2 Daniel Bates 2018-02-07 14:53:01 PST
Created attachment 333316 [details]
Patch and layout tests
Comment 3 Daniel Bates 2018-02-07 15:06:50 PST
Created attachment 333318 [details]
Patch and layout tests
Comment 4 Daniel Bates 2018-02-07 15:12:04 PST
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.
Comment 5 Daniel Bates 2018-02-07 15:17:53 PST
Created attachment 333321 [details]
Patch and layout tests
Comment 6 Build Bot 2018-02-07 16:37:11 PST Comment hidden (obsolete)
Comment 7 Build Bot 2018-02-07 16:37:12 PST Comment hidden (obsolete)
Comment 8 Daniel Bates 2018-02-07 16:41:14 PST
Created attachment 333337 [details]
Patch and layout tests
Comment 9 Build Bot 2018-02-07 18:57:38 PST Comment hidden (obsolete)
Comment 10 Build Bot 2018-02-07 18:57:48 PST Comment hidden (obsolete)
Comment 11 Daniel Bates 2018-02-08 11:43:14 PST
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.
Comment 12 Daniel Bates 2018-02-08 11:45:31 PST
(In reply to Daniel Bates from comment #11)
> [...] See <https://bugs.webkit.org/show_bug.cgi?id=17328> for more details.

*bug #173281
Comment 13 Daniel Bates 2018-02-08 11:45:58 PST
Created attachment 333398 [details]
Patch and layout tests
Comment 14 Daniel Bates 2018-02-09 14:51:26 PST
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.
Comment 15 Andy Estes 2018-02-14 10:47:27 PST
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.
Comment 16 Daniel Bates 2018-02-14 11:10:15 PST
(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.
Comment 17 Daniel Bates 2018-02-14 11:15:20 PST
(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?
Comment 18 Daniel Bates 2018-02-14 14:28:03 PST
Committed r228486: <https://trac.webkit.org/changeset/228486>