Summary: | Move HTTPS_UPGRADE code behind a runtime flag, off by default | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, aestes, commit-queue, ggaren, jberlin, v_seth, webkit-bug-importer, youennf | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2018-12-20 09:19:38 PST
Created attachment 357821 [details]
Patch
Created attachment 357823 [details]
Patch
Created attachment 357826 [details]
Patch
Created attachment 357827 [details]
Patch
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 :/ 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 ? (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. Created attachment 357830 [details]
Patch
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? Created attachment 357833 [details]
Patch
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? > /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);
});
}
Created attachment 357841 [details]
Patch
(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. 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. Comment on attachment 357841 [details]
Patch
Looks good to me!
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. 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 :) Created attachment 357886 [details]
Patch
Created attachment 357888 [details]
Patch
Comment on attachment 357888 [details] Patch Clearing flags on attachment: 357888 Committed r239474: <https://trac.webkit.org/changeset/239474> All reviewed patches have been landed. Closing bug. |