Bug 189318 - [Curl] Respond with requested authentication scheme for authentication challenge.
Summary: [Curl] Respond with requested authentication scheme for authentication challe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on: 189601
Blocks:
  Show dependency treegraph
 
Reported: 2018-09-05 14:58 PDT by Basuke Suzuki
Modified: 2018-09-17 20:28 PDT (History)
10 users (show)

See Also:


Attachments
PATCH (17.43 KB, patch)
2018-09-11 16:24 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
FIX (17.43 KB, patch)
2018-09-12 13:15 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
PATCH (23.73 KB, patch)
2018-09-14 16:34 PDT, Basuke Suzuki
achristensen: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
PATCH (24.56 KB, patch)
2018-09-17 10:42 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2018-09-05 14:58:51 PDT
Curl port depends on libcurl's authentication handling by enabling CURLAUTH_ANY. With this mode, the round-trip communication between the client and the server is handled by libcurl internally. That's okay for many cases. But when initial request has a credentials (i.e. XMLHttpRequest), there's no valid chance to store credential to the storage because the returned response is not 401.
Comment 1 Basuke Suzuki 2018-09-11 16:24:16 PDT
Created attachment 349489 [details]
PATCH
Comment 2 Basuke Suzuki 2018-09-11 16:26:56 PDT
Comment on attachment 349489 [details]
PATCH

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

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:335
> +std::optional<Credential> ResourceHandle::getCredential(const ResourceRequest& request, bool redirect)

These changes follow the Mac port implementation.

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:403
> +    }

To set the default authentication in the credential storage.
Comment 3 Alex Christensen 2018-09-11 19:09:35 PDT
Comment on attachment 349489 [details]
PATCH

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

Do we need to do something similar for NetworkDataTask?

> LayoutTests/ChangeLog:3
> +        [Curl] Response with collect authentication scheme for authentication challenge.

This title doesn't make sense.
Comment 4 Basuke Suzuki 2018-09-12 10:58:23 PDT
(In reply to Alex Christensen from comment #3)
> Comment on attachment 349489 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349489&action=review
> 
> Do we need to do something similar for NetworkDataTask?

Yes, but we don't get ready for WebKitTestRunner for WinCairo yet. I'm not comfortable to fix the behavior without testing because these sequence is complicated. As my first comment mentioned, the behavior is almost okay without this fix. After our effort to the WebKitTestRunner finish, we will port this to NetworkDataTask. That's my idea.

> > LayoutTests/ChangeLog:3
> > +        [Curl] Response with collect authentication scheme for authentication challenge.
> 
> This title doesn't make sense.

How about new one?
"Respond with requested authentication scheme for authentication challenge."
Comment 5 Basuke Suzuki 2018-09-12 13:15:59 PDT
Created attachment 349575 [details]
FIX
Comment 6 Alex Christensen 2018-09-12 14:00:00 PDT
Comment on attachment 349575 [details]
FIX

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

I think it's a bad idea to change ResourceHandle but not NetworkDataTask.

> Source/WebCore/ChangeLog:15
> +        - http/tests/websocket/tests/hybi/httponly-cookie.pl [ Failure ]

No need for [ Failure ] here.
Comment 7 Basuke Suzuki 2018-09-12 14:07:54 PDT
(In reply to Alex Christensen from comment #6)
> Comment on attachment 349575 [details]
> FIX
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349575&action=review
> 
> I think it's a bad idea to change ResourceHandle but not NetworkDataTask.

But fixing without testing is also bad. Okay, I do manual test using MiniBrowser for NDT.
Comment 8 Basuke Suzuki 2018-09-14 16:34:43 PDT
Created attachment 349828 [details]
PATCH

Add NetworkDataTask implementation.
Comment 9 WebKit Commit Bot 2018-09-14 17:21:17 PDT
Comment on attachment 349828 [details]
PATCH

Rejecting attachment 349828 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 349828, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=349828&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=189318&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 349828 from bug 189318.
Fetching: https://bugs.webkit.org/attachment.cgi?id=349828
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Alex Christensen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 12 diffs from patch file(s).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/wincairo/TestExpectations
Hunk #1 FAILED at 915.
Hunk #2 FAILED at 935.
2 out of 2 hunks FAILED -- saving rejects to file LayoutTests/platform/wincairo/TestExpectations.rej
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/platform/network/ResourceHandle.h
patching file Source/WebCore/platform/network/curl/CurlContext.cpp
patching file Source/WebCore/platform/network/curl/CurlContext.h
patching file Source/WebCore/platform/network/curl/CurlRequest.cpp
patching file Source/WebCore/platform/network/curl/CurlRequest.h
patching file Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
patching file Source/WebKit/ChangeLog
patching file Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp
patching file Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Alex Christensen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/9222335
Comment 10 WebKit Commit Bot 2018-09-17 10:15:45 PDT
Comment on attachment 349828 [details]
PATCH

Rejecting attachment 349828 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 349828, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=349828&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=189318&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 349828 from bug 189318.
Fetching: https://bugs.webkit.org/attachment.cgi?id=349828
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Alex Christensen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 12 diffs from patch file(s).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/wincairo/TestExpectations
Hunk #1 FAILED at 915.
Hunk #2 FAILED at 935.
2 out of 2 hunks FAILED -- saving rejects to file LayoutTests/platform/wincairo/TestExpectations.rej
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/platform/network/ResourceHandle.h
patching file Source/WebCore/platform/network/curl/CurlContext.cpp
patching file Source/WebCore/platform/network/curl/CurlContext.h
patching file Source/WebCore/platform/network/curl/CurlRequest.cpp
Hunk #2 succeeded at 211 (offset 3 lines).
patching file Source/WebCore/platform/network/curl/CurlRequest.h
Hunk #1 succeeded at 34 with fuzz 1.
Hunk #2 succeeded at 68 with fuzz 2 (offset 1 line).
Hunk #3 succeeded at 167 (offset 2 lines).
patching file Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
Hunk #1 succeeded at 85 with fuzz 2 (offset 2 lines).
Hunk #2 succeeded at 233 (offset 3 lines).
Hunk #3 succeeded at 258 (offset 3 lines).
Hunk #4 succeeded at 293 (offset 3 lines).
Hunk #5 succeeded at 335 (offset 3 lines).
Hunk #6 succeeded at 352 (offset 3 lines).
Hunk #7 FAILED at 373.
Hunk #8 FAILED at 399.
Hunk #9 succeeded at 524 with fuzz 2 (offset 7 lines).
2 out of 9 hunks FAILED -- saving rejects to file Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp.rej
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp
Hunk #1 FAILED at 62.
Hunk #2 FAILED at 307.
Hunk #3 succeeded at 331 (offset 7 lines).
Hunk #4 succeeded at 353 (offset 7 lines).
Hunk #5 succeeded at 381 (offset 7 lines).
Hunk #6 succeeded at 409 (offset 7 lines).
Hunk #7 succeeded at 417 (offset 7 lines).
Hunk #8 succeeded at 425 with fuzz 2 (offset 7 lines).
2 out of 8 hunks FAILED -- saving rejects to file Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp.rej
patching file Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Alex Christensen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/9244213
Comment 11 Basuke Suzuki 2018-09-17 10:42:45 PDT
Created attachment 349899 [details]
PATCH

Thanks Alex for r+
Comment 12 WebKit Commit Bot 2018-09-17 11:19:42 PDT
Comment on attachment 349899 [details]
PATCH

Clearing flags on attachment: 349899

Committed r236073: <https://trac.webkit.org/changeset/236073>
Comment 13 WebKit Commit Bot 2018-09-17 11:19:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2018-09-17 11:20:49 PDT
<rdar://problem/44527129>
Comment 15 Fujii Hironori 2018-09-17 20:28:22 PDT
Following test cases are constantly failing since r236073.

http/tests/xmlhttprequest/basic-auth-default.html
http/tests/xmlhttprequest/cross-origin-authorization.html
http/tests/xmlhttprequest/logout.html
http/tests/xmlhttprequest/re-login-async.html
http/tests/xmlhttprequest/re-login.html
http/tests/xmlhttprequest/redirect-credentials-responseURL.html
http/tests/xmlhttprequest/remember-bad-password.html