From e29b64a18ae03df38beec6550d28ad64d57987b1 Mon Sep 17 00:00:00 2001 From: Greg Nolle Date: Wed, 9 Nov 2016 13:05:46 +0000 Subject: [PATCH 1/8] modify CertVerifier Class * respond to multiple similar verification requests. * accept net error result as callback response. --- atom/browser/api/atom_api_session.cc | 2 +- atom/browser/net/atom_cert_verifier.cc | 167 +++++++++++++++++++++---- atom/browser/net/atom_cert_verifier.h | 22 +++- docs/api/session.md | 8 +- lib/browser/api/session.js | 21 +++- lib/browser/rpc-server.js | 1 + lib/renderer/api/remote.js | 3 +- spec/api-session-spec.js | 8 +- 8 files changed, 199 insertions(+), 33 deletions(-) diff --git a/atom/browser/api/atom_api_session.cc b/atom/browser/api/atom_api_session.cc index 45bb9c2cf3..e3b55cb37c 100644 --- a/atom/browser/api/atom_api_session.cc +++ b/atom/browser/api/atom_api_session.cc @@ -738,7 +738,7 @@ void Session::BuildPrototype(v8::Isolate* isolate, .SetMethod("setDownloadPath", &Session::SetDownloadPath) .SetMethod("enableNetworkEmulation", &Session::EnableNetworkEmulation) .SetMethod("disableNetworkEmulation", &Session::DisableNetworkEmulation) - .SetMethod("setCertificateVerifyProc", &Session::SetCertVerifyProc) + .SetMethod("_setCertificateVerifyProc", &Session::SetCertVerifyProc) .SetMethod("setPermissionRequestHandler", &Session::SetPermissionRequestHandler) .SetMethod("clearHostResolverCache", &Session::ClearHostResolverCache) diff --git a/atom/browser/net/atom_cert_verifier.cc b/atom/browser/net/atom_cert_verifier.cc index 61c7439e27..ec7b8755fc 100644 --- a/atom/browser/net/atom_cert_verifier.cc +++ b/atom/browser/net/atom_cert_verifier.cc @@ -7,6 +7,9 @@ #include "atom/browser/browser.h" #include "atom/browser/net/atom_ct_delegate.h" #include "atom/common/native_mate_converters/net_converter.h" +#include "base/containers/linked_list.h" +#include "base/memory/ptr_util.h" +#include "base/memory/weak_ptr.h" #include "content/public/browser/browser_thread.h" #include "net/base/net_errors.h" #include "net/cert/cert_verify_result.h" @@ -19,17 +22,119 @@ namespace atom { namespace { -void OnResult( - net::CertVerifyResult* verify_result, - const net::CompletionCallback& callback, - bool result) { - BrowserThread::PostTask( - BrowserThread::IO, FROM_HERE, - base::Bind(callback, result ? net::OK : net::ERR_FAILED)); -} +class Response : public base::LinkNode { + public: + Response(net::CertVerifyResult* verify_result, + const net::CompletionCallback& callback) + : verify_result_(verify_result), callback_(callback) {} + net::CertVerifyResult* verify_result() { return verify_result_; } + net::CompletionCallback callback() { return callback_; } + + private: + net::CertVerifyResult* verify_result_; + net::CompletionCallback callback_; + + DISALLOW_COPY_AND_ASSIGN(Response); +}; } // namespace +class CertVerifierRequest : public AtomCertVerifier::Request { + public: + CertVerifierRequest(const AtomCertVerifier::RequestParams& params, + AtomCertVerifier* cert_verifier) + : params_(params), + cert_verifier_(cert_verifier), + error_(net::ERR_IO_PENDING), + custom_response_(net::ERR_IO_PENDING), + first_response_(true), + weak_ptr_factory_(this) {} + + ~CertVerifierRequest() { + cert_verifier_->RemoveRequest(params_); + default_verifier_request_.reset(); + while (!response_list_.empty() && !first_response_) { + base::LinkNode* response_node = response_list_.head(); + response_node->RemoveFromList(); + Response* response = response_node->value(); + RunResponse(response); + } + cert_verifier_ = nullptr; + weak_ptr_factory_.InvalidateWeakPtrs(); + } + + void RunResponse(Response* response) { + if (custom_response_ == net::ERR_ABORTED) { + *(response->verify_result()) = result_; + response->callback().Run(error_); + } else { + response->verify_result()->Reset(); + response->verify_result()->verified_cert = params_.certificate(); + cert_verifier_->ct_delegate()->AddCTExcludedHost(params_.hostname()); + response->callback().Run(custom_response_); + } + delete response; + } + + void Start(net::CRLSet* crl_set, + const net::BoundNetLog& net_log) { + int error = cert_verifier_->default_verifier()->Verify( + params_, crl_set, &result_, + base::Bind(&CertVerifierRequest::OnDefaultVerificationDone, + weak_ptr_factory_.GetWeakPtr()), + &default_verifier_request_, net_log); + if (error != net::ERR_IO_PENDING) + OnDefaultVerificationDone(error); + } + + void OnDefaultVerificationDone(int error) { + error_ = error; + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, + base::Bind(cert_verifier_->verify_proc(), params_.hostname(), + params_.certificate(), net::ErrorToString(error), + base::Bind(&CertVerifierRequest::OnResponseInUI, + weak_ptr_factory_.GetWeakPtr()))); + } + + void OnResponseInUI(int result) { + BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, + base::Bind(&CertVerifierRequest::NotifyResponseInIO, + weak_ptr_factory_.GetWeakPtr(), result)); + } + + void NotifyResponseInIO(int result) { + custom_response_ = result; + first_response_ = false; + // Responding to first request in the list will initiate destruction of + // the class, respond to others in the list inside destructor. + base::LinkNode* response_node = response_list_.head(); + response_node->RemoveFromList(); + Response* response = response_node->value(); + RunResponse(response); + } + + void AddResponseListener(net::CertVerifyResult* verify_result, + const net::CompletionCallback& callback) { + response_list_.Append(new Response(verify_result, callback)); + } + + const AtomCertVerifier::RequestParams& params() const { return params_; } + + private: + using ResponseList = base::LinkedList; + + const AtomCertVerifier::RequestParams params_; + AtomCertVerifier* cert_verifier_; + int error_; + int custom_response_; + bool first_response_; + ResponseList response_list_; + net::CertVerifyResult result_; + std::unique_ptr default_verifier_request_; + base::WeakPtrFactory weak_ptr_factory_; +}; + AtomCertVerifier::AtomCertVerifier(AtomCTDelegate* ct_delegate) : default_cert_verifier_(net::CertVerifier::CreateDefault()), ct_delegate_(ct_delegate) {} @@ -51,23 +156,43 @@ int AtomCertVerifier::Verify( if (verify_proc_.is_null()) { ct_delegate_->ClearCTExcludedHostsList(); - return default_cert_verifier_->Verify( - params, crl_set, verify_result, callback, out_req, net_log); + return default_cert_verifier_->Verify(params, crl_set, verify_result, + callback, out_req, net_log); + } else { + CertVerifierRequest* request = FindRequest(params); + if (!request) { + out_req->reset(); + std::unique_ptr new_request = + base::MakeUnique(params, this); + new_request->Start(crl_set, net_log); + request = new_request.get(); + *out_req = std::move(new_request); + inflight_requests_[params] = request; + } + request->AddResponseListener(verify_result, callback); + + return net::ERR_IO_PENDING; } - - verify_result->Reset(); - verify_result->verified_cert = params.certificate(); - ct_delegate_->AddCTExcludedHost(params.hostname()); - - BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - base::Bind(verify_proc_, params.hostname(), params.certificate(), - base::Bind(OnResult, verify_result, callback))); - return net::ERR_IO_PENDING; } bool AtomCertVerifier::SupportsOCSPStapling() { - return true; + if (verify_proc_.is_null()) + return default_cert_verifier_->SupportsOCSPStapling(); + return false; +} + +void AtomCertVerifier::RemoveRequest(const RequestParams& params) { + auto it = inflight_requests_.find(params); + if (it != inflight_requests_.end()) + inflight_requests_.erase(it); +} + +CertVerifierRequest* AtomCertVerifier::FindRequest( + const RequestParams& params) { + auto it = inflight_requests_.find(params); + if (it != inflight_requests_.end()) + return it->second; + return nullptr; } } // namespace atom diff --git a/atom/browser/net/atom_cert_verifier.h b/atom/browser/net/atom_cert_verifier.h index e321f7d3d9..ff60908ee6 100644 --- a/atom/browser/net/atom_cert_verifier.h +++ b/atom/browser/net/atom_cert_verifier.h @@ -5,6 +5,7 @@ #ifndef ATOM_BROWSER_NET_ATOM_CERT_VERIFIER_H_ #define ATOM_BROWSER_NET_ATOM_CERT_VERIFIER_H_ +#include #include #include @@ -13,19 +14,26 @@ namespace atom { class AtomCTDelegate; +class CertVerifierRequest; class AtomCertVerifier : public net::CertVerifier { public: explicit AtomCertVerifier(AtomCTDelegate* ct_delegate); virtual ~AtomCertVerifier(); - using VerifyProc = - base::Callback, - const base::Callback&)>; + using VerifyProc = base::Callback, + const std::string& default_result, + const net::CompletionCallback&)>; void SetVerifyProc(const VerifyProc& proc); + const VerifyProc verify_proc() const { return verify_proc_; } + AtomCTDelegate* ct_delegate() const { return ct_delegate_; } + net::CertVerifier* default_verifier() const { + return default_cert_verifier_.get(); + } + protected: // net::CertVerifier: int Verify(const RequestParams& params, @@ -37,6 +45,12 @@ class AtomCertVerifier : public net::CertVerifier { bool SupportsOCSPStapling() override; private: + friend class CertVerifierRequest; + + void RemoveRequest(const RequestParams& params); + CertVerifierRequest* FindRequest(const RequestParams& params); + + std::map inflight_requests_; VerifyProc verify_proc_; std::unique_ptr default_cert_verifier_; AtomCTDelegate* ct_delegate_; diff --git a/docs/api/session.md b/docs/api/session.md index 58de08d39a..5155fabaed 100644 --- a/docs/api/session.md +++ b/docs/api/session.md @@ -252,8 +252,14 @@ the original network configuration. * `proc` Function * `hostname` String * `certificate` [Certificate](structures/certificate.md) + * `error` String - Verification result from chromium. * `callback` Function - * `isTrusted` Boolean - Determines if the certificate should be trusted + * `verificationResult` Integer - Value can be one of certificate error codes + from [here](https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_error_list.h). + Apart from the certificate error codes, the following special codes can be used. + * `0` - Indicates success and disables Certificate Transperancy verification. + * `-2` - Indicates failure. + * `-3` - Uses the verification result from chromium. Sets the certificate verify proc for `session`, the `proc` will be called with `proc(hostname, certificate, callback)` whenever a server certificate diff --git a/lib/browser/api/session.js b/lib/browser/api/session.js index ac47244db3..bf25029952 100644 --- a/lib/browser/api/session.js +++ b/lib/browser/api/session.js @@ -1,5 +1,5 @@ const {EventEmitter} = require('events') -const {app} = require('electron') +const {app, deprecate} = require('electron') const {fromPartition, Session, Cookies} = process.atomBinding('session') // Public API. @@ -20,3 +20,22 @@ Object.setPrototypeOf(Cookies.prototype, EventEmitter.prototype) Session.prototype._init = function () { app.emit('session-created', this) } + +// Remove after 2.0 +Session.prototype.setCertificateVerifyProc = function (verifyProc) { + if (!verifyProc) { + this._setCertificateVerifyProc(null) + return + } + if (verifyProc.length <= 3) { + deprecate.warn('setCertificateVerifyproc(hostname, certificate, callback)', + 'setCertificateVerifyproc(hostname, certificate, error, callback)') + this._setCertificateVerifyProc((hostname, certificate, error, cb) => { + verifyProc(hostname, certificate, (result) => { + cb(result ? 0 : -2) + }) + }) + } else { + this._setCertificateVerifyProc(verifyProc) + } +} diff --git a/lib/browser/rpc-server.js b/lib/browser/rpc-server.js index 1dac516459..2cafc93439 100644 --- a/lib/browser/rpc-server.js +++ b/lib/browser/rpc-server.js @@ -222,6 +222,7 @@ const unwrapArgs = function (sender, args) { removeRemoteListenersAndLogWarning(meta, args, callIntoRenderer) } } + Object.defineProperty(callIntoRenderer, 'length', { value: meta.length }) v8Util.setRemoteCallbackFreer(callIntoRenderer, meta.id, sender) rendererFunctions.set(objectId, callIntoRenderer) diff --git a/lib/renderer/api/remote.js b/lib/renderer/api/remote.js index c3c8e3b89c..8258a91a66 100644 --- a/lib/renderer/api/remote.js +++ b/lib/renderer/api/remote.js @@ -79,7 +79,8 @@ const wrapArgs = function (args, visited) { return { type: 'function', id: callbacksRegistry.add(value), - location: v8Util.getHiddenValue(value, 'location') + location: v8Util.getHiddenValue(value, 'location'), + length: value.length } } else { return { diff --git a/spec/api-session-spec.js b/spec/api-session-spec.js index 1f31ff7123..6b907b9bf2 100644 --- a/spec/api-session-spec.js +++ b/spec/api-session-spec.js @@ -557,8 +557,8 @@ describe('session module', function () { }) it('accepts the request when the callback is called with true', function (done) { - session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, callback) { - callback(true) + session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, error, callback) { + callback(0) }) w.webContents.once('did-finish-load', function () { @@ -569,7 +569,7 @@ describe('session module', function () { }) it('rejects the request when the callback is called with false', function (done) { - session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, callback) { + session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, error, callback) { assert.equal(hostname, '127.0.0.1') assert.equal(certificate.issuerName, 'Intermediate CA') assert.equal(certificate.subjectName, 'localhost') @@ -580,7 +580,7 @@ describe('session module', function () { assert.equal(certificate.issuerCert.issuerCert.issuer.commonName, 'Root CA') assert.equal(certificate.issuerCert.issuerCert.subject.commonName, 'Root CA') assert.equal(certificate.issuerCert.issuerCert.issuerCert, undefined) - callback(false) + callback(-2) }) var url = `https://127.0.0.1:${server.address().port}` From 37db8040991c3c3238b0524ff33b43f29625b96f Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 7 Feb 2017 13:58:05 -0800 Subject: [PATCH 2/8] Use NetLogWithSource since BoundNetLog no longer exists --- atom/browser/net/atom_cert_verifier.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atom/browser/net/atom_cert_verifier.cc b/atom/browser/net/atom_cert_verifier.cc index ec7b8755fc..b5382dd623 100644 --- a/atom/browser/net/atom_cert_verifier.cc +++ b/atom/browser/net/atom_cert_verifier.cc @@ -77,7 +77,7 @@ class CertVerifierRequest : public AtomCertVerifier::Request { } void Start(net::CRLSet* crl_set, - const net::BoundNetLog& net_log) { + const net::NetLogWithSource& net_log) { int error = cert_verifier_->default_verifier()->Verify( params_, crl_set, &result_, base::Bind(&CertVerifierRequest::OnDefaultVerificationDone, From 6b56dfd94b9dccb88b0e58ae03dacd5079cdb995 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 7 Feb 2017 15:24:49 -0800 Subject: [PATCH 3/8] Add spec for remote callback length --- spec/api-ipc-spec.js | 7 +++++++ spec/fixtures/module/function-with-args.js | 3 +++ 2 files changed, 10 insertions(+) create mode 100644 spec/fixtures/module/function-with-args.js diff --git a/spec/api-ipc-spec.js b/spec/api-ipc-spec.js index b1ca29c6a1..372855619d 100644 --- a/spec/api-ipc-spec.js +++ b/spec/api-ipc-spec.js @@ -85,6 +85,13 @@ describe('ipc module', function () { assert.equal(foo.baz(), 123) }) + it('includes the length of functions specified as arguments', function () { + var a = remote.require(path.join(fixtures, 'module', 'function-with-args.js')) + assert.equal(a(function (a, b, c, d, f) {}), 5) + assert.equal(a((a) => {}), 1) + assert.equal(a((...args) => {}), 0) + }) + it('handles circular references in arrays and objects', function () { var a = remote.require(path.join(fixtures, 'module', 'circular.js')) diff --git a/spec/fixtures/module/function-with-args.js b/spec/fixtures/module/function-with-args.js new file mode 100644 index 0000000000..ed636e5988 --- /dev/null +++ b/spec/fixtures/module/function-with-args.js @@ -0,0 +1,3 @@ +module.exports = function (cb) { + return cb.length +} From 9c134e7bf39c955f610f7633374f76bbc34bd899 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 7 Feb 2017 15:39:18 -0800 Subject: [PATCH 4/8] Assert certificate error --- spec/api-session-spec.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/api-session-spec.js b/spec/api-session-spec.js index 6b907b9bf2..a8773ca629 100644 --- a/spec/api-session-spec.js +++ b/spec/api-session-spec.js @@ -558,6 +558,7 @@ describe('session module', function () { it('accepts the request when the callback is called with true', function (done) { session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, error, callback) { + assert.equal(error, 'net::ERR_CERT_AUTHORITY_INVALID') callback(0) }) @@ -580,6 +581,7 @@ describe('session module', function () { assert.equal(certificate.issuerCert.issuerCert.issuer.commonName, 'Root CA') assert.equal(certificate.issuerCert.issuerCert.subject.commonName, 'Root CA') assert.equal(certificate.issuerCert.issuerCert.issuerCert, undefined) + assert.equal(error, 'net::ERR_CERT_AUTHORITY_INVALID') callback(-2) }) From 5245d42d15482d3cc0b4b0dc06d4768cf6e08c01 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 7 Feb 2017 15:44:56 -0800 Subject: [PATCH 5/8] Only document deprecation for now --- docs/tutorial/planned-breaking-changes.md | 13 +++++++++++++ lib/browser/api/session.js | 12 +++--------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/docs/tutorial/planned-breaking-changes.md b/docs/tutorial/planned-breaking-changes.md index e2d0e3a939..a9ffd981b1 100644 --- a/docs/tutorial/planned-breaking-changes.md +++ b/docs/tutorial/planned-breaking-changes.md @@ -91,6 +91,19 @@ process.versions.electron read-only properties for consistency with the other `process.versions` properties set by Node. +## `session` + +```js +// Deprecated +ses.setCertificateVerifyProc(function (hostname, certificate, callback) { + callback(true) +}) +// Replace with +ses.setCertificateVerifyProc(function (hostname, certificate, error, callback) { + callback(0) +}) +``` + ## `Tray` ```js diff --git a/lib/browser/api/session.js b/lib/browser/api/session.js index bf25029952..8c1c856258 100644 --- a/lib/browser/api/session.js +++ b/lib/browser/api/session.js @@ -1,5 +1,5 @@ const {EventEmitter} = require('events') -const {app, deprecate} = require('electron') +const {app} = require('electron') const {fromPartition, Session, Cookies} = process.atomBinding('session') // Public API. @@ -21,15 +21,9 @@ Session.prototype._init = function () { app.emit('session-created', this) } -// Remove after 2.0 Session.prototype.setCertificateVerifyProc = function (verifyProc) { - if (!verifyProc) { - this._setCertificateVerifyProc(null) - return - } - if (verifyProc.length <= 3) { - deprecate.warn('setCertificateVerifyproc(hostname, certificate, callback)', - 'setCertificateVerifyproc(hostname, certificate, error, callback)') + if (verifyProc != null && verifyProc.length <= 3) { + // TODO(kevinsawicki): Remove in 2.0, deprecate before then with warnings this._setCertificateVerifyProc((hostname, certificate, error, cb) => { verifyProc(hostname, certificate, (result) => { cb(result ? 0 : -2) From 70178adb6e6d10b2083b720a3902e676af1bd50a Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 7 Feb 2017 16:35:37 -0800 Subject: [PATCH 6/8] Use object for verification request --- atom/browser/api/atom_api_session.cc | 12 ++++++++++++ atom/browser/net/atom_cert_verifier.cc | 8 ++++++-- atom/browser/net/atom_cert_verifier.h | 10 +++++++--- docs/api/session.md | 13 +++++++------ docs/tutorial/planned-breaking-changes.md | 2 +- lib/browser/api/session.js | 4 ++-- spec/api-session-spec.js | 21 +++++++++++++++++---- 7 files changed, 52 insertions(+), 18 deletions(-) diff --git a/atom/browser/api/atom_api_session.cc b/atom/browser/api/atom_api_session.cc index e3b55cb37c..44166b553b 100644 --- a/atom/browser/api/atom_api_session.cc +++ b/atom/browser/api/atom_api_session.cc @@ -204,6 +204,18 @@ struct Converter { } }; +template<> +struct Converter { + static v8::Local ToV8(v8::Isolate* isolate, + atom::VerifyRequest val) { + mate::Dictionary dict = mate::Dictionary::CreateEmpty(isolate); + dict.Set("hostname", val.hostname); + dict.Set("certificate", val.certificate); + dict.Set("verificationResult", val.default_result); + return dict.GetHandle(); + } +}; + } // namespace mate namespace atom { diff --git a/atom/browser/net/atom_cert_verifier.cc b/atom/browser/net/atom_cert_verifier.cc index b5382dd623..7ac003eadf 100644 --- a/atom/browser/net/atom_cert_verifier.cc +++ b/atom/browser/net/atom_cert_verifier.cc @@ -89,10 +89,14 @@ class CertVerifierRequest : public AtomCertVerifier::Request { void OnDefaultVerificationDone(int error) { error_ = error; + VerifyRequest request = { + params_.hostname(), + net::ErrorToString(error), + params_.certificate() + }; BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, - base::Bind(cert_verifier_->verify_proc(), params_.hostname(), - params_.certificate(), net::ErrorToString(error), + base::Bind(cert_verifier_->verify_proc(), request, base::Bind(&CertVerifierRequest::OnResponseInUI, weak_ptr_factory_.GetWeakPtr()))); } diff --git a/atom/browser/net/atom_cert_verifier.h b/atom/browser/net/atom_cert_verifier.h index ff60908ee6..b2346553b9 100644 --- a/atom/browser/net/atom_cert_verifier.h +++ b/atom/browser/net/atom_cert_verifier.h @@ -16,14 +16,18 @@ namespace atom { class AtomCTDelegate; class CertVerifierRequest; +struct VerifyRequest { + std::string hostname; + std::string default_result; + scoped_refptr certificate; +}; + class AtomCertVerifier : public net::CertVerifier { public: explicit AtomCertVerifier(AtomCTDelegate* ct_delegate); virtual ~AtomCertVerifier(); - using VerifyProc = base::Callback, - const std::string& default_result, + using VerifyProc = base::Callback; void SetVerifyProc(const VerifyProc& proc); diff --git a/docs/api/session.md b/docs/api/session.md index 5155fabaed..d0814dfdb3 100644 --- a/docs/api/session.md +++ b/docs/api/session.md @@ -250,9 +250,10 @@ the original network configuration. #### `ses.setCertificateVerifyProc(proc)` * `proc` Function - * `hostname` String - * `certificate` [Certificate](structures/certificate.md) - * `error` String - Verification result from chromium. + * `request` Object + * `hostname` String + * `certificate` [Certificate](structures/certificate.md) + * `error` String - Verification result from chromium. * `callback` Function * `verificationResult` Integer - Value can be one of certificate error codes from [here](https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_error_list.h). @@ -262,9 +263,9 @@ the original network configuration. * `-3` - Uses the verification result from chromium. Sets the certificate verify proc for `session`, the `proc` will be called with -`proc(hostname, certificate, callback)` whenever a server certificate -verification is requested. Calling `callback(true)` accepts the certificate, -calling `callback(false)` rejects it. +`proc(request, callback)` whenever a server certificate +verification is requested. Calling `callback(0)` accepts the certificate, +calling `callback(-2)` rejects it. Calling `setCertificateVerifyProc(null)` will revert back to default certificate verify proc. diff --git a/docs/tutorial/planned-breaking-changes.md b/docs/tutorial/planned-breaking-changes.md index a9ffd981b1..57505d0b66 100644 --- a/docs/tutorial/planned-breaking-changes.md +++ b/docs/tutorial/planned-breaking-changes.md @@ -99,7 +99,7 @@ ses.setCertificateVerifyProc(function (hostname, certificate, callback) { callback(true) }) // Replace with -ses.setCertificateVerifyProc(function (hostname, certificate, error, callback) { +ses.setCertificateVerifyProc(function (request, callback) { callback(0) }) ``` diff --git a/lib/browser/api/session.js b/lib/browser/api/session.js index 8c1c856258..33f3b47dee 100644 --- a/lib/browser/api/session.js +++ b/lib/browser/api/session.js @@ -22,9 +22,9 @@ Session.prototype._init = function () { } Session.prototype.setCertificateVerifyProc = function (verifyProc) { - if (verifyProc != null && verifyProc.length <= 3) { + if (verifyProc != null && verifyProc.length > 2) { // TODO(kevinsawicki): Remove in 2.0, deprecate before then with warnings - this._setCertificateVerifyProc((hostname, certificate, error, cb) => { + this._setCertificateVerifyProc(({hostname, certificate, verificationResult}, cb) => { verifyProc(hostname, certificate, (result) => { cb(result ? 0 : -2) }) diff --git a/spec/api-session-spec.js b/spec/api-session-spec.js index a8773ca629..4d6fc64c4d 100644 --- a/spec/api-session-spec.js +++ b/spec/api-session-spec.js @@ -557,8 +557,8 @@ describe('session module', function () { }) it('accepts the request when the callback is called with true', function (done) { - session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, error, callback) { - assert.equal(error, 'net::ERR_CERT_AUTHORITY_INVALID') + session.defaultSession.setCertificateVerifyProc(function ({hostname, certificate, verificationResult}, callback) { + assert.equal(verificationResult, 'net::ERR_CERT_AUTHORITY_INVALID') callback(0) }) @@ -569,8 +569,21 @@ describe('session module', function () { w.loadURL(`https://127.0.0.1:${server.address().port}`) }) + it('supports the old function signature', function (done) { + session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, callback) { + assert.equal(hostname, '127.0.0.1') + callback(true) + }) + + w.webContents.once('did-finish-load', function () { + assert.equal(w.webContents.getTitle(), 'hello') + done() + }) + w.loadURL(`https://127.0.0.1:${server.address().port}`) + }) + it('rejects the request when the callback is called with false', function (done) { - session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, error, callback) { + session.defaultSession.setCertificateVerifyProc(function ({hostname, certificate, verificationResult}, callback) { assert.equal(hostname, '127.0.0.1') assert.equal(certificate.issuerName, 'Intermediate CA') assert.equal(certificate.subjectName, 'localhost') @@ -581,7 +594,7 @@ describe('session module', function () { assert.equal(certificate.issuerCert.issuerCert.issuer.commonName, 'Root CA') assert.equal(certificate.issuerCert.issuerCert.subject.commonName, 'Root CA') assert.equal(certificate.issuerCert.issuerCert.issuerCert, undefined) - assert.equal(error, 'net::ERR_CERT_AUTHORITY_INVALID') + assert.equal(verificationResult, 'net::ERR_CERT_AUTHORITY_INVALID') callback(-2) }) From 1e581d68149eaa4101bcc3714d46ae3959bae531 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 8 Feb 2017 10:34:07 -0800 Subject: [PATCH 7/8] Use unique_ptr for passing VerifyRequestParams --- atom/browser/api/atom_api_session.cc | 4 ++-- atom/browser/net/atom_cert_verifier.cc | 23 +++++++++++++++-------- atom/browser/net/atom_cert_verifier.h | 4 ++-- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/atom/browser/api/atom_api_session.cc b/atom/browser/api/atom_api_session.cc index 44166b553b..0c850888e8 100644 --- a/atom/browser/api/atom_api_session.cc +++ b/atom/browser/api/atom_api_session.cc @@ -205,9 +205,9 @@ struct Converter { }; template<> -struct Converter { +struct Converter { static v8::Local ToV8(v8::Isolate* isolate, - atom::VerifyRequest val) { + atom::VerifyRequestParams val) { mate::Dictionary dict = mate::Dictionary::CreateEmpty(isolate); dict.Set("hostname", val.hostname); dict.Set("certificate", val.certificate); diff --git a/atom/browser/net/atom_cert_verifier.cc b/atom/browser/net/atom_cert_verifier.cc index 7ac003eadf..5dee107eb3 100644 --- a/atom/browser/net/atom_cert_verifier.cc +++ b/atom/browser/net/atom_cert_verifier.cc @@ -89,16 +89,23 @@ class CertVerifierRequest : public AtomCertVerifier::Request { void OnDefaultVerificationDone(int error) { error_ = error; - VerifyRequest request = { - params_.hostname(), - net::ErrorToString(error), - params_.certificate() - }; + std::unique_ptr request(new VerifyRequestParams()); + request->hostname = params_.hostname(); + request->default_result = net::ErrorToString(error); + request->certificate = params_.certificate(); BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, - base::Bind(cert_verifier_->verify_proc(), request, - base::Bind(&CertVerifierRequest::OnResponseInUI, - weak_ptr_factory_.GetWeakPtr()))); + base::Bind(&CertVerifierRequest::OnVerifyRequestInUI, + weak_ptr_factory_.GetWeakPtr(), + cert_verifier_->verify_proc(), + base::Passed(&request))); + } + + void OnVerifyRequestInUI(const AtomCertVerifier::VerifyProc& verify_proc, + std::unique_ptr request) { + verify_proc.Run(*(request.get()), + base::Bind(&CertVerifierRequest::OnResponseInUI, + weak_ptr_factory_.GetWeakPtr())); } void OnResponseInUI(int result) { diff --git a/atom/browser/net/atom_cert_verifier.h b/atom/browser/net/atom_cert_verifier.h index b2346553b9..09fa0f2778 100644 --- a/atom/browser/net/atom_cert_verifier.h +++ b/atom/browser/net/atom_cert_verifier.h @@ -16,7 +16,7 @@ namespace atom { class AtomCTDelegate; class CertVerifierRequest; -struct VerifyRequest { +struct VerifyRequestParams { std::string hostname; std::string default_result; scoped_refptr certificate; @@ -27,7 +27,7 @@ class AtomCertVerifier : public net::CertVerifier { explicit AtomCertVerifier(AtomCTDelegate* ct_delegate); virtual ~AtomCertVerifier(); - using VerifyProc = base::Callback; void SetVerifyProc(const VerifyProc& proc); From 18e15a1e53cc9efc1b5e99295c84a0ca3a1b130d Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 8 Feb 2017 12:51:24 -0800 Subject: [PATCH 8/8] Add spec for rejecting using old signature --- spec/api-session-spec.js | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/spec/api-session-spec.js b/spec/api-session-spec.js index 4d6fc64c4d..f8e0e27dc3 100644 --- a/spec/api-session-spec.js +++ b/spec/api-session-spec.js @@ -569,17 +569,33 @@ describe('session module', function () { w.loadURL(`https://127.0.0.1:${server.address().port}`) }) - it('supports the old function signature', function (done) { - session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, callback) { - assert.equal(hostname, '127.0.0.1') - callback(true) + describe('deprecated function signature', function () { + it('supports accepting the request', function (done) { + session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, callback) { + assert.equal(hostname, '127.0.0.1') + callback(true) + }) + + w.webContents.once('did-finish-load', function () { + assert.equal(w.webContents.getTitle(), 'hello') + done() + }) + w.loadURL(`https://127.0.0.1:${server.address().port}`) }) - w.webContents.once('did-finish-load', function () { - assert.equal(w.webContents.getTitle(), 'hello') - done() + it('supports rejecting the request', function (done) { + session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, callback) { + assert.equal(hostname, '127.0.0.1') + callback(false) + }) + + var url = `https://127.0.0.1:${server.address().port}` + w.webContents.once('did-finish-load', function () { + assert.equal(w.webContents.getTitle(), url) + done() + }) + w.loadURL(url) }) - w.loadURL(`https://127.0.0.1:${server.address().port}`) }) it('rejects the request when the callback is called with false', function (done) {