From a18a71f89a845d9acc83ca9ddbac35b9a463633f Mon Sep 17 00:00:00 2001 From: jww Date: Thu, 4 Sep 2014 21:57:25 -0700 Subject: [PATCH] Cleanup of SSLHostStateDelegate and related code. This is a general cleanup of some minor issues, mostly style and comments, for SSLHostStateDelegate and related classes and methods. This was done originally as a C++ readability review request, originally based on https://codereview.chromium.org/369703002/. Review URL: https://codereview.chromium.org/441043005 Cr-Commit-Position: refs/heads/master@{#293443} --- .../browser/ssl/chrome_ssl_host_state_delegate.cc | 60 ++++++++------ .../browser/ssl/chrome_ssl_host_state_delegate.h | 21 ++--- .../ssl/chrome_ssl_host_state_delegate_factory.cc | 2 +- .../ssl/chrome_ssl_host_state_delegate_test.cc | 95 +++++++++++----------- content/browser/ssl/ssl_policy.cc | 8 +- content/browser/ssl/ssl_policy_backend.cc | 6 +- content/browser/ssl/ssl_policy_backend.h | 6 +- content/public/browser/ssl_host_state_delegate.h | 6 +- 8 files changed, 105 insertions(+), 99 deletions(-) diff --git a/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc b/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc index c306b22f0a92..310c3f4fce17 100644 --- a/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc +++ b/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc @@ -4,6 +4,8 @@ #include "chrome/browser/ssl/chrome_ssl_host_state_delegate.h" +#include + #include "base/base64.h" #include "base/bind.h" #include "base/command_line.h" @@ -13,6 +15,7 @@ #include "base/time/clock.h" #include "base/time/default_clock.h" #include "base/time/time.h" +#include "base/values.h" #include "chrome/browser/content_settings/host_content_settings_map.h" #include "chrome/browser/profiles/profile.h" #include "chrome/common/chrome_switches.h" @@ -72,7 +75,7 @@ GURL GetSecureGURLForHost(const std::string& host) { // This is a helper function that returns the length of time before a // certificate decision expires based on the command line flags. Returns a -// non-negative value in seconds or a value of -1 indicating that decisions +// non-negative value in seconds or a value of -1 indicating that decisions // should not be remembered after the current session has ended (but should be // remembered indefinitely as long as the session does not end), which is the // "old" style of certificate decision memory. Uses the experimental group @@ -118,12 +121,12 @@ int64 GetExpirationDelta() { return kForgetAtSessionEndSwitchValue; } -std::string GetKey(net::X509Certificate* cert, net::CertStatus error) { +std::string GetKey(const net::X509Certificate& cert, net::CertStatus error) { // Since a security decision will be made based on the fingerprint, Chrome // should use the SHA-256 fingerprint for the certificate. net::SHA256HashValue fingerprint = net::X509Certificate::CalculateChainFingerprint256( - cert->os_cert_handle(), cert->GetIntermediateCertificates()); + cert.os_cert_handle(), cert.GetIntermediateCertificates()); std::string base64_fingerprint; base::Base64Encode( base::StringPiece(reinterpret_cast(fingerprint.data), @@ -139,12 +142,12 @@ std::string GetKey(net::X509Certificate* cert, net::CertStatus error) { // dictionary that has been passed in. The returned pointer is owned by the the // argument dict that is passed in. // -// If create_entries is set to |DoNotCreateDictionaryEntries|, +// If create_entries is set to |DO_NOT_CREATE_DICTIONARY_ENTRIES|, // GetValidCertDecisionsDict will return NULL if there is anything invalid about // the setting, such as an invalid version or invalid value types (in addition -// to there not be any values in the dictionary). If create_entries is set to -// |CreateDictionaryEntries|, if no dictionary is found or the decisions are -// expired, a new dictionary will be created +// to there not being any values in the dictionary). If create_entries is set to +// |CREATE_DICTIONARY_ENTRIES|, if no dictionary is found or the decisions are +// expired, a new dictionary will be created. base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict( base::DictionaryValue* dict, CreateDictionaryEntriesDisposition create_entries, @@ -158,7 +161,7 @@ base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict( int version; bool success = dict->GetInteger(kSSLCertDecisionVersionKey, &version); if (!success) { - if (create_entries == DoNotCreateDictionaryEntries) + if (create_entries == DO_NOT_CREATE_DICTIONARY_ENTRIES) return NULL; dict->SetInteger(kSSLCertDecisionVersionKey, @@ -200,16 +203,16 @@ base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict( } // Check to see if the user's certificate decision has expired. - // - Expired and |create_entries| is DoNotCreateDictionaryEntries, return + // - Expired and |create_entries| is DO_NOT_CREATE_DICTIONARY_ENTRIES, return // NULL. - // - Expired and |create_entries| is CreateDictionaryEntries, update the + // - Expired and |create_entries| is CREATE_DICTIONARY_ENTRIES, update the // expiration time. if (should_remember_ssl_decisions_ != - ForgetSSLExceptionDecisionsAtSessionEnd && + FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END && decision_expiration.ToInternalValue() <= now.ToInternalValue()) { *expired_previous_decision = true; - if (create_entries == DoNotCreateDictionaryEntries) + if (create_entries == DO_NOT_CREATE_DICTIONARY_ENTRIES) return NULL; expired = true; @@ -226,7 +229,7 @@ base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict( base::DictionaryValue* cert_error_dict = NULL; // Will be owned by dict if (expired || !dict->GetDictionary(kSSLCertDecisionCertErrorMapKey, &cert_error_dict)) { - if (create_entries == DoNotCreateDictionaryEntries) + if (create_entries == DO_NOT_CREATE_DICTIONARY_ENTRIES) return NULL; cert_error_dict = new base::DictionaryValue(); @@ -238,7 +241,7 @@ base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict( } // If |should_remember_ssl_decisions_| is -// ForgetSSLExceptionDecisionsAtSessionEnd, that means that all invalid +// FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END, that means that all invalid // certificate proceed decisions should be forgotten when the session ends. At // attempt is made in the destructor to remove the entries, but in the case that // things didn't shut down cleanly, on start, Clear is called to guarantee a @@ -247,29 +250,31 @@ ChromeSSLHostStateDelegate::ChromeSSLHostStateDelegate(Profile* profile) : clock_(new base::DefaultClock()), profile_(profile) { int64 expiration_delta = GetExpirationDelta(); if (expiration_delta == kForgetAtSessionEndSwitchValue) { - should_remember_ssl_decisions_ = ForgetSSLExceptionDecisionsAtSessionEnd; + should_remember_ssl_decisions_ = + FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END; expiration_delta = 0; Clear(); } else { - should_remember_ssl_decisions_ = RememberSSLExceptionDecisionsForDelta; + should_remember_ssl_decisions_ = REMEMBER_SSL_EXCEPTION_DECISIONS_FOR_DELTA; } default_ssl_cert_decision_expiration_delta_ = base::TimeDelta::FromSeconds(expiration_delta); } ChromeSSLHostStateDelegate::~ChromeSSLHostStateDelegate() { - if (should_remember_ssl_decisions_ == ForgetSSLExceptionDecisionsAtSessionEnd) + if (should_remember_ssl_decisions_ == + FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END) Clear(); } void ChromeSSLHostStateDelegate::DenyCert(const std::string& host, - net::X509Certificate* cert, + const net::X509Certificate& cert, net::CertStatus error) { ChangeCertPolicy(host, cert, error, net::CertPolicy::DENIED); } void ChromeSSLHostStateDelegate::AllowCert(const std::string& host, - net::X509Certificate* cert, + const net::X509Certificate& cert, net::CertStatus error) { ChangeCertPolicy(host, cert, error, net::CertPolicy::ALLOWED); } @@ -281,7 +286,7 @@ void ChromeSSLHostStateDelegate::Clear() { net::CertPolicy::Judgment ChromeSSLHostStateDelegate::QueryPolicy( const std::string& host, - net::X509Certificate* cert, + const net::X509Certificate& cert, net::CertStatus error, bool* expired_previous_decision) { HostContentSettingsMap* map = profile_->GetHostContentSettingsMap(); @@ -302,9 +307,9 @@ net::CertPolicy::Judgment ChromeSSLHostStateDelegate::QueryPolicy( base::DictionaryValue* cert_error_dict; // Owned by value cert_error_dict = GetValidCertDecisionsDict( - dict, DoNotCreateDictionaryEntries, expired_previous_decision); + dict, DO_NOT_CREATE_DICTIONARY_ENTRIES, expired_previous_decision); if (!cert_error_dict) { - // This revoke is necessary to clear any old expired setting that may + // This revoke is necessary to clear any old expired setting that may be // lingering in the case that an old decision expried. RevokeUserDecisions(host); return net::CertPolicy::UNKNOWN; @@ -356,11 +361,12 @@ void ChromeSSLHostStateDelegate::RevokeUserDecisionsHard( RevokeUserDecisions(host); scoped_refptr getter( profile_->GetRequestContext()); - profile_->GetRequestContext()->GetNetworkTaskRunner()->PostTask( + getter->GetNetworkTaskRunner()->PostTask( FROM_HERE, base::Bind(&CloseIdleConnections, getter)); } -bool ChromeSSLHostStateDelegate::HasUserDecision(const std::string& host) { +bool ChromeSSLHostStateDelegate::HasUserDecision( + const std::string& host) const { GURL url = GetSecureGURLForHost(host); const ContentSettingsPattern pattern = ContentSettingsPattern::FromURLNoWildcard(url); @@ -403,9 +409,9 @@ void ChromeSSLHostStateDelegate::SetClock(scoped_ptr clock) { void ChromeSSLHostStateDelegate::ChangeCertPolicy( const std::string& host, - net::X509Certificate* cert, + const net::X509Certificate& cert, net::CertStatus error, - net::CertPolicy::Judgment judgment) { + const net::CertPolicy::Judgment judgment) { GURL url = GetSecureGURLForHost(host); const ContentSettingsPattern pattern = ContentSettingsPattern::FromURLNoWildcard(url); @@ -422,7 +428,7 @@ void ChromeSSLHostStateDelegate::ChangeCertPolicy( bool expired_previous_decision; // unused value in this function base::DictionaryValue* cert_dict = GetValidCertDecisionsDict( - dict, CreateDictionaryEntries, &expired_previous_decision); + dict, CREATE_DICTIONARY_ENTRIES, &expired_previous_decision); // If a a valid certificate dictionary cannot be extracted from the content // setting, that means it's in an unknown format. Unfortunately, there's // nothing to be done in that case, so a silent fail is the only option. diff --git a/chrome/browser/ssl/chrome_ssl_host_state_delegate.h b/chrome/browser/ssl/chrome_ssl_host_state_delegate.h index 1353c20020a5..9365ea9a1275 100644 --- a/chrome/browser/ssl/chrome_ssl_host_state_delegate.h +++ b/chrome/browser/ssl/chrome_ssl_host_state_delegate.h @@ -29,15 +29,15 @@ class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate { // SSLHostStateDelegate: virtual void DenyCert(const std::string& host, - net::X509Certificate* cert, + const net::X509Certificate& cert, net::CertStatus error) OVERRIDE; virtual void AllowCert(const std::string& host, - net::X509Certificate* cert, + const net::X509Certificate& cert, net::CertStatus error) OVERRIDE; virtual void Clear() OVERRIDE; virtual net::CertPolicy::Judgment QueryPolicy( const std::string& host, - net::X509Certificate* cert, + const net::X509Certificate& cert, net::CertStatus error, bool* expired_previous_decision) OVERRIDE; virtual void HostRanInsecureContent(const std::string& host, @@ -55,10 +55,7 @@ class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate { // Returns true if any decisions has been recorded for |host| for the given // Profile, otherwise false. - virtual bool HasUserDecision(const std::string& host); - - // Called on the UI thread when the profile is about to be destroyed. - void ShutdownOnUIThread() {} + virtual bool HasUserDecision(const std::string& host) const; protected: // SetClock takes ownership of the passed in clock. @@ -74,8 +71,8 @@ class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate { // Used to specify whether new content setting entries should be created if // they don't already exist when querying the user's settings. enum CreateDictionaryEntriesDisposition { - CreateDictionaryEntries, - DoNotCreateDictionaryEntries + CREATE_DICTIONARY_ENTRIES, + DO_NOT_CREATE_DICTIONARY_ENTRIES }; // Specifies whether user SSL error decisions should be forgetten at the end @@ -84,8 +81,8 @@ class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate { // length of time, deteremined by // |default_ssl_cert_decision_expiration_delta_|. enum RememberSSLExceptionDecisionsDisposition { - ForgetSSLExceptionDecisionsAtSessionEnd, - RememberSSLExceptionDecisionsForDelta + FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END, + REMEMBER_SSL_EXCEPTION_DECISIONS_FOR_DELTA }; // Modify the user's content settings to specify a judgement made for a @@ -93,7 +90,7 @@ class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate { // is the certificate with an error, |error| is the error in the certificate, // and |judgement| is the user decision to be recorded. void ChangeCertPolicy(const std::string& host, - net::X509Certificate* cert, + const net::X509Certificate& cert, net::CertStatus error, net::CertPolicy::Judgment judgment); diff --git a/chrome/browser/ssl/chrome_ssl_host_state_delegate_factory.cc b/chrome/browser/ssl/chrome_ssl_host_state_delegate_factory.cc index 953a3b017453..e55b243176e4 100644 --- a/chrome/browser/ssl/chrome_ssl_host_state_delegate_factory.cc +++ b/chrome/browser/ssl/chrome_ssl_host_state_delegate_factory.cc @@ -20,7 +20,7 @@ class Service : public KeyedService { ChromeSSLHostStateDelegate* decisions() { return decisions_.get(); } - virtual void Shutdown() OVERRIDE { decisions_->ShutdownOnUIThread(); } + virtual void Shutdown() OVERRIDE {} private: scoped_ptr decisions_; diff --git a/chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc b/chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc index dfa4f512084c..c9f0f488aa22 100644 --- a/chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc +++ b/chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "chrome/browser/ssl/chrome_ssl_host_state_delegate.h" + #include #include "base/command_line.h" @@ -11,7 +13,6 @@ #include "chrome/browser/browsing_data/browsing_data_remover.h" #include "chrome/browser/browsing_data/browsing_data_remover_test_util.h" #include "chrome/browser/profiles/profile.h" -#include "chrome/browser/ssl/chrome_ssl_host_state_delegate.h" #include "chrome/browser/ssl/chrome_ssl_host_state_delegate_factory.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" @@ -32,9 +33,9 @@ const char kWWWGoogleHost[] = "www.google.com"; const char kGoogleHost[] = "google.com"; const char kExampleHost[] = "example.com"; -const char* kForgetAtSessionEnd = "-1"; -const char* kForgetInstantly = "0"; -const char* kDeltaSecondsString = "86400"; +const char kForgetAtSessionEnd[] = "-1"; +const char kForgetInstantly[] = "0"; +const char kDeltaSecondsString[] = "86400"; const uint64_t kDeltaOneDayInSeconds = UINT64_C(86400); scoped_refptr GetGoogleCert() { @@ -66,85 +67,85 @@ IN_PROC_BROWSER_TEST_F(ChromeSSLHostStateDelegateTest, QueryPolicy) { // before any action has been taken. EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kExampleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); // Simulate a user decision to allow an invalid certificate exception for // kWWWGoogleHost. state->AllowCert( - kWWWGoogleHost, google_cert.get(), net::CERT_STATUS_DATE_INVALID); + kWWWGoogleHost, *google_cert.get(), net::CERT_STATUS_DATE_INVALID); // Verify that only kWWWGoogleHost is allowed and that the other two certs // being tested still have no decision associated with them. EXPECT_EQ(net::CertPolicy::ALLOWED, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kExampleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); // Simulate a user decision to allow an invalid certificate exception for // kExampleHost. state->AllowCert( - kExampleHost, google_cert.get(), net::CERT_STATUS_DATE_INVALID); + kExampleHost, *google_cert.get(), net::CERT_STATUS_DATE_INVALID); // Verify that both kWWWGoogleHost and kExampleHost have allow exceptions // while kGoogleHost still has no associated decision. EXPECT_EQ(net::CertPolicy::ALLOWED, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); EXPECT_EQ(net::CertPolicy::ALLOWED, state->QueryPolicy(kExampleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); // Simulate a user decision to deny an invalid certificate for kExampleHost. state->DenyCert( - kExampleHost, google_cert.get(), net::CERT_STATUS_DATE_INVALID); + kExampleHost, *google_cert.get(), net::CERT_STATUS_DATE_INVALID); // Verify that kWWWGoogleHost is allowed and kExampleHost is denied while // kGoogleHost still has no associated decision. EXPECT_EQ(net::CertPolicy::ALLOWED, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); EXPECT_EQ(net::CertPolicy::DENIED, state->QueryPolicy(kExampleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); } @@ -164,9 +165,9 @@ IN_PROC_BROWSER_TEST_F(ChromeSSLHostStateDelegateTest, HasPolicyAndRevoke) { // Simulate a user decision to allow an invalid certificate exception for // kWWWGoogleHost and for kExampleHost. state->AllowCert( - kWWWGoogleHost, google_cert.get(), net::CERT_STATUS_DATE_INVALID); + kWWWGoogleHost, *google_cert.get(), net::CERT_STATUS_DATE_INVALID); state->AllowCert( - kExampleHost, google_cert.get(), net::CERT_STATUS_DATE_INVALID); + kExampleHost, *google_cert.get(), net::CERT_STATUS_DATE_INVALID); // Verify that HasUserDecision correctly acknowledges that a user decision has // been made about kWWWGoogleHost. Then verify that HasUserDecision correctly @@ -176,7 +177,7 @@ IN_PROC_BROWSER_TEST_F(ChromeSSLHostStateDelegateTest, HasPolicyAndRevoke) { EXPECT_FALSE(state->HasUserDecision(kWWWGoogleHost)); EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); @@ -206,7 +207,7 @@ IN_PROC_BROWSER_TEST_F(ChromeSSLHostStateDelegateTest, Clear) { // Simulate a user decision to allow an invalid certificate exception for // kWWWGoogleHost and for kExampleHost. state->AllowCert( - kWWWGoogleHost, google_cert.get(), net::CERT_STATUS_DATE_INVALID); + kWWWGoogleHost, *google_cert.get(), net::CERT_STATUS_DATE_INVALID); // Do a full clear, then make sure that both kWWWGoogleHost, which had a // decision made, and kExampleHost, which was untouched, are now in a @@ -215,13 +216,13 @@ IN_PROC_BROWSER_TEST_F(ChromeSSLHostStateDelegateTest, Clear) { EXPECT_FALSE(state->HasUserDecision(kWWWGoogleHost)); EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); EXPECT_FALSE(state->HasUserDecision(kExampleHost)); EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kExampleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); } @@ -275,7 +276,7 @@ IN_PROC_BROWSER_TEST_F(IncognitoSSLHostStateDelegateTest, PRE_AfterRestart) { // Add a cert exception to the profile and then verify that it still exists // in the incognito profile. state->AllowCert( - kWWWGoogleHost, google_cert.get(), net::CERT_STATUS_DATE_INVALID); + kWWWGoogleHost, *google_cert.get(), net::CERT_STATUS_DATE_INVALID); scoped_ptr incognito(profile->CreateOffTheRecordProfile()); content::SSLHostStateDelegate* incognito_state = @@ -283,7 +284,7 @@ IN_PROC_BROWSER_TEST_F(IncognitoSSLHostStateDelegateTest, PRE_AfterRestart) { EXPECT_EQ(net::CertPolicy::ALLOWED, incognito_state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); @@ -292,11 +293,11 @@ IN_PROC_BROWSER_TEST_F(IncognitoSSLHostStateDelegateTest, PRE_AfterRestart) { // error than above thus mapping to a second exception. Also validate that it // was not added as an exception to the regular profile. incognito_state->AllowCert( - kGoogleHost, google_cert.get(), net::CERT_STATUS_COMMON_NAME_INVALID); + kGoogleHost, *google_cert.get(), net::CERT_STATUS_COMMON_NAME_INVALID); EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_COMMON_NAME_INVALID, &unused_value)); } @@ -317,7 +318,7 @@ IN_PROC_BROWSER_TEST_F(IncognitoSSLHostStateDelegateTest, AfterRestart) { // incognito session ended. EXPECT_EQ(net::CertPolicy::ALLOWED, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); @@ -329,7 +330,7 @@ IN_PROC_BROWSER_TEST_F(IncognitoSSLHostStateDelegateTest, AfterRestart) { // cleared when the incognito session ended. EXPECT_EQ(net::CertPolicy::UNKNOWN, incognito_state->QueryPolicy(kGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_COMMON_NAME_INVALID, &unused_value)); } @@ -354,10 +355,10 @@ IN_PROC_BROWSER_TEST_F(ForGetSSLHostStateDelegateTest, PRE_AfterRestart) { bool unused_value; state->AllowCert( - kWWWGoogleHost, google_cert.get(), net::CERT_STATUS_DATE_INVALID); + kWWWGoogleHost, *google_cert.get(), net::CERT_STATUS_DATE_INVALID); EXPECT_EQ(net::CertPolicy::ALLOWED, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); } @@ -374,7 +375,7 @@ IN_PROC_BROWSER_TEST_F(ForGetSSLHostStateDelegateTest, AfterRestart) { // exceptions after session end. EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); } @@ -411,10 +412,10 @@ IN_PROC_BROWSER_TEST_F(ForgetInstantlySSLHostStateDelegateTest, clock->SetNow(base::Time::NowFromSystemTime()); state->AllowCert( - kWWWGoogleHost, google_cert.get(), net::CERT_STATUS_DATE_INVALID); + kWWWGoogleHost, *google_cert.get(), net::CERT_STATUS_DATE_INVALID); EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); } @@ -440,10 +441,10 @@ IN_PROC_BROWSER_TEST_F(RememberSSLHostStateDelegateTest, PRE_AfterRestart) { bool unused_value; state->AllowCert( - kWWWGoogleHost, google_cert.get(), net::CERT_STATUS_DATE_INVALID); + kWWWGoogleHost, *google_cert.get(), net::CERT_STATUS_DATE_INVALID); EXPECT_EQ(net::CertPolicy::ALLOWED, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); } @@ -469,7 +470,7 @@ IN_PROC_BROWSER_TEST_F(RememberSSLHostStateDelegateTest, AfterRestart) { // and thus has now been rememebered across browser restarts. EXPECT_EQ(net::CertPolicy::ALLOWED, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); @@ -479,7 +480,7 @@ IN_PROC_BROWSER_TEST_F(RememberSSLHostStateDelegateTest, AfterRestart) { // The cert should now be |UNKONWN| because the specified delta has passed. EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); } @@ -508,7 +509,7 @@ IN_PROC_BROWSER_TEST_F(RememberSSLHostStateDelegateTest, QueryPolicyExpired) { // should also indicate that it hasn't expired. EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &expired_previous_decision)); EXPECT_FALSE(expired_previous_decision); @@ -516,10 +517,10 @@ IN_PROC_BROWSER_TEST_F(RememberSSLHostStateDelegateTest, QueryPolicyExpired) { // After allowing the certificate, a query should say that it is allowed and // also specify that it hasn't expired. state->AllowCert( - kWWWGoogleHost, google_cert.get(), net::CERT_STATUS_DATE_INVALID); + kWWWGoogleHost, *google_cert.get(), net::CERT_STATUS_DATE_INVALID); EXPECT_EQ(net::CertPolicy::ALLOWED, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &expired_previous_decision)); EXPECT_FALSE(expired_previous_decision); @@ -532,7 +533,7 @@ IN_PROC_BROWSER_TEST_F(RememberSSLHostStateDelegateTest, QueryPolicyExpired) { // query. EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &expired_previous_decision)); EXPECT_TRUE(expired_previous_decision); @@ -541,7 +542,7 @@ IN_PROC_BROWSER_TEST_F(RememberSSLHostStateDelegateTest, QueryPolicyExpired) { // occurred. EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kWWWGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &expired_previous_decision)); EXPECT_FALSE(expired_previous_decision); @@ -574,11 +575,11 @@ IN_PROC_BROWSER_TEST_F(RemoveBrowsingHistorySSLHostStateDelegateTest, // Add an exception for an invalid certificate. Then remove the last hour's // worth of browsing history and verify that the exception has been deleted. state->AllowCert( - kGoogleHost, google_cert.get(), net::CERT_STATUS_DATE_INVALID); + kGoogleHost, *google_cert.get(), net::CERT_STATUS_DATE_INVALID); RemoveAndWait(profile); EXPECT_EQ(net::CertPolicy::UNKNOWN, state->QueryPolicy(kGoogleHost, - google_cert.get(), + *google_cert.get(), net::CERT_STATUS_DATE_INVALID, &unused_value)); } diff --git a/content/browser/ssl/ssl_policy.cc b/content/browser/ssl/ssl_policy.cc index 18fdde4dfe2b..d5a3a76fa8a0 100644 --- a/content/browser/ssl/ssl_policy.cc +++ b/content/browser/ssl/ssl_policy.cc @@ -34,8 +34,9 @@ SSLPolicy::SSLPolicy(SSLPolicyBackend* backend) void SSLPolicy::OnCertError(SSLCertErrorHandler* handler) { bool expired_previous_decision; // First we check if we know the policy for this error. + DCHECK(handler->ssl_info().is_valid()); net::CertPolicy::Judgment judgment = - backend_->QueryPolicy(handler->ssl_info().cert.get(), + backend_->QueryPolicy(*handler->ssl_info().cert.get(), handler->request_url().host(), handler->cert_error(), &expired_previous_decision); @@ -163,6 +164,7 @@ void SSLPolicy::UpdateEntry(NavigationEntryImpl* entry, void SSLPolicy::OnAllowCertificate(scoped_refptr handler, bool allow) { + DCHECK(handler->ssl_info().is_valid()); if (allow) { // Default behavior for accepting a certificate. // Note that we should not call SetMaxSecurityStyle here, because the active @@ -174,7 +176,7 @@ void SSLPolicy::OnAllowCertificate(scoped_refptr handler, // While AllowCertForHost() executes synchronously on this thread, // ContinueRequest() gets posted to a different thread. Calling // AllowCertForHost() first ensures deterministic ordering. - backend_->AllowCertForHost(handler->ssl_info().cert.get(), + backend_->AllowCertForHost(*handler->ssl_info().cert.get(), handler->request_url().host(), handler->cert_error()); handler->ContinueRequest(); @@ -184,7 +186,7 @@ void SSLPolicy::OnAllowCertificate(scoped_refptr handler, // While DenyCertForHost() executes synchronously on this thread, // CancelRequest() gets posted to a different thread. Calling // DenyCertForHost() first ensures deterministic ordering. - backend_->DenyCertForHost(handler->ssl_info().cert.get(), + backend_->DenyCertForHost(*handler->ssl_info().cert.get(), handler->request_url().host(), handler->cert_error()); handler->CancelRequest(); diff --git a/content/browser/ssl/ssl_policy_backend.cc b/content/browser/ssl/ssl_policy_backend.cc index e81c35d16281..49817d04fdd8 100644 --- a/content/browser/ssl/ssl_policy_backend.cc +++ b/content/browser/ssl/ssl_policy_backend.cc @@ -31,14 +31,14 @@ bool SSLPolicyBackend::DidHostRunInsecureContent(const std::string& host, return ssl_host_state_delegate_->DidHostRunInsecureContent(host, pid); } -void SSLPolicyBackend::DenyCertForHost(net::X509Certificate* cert, +void SSLPolicyBackend::DenyCertForHost(const net::X509Certificate& cert, const std::string& host, net::CertStatus error) { if (ssl_host_state_delegate_) ssl_host_state_delegate_->DenyCert(host, cert, error); } -void SSLPolicyBackend::AllowCertForHost(net::X509Certificate* cert, +void SSLPolicyBackend::AllowCertForHost(const net::X509Certificate& cert, const std::string& host, net::CertStatus error) { if (ssl_host_state_delegate_) @@ -46,7 +46,7 @@ void SSLPolicyBackend::AllowCertForHost(net::X509Certificate* cert, } net::CertPolicy::Judgment SSLPolicyBackend::QueryPolicy( - net::X509Certificate* cert, + const net::X509Certificate& cert, const std::string& host, net::CertStatus error, bool* expired_previous_decision) { diff --git a/content/browser/ssl/ssl_policy_backend.h b/content/browser/ssl/ssl_policy_backend.h index 5997b289aabd..28ba50a66523 100644 --- a/content/browser/ssl/ssl_policy_backend.h +++ b/content/browser/ssl/ssl_policy_backend.h @@ -29,20 +29,20 @@ class SSLPolicyBackend { // Records that |cert| is not permitted to be used for |host| in the future, // for a specific error type. - void DenyCertForHost(net::X509Certificate* cert, + void DenyCertForHost(const net::X509Certificate& cert, const std::string& host, net::CertStatus error); // Records that |cert| is permitted to be used for |host| in the future, for // a specific error type. - void AllowCertForHost(net::X509Certificate* cert, + void AllowCertForHost(const net::X509Certificate& cert, const std::string& host, net::CertStatus error); // Queries whether |cert| is allowed or denied for |host|. Returns true in // |expired_previous_decision| if a user decision had been made previously but // that decision has expired, otherwise false. - net::CertPolicy::Judgment QueryPolicy(net::X509Certificate* cert, + net::CertPolicy::Judgment QueryPolicy(const net::X509Certificate& cert, const std::string& host, net::CertStatus error, bool* expired_previous_decision); diff --git a/content/public/browser/ssl_host_state_delegate.h b/content/public/browser/ssl_host_state_delegate.h index bcacd7fdef1a..9f564a194b1a 100644 --- a/content/public/browser/ssl_host_state_delegate.h +++ b/content/public/browser/ssl_host_state_delegate.h @@ -27,13 +27,13 @@ class SSLHostStateDelegate { // Records that |cert| is not permitted to be used for |host| in the future, // for a specified |error| type. virtual void DenyCert(const std::string& host, - net::X509Certificate* cert, + const net::X509Certificate& cert, net::CertStatus error) = 0; // Records that |cert| is permitted to be used for |host| in the future, for // a specified |error| type. virtual void AllowCert(const std::string&, - net::X509Certificate* cert, + const net::X509Certificate& cert, net::CertStatus error) = 0; // Clear all allow/deny preferences. @@ -44,7 +44,7 @@ class SSLHostStateDelegate { // immediately prior to this query, otherwise false. virtual net::CertPolicy::Judgment QueryPolicy( const std::string& host, - net::X509Certificate* cert, + const net::X509Certificate& cert, net::CertStatus error, bool* expired_previous_decision) = 0; -- 2.11.4.GIT