Bug 189057 – [Curl] Stop sending request with credential if no authorization requested.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Basuke Suzuki]

When 401 response returns without 'www-authenticate' header, suppress another request with credential.

[Basuke Suzuki]

Created attachment 348350 [details]
PATCH

[Fujii Hironori]

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

[Basuke Suzuki]

(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.

[Fujii Hironori]

(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);

[Basuke Suzuki]

(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.

[Alex Christensen]

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.

[Basuke Suzuki]

Created attachment 349053 [details]
PATCH

Stop using sync test.
Separate sub resources.
Add more detail for this test purpose.

[Fujii Hironori]

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.

[Basuke Suzuki]

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.
.

[Basuke Suzuki]

Created attachment 349204 [details]
Fix tests to be run after onload.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44245852>