Bug 83259 – [BlackBerry] Networking - Assertion failed when it store FTP credentials

• Component: Platform
• Status: RESOLVED INVALID

[Sean Wang]

For blackberry porting, When access a FTP site with basic authentication, after input credential and click the OK button, it assertion fails at CredentialStorage::set(). It is asserting that ASSERT(protectionSpace.isProxy() || url.protocolInHTTPFamily()). But blackberry porting is saving FTP credential using this function. We should support for FTP to store its credentials.

[Sean Wang]

Created attachment 135975 [details]
Patch

[Rob Buis]

Comment on attachment 135975 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        passed else it is failed.

You should not give the steps to reproduce in the ChangeLog, the bug report itself should contain this information. Is it possible to add an auto/manual test for this?

[Alexey Proskuryakov]

Comment on attachment 135975 [details]
Patch

I think that the correct answer is to not use HTTP credential storage code for ftp. It's specifically tailored for HTTP credential semantics.

[Sean Wang]

(In reply to comment #3)
> (From update of attachment 135975 [details])
> I think that the correct answer is to not use HTTP credential storage code for ftp. It's specifically tailored for HTTP credential semantics.

Currently, only the CF and BlackBerry porting are using the CredentialStorage as I grepped CredentialStorage::set. There are 3 type browsers which support FTP credential storage. They are Firefox 11.0, Chromium 18.0 and IE 8 as I tested. So I think storing the FTP credentials makes sense.

As the credentials are stored in class ProtectionSpace, and it supports many protocols in its ProtectionSpaceServerType and it supports ServerType comparing in its "operator ==",  So storing other protocol's credentials should work with the current implementations.

What is your concern?

[Sean Wang]

Created attachment 136655 [details]
Patch 2

I changed the comments of "No new test" section. It can't write a auto-test case, since it needs a FTP server to setup the test case.

[Alexey Proskuryakov]

Comment on attachment 136655 [details]
Patch 2

Again, this class is not designed to keep ftp credentials. You should find a different place for those.

[Brady Eidson]

(In reply to comment #4)
>There are 3 type browsers which support FTP credential storage. They are Firefox 11.0, Chromium 18.0 and IE 8 as I tested. So I think storing the FTP credentials makes sense.

Why do you think only those 3 browsers support FTP credential storage?  All Mac WebKit browsers do, for example, by virtue of using the keychain for persistent credential storage and having the networking layer and/or the browser itself try that keychain.

Also it's not clear to me based on your statement but it seems possible that you think the credential store is persistent, while it is not.

> (In reply to comment #3)
> > (From update of attachment 135975 [details] [details])
> > I think that the correct answer is to not use HTTP credential storage code for ftp. It's specifically tailored for HTTP credential semantics.
> 
> Currently, only the CF and BlackBerry porting are using the CredentialStorage as I grepped CredentialStorage::set. 
> ...
> What is your concern?

Our concern - that Alexey explicitly stated - is that the CredentialStorage class is used solely for HTTP(S) credentials and has the semantics of that usage.

You're also mistaken about how "limited" the use is:

`grep -r "CredentialStorage::set" . | grep -v ChangeLog`
./WebCore/platform/network/blackberry/NetworkJob.cpp:    CredentialStorage::set(challenge.proposedCredential(), challenge.protectionSpace(), m_response.url());
./WebCore/platform/network/cf/ResourceHandleCFNet.cpp:            CredentialStorage::set(Credential(d->m_user, d->m_pass, CredentialPersistenceNone), firstRequest().url());
./WebCore/platform/network/cf/ResourceHandleCFNet.cpp:        CredentialStorage::set(core(credential.get()), challenge.protectionSpace(), urlToStore);
./WebCore/platform/network/cf/ResourceHandleCFNet.cpp:                    CredentialStorage::set(credential, challenge.protectionSpace(), challenge.failureResponse().url());
./WebCore/platform/network/cf/ResourceHandleCFNet.cpp:        CredentialStorage::set(webCredential, challenge.protectionSpace(), urlToStore);
./WebCore/platform/network/CredentialStorage.cpp:void CredentialStorage::set(const Credential& credential, const ProtectionSpace& protectionSpace, const KURL& url)
./WebCore/platform/network/CredentialStorage.cpp:bool CredentialStorage::set(const Credential& credential, const KURL& url)
./WebCore/platform/network/mac/ResourceHandleMac.mm:            CredentialStorage::set(Credential(d->m_user, d->m_pass, CredentialPersistenceNone), firstRequest().url());
./WebCore/platform/network/mac/ResourceHandleMac.mm:                    CredentialStorage::set(credential, challenge.protectionSpace(), challenge.failureResponse().url());
./WebCore/platform/network/mac/ResourceHandleMac.mm:        CredentialStorage::set(webCredential, core([d->m_currentMacChallenge protectionSpace]), urlToStore);

In addition to BlackBerry it is used by Windows, Mac, and iOS ports..  In fact it was added for the mainline Apple ports.

We apologize for the confusion that this is a generic credential store when - as stated a few times - it is explicitly about HTTP credentials.  I've filed https://bugs.webkit.org/show_bug.cgi?id=83695 to correct that so no one makes the same mistake again.

[Brady Eidson]

Since this bug was premised on the BlackBerry port incorrectly using CredentialStorage for FTP credentials, it seems it is invalid.

I would recommend filing a new bug about adding an FTP credential store for BlackBerry's sole use.