Bug 189057

Summary: [Curl] Stop sending request with credential if no authorization requested.
Product: WebKit Reporter: Basuke Suzuki <Basuke.Suzuki>
Component: PlatformAssignee: Basuke Suzuki <Basuke.Suzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, Basuke.Suzuki, commit-queue, darin, ews-watchlist, galpeter, Hironori.Fujii, pvollan, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
PATCH
achristensen: review-, achristensen: commit-queue-
PATCH
none
Fix tests to be run after onload. none

Description Basuke Suzuki 2018-08-28 14:32:27 PDT
When 401 response returns without 'www-authenticate' header, suppress another request with credential.
Comment 1 Basuke Suzuki 2018-08-28 15:14:44 PDT
Created attachment 348350 [details]
PATCH
Comment 2 Fujii Hironori 2018-09-04 20:51:11 PDT
Comment on attachment 348350 [details]
PATCH

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

> LayoutTests/ChangeLog:9
> +        * http/tests/xmlhttprequest/unauthorized-without-authenticate-header.php: Added.

I can't understand what does this test case do.
Why don't you check whether Authorization header is sent?

> LayoutTests/http/tests/xmlhttprequest/unauthorized-without-authenticate-header.php:21
> +    req.open("GET", "<?php echo basename(__FILE__) . '?auth=1'; ?>", false);

I prefer using JS API to get current URL instead of PHP.
window.location.href
Comment 3 Basuke Suzuki 2018-09-04 23:54:05 PDT
(In reply to Fujii Hironori from comment #2)
> Comment on attachment 348350 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348350&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        * http/tests/xmlhttprequest/unauthorized-without-authenticate-header.php: Added.
> 
> I can't understand what does this test case do.
> Why don't you check whether Authorization header is sent?

The browser behaves differently whether authorization exists or not.

> 
> > LayoutTests/http/tests/xmlhttprequest/unauthorized-without-authenticate-header.php:21
> > +    req.open("GET", "<?php echo basename(__FILE__) . '?auth=1'; ?>", false);
> 
> I prefer using JS API to get current URL instead of PHP.
> window.location.href

I don't agree with that, but I can change as you wish if that is the requirement of reviewing.
Comment 4 Fujii Hironori 2018-09-05 20:10:10 PDT
(In reply to Basuke Suzuki from comment #3)
> > I can't understand what does this test case do.
> > Why don't you check whether Authorization header is sent?
> 
> The browser behaves differently whether authorization exists or not.

I don't understand this.
Authorization is a request header sent by clients (browsers).

> > > LayoutTests/http/tests/xmlhttprequest/unauthorized-without-authenticate-header.php:21
> > > +    req.open("GET", "<?php echo basename(__FILE__) . '?auth=1'; ?>", false);
> > 
> > I prefer using JS API to get current URL instead of PHP.
> > window.location.href
> 
> I don't agree with that, but I can change as you wish if that is the
> requirement of reviewing.

Mixing JS and PHP makes code ugly. 
I think you can write as following in this case.
> req.open("GET", '?auth=1', false);
Comment 5 Basuke Suzuki 2018-09-06 11:08:29 PDT
(In reply to Fujii Hironori from comment #4)
> (In reply to Basuke Suzuki from comment #3)
> > > I can't understand what does this test case do.
> > > Why don't you check whether Authorization header is sent?
> > 
> > The browser behaves differently whether authorization exists or not.
> 
> I don't understand this.
> Authorization is a request header sent by clients (browsers).



> 
> > > > LayoutTests/http/tests/xmlhttprequest/unauthorized-without-authenticate-header.php:21
> > > > +    req.open("GET", "<?php echo basename(__FILE__) . '?auth=1'; ?>", false);
> > > 
> > > I prefer using JS API to get current URL instead of PHP.
> > > window.location.href
> > 
> > I don't agree with that, but I can change as you wish if that is the
> > requirement of reviewing.
> 
> Mixing JS and PHP makes code ugly. 
> I think you can write as following in this case.
> > req.open("GET", '?auth=1', false);

(In reply to Fujii Hironori from comment #4)
> (In reply to Basuke Suzuki from comment #3)
> > > I can't understand what does this test case do.
> > > Why don't you check whether Authorization header is sent?
> > 
> > The browser behaves differently whether authorization exists or not.
> 
> I don't understand this.
> Authorization is a request header sent by clients (browsers).


My bad. "authenticate" header. And it seems test file was missing with actual check. It was there and gone. I don't know why, but I'm fixing this.


> 
> > > > LayoutTests/http/tests/xmlhttprequest/unauthorized-without-authenticate-header.php:21
> > > > +    req.open("GET", "<?php echo basename(__FILE__) . '?auth=1'; ?>", false);
> > > 
> > > I prefer using JS API to get current URL instead of PHP.
> > > window.location.href
> > 
> > I don't agree with that, but I can change as you wish if that is the
> > requirement of reviewing.
> 
> Mixing JS and PHP makes code ugly. 
> I think you can write as following in this case.
> > req.open("GET", '?auth=1', false);

Okay.
Comment 6 Alex Christensen 2018-09-06 11:26:32 PDT
Comment on attachment 348350 [details]
PATCH

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

>>> LayoutTests/http/tests/xmlhttprequest/unauthorized-without-authenticate-header.php:21
>>> +    req.open("GET", "<?php echo basename(__FILE__) . '?auth=1'; ?>", false);
>> 
>> I prefer using JS API to get current URL instead of PHP.
>> window.location.href
> 
> I don't agree with that, but I can change as you wish if that is the requirement of reviewing.

I agree with Fujii, and please don't use synchronous xmlhttprequest unless you're explicitly testing synchronous behavior, which you're not.
Comment 7 Basuke Suzuki 2018-09-06 12:30:40 PDT
Created attachment 349053 [details]
PATCH

Stop using sync test.
Separate sub resources.
Add more detail for this test purpose.
Comment 8 Fujii Hironori 2018-09-06 20:55:25 PDT
Comment on attachment 349053 [details]
PATCH

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

> LayoutTests/http/tests/xmlhttprequest/unauthorized-without-authenticate-header.html:11
> +            shouldBe("xhr.status", "401");

Do you expect this check fail unless this patch is applied?
I confirmed this says "PASS xhr.status is 401" even without applying this change.
Comment 9 Basuke Suzuki 2018-09-07 15:08:14 PDT
Comment on attachment 349053 [details]
PATCH

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

>> LayoutTests/http/tests/xmlhttprequest/unauthorized-without-authenticate-header.html:11
>> +            shouldBe("xhr.status", "401");
> 
> Do you expect this check fail unless this patch is applied?
> I confirmed this says "PASS xhr.status is 401" even without applying this change.

That's okay. The server script returns 401, so the result is expected. What I want to check here is that the response is 401 and no authentication challenge should be invoked.

Latter is checked by the absence of 'http://127.0.0.1:8000/xmlhttprequest/resources/no-authenticate-header-401.php - didReceiveAuthenticationChallenge - Simulating cancelled authentication sheet' debug message.
.
Comment 10 Basuke Suzuki 2018-09-07 15:23:53 PDT
Created attachment 349204 [details]
Fix tests to be run after onload.
Comment 11 WebKit Commit Bot 2018-09-07 17:32:20 PDT
Comment on attachment 349204 [details]
Fix tests to be run after onload.

Clearing flags on attachment: 349204

Committed r235821: <https://trac.webkit.org/changeset/235821>
Comment 12 WebKit Commit Bot 2018-09-07 17:32:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-09-07 17:33:42 PDT
<rdar://problem/44245852>