Bug 192937 – Move HTTPS_UPGRADE code behind a runtime flag, off by default

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Move HTTP_UPGRADE code behind a runtime flag, off by default and drop the build time flag.

[Chris Dumez]

Created attachment 357821 [details]
Patch

[Chris Dumez]

Created attachment 357823 [details]
Patch

[Chris Dumez]

Created attachment 357826 [details]
Patch

[Chris Dumez]

Created attachment 357827 [details]
Patch

[Chris Dumez]

make: *** No rule to make target `HTTPSUpgradeList.txt', needed by `HTTPSUpgradeList.db'.  Stop.
make: *** Waiting for unfinished jobs....
Command /bin/sh failed with exit code 2

:/

[Chris Dumez]

Comment on attachment 357827 [details]
Patch

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

> Source/WebKit/DerivedSources.make:320
>  HTTPSUpgradeList.db : HTTPSUpgradeList.txt $(WebKit2)/Scripts/generate-https-upgrade-database.sh

Should this be in WebKitDerivedSourcesAdditions.make ?

[Andy Estes]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 357827 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357827&action=review
> 
> > Source/WebKit/DerivedSources.make:320
> >  HTTPSUpgradeList.db : HTTPSUpgradeList.txt $(WebKit2)/Scripts/generate-https-upgrade-database.sh
> 
> Should this be in WebKitDerivedSourcesAdditions.make ?

Yes.

[Chris Dumez]

Created attachment 357830 [details]
Patch

[Vivek Seth]

Comment on attachment 357830 [details]
Patch

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

> Source/WebKit/Shared/HTTPSUpgrade/HTTPSUpgradeList.txt:1
> +www.bbc.com

Should we use a larger list of domains from the HSTS list?

[Chris Dumez]

Created attachment 357833 [details]
Patch

[Vivek Seth]

Comment on attachment 357833 [details]
Patch

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

> Source/WebKit/DerivedSources.make:319
> +VPATH += $(WebKit2)/Shared/HTTPSUpgrade/

I thought we were adding this to WebKitDerivedSourcesAdditions.make. Did that not work?

[Vivek Seth]

> /Volumes/Data/EWS/WebKit/Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:95> :13: error: unused variable 'bindTextResult' [-Werror,-Wunused-variable]
>         int bindTextResult = m_statement->bindText(1, WTFMove(host));
>             ^
> /Volumes/Data/EWS/WebKit/Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:10> 1:13: error: unused variable 'resetResult' [-Werror,-Wunused-variable]
>         int resetResult = m_statement->reset();
>             ^
> 2 errors generated.


This was my mistake, ASSERTS are compiled out on release builds. I didn't realize unused variables would cause errors. 

Here is one solution: 

void NetworkHTTPSUpgradeChecker::query(String&& host, CompletionHandler<void(bool)>&& callback)
{
   ASSERT(isMainThread());

   if (!this->m_isSetupComplete) {
       RELEASE_LOG_IF_ALLOWED("query - database setup is not complete, ignoring query.");
       callback(false);
       return;
   }

   m_workQueue->dispatch([this, host = WTFMove(host), callback = WTFMove(callback)] () mutable {
       auto runCallbackOnMainThread = [callback = WTFMove(callback)] (bool foundHost) mutable {
           WTF::RunLoop::main().dispatch([foundHost, callback = WTFMove(callback)] () mutable {
               callback(foundHost);
           });
       };

        if (this->m_statement->bindText(1, WTFMove(host)) != SQLITE_OK) {
            ASSERT(false);
            runCallbackOnMainThread(false);
            return;
        }

        int stepResult = this->m_statement->step();
        if (stepResult != SQLITE_ROW && stepResult != SQLITE_DONE) {
            ASSERT(false);
            runCallbackOnMainThread(false);
            return;
        }

        if (this->m_statement->reset() != SQLITE_OK) {
            ASSERT(false);
            runCallbackOnMainThread(false);
            return;
        }

        bool foundHost = (stepResult == SQLITE_ROW);
        RELEASE_LOG_IF_ALLOWED("query - Ran successfully. Result = %s", (foundHost ? "true" : "false"));
        runCallbackOnMainThread(foundHost);
    });
}

[Chris Dumez]

Created attachment 357841 [details]
Patch

[Chris Dumez]

(In reply to Vivek Seth from comment #11)
> Comment on attachment 357833 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357833&action=review
> 
> > Source/WebKit/DerivedSources.make:319
> > +VPATH += $(WebKit2)/Shared/HTTPSUpgrade/
> 
> I thought we were adding this to WebKitDerivedSourcesAdditions.make. Did
> that not work?

I haven't tried, I thought it'd be better to have a minimal db in OpenSource.

[Vivek Seth]

Comment on attachment 357833 [details]
Patch

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

>>> Source/WebKit/DerivedSources.make:319
>>> +VPATH += $(WebKit2)/Shared/HTTPSUpgrade/
>> 
>> I thought we were adding this to WebKitDerivedSourcesAdditions.make. Did that not work?
> 
> I haven't tried, I thought it'd be better to have a minimal db in OpenSource.

Yeah that would be good.

[Vivek Seth]

Comment on attachment 357841 [details]
Patch

Looks good to me!

[youenn fablet]

Comment on attachment 357841 [details]
Patch

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

> Source/WebKit/NetworkProcess/PingLoad.cpp:46
> +    , m_networkLoadChecker(makeUniqueRef<NetworkLoadChecker>(FetchOptions { m_parameters.options}, m_parameters.sessionID, m_parameters.webPageID, m_parameters.webFrameID, WTFMove(m_parameters.originalRequestHeaders), URL { m_parameters.request.url() }, m_parameters.sourceOrigin.copyRef(), m_parameters.preflightPolicy, false, m_parameters.request.httpReferrer()))

Not very clear what false means here.
Maybe isHTTPSUpgradeEnabled should be the last parameter and set to false by default, like requestLoadType.

[Andy Estes]

Comment on attachment 357841 [details]
Patch

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

> Source/WebKit/DerivedSources.make:319
> -ifeq ($(ENABLE_HTTPS_UPGRADE),ENABLE_HTTPS_UPGRADE)
> +VPATH += $(WebKit2)/Shared/HTTPSUpgrade/

Nice

> Source/WebKit/Shared/HTTPSUpgrade/HTTPSUpgradeList.txt:3
> +www.bbc.com
> +www.speedtest.net
> +www.bea.gov

webkit.org?

> Source/WebKit/Shared/WebPreferences.yaml:42
> +   humanReadableDescription: "Automatic HTTPS upgrade for sites that support it"

For the sites that we know about, at least :)

[Chris Dumez]

Created attachment 357886 [details]
Patch

[Chris Dumez]

Created attachment 357888 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 357888 [details]
Patch

Clearing flags on attachment: 357888

Committed r239474: <https://trac.webkit.org/changeset/239474>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46887131>