Bug 174533 – Regression(ITP): Can no longer log in on abc.go.com

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

We have found a handful of sites attempting to authenticate against an unrelated third party. If enough sites use the third party, it get's recognized as a prevalent resource and is blocked or partitioned.

Add a temporary compatibility hack to support testing while these sites correct their content.

[Brent Fulgham]

Created attachment 315507 [details]
Patch

[Build Bot]

Comment on attachment 315507 [details]
Patch

Attachment 315507 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4122733

New failing tests:
security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html
storage/indexeddb/modern/new-database-after-user-delete.html

[Build Bot]

Created attachment 315522 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315507 [details]
Patch

Attachment 315507 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4123320

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html

[Build Bot]

Created attachment 315528 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Geoffrey Garen]

Comment on attachment 315507 [details]
Patch

r=me

[Chris Dumez]

Comment on attachment 315507 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:139
> +static bool resourceNeedsSSOQuirk(const URL& potentialSSO)

potentialSSO -> potentialSSOURL ?

> Source/WebCore/loader/ResourceLoadObserver.cpp:142
> +        static const char* const ssoOrigns[] = {

typo: ssoOrigns

> Source/WebCore/loader/ResourceLoadObserver.cpp:151
> +    const auto& lowercaseDomain = potentialSSO.host().convertToASCIILowercase();

Why do we convert to lowercase if the HashSet uses ASCIICaseInsensitiveHash ?

[Chris Dumez]

<rdar://problem/33325881>

[Chris Dumez]

Created attachment 315957 [details]
Patch

[Sam Weinig]

Comment on attachment 315957 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:139
> +// FIXME: This quirk was added to address <rdar://problem/33325881> and should be removed once content is fixed.

What does it mean for the content to be fixed? What is broken?

> Source/WebCore/loader/ResourceLoadObserver.cpp:146
> +    static NeverDestroyed<HashSet<String, ASCIICaseInsensitiveHash>> ssoOriginsHash = [] {
> +        HashSet<String, ASCIICaseInsensitiveHash> set;
> +        set.add(ASCIILiteral("sp.auth.adobe.com"));
> +        return set;
> +    }();

This can be written as 

static const auto ssoOriginsHash = makeNeverDestroyed(HashSet<String, ASCIICaseInsensitiveHash> {
    "sp.auth.adobe.com"
};

[Chris Dumez]

Comment on attachment 315957 [details]
Patch

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

>> Source/WebCore/loader/ResourceLoadObserver.cpp:139
>> +// FIXME: This quirk was added to address <rdar://problem/33325881> and should be removed once content is fixed.
> 
> What does it mean for the content to be fixed? What is broken?

You cannot currently log into abc.go.com?

>> Source/WebCore/loader/ResourceLoadObserver.cpp:146
>> +    }();
> 
> This can be written as 
> 
> static const auto ssoOriginsHash = makeNeverDestroyed(HashSet<String, ASCIICaseInsensitiveHash> {
>     "sp.auth.adobe.com"
> };

Looks nicer, will do.

[Chris Dumez]

Created attachment 315991 [details]
Patch

[Sam Weinig]

(In reply to Chris Dumez from comment #11)
> Comment on attachment 315957 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315957&action=review
> 
> >> Source/WebCore/loader/ResourceLoadObserver.cpp:139
> >> +// FIXME: This quirk was added to address <rdar://problem/33325881> and should be removed once content is fixed.
> > 
> > What does it mean for the content to be fixed? What is broken?
> 
> You cannot currently log into abc.go.com?

I meant, what exactly is sp.auth.adobe.com need to do to fix their content? 

Should this be disabled by the "Disable Site-specific Hacks" menu item (which is bound to Settings's needsSiteSpecificQuirks member)?

[Chris Dumez]

(In reply to Sam Weinig from comment #13)
> (In reply to Chris Dumez from comment #11)
> > Comment on attachment 315957 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=315957&action=review
> > 
> > >> Source/WebCore/loader/ResourceLoadObserver.cpp:139
> > >> +// FIXME: This quirk was added to address <rdar://problem/33325881> and should be removed once content is fixed.
> > > 
> > > What does it mean for the content to be fixed? What is broken?
> > 
> > You cannot currently log into abc.go.com?
> 
> I meant, what exactly is sp.auth.adobe.com need to do to fix their content? 

This information is in the radar and I don't think the comment is the right place to provide such information.

> 
> Should this be disabled by the "Disable Site-specific Hacks" menu item
> (which is bound to Settings's needsSiteSpecificQuirks member)?

I can look into it but I seem to remember that last time Simon suggested this and we looked into it, that setting was disabled for some configurations we cared about.

[Chris Dumez]

(In reply to Chris Dumez from comment #14)
> (In reply to Sam Weinig from comment #13)
> > (In reply to Chris Dumez from comment #11)
> > > Comment on attachment 315957 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=315957&action=review
> > > 
> > > >> Source/WebCore/loader/ResourceLoadObserver.cpp:139
> > > >> +// FIXME: This quirk was added to address <rdar://problem/33325881> and should be removed once content is fixed.
> > > > 
> > > > What does it mean for the content to be fixed? What is broken?
> > > 
> > > You cannot currently log into abc.go.com?
> > 
> > I meant, what exactly is sp.auth.adobe.com need to do to fix their content? 
> 
> This information is in the radar and I don't think the comment is the right
> place to provide such information.
> 
> > 
> > Should this be disabled by the "Disable Site-specific Hacks" menu item
> > (which is bound to Settings's needsSiteSpecificQuirks member)?
> 
> I can look into it but I seem to remember that last time Simon suggested
> this and we looked into it, that setting was disabled for some
> configurations we cared about.

I think it was iOS. I cannot find any Cocoa API to get this setting and the setting is off by default. Am I missing something?

[Sam Weinig]

(In reply to Chris Dumez from comment #15)
> (In reply to Chris Dumez from comment #14)
> > (In reply to Sam Weinig from comment #13)
> > > (In reply to Chris Dumez from comment #11)
> > > > Comment on attachment 315957 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=315957&action=review
> > > > 
> > > > >> Source/WebCore/loader/ResourceLoadObserver.cpp:139
> > > > >> +// FIXME: This quirk was added to address <rdar://problem/33325881> and should be removed once content is fixed.
> > > > > 
> > > > > What does it mean for the content to be fixed? What is broken?
> > > > 
> > > > You cannot currently log into abc.go.com?
> > > 
> > > I meant, what exactly is sp.auth.adobe.com need to do to fix their content? 
> > 
> > This information is in the radar and I don't think the comment is the right
> > place to provide such information.
> > 
> > > 
> > > Should this be disabled by the "Disable Site-specific Hacks" menu item
> > > (which is bound to Settings's needsSiteSpecificQuirks member)?
> > 
> > I can look into it but I seem to remember that last time Simon suggested
> > this and we looked into it, that setting was disabled for some
> > configurations we cared about.
> 
> I think it was iOS. I cannot find any Cocoa API to get this setting and the
> setting is off by default. Am I missing something?

Just because it is missing on iOS, why wouldn't we want to provide the functionality on Mac? That said, there may be no objc API for this yet, but it would be easy to add, and if you want, one can just cast WKPreferences to WKPreferencesRef and use the C-SPI (though I would recommend adding the objc API if you want this functionality on iOS).

[Geoffrey Garen]

Comment on attachment 315991 [details]
Patch

r=me

[Geoffrey Garen]

> Should this be disabled by the "Disable Site-specific Hacks" menu item
> (which is bound to Settings's needsSiteSpecificQuirks member)?

Yeah, I agree that it would be nice for the menu item to disable this workaround, so folks working on a fix can test the fix.

[WebKit Commit Bot]

Comment on attachment 315991 [details]
Patch

Clearing flags on attachment: 315991

Committed r219701: <http://trac.webkit.org/changeset/219701>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #18)
> > Should this be disabled by the "Disable Site-specific Hacks" menu item
> > (which is bound to Settings's needsSiteSpecificQuirks member)?
> 
> Yeah, I agree that it would be nice for the menu item to disable this
> workaround, so folks working on a fix can test the fix.

Looking into this now. Because the setting is currently disabled on iOS, and because I do not want to enable this setting on iOS at this point to avoid unexpected changes in behavior. I guess we can use this setting but only rely on it on Mac?

[Chris Dumez]

(In reply to Chris Dumez from comment #21)
> (In reply to Geoffrey Garen from comment #18)
> > > Should this be disabled by the "Disable Site-specific Hacks" menu item
> > > (which is bound to Settings's needsSiteSpecificQuirks member)?
> > 
> > Yeah, I agree that it would be nice for the menu item to disable this
> > workaround, so folks working on a fix can test the fix.
> 
> Looking into this now. Because the setting is currently disabled on iOS, and
> because I do not want to enable this setting on iOS at this point to avoid
> unexpected changes in behavior. I guess we can use this setting but only
> rely on it on Mac?

https://bugs.webkit.org/show_bug.cgi?id=174691

[Darin Adler]

Comment on attachment 315991 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:145
> +    static const auto ssoOriginsHash = makeNeverDestroyed(HashSet<String, ASCIICaseInsensitiveHash> {
> +        "sp.auth.adobe.com"
> +    });
> +    return ssoOriginsHash.get().contains(url.host());

Why not just call equalLetterIgnoringASCIICase here? Seems overkill to construct a single element HashSet.

[Chris Dumez]

(In reply to Darin Adler from comment #23)
> Comment on attachment 315991 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315991&action=review
> 
> > Source/WebCore/loader/ResourceLoadObserver.cpp:145
> > +    static const auto ssoOriginsHash = makeNeverDestroyed(HashSet<String, ASCIICaseInsensitiveHash> {
> > +        "sp.auth.adobe.com"
> > +    });
> > +    return ssoOriginsHash.get().contains(url.host());
> 
> Why not just call equalLetterIgnoringASCIICase here? Seems overkill to
> construct a single element HashSet.

I thought about it but using the set is more scalable given that I expect more domains may need to get added. That said, I do not mind using equalLetterIgnoringASCIICase() until we add more domains.

[Darin Adler]

(In reply to Chris Dumez from comment #24)
> I thought about it but using the set is more scalable given that I expect
> more domains may need to get added. That said, I do not mind using
> equalLetterIgnoringASCIICase() until we add more domains.

I am annoyed by the overuse of HashSet for fixed sets where more efficient algorithms may work better. I think I need to build a FixedSet family of class templates. Then it can be a HashSet only when the set is huge, an array with binary search when it’s smaller, an array with linear search when it’s even smaller, and a single function call when it has exactly one element. Either FixedSet can automatically do the right thing, we can have multiple FixedSet classes with identical interfaces but different implementations (FixedSingleItemSet, FixedLinearSearchSet, FixedBinarySearchSet, FixedHashSet), or FixedSet can take a template parameter to tell it which type of implementation to generate.

[Sam Weinig]

(In reply to Darin Adler from comment #25)
> (In reply to Chris Dumez from comment #24)
> > I thought about it but using the set is more scalable given that I expect
> > more domains may need to get added. That said, I do not mind using
> > equalLetterIgnoringASCIICase() until we add more domains.
> 
> I am annoyed by the overuse of HashSet for fixed sets where more efficient
> algorithms may work better. I think I need to build a FixedSet family of
> class templates. Then it can be a HashSet only when the set is huge, an
> array with binary search when it’s smaller, an array with linear search when
> it’s even smaller, and a single function call when it has exactly one
> element. Either FixedSet can automatically do the right thing, we can have
> multiple FixedSet classes with identical interfaces but different
> implementations (FixedSingleItemSet, FixedLinearSearchSet,
> FixedBinarySearchSet, FixedHashSet), or FixedSet can take a template
> parameter to tell it which type of implementation to generate.

I filed bug 174715 to track this.