-
Notifications
You must be signed in to change notification settings - Fork 16.2k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -378,9 +378,21 @@ void OnClientCertificateSelected( | |
v8::Isolate* isolate, | ||
std::shared_ptr<content::ClientCertificateDelegate> delegate, | ||
mate::Arguments* args) { | ||
if (args->Length() == 2) { | ||
delegate->ContinueWithCertificate(nullptr); | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
|
There was a problem hiding this comment.
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 callingcallback(cert, null, null)
would continue with the cert.What case is this targeting?
There was a problem hiding this comment.
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. Herecallback
has already two bound arguments on c++ side, so whencallback()
is called, the condition will be satisfied.There was a problem hiding this comment.
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 👍