Bug 130963 – [SOUP] Libsoup internal credential setting should be controlled by loader decision

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

[youenn fablet]

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.

[youenn fablet]

Created attachment 228165 [details]
First fix (cookie & auth, async & sync)

[Martin Robinson]

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]

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

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

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

Created attachment 228509 [details]
Fix over bug 131026 patch

[youenn fablet]

Created attachment 228580 [details]
Limit when to disable auth manager

[youenn fablet]

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]

Created attachment 228840 [details]
Rebasing patch

[Darin Adler]

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 on attachment 228840 [details]
Rebasing patch

Clearing flags on attachment: 228840

Committed r167185: <http://trac.webkit.org/changeset/167185>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.