WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
130963
[SOUP] Libsoup internal credential setting should be controlled by loader decision
https://bugs.webkit.org/show_bug.cgi?id=130963
Summary
[SOUP] Libsoup internal credential setting should be controlled by loader dec...
youenn fablet
Reported
2014-03-31 03:15:01 PDT
The soup backend sends any credential previously gathered from past interactions. This seems to cause http/tests/xmlhttprequest/cross-origin-no-authorization.html failure. For instance: - A first same-origin XHR request is sent with explicit credentials. - A second cross-origin XHR request is sent without setting withCredentials to true. It is expected that the second HTTP request will not contain any credential. The soup backend is currently implicitly setting credentials for the second request using the ones from the first request. This seems to apply to user auth and cookies.
Attachments
First fix (cookie & auth, async & sync)
(6.40 KB, patch)
2014-03-31 03:28 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Fix over bug 131026 patch
(10.33 KB, patch)
2014-04-03 07:34 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Limit when to disable auth manager
(10.68 KB, patch)
2014-04-04 01:12 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Rebasing patch
(10.61 KB, patch)
2014-04-08 08:56 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2014-03-31 03:28:48 PDT
Created
attachment 228165
[details]
First fix (cookie & auth, async & sync)
Martin Robinson
Comment 2
2014-03-31 07:14:27 PDT
Comment on
attachment 228165
[details]
First fix (cookie & auth, async & sync) View in context:
https://bugs.webkit.org/attachment.cgi?id=228165&action=review
> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:945 > + soup_message_disable_feature(soupMessage, SOUP_TYPE_AUTH_MANAGER);
Will this still allow for authentication signals to pass to the WebKit layer? What version of libsoup is this available in? It might need to be protected by an #ifdef to avoid introducing a hidden dependency on a new version of libsoup.
> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:946 > + soup_message_disable_feature(soupMessage, SOUP_TYPE_COOKIE_JAR);
Why are you also disabling the cookie jar?
youenn fablet
Comment 3
2014-03-31 08:16:23 PDT
(In reply to
comment #2
)
> (From update of
attachment 228165
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=228165&action=review
> > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:945 > > + soup_message_disable_feature(soupMessage, SOUP_TYPE_AUTH_MANAGER); > > Will this still allow for authentication signals to pass to the WebKit layer? What version of libsoup is this available in? It might need to be protected by an #ifdef to avoid introducing a hidden dependency on a new version of libsoup.
SOUP_TYPE_AUTH_MANAGER is available since libsoup 2.42. This seems to meet EFL and GTK current requirements. I guess that authentication signals will not be passed anymore to the WebKit layer, but only for that particular message. This is fine if that happens in the right case: when doing a CORS request and the "omit credential" flag is set. The current test (using shouldUseCredentialStorage) seems OK in the context of XHR. I need to further check this (image loading in CORS mode e.g.). A cross-check would be great.
> > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:946 > > + soup_message_disable_feature(soupMessage, SOUP_TYPE_COOKIE_JAR); > > Why are you also disabling the cookie jar?
The purpose of this line is to disable sending and setting of cookies. In XHR, user credentials include HTTP auth, cookies and client-side SSL certificates. Disabling the sending of cookies seems implied in
http://fetch.spec.whatwg.org/#basic-fetch
(bullet 6). Disabling the setting of cookies seems also implied in the paragraph on cookies shortly after bullet 8. Would a comment be helpful? The current patch is WIP: it does not yet work for GTK-WK2 and EFL (probably related to the returned value of shouldUseCredentialStorage).
Martin Robinson
Comment 4
2014-03-31 08:28:22 PDT
(In reply to
comment #3
)
> SOUP_TYPE_AUTH_MANAGER is available since libsoup 2.42. > This seems to meet EFL and GTK current requirements. > > I guess that authentication signals will not be passed anymore to the WebKit layer, but only for that particular message. > > This is fine if that happens in the right case: when doing a CORS request and the "omit credential" flag is set. > The current test (using shouldUseCredentialStorage) seems OK in the context of XHR. > I need to further check this (image loading in CORS mode e.g.). > A cross-check would be great.
Credential storage is different than credentials that are input by the user interactively though. I guess we need to decide what to do for XMLHttpRequests that need interactive credential entry.
> > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:946 > > > + soup_message_disable_feature(soupMessage, SOUP_TYPE_COOKIE_JAR); > > > > Why are you also disabling the cookie jar? > > The purpose of this line is to disable sending and setting of cookies. > In XHR, user credentials include HTTP auth, cookies and client-side SSL certificates. > Disabling the sending of cookies seems implied in
http://fetch.spec.whatwg.org/#basic-fetch
(bullet 6). > Disabling the setting of cookies seems also implied in the paragraph on cookies shortly after bullet 8. > > Would a comment be helpful?
Yeah, I think a comment with a link to the spec would be great.
youenn fablet
Comment 5
2014-04-01 04:59:19 PDT
(In reply to
comment #4
)
> (In reply to
comment #3
) > > Credential storage is different than credentials that are input by the user interactively though. I guess we need to decide what to do for XMLHttpRequests that need interactive credential entry.
Interactive credentials should be restricted to same-origin requests. It would probably be safer to disable the authentication manager to the case of not allowing stored credentials AND not asking for user credentials. But AFAICT, credentials are not asked to the user if stored credentials are not allowed. Also, the latter information is not available at ResourceHandle level. As of cookies, ResourceRequest.allowCookies seems a better way to control cookie management. I will file a separate bug for it.
youenn fablet
Comment 6
2014-04-03 07:34:03 PDT
Created
attachment 228509
[details]
Fix over
bug 131026
patch
youenn fablet
Comment 7
2014-04-04 01:12:47 PDT
Created
attachment 228580
[details]
Limit when to disable auth manager
youenn fablet
Comment 8
2014-04-04 01:15:31 PDT
This third patch (with
bug 131026
second patch) works successfully on GTK/WK1, GTK/WK2 and EFL/WK1 (cannot test on EFLWK2). This patch disables auth manager when stored credentials are not allowed and ResourceHandle URL has no user/password information. The case that this patch makes impossible is when stored credentials cannot be used but user can be asked for credentials. This case cannot happen when the client is a ResourceLoader as user is asked for credentials if stored credentials are also allowed. This covers XHR, ImageLoader, TextTrackLoader... I am not aware of any other use of ResourceHandle that may end up in this particular case.
youenn fablet
Comment 9
2014-04-08 08:56:56 PDT
Created
attachment 228840
[details]
Rebasing patch
Darin Adler
Comment 10
2014-04-12 12:31:55 PDT
Comment on
attachment 228840
[details]
Rebasing patch I marked this review+ even though I am not completely an expert. Sorry if that was a mistake.
WebKit Commit Bot
Comment 11
2014-04-12 13:02:13 PDT
Comment on
attachment 228840
[details]
Rebasing patch Clearing flags on attachment: 228840 Committed
r167185
: <
http://trac.webkit.org/changeset/167185
>
WebKit Commit Bot
Comment 12
2014-04-12 13:02:21 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug