Summary: | [Curl] Stop sending request with credential if no authorization requested. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Basuke Suzuki <Basuke.Suzuki> | ||||||||
Component: | Platform | Assignee: | 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
Basuke Suzuki
2018-08-28 14:32:27 PDT
Created attachment 348350 [details]
PATCH
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 (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. (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). > > > > > 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 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. Created attachment 349053 [details]
PATCH
Stop using sync test.
Separate sub resources.
Add more detail for this test purpose.
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 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. . Created attachment 349204 [details]
Fix tests to be run after onload.
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> All reviewed patches have been landed. Closing bug. |