Bug 133161 – Expose time-based HSTS clearing

• Component: WebKit API
• Status: ASSIGNED

[Matt Lilek]

Right now you can only clear all HSTS hosts on a WKContextRef.

We should:
- Expose the ability to clear all HSTS hosts on WKProcessPool.
- Expose a time-based version of this on both WKContextRef and WKProcessPool.

[Matt Lilek]

Created attachment 231945 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 231945 [details]
patch

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

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:554
> +#if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090

I don't think that this is the right check (will follow up in person).

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:557
> +    _CFNetworkResetHSTSHostsSinceDate(privateBrowsingSession(), (CFDateRef)date);

This is very confusing. If we are about to automagically handle sessions here, why just one? I understand that there is a precedent just above, but that seems bad too.

A better API would be to pass a session from the client explicitly.

[Alexey Proskuryakov]

Comment on attachment 231945 [details]
patch

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

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:64
> +extern "C" void _CFNetworkResetHSTSHostsSinceDate(CFURLStorageSessionRef session, CFDateRef date);

This is a pre-existing issue, but the correct idiom is to declare functions unconditionally, even when a private header exists. This way, we'll get a build failure right away when an SPI changes, and won't have to chase subtle differences between internal and open source builds later.

[Matt Lilek]

(In reply to comment #2)
> (From update of attachment 231945 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231945&action=review
> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:557
> > +    _CFNetworkResetHSTSHostsSinceDate(privateBrowsingSession(), (CFDateRef)date);
> 
> This is very confusing. If we are about to automagically handle sessions here, why just one? I understand that there is a precedent just above, but that seems bad too.
> 
> A better API would be to pass a session from the client explicitly.

Fair warning, this is from a cursory reading of the relevant code, so I could be completely wrong.

It looks like we only ever use two CFURLStorageSessions. The one created implicitly by CFNetwork by default or when passed a null identifier. For the private browsing session, everywhere I've found, we create the same session of <bundle identifier>.PrivateBrowsing.

We don't seem to expose the CFURLStorageSessionRefs in any API and given that we only seem to create two of these sessions, it doesn't look like they really map to anything else that could be passed into an API.

I'll gladly change this if I'm wrong.

[Matt Lilek]

(In reply to comment #3)
> (From update of attachment 231945 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231945&action=review
> 
> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:64
> > +extern "C" void _CFNetworkResetHSTSHostsSinceDate(CFURLStorageSessionRef session, CFDateRef date);
> 
> This is a pre-existing issue, but the correct idiom is to declare functions unconditionally, even when a private header exists. This way, we'll get a build failure right away when an SPI changes, and won't have to chase subtle differences between internal and open source builds later.

I feel like there'll be a v2 of this patch so I'll just remove the private #includes and declare these unconditionally for the next version.

[Tim Horton]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 231945 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=231945&action=review
> > 
> > > Source/WebKit2/UIProcess/mac/WebContextMac.mm:64
> > > +extern "C" void _CFNetworkResetHSTSHostsSinceDate(CFURLStorageSessionRef session, CFDateRef date);
> > 
> > This is a pre-existing issue, but the correct idiom is to declare functions unconditionally, even when a private header exists. This way, we'll get a build failure right away when an SPI changes, and won't have to chase subtle differences between internal and open source builds later.
> 
> I feel like there'll be a v2 of this patch so I'll just remove the private #includes and declare these unconditionally for the next version.

No, keep the private #includes *and* declare these unconditionally.

[Alexey Proskuryakov]

Looks like the privateBrowsingSession() function that is used in both the existing function and the newly added is obsolete and broken. We have switched to using multiple private browsing sessions via session API, and these sessions have different identifiers:

    SessionTracker::setSession(sessionID, NetworkStorageSession::createPrivateBrowsingSession(base + '.' + String::number(sessionID.sessionID())));

We now let the client create an arbitrary number of sessions with WKSessionCreate() API, and assign these to pages with WKPageSetSession(). Most WebKit2 C APIs like WKCookieManagerDeleteAllCookiesModifiedAfterDate() don't recognize this - I believe primarily because we couldn't think of any that cared about sessions other than the main one. But these HSTS functions are clearly ones we should have thought about!

I'm not 100% up to date on how we expose sessions via WebKit2 Objective-C API. Martin and Anders should know a lot more.

It is also possible that clearing HSTS data from private browsing session here is unnecessary, and we should only do that for default session.

[Alexey Proskuryakov]

Comment on attachment 231945 [details]
patch

r- based on the above discussion.

[Matt Lilek]

(In reply to comment #7)
> It is also possible that clearing HSTS data from private browsing session here is unnecessary, and we should only do that for default session.

We discussed this and decided this is the case. I'll post a new patch shortly that should address everything.