Bug 29160 – Safari 4 unable to change settings on Linksys Routers

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brady Eidson]

Safari 4 unable to change settings on Linksys Routers.

This originally broke in http://trac.webkit.org/changeset/42483 where - on Mac and Windows platforms - WebKit stopped automatically sending HTTP Basic Authentication credentials out on subsequent requests.

Linksys routers depend on getting the credential without having to ask for it.

In Radar as <rdar://problem/7174050>

And I have a patch coming that augments the WebCore session credential store to handle this.

[Brady Eidson]

Created attachment 39407 [details]
Proposed fix (layout test is coming seperately)

[Brady Eidson]

Created attachment 39469 [details]
Updated fix + DRT enhancements + Layout Test

[Alexey Proskuryakov]

Comment on attachment 39469 [details]
Updated fix + DRT enhancements + Layout Test

This only changes the behavior for Basic authentication scheme. I think that's worth mentioning in ChangeLog.

+static OriginToDefaultCredentialMap& originToDefaultCredentialMap()

Maybe it's also worth adding "Basic" somewhere in this name, or in a comment next to it.

+#include "Credential.h"
+#include "CString.h"

This is not properly sorted, we use case-sensitive sort for headers.

 #include "CookieStorageWin.h"
+#include "CredentialStorage.h"
 #include "CString.h"

Same issue (pre-existing).

+            // This case should never happen, as all URL paths should start with a leading /

All _HTTP_ paths? There are protocolInHTTPFamily() checks at call sites, seems worth adding an assertion in the function itself.

+        // <rdar://problem/7174050> - For URLs that match the paths of those previously challenged for HTTP Basic authentication, 
+        // we need to try and reuse the credential preemptively, as allowed by RFC 2617.

Two times in a row, I read this as a FIXME comment. Maybe it's just me, or maybe it needs to be more assertive than "we need to" :)

+        CString encoded = unencoded.utf8().base64Encode();

We should ideally check if it's UTF-8 is what other browsers use - it may also be page encoding. But I'm not sure if credentials encoding is something we do correctly now.

     if (!challenge.previousFailureCount() && (!client() || client()->shouldUseCredentialStorage(this))) {
-        NSURLCredential *credential = WebCoreCredentialStorage::get([mac(challenge) protectionSpace]);
-        if (credential) {
-            [challenge.sender() useCredential:credential forAuthenticationChallenge:mac(challenge)];
+        Credential credential = CredentialStorage::get(challenge.protectionSpace());
+        if (!credential.isEmpty()) {
+            [challenge.sender() useCredential:mac(credential) forAuthenticationChallenge:mac(challenge)];
             return;
         }

If default credentials are incorrect, are we going to send them again on 401, and only then ask the user?

A separate question - aren't we going to store proxy credentials as default credentials for an URL after handling a 407 response?

This patch looks really good to me, both code changes and the fact that it greatly improves DRT capabilities. I have to say r- since 407 looks like an important issue, even though I'm not 100% sure that it's real.

[Brady Eidson]

Created attachment 39519 [details]
Patch with changes

Except for the unanswered question about credential encoding, which I'm sure we get wrong in plenty of other contexts, this patch addresses all of Alexeys comments, including most importantly:
-Not sending the same incorrect credentials twice in a row
and
-Not storing credentials on 407 (by limiting to 401 only)

[Brady Eidson]

Created attachment 39554 [details]
Updated fix - Same code changes as my Saturday patch, but a new layout test for the "resending the same wrong credentials" behavior.

I also wanted to layout test the 407 behavior, but CFNetwork won't send proxy credentials unless you're actually connected to a proxy, and testing that is outside the scope of our current layout test infrastructure.

[Alexey Proskuryakov]

Comment on attachment 39554 [details]
Updated fix - Same code changes as my Saturday patch, but a new layout test for the "resending the same wrong credentials" behavior.

+    const HashMap<String, Credential>& pathToCredentialMap(originToDefaultBasicCredentialMap().get(origin));

Good catch with copying the map!

+        Credential m_initialCredential;

This seems difficult to understand without a context, I think it's worth a comment (yeah, copied many times).

+        NSLog(@"Telling send to use %@ for challenge %@", mac(webCredential), d->m_currentMacChallenge);

Looks unintentional (maybe there's something to log with WebCore mechanisms though?) 

r=me

[Brady Eidson]

Whoops - that log was in there for last minute testing with the new layout test.

Thanks for the review!

Will be landing shortly

[Brady Eidson]

Committed revision 48363
http://trac.webkit.org/changeset/48363

[Brady Eidson]

Committed revision 48364
http://trac.webkit.org/changeset/48364

Windows build fix.

[Brady Eidson]

Committed revision 48365
http://trac.webkit.org/changeset/48365

Windows DRT build fix.