8000 app: select-client-certificate event callback can accept certificate optionally by deepak1556 · Pull Request #8134 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

app: select-client-certificate event callback can accept certificate optionally #8134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions atom/browser/api/atom_api_app.cc
Original file line number Diff line number Diff line change
Expand Up @@ -378,9 +378,21 @@ void OnClientCertificateSelected(
v8::Isolate* isolate,
std::shared_ptr<content::ClientCertificateDelegate> delegate,
mate::Arguments* args) {
if (args->Length() == 2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this check for 2 here.

Calling callback(cert, null) would hit here and return early and not use the cert but calling callback(cert, null, null) would continue with the cert.

What case is this targeting?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want request to continue with nullptr when the callback is called with no arguments or null. Here callback has already two bound arguments on c++ side, so when callback() is called, the condition will be satisfied.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh awesome, totally makes sense, thanks for explaining 👍

delegate->ContinueWithCertificate(nullptr);
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if you call the callback with an empty object, string, number, etc. now it will continue the request where previously it would error out.

Perhaps it would be better to explicitly check for the argument being null/undefined and still throw an error when an invalid parameter was specified to prevent accidental continuing.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to throw error for invalid arguments.

}

v8::Local<v8::Value> val;
args->GetNext(&val);
if (val->IsNull()) {
delegate->ContinueWithCertificate(nullptr);
return;
}

mate::Dictionary cert_data;
if (!args->GetNext(&cert_data)) {
args->ThrowError();
if (!mate::ConvertFromV8(isolate, val, &cert_data)) {
args->ThrowError("Must pass valid certificate object.");
return;
}

Expand Down
6 changes: 3 additions & 3 deletions docs/api/app.md
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ Returns:
* `url` String

Emitted when the user wants to open a URL with the application. Your application's
`Info.plist` file must define the url scheme within the `CFBundleURLTypes` key, and
`Info.plist` file must define the url scheme within the `CFBundleURLTypes` key, and
set `NSPrincipalClass` to `AtomApplication`.

You should call `event.preventDefault()` if you want to handle this event.
Expand Down Expand Up @@ -222,12 +222,12 @@ Returns:
* `url` URL
* `certificateList` [Certificate[]](structures/certificate.md)
* `callback` Function
* `certificate` [Certificate](structures/certificate.md)
* `certificate` [Certificate](structures/certificate.md) (optional)

Emitted when a client certificate is requested.

The `url` corresponds to the navigation entry requesting the client certificate
and `callback` needs to be called with an entry filtered from the list. Using
and `callback` can be called with an entry filtered from the list. Using
`event.preventDefault()` prevents the application from using the first
certificate from the store.

Expand Down
96 changes: 70 additions & 26 deletions spec/api-app-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const https = require('https')
const net = require('net')
const fs = require('fs')
const path = require('path')
const {remote} = require('electron')
const {ipcRenderer, remote} = require('electron')
const {closeWindow} = require('./window-helpers')

const {app, BrowserWindow, ipcMain} = remote
Expand Down Expand Up @@ -41,6 +41,41 @@ describe('electron module', function () {
})

describe('app module', function () {
let server, secureUrl
const certPath = path.join(__dirname, 'fixtures', 'certificates')

before(function () {
const options = {
key: fs.readFileSync(path.join(certPath, 'server.key')),
cert: fs.readFileSync(path.join(certPath, 'server.pem')),
ca: [
fs.readFileSync(path.join(certPath, 'rootCA.pem')),
fs.readFileSync(path.join(certPath, 'intermediateCA.pem'))
],
requestCert: true,
rejectUnauthorized: false
}

server = https.createServer(options, function (req, res) {
if (req.client.authorized) {
res.writeHead(200)
res.end('<title>authorized</title>')
} else {
res.writeHead(401)
res.end('<title>denied</title>')
}
})

server.listen(0, '127.0.0.1', function () {
const port = server.address().port
secureUrl = `https://127.0.0.1:${port}`
})
})

after(function () {
server.close()
})

describe('app.getVersion()', function () {
it('returns the version field of package.json', function () {
assert.equal(app.getVersion(), '0.1.0')
Expand Down Expand Up @@ -165,24 +200,6 @@ describe('app module', function () {
if (process.platform !== 'linux') return

var w = null
var certPath = path.join(__dirname, 'fixtures', 'certificates')
var options = {
key: fs.readFileSync(path.join(certPath, 'server.key')),
cert: fs.readFileSync(path.join(certPath, 'server.pem')),
ca: [
fs.readFileSync(path.join(certPath, 'rootCA.pem')),
fs.readFileSync(path.join(certPath, 'intermediateCA.pem'))
],
requestCert: true,
rejectUnauthorized: false
}

var server = https.createServer(options, function (req, res) {
if (req.client.authorized) {
res.writeHead(200)
res.end('authorized')
}
})

afterEach(function () {
return closeWindow(w).then(function () { w = null })
Expand All @@ -199,25 +216,24 @@ describe('app module', function () {
})

w.webContents.on('did-finish-load', function () {
server.close()
assert.equal(w.webContents.getTitle(), 'authorized')
done()
})

app.on('select-client-certificate', function (event, webContents, url, list, callback) {
ipcRenderer.once('select-client-certificate', function (event, webContentsId, list) {
assert.equal(webContentsId, w.webContents.id)
assert.equal(list.length, 1)
assert.equal(list[0].issuerName, 'Intermediate CA')
assert.equal(list[0].subjectName, 'Client Cert')
assert.equal(list[0].issuer.commonName, 'Intermediate CA')
assert.equal(list[0].subject.commonName, 'Client Cert')
callback(list[0])
event.sender.send('client-certificate-response', list[0])
})

app.importCertificate(options, function (result) {
assert(!result)
server.listen(0, '127.0.0.1', function () {
var port = server.address().port
w.loadURL(`https://127.0.0.1:${port}`)
})
ipcRenderer.sendSync('set-client-certificate-option', false)
w.loadURL(secureUrl)
})
})
})
Expand Down Expand Up @@ -359,4 +375,32 @@ describe('app module', function () {
assert.equal(app.getPath('music'), __dirname)
})
})

describe('select-client-certificate event', function () {
let w = null

beforeEach(function () {
w = new BrowserWindow({
show: false,
webPreferences: {
partition: 'empty-certificate'
}
})
})

afterEach(function () {
return closeWindow(w).then(function () { w = null })
})

it('can respond with empty certificate list', function (done) {
w.webContents.on('did-finish-load', function () {
assert.equal(w.webContents.getTitle(), 'denied')
server.close()
done()
})

ipcRenderer.sendSync('set-client-certificate-option', true)
w.webContents.loadURL(secureUrl)
})
})
})
15 changes: 15 additions & 0 deletions spec/static/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,3 +184,18 @@ app.on('ready', function () {
}
})
})

ipcMain.on('set-client-certificate-option', function (event, skip) {
app.once('select-client-certificate', function (event, webContents, url, list, callback) {
event.preventDefault()
if (skip) {
callback()
} else {
ipcMain.on('client-certificate-response', function (event, certificate) {
callback(certificate)
})
window.webContents.send('select-client-certificate', webContents.id, list)
}
})
event.returnValue = 'done'
})
0