|
|
Created:
3 years, 11 months ago by irisu Modified:
3 years, 9 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, jam, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, nasko+codewatch_chromium.org, pfeldman+blink_chromium.org, Sami, clamy, Ryan Sleevi Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd certificate error handling to devtools.
This is necessary as headless chrome cannot show a UI warning for SSL
certificate errors. Instead, we can expose the errors as DevTools events
and control the action to take through a DevTools command.
BUG=659662
Review-Url: https://codereview.chromium.org/2639203003
Cr-Commit-Position: refs/heads/master@{#458673}
Committed: https://chromium.googlesource.com/chromium/src/+/8452ddd64771d3573cd7083f3e6e263cf67fbe1b
Patch Set 1 #Patch Set 2 : format #
Total comments: 28
Patch Set 3 : Add certificate error command tests #
Total comments: 2
Patch Set 4 : Fixed test. #
Total comments: 27
Patch Set 5 : Add event parameters #
Total comments: 14
Patch Set 6 : plumb error through agent host #
Total comments: 12
Patch Set 7 : Remove error code from certificateError event parameters #
Total comments: 2
Patch Set 8 : Send event straight through SecurityHandler from SSLManager #Patch Set 9 : Fix tests with PlzNavigate #
Total comments: 30
Patch Set 10 : Fix nits #Patch Set 11 : Fix comments #
Total comments: 12
Patch Set 12 : Add subresource test #
Total comments: 4
Patch Set 13 : Fix tests #
Total comments: 6
Patch Set 14 : Fix nits #Messages
Total messages: 131 (82 generated)
Description was changed from ========== Add certificate error handling to devtools. BUG=659662 ========== to ========== Add certificate error handling to devtools. BUG=659662 ==========
irisu@chromium.org changed reviewers: + eseckler@chromium.org
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks, Iris - This looks great! :) I think we can also loop in one of the DevTools owners (probably makes sense to send this to pfeldman@), they can advise on naming/location of the DevTools commands/events etc. https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1284: SendCommand("Security.enable", nullptr, true); I'm considering whether we should add an additional command that enables certificate error handling separately (e.g. "Security.enableCertificateErrorHandling"). That would allow clients to receive securityStateChanged notifications without also having to handle certificateError events - and means that things don't change for existing clients (which wouldn't know that they need to respond to certificateError events with the handleCertificateError command). WDYT? :) https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1287: WaitForNotification("Security.certificateError", true); Shall we also add tests for handleCertificateError and the different actions that we can take? https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:140: handlers_[++cert_error_id_] = handler; nit: with next_cert_error_id_, this should become next_cert_error_id_++. alternative is last_cert_error_id_ if you prefer :) https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:187: handlers_[event_id].Run(type); We should probably check above whether the provided event_id is valid, and otherwise answer with an error, too. https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:35: Response HandleCertificateError(int, const String&) override; nit: int event_id, const String& action (I think the style guide regarding parameter names in func def is to only omit them if they're obvious.) https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:49: int cert_error_id_; nit: next_cert_error_id_ We should probably also init this in constructor to some sensible value (0?). https://codereview.chromium.org/2639203003/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2639203003/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1473: handled = true; I think we have to break the loop in this case - otherwise, we could end up with two observers that want to handle the error / issue the callback. https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... content/public/browser/web_contents.h:436: using CertErrorHandler = nit: maybe CertErrorCallback is more descriptive of what this is? https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... content/public/browser/web_contents.h:438: // Invoked when a certificate error occurs. nit: mention in comment that this is to give observers a chance to handle the error and that return value indicates whether the certificate error will be handled by one of the observers. https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... content/public/browser/web_contents.h:439: virtual bool NotifyCertificateError(CertErrorHandler handler); nit: how about making this pure-virtual, i.e. virtual bool NotifyCertificateError(CertErrorHandler handler) = 0; https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... content/public/browser/web_contents_observer.h:302: using CertErrorHandler = nit: using CertErrorHandler = WebContents::CertErrorHandler https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... content/public/browser/web_contents_observer.h:304: // This method is invoked when there is an internal certificate error. nit: mention what return value indicates :) https://codereview.chromium.org/2639203003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:891: "description": "The action to take when a certificate error occurs." nit: add a short description about what each enum value means. https://codereview.chromium.org/2639203003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:929: "name": "certificateError", The bot seems to complain that Security.SecurityDispatcher doesn't yet handle this event. Looks like you'll have to add a stub implementation to the class in third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js
https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1284: SendCommand("Security.enable", nullptr, true); On 2017/01/19 11:42:28, Eric Seckler wrote: > I'm considering whether we should add an additional command that enables > certificate error handling separately (e.g. > "Security.enableCertificateErrorHandling"). > > That would allow clients to receive securityStateChanged notifications without > also having to handle certificateError events - and means that things don't > change for existing clients (which wouldn't know that they need to respond to > certificateError events with the handleCertificateError command). > > WDYT? :) Done. https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1287: WaitForNotification("Security.certificateError", true); On 2017/01/19 11:42:28, Eric Seckler wrote: > Shall we also add tests for handleCertificateError and the different actions > that we can take? As discussed - this has been a struggle. https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:140: handlers_[++cert_error_id_] = handler; On 2017/01/19 11:42:28, Eric Seckler wrote: > nit: with next_cert_error_id_, this should become next_cert_error_id_++. > alternative is last_cert_error_id_ if you prefer :) Done. https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:187: handlers_[event_id].Run(type); On 2017/01/19 11:42:28, Eric Seckler wrote: > We should probably check above whether the provided event_id is valid, and > otherwise answer with an error, too. Done. https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:35: Response HandleCertificateError(int, const String&) override; On 2017/01/19 11:42:28, Eric Seckler wrote: > nit: int event_id, const String& action > > (I think the style guide regarding parameter names in func def is to only omit > them if they're obvious.) Done. https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:49: int cert_error_id_; On 2017/01/19 11:42:28, Eric Seckler wrote: > nit: next_cert_error_id_ > > We should probably also init this in constructor to some sensible value (0?). Done. https://codereview.chromium.org/2639203003/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2639203003/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1473: handled = true; On 2017/01/19 11:42:28, Eric Seckler wrote: > I think we have to break the loop in this case - otherwise, we could end up with > two observers that want to handle the error / issue the callback. Done. https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... content/public/browser/web_contents.h:436: using CertErrorHandler = On 2017/01/19 11:42:28, Eric Seckler wrote: > nit: maybe CertErrorCallback is more descriptive of what this is? Done. https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... content/public/browser/web_contents.h:438: // Invoked when a certificate error occurs. On 2017/01/19 11:42:28, Eric Seckler wrote: > nit: mention in comment that this is to give observers a chance to handle the > error and that return value indicates whether the certificate error will be > handled by one of the observers. Done. https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... content/public/browser/web_contents.h:439: virtual bool NotifyCertificateError(CertErrorHandler handler); On 2017/01/19 11:42:28, Eric Seckler wrote: > nit: how about making this pure-virtual, i.e. > virtual bool NotifyCertificateError(CertErrorHandler handler) = 0; Done. https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... content/public/browser/web_contents_observer.h:302: using CertErrorHandler = On 2017/01/19 11:42:28, Eric Seckler wrote: > nit: using CertErrorHandler = WebContents::CertErrorHandler Done. https://codereview.chromium.org/2639203003/diff/20001/content/public/browser/... content/public/browser/web_contents_observer.h:304: // This method is invoked when there is an internal certificate error. On 2017/01/19 11:42:28, Eric Seckler wrote: > nit: mention what return value indicates :) Done. https://codereview.chromium.org/2639203003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:891: "description": "The action to take when a certificate error occurs." On 2017/01/19 11:42:28, Eric Seckler wrote: > nit: add a short description about what each enum value means. Done. https://codereview.chromium.org/2639203003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:929: "name": "certificateError", On 2017/01/19 11:42:28, Eric Seckler wrote: > The bot seems to complain that Security.SecurityDispatcher doesn't yet handle > this event. Looks like you'll have to add a stub implementation to the class in > third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js Done.
https://codereview.chromium.org/2639203003/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2639203003/diff/40001/content/browser/devtool... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1424: Attach(); Looks like loading a blank page first and reshuffling the WaitForLoadStop/Attach seems to work. Try: shell()->LoadURL(GURL("about://blank")); WaitForLoadStop(shell()->web_contents()); Attach(); SendCommand("Network.enable", nullptr, true); SendCommand("Security.enableCertificateErrorHandling", nullptr, true);
https://codereview.chromium.org/2639203003/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2639203003/diff/40001/content/browser/devtool... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1424: Attach(); On 2017/02/08 02:40:35, Eric Seckler wrote: > Looks like loading a blank page first and reshuffling the WaitForLoadStop/Attach > seems to work. Try: > > shell()->LoadURL(GURL("about://blank")); > WaitForLoadStop(shell()->web_contents()); > > Attach(); > SendCommand("Network.enable", nullptr, true); > SendCommand("Security.enableCertificateErrorHandling", nullptr, true); Awesome, Thanks!! Done.
irisu@chromium.org changed reviewers: + pfeldman@chromium.org
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
This looks great! Just some minor nits. https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:190: if (callbacks_.find(event_id) == callbacks_.end()) nit: add {} please https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:200: if (!enabled_) { I think we should just require the user to also call Enable() instead doing it for them. https://codereview.chromium.org/2639203003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1473: for (auto& observer : observers_) nit: I'd add braces around the multi-line for loop. https://codereview.chromium.org/2639203003/diff/60001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2639203003/diff/60001/content/public/browser/... content/public/browser/web_contents.h:439: // handle the error. The returned value indictates that an observer typo: indicates https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:904: "description": "The action to take when a certificate error occurs. continue will continue processing the request, cancel will cancel the request using net::ERR_ANORTED and deny will deny the request using net::ERR_INSECURE_RESPONSE." typo: ERR_ABORTED
Did you just want auto-responder here? That might be easier to implement altogether. https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:144: frontend_->CertificateError(last_cert_error_id_); You should report certificate errors whenever Security domain is enabled. Store handler conditionally instead. https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:156: Response SecurityHandler::Disable() { You should release all the handlers here with default values. https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:200: if (!enabled_) { +1, you also need to provide a way to disable it. setOverrideCertificateErrors https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl... content/browser/ssl/ssl_manager.cc:364: if (!web_contents->NotifyCertificateError(callback)) { Fetch devtools_agent_host_impl associated with the web_contents and plumb error there. https://codereview.chromium.org/2639203003/diff/60001/content/public/browser/... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/2639203003/diff/60001/content/public/browser/... content/public/browser/web_contents_observer.h:303: using CertErrorCallback = WebContents::CertErrorCallback; The code you change is all within content/, so you should not touch public API. https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:904: "description": "The action to take when a certificate error occurs. continue will continue processing the request, cancel will cancel the request using net::ERR_ANORTED and deny will deny the request using net::ERR_INSECURE_RESPONSE." For better or worse, we don't use internal terms when describing what protocol command does. https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:949: { "name": "eventID", "type": "integer", "description": "The ID of the event."} It is unlikely that I can make a decision programmatically just based on the event id, please provide error details. Or did you just want auto-responder here? That might be easier to implement altogether.
https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:949: { "name": "eventID", "type": "integer", "description": "The ID of the event."} On 2017/02/08 18:24:00, pfeldman wrote: > It is unlikely that I can make a decision programmatically just based on the > event id, please provide error details. Or did you just want auto-responder > here? That might be easier to implement altogether. Let's add some more information from the SSLManager to the event. I think this part is still WIP :) I'm not familiar with "auto-responder", but I think we'd like to let headless embedders make decisions.
https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:144: frontend_->CertificateError(last_cert_error_id_); On 2017/02/08 18:24:00, pfeldman wrote: > You should report certificate errors whenever Security domain is enabled. Store > handler conditionally instead. Done. https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:156: Response SecurityHandler::Disable() { On 2017/02/08 18:24:00, pfeldman wrote: > You should release all the handlers here with default values. Done. https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:190: if (callbacks_.find(event_id) == callbacks_.end()) On 2017/02/08 14:58:43, Sami wrote: > nit: add {} please Done. https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:200: if (!enabled_) { On 2017/02/08 18:24:00, pfeldman wrote: > +1, you also need to provide a way to disable it. setOverrideCertificateErrors Done. https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl... content/browser/ssl/ssl_manager.cc:364: if (!web_contents->NotifyCertificateError(callback)) { On 2017/02/08 18:24:00, pfeldman wrote: > Fetch devtools_agent_host_impl associated with the web_contents and plumb error > there. I'm not exactly sure how to do this. I can fetch the devtools_agent_host, but from there I'm not sure how to get to the security_handler. To my understanding, the handlers are attached to the devtools_session but I'm not sure how to access that. Mind giving me some pointers there, please? :) https://codereview.chromium.org/2639203003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1473: for (auto& observer : observers_) On 2017/02/08 14:58:43, Sami wrote: > nit: I'd add braces around the multi-line for loop. Done. https://codereview.chromium.org/2639203003/diff/60001/content/public/browser/... File content/public/browser/web_contents.h (right): https://codereview.chromium.org/2639203003/diff/60001/content/public/browser/... content/public/browser/web_contents.h:439: // handle the error. The returned value indictates that an observer On 2017/02/08 14:58:43, Sami wrote: > typo: indicates Done. https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:904: "description": "The action to take when a certificate error occurs. continue will continue processing the request, cancel will cancel the request using net::ERR_ANORTED and deny will deny the request using net::ERR_INSECURE_RESPONSE." On 2017/02/08 18:24:00, pfeldman wrote: > For better or worse, we don't use internal terms when describing what protocol > command does. Removed. https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:949: { "name": "eventID", "type": "integer", "description": "The ID of the event."} On 2017/02/08 18:53:10, Eric Seckler wrote: > On 2017/02/08 18:24:00, pfeldman wrote: > > It is unlikely that I can make a decision programmatically just based on the > > event id, please provide error details. Or did you just want auto-responder > > here? That might be easier to implement altogether. > > Let's add some more information from the SSLManager to the event. I think this > part is still WIP :) > > I'm not familiar with "auto-responder", but I think we'd like to let headless > embedders make decisions. Added extra parameters. Any more parameters you'd like added?
https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl... content/browser/ssl/ssl_manager.cc:364: if (!web_contents->NotifyCertificateError(callback)) { On 2017/02/14 05:46:14, irisu wrote: > On 2017/02/08 18:24:00, pfeldman wrote: > > Fetch devtools_agent_host_impl associated with the web_contents and plumb > error > > there. > > I'm not exactly sure how to do this. I can fetch the devtools_agent_host, but > from there I'm not sure how to get to the security_handler. To my understanding, > the handlers are attached to the devtools_session but I'm not sure how to access > that. Mind giving me some pointers there, please? :) We could add a static method to RenderFrameDevToolsAgentHost, similar to RenderFrameDevToolsAgentHost::UserAgentOverride. See its implementation here: https://cs.chromium.org/chromium/src/content/browser/devtools/render_frame_de... Pavel can advise whether that's the way to go :) https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:949: { "name": "eventID", "type": "integer", "description": "The ID of the event."} On 2017/02/14 05:46:14, irisu wrote: > On 2017/02/08 18:53:10, Eric Seckler wrote: > > On 2017/02/08 18:24:00, pfeldman wrote: > > > It is unlikely that I can make a decision programmatically just based on the > > > event id, please provide error details. Or did you just want auto-responder > > > here? That might be easier to implement altogether. > > > > Let's add some more information from the SSLManager to the event. I think this > > part is still WIP :) > > > > I'm not familiar with "auto-responder", but I think we'd like to let headless > > embedders make decisions. > > Added extra parameters. Any more parameters you'd like added? I'd be great to include some info about the certificate, but I'd be happy to add that later, too. WDYT, Pavel? https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:144: net::ErrorToShortString(cert_error), might be useful to have this both as descriptive string and as error code?
https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl... content/browser/ssl/ssl_manager.cc:364: if (!web_contents->NotifyCertificateError(callback)) { Something like that would do, I would even put this static method on the security_handler itself - that way you can emphasize that it covers non-frame target types as well. https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:187: if (action == "continue") { CertificateErrorActionTypeEnum::Continue https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:197: if (callbacks_.find(event_id) == callbacks_.end()) { You could do this first. https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:208: return Response::Error("Security not enabled"); Security domain not enabled https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:209: certificate_error_enabled_ = override; If override == false, you want to flush existing pending requests. Also, you might want to flush everything upon navigation. https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:52: int last_cert_error_id_; nit: you can now say = 0; here! https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:54: bool certificate_error_enabled_; nit: = false; !
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl... content/browser/ssl/ssl_manager.cc:364: if (!web_contents->NotifyCertificateError(callback)) { On 2017/02/14 19:00:24, pfeldman wrote: > Something like that would do, I would even put this static method on the > security_handler itself - that way you can emphasize that it covers non-frame > target types as well. I've added this in RenderFrameDevToolsAgentHost for now, as to my understanding, I can't use agent_host->session() outside of that since it is protected. Is there a different way I'm missing? https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:949: { "name": "eventID", "type": "integer", "description": "The ID of the event."} On 2017/02/14 18:42:24, Eric Seckler wrote: > On 2017/02/14 05:46:14, irisu wrote: > > On 2017/02/08 18:53:10, Eric Seckler wrote: > > > On 2017/02/08 18:24:00, pfeldman wrote: > > > > It is unlikely that I can make a decision programmatically just based on > the > > > > event id, please provide error details. Or did you just want > auto-responder > > > > here? That might be easier to implement altogether. > > > > > > Let's add some more information from the SSLManager to the event. I think > this > > > part is still WIP :) > > > > > > I'm not familiar with "auto-responder", but I think we'd like to let > headless > > > embedders make decisions. > > > > Added extra parameters. Any more parameters you'd like added? > > I'd be great to include some info about the certificate, but I'd be happy to add > that later, too. WDYT, Pavel? SSLInfo has a lot of fields we could add, but I'm not sure which would be the most useful here. I think we could leave that for later. https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:144: net::ErrorToShortString(cert_error), On 2017/02/14 18:42:25, Eric Seckler wrote: > might be useful to have this both as descriptive string and as error code? Done. https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:187: if (action == "continue") { On 2017/02/14 19:00:24, pfeldman wrote: > CertificateErrorActionTypeEnum::Continue Done. https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:197: if (callbacks_.find(event_id) == callbacks_.end()) { On 2017/02/14 19:00:24, pfeldman wrote: > You could do this first. Done. https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:208: return Response::Error("Security not enabled"); On 2017/02/14 19:00:24, pfeldman wrote: > Security domain not enabled Done. https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:209: certificate_error_enabled_ = override; On 2017/02/14 19:00:24, pfeldman wrote: > If override == false, you want to flush existing pending requests. Also, you > might want to flush everything upon navigation. Done. https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:52: int last_cert_error_id_; On 2017/02/14 19:00:24, pfeldman wrote: > nit: you can now say = 0; here! Done. https://codereview.chromium.org/2639203003/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:54: bool certificate_error_enabled_; On 2017/02/14 19:00:24, pfeldman wrote: > nit: = false; ! Done.
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:64: last_cert_error_id_(0), you no longer need these! https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:162: frontend_->CertificateError(++last_cert_error_id_, cert_error, if (!enabled_) return; https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:163: net::ErrorToShortString(cert_error), cert_error is internal to chrome, so we should not be providing it. https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:165: if (!certificate_error_enabled_) nit: this now needs a new name (certificate_errors_overriden_ or alike) https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:439: base::Callback<void(content::CertificateRequestResultType)> callback) { Otherwise this looks awkward: you call a static method and pass actual instance of that class as a first argument. Also, it works for workers as well, so going through this class is misleading. https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.h (right): https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.h:63: static bool NotifyCertificateError( You should talk directly to the security handler, there is no need to go through this class.
https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:163: net::ErrorToShortString(cert_error), On 2017/02/16 01:35:34, pfeldman wrote: > cert_error is internal to chrome, so we should not be providing it. Shall we add an enum type to the Security domain that covers the "Certificate error codes" from net/base/net_error_list.h? https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:439: base::Callback<void(content::CertificateRequestResultType)> callback) { On 2017/02/16 01:35:34, pfeldman wrote: > Otherwise this looks awkward: you call a static method and pass actual instance > of that class as a first argument. Also, it works for workers as well, so going > through this class is misleading. I think Iris was wondering if we can make DevToolsAgentHostImpl::session() a public accessor. It's protected at the moment, but also has a "TODO(dgozman): remove this accessor", so it's not really clear to us what's up with that :)
https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:64: last_cert_error_id_(0), On 2017/02/16 01:35:34, pfeldman wrote: > you no longer need these! Done. https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:162: frontend_->CertificateError(++last_cert_error_id_, cert_error, On 2017/02/16 01:35:34, pfeldman wrote: > if (!enabled_) > return; Done. https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:165: if (!certificate_error_enabled_) On 2017/02/16 01:35:34, pfeldman wrote: > nit: this now needs a new name (certificate_errors_overriden_ or alike) Done. https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:439: base::Callback<void(content::CertificateRequestResultType)> callback) { On 2017/02/16 02:01:07, Eric Seckler wrote: > On 2017/02/16 01:35:34, pfeldman wrote: > > Otherwise this looks awkward: you call a static method and pass actual > instance > > of that class as a first argument. Also, it works for workers as well, so > going > > through this class is misleading. > > I think Iris was wondering if we can make DevToolsAgentHostImpl::session() a > public accessor. It's protected at the moment, but also has a "TODO(dgozman): > remove this accessor", so it's not really clear to us what's up with that :) Yes, that's what I'm confused about.
Patchset #7 (id:140001) has been deleted
>> Shall we add an enum type to the Security domain that covers the "Certificate error codes" from net/base/net_error_list.h? I'd just pass string. >> I think Iris was wondering if we can make DevToolsAgentHostImpl::session() ... https://codereview.chromium.org/2698883004/
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/16 at 22:18:22, pfeldman wrote: > >> Shall we add an enum type to the Security domain that covers the "Certificate error codes" from net/base/net_error_list.h? > > I'd just pass string. > > >> I think Iris was wondering if we can make DevToolsAgentHostImpl::session() ... > > https://codereview.chromium.org/2698883004/ Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #10 (id:220001) has been deleted
Patchset #9 (id:200001) has been deleted
I've hit another problem with the Network Domain events. I need to run the test with the enable-browser-side-navigation flag in order for the trybot tests to pass. When I do, the behaviour of the network domain changes. Firstly, for the 'cancel' action, no network events are received after cancelling, including the Network.loadingFailed event. Secondly, for the 'deny' action, I get Network.requestServedFromCache, followed by Network.loadingFinished. I've also tried sending the command Network.clearBrowserCache before each of the cases, but the issue remains. Should I modify the test such that the test will pass in both cases? Or use something other than the network domain to test the different actions?
Description was changed from ========== Add certificate error handling to devtools. BUG=659662 ========== to ========== Add certificate error handling to devtools. BUG=659662 ==========
Camille, can you comment on the enable-browser-side-navigation (PlzNavigate) question here? Is the behavior change expected, and should we just make sure the tests work in the new world where browser side navigation is on?
pfeldman@chromium.org changed reviewers: + dgozman@chromium.org
+dgozman who was making sure network domain behaves with PlzNavigate.
one of the comments i forgot to send out previously. https://codereview.chromium.org/2639203003/diff/160001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/160001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:145: void SecurityHandler::DidFinishLoad(RenderFrameHost* render_frame_host, This is too late, use DidFinishNavigation instead.
> Firstly, for the 'cancel' action, no network events are received after > cancelling, including the Network.loadingFailed event. That's interesting. Sounds like a bug. Camille? > Secondly, for the 'deny' action, I get Network.requestServedFromCache, followed > by Network.loadingFinished. I've also tried sending the command > Network.clearBrowserCache before each of the cases, but the issue remains. Can you print out the payload? What content is coming from the cache? > Should I modify the test such that the test will pass in both cases? Or use > something other than the network domain to test the different actions? If you use XHR instead of navigation, PlzNavigate will not stand on your way. But I think ensuring this actually works with PlzNavigate is important. Did you test this manually - does it navigate or not?
On 2017/02/21 19:34:05, dgozman wrote: > > Firstly, for the 'cancel' action, no network events are received after > > cancelling, including the Network.loadingFailed event. > That's interesting. Sounds like a bug. Camille? > > > Secondly, for the 'deny' action, I get Network.requestServedFromCache, > followed > > by Network.loadingFinished. I've also tried sending the command > > Network.clearBrowserCache before each of the cases, but the issue remains. > Can you print out the payload? What content is coming from the cache? I added a command: {"id":10,"method":"DOM.getFlattenedDocument"} which gives this reply: {"id":10,"result":{"nodes":[{"nodeId":3,"parentId":2,"backendNodeId":4,"nodeType":1,"nodeName":"HEAD","localName":"head","nodeValue":"","childNodeCount":0,"children":[],"attributes":[]},{"nodeId":4,"parentId":2,"backendNodeId":5,"nodeType":1,"nodeName":"BODY","localName":"body","nodeValue":"","childNodeCount":0,"children":[],"attributes":[]},{"nodeId":2,"parentId":1,"backendNodeId":3,"nodeType":1,"nodeName":"HTML","localName":"html","nodeValue":"","childNodeCount":2,"children":[],"attributes":[],"frameId":"109365.1"},{"nodeId":1,"backendNodeId":2,"nodeType":9,"nodeName":"#document","localName":"","nodeValue":"","childNodeCount":1,"children":[],"documentURL":"data:text/html,chromewebdata","baseURL":"data:text/html,chromewebdata","xmlVersion":""}]}} > > > Should I modify the test such that the test will pass in both cases? Or use > > something other than the network domain to test the different actions? > If you use XHR instead of navigation, PlzNavigate will not stand on your way. > But I think ensuring this actually works with PlzNavigate is important. Did you > test this manually - does it navigate or not? I've tried manually sending devtools commands. 'Cancel' does not navigate. 'Deny' depends on the history: If I've previously sent 'continue', it will immediately load the page without waiting for the handleCertificateError command (even though I've done both Network.clearBrowserCache and Network.clearBrowserCookies). If I have not continued with the request previously or if I clear the browser history manually through chrome://settings/clearBrowserData , then I get a page with "This site can't be reached" and "ERR_INSECURE_RESPONSE".
https://codereview.chromium.org/2639203003/diff/160001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/160001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:145: void SecurityHandler::DidFinishLoad(RenderFrameHost* render_frame_host, On 2017/02/21 18:51:41, pfeldman wrote: > This is too late, use DidFinishNavigation instead. Done.
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Fixed the tests to work with PlzNavigate.
eseckler@chromium.org changed reviewers: + palmer@chromium.org
small nits, otherwise lgtm. Adding palmer@ as one of the security owners/reviewers, since they should probably be aware of this too. +cc rsleevi@ https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1428: SendCommand("Network.enable", nullptr, false); Not quite sure, but as I understood it, we should wait for the response to Network.enable to support PlzNavigate in a flakeless manner? https://codereview.chromium.org/2639203003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:940: "description": "Enable/disable overriding certificate errors.", maybe add "If enabled, all certificate error events need to be handled by the DevTools client and should be answered with handleCertificateError commands." https://codereview.chromium.org/2639203003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:942: { "name": "override", "type": "boolean", "description": "If true, certificate errors will be overriden."} nit: overridden.
rsleevi@chromium.org changed reviewers: + estark@chromium.org, rsleevi@chromium.org
Going to ask estark to look at this, as she's probably got the most knowledge in this space, especially around the security risks of having devtools be able to bypass errors as well as any performance and layering implications from adding this to the SSL manager.
Also, can you please update the description to be more descriptive. http://chris.beams.io/posts/git-commit/ is a useful resource for the principles here.
Description was changed from ========== Add certificate error handling to devtools. BUG=659662 ========== to ========== Add certificate error handling to devtools. This is necessary as headless chrome cannot show a UI warning for SSL certificate errors. Instead, we can expose the errors as DevTools events and control the action to take through a DevTools command. BUG=659662 ==========
On 2017/02/28 18:53:04, Ryan Sleevi (overloaded) wrote: > Also, can you please update the description to be more descriptive. > > http://chris.beams.io/posts/git-commit/ is a useful resource for the principles > here. Done.
Description was changed from ========== Add certificate error handling to devtools. This is necessary as headless chrome cannot show a UI warning for SSL certificate errors. Instead, we can expose the errors as DevTools events and control the action to take through a DevTools command. BUG=659662 ========== to ========== Add certificate error handling to devtools. This is necessary as headless chrome cannot show a UI warning for SSL certificate errors. Instead, we can expose the errors as DevTools events and control the action to take through a DevTools command. BUG=659662 ==========
https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1428: SendCommand("Network.enable", nullptr, false); On 2017/02/28 08:41:17, Eric Seckler wrote: > Not quite sure, but as I understood it, we should wait for the response to > Network.enable to support PlzNavigate in a flakeless manner? Done. https://codereview.chromium.org/2639203003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:940: "description": "Enable/disable overriding certificate errors.", On 2017/02/28 08:41:17, Eric Seckler wrote: > maybe add "If enabled, all certificate error events need to be handled by the > DevTools client and should be answered with handleCertificateError commands." Done. https://codereview.chromium.org/2639203003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:942: { "name": "override", "type": "boolean", "description": "If true, certificate errors will be overriden."} On 2017/02/28 08:41:17, Eric Seckler wrote: > nit: overridden. Done.
I *think* that this is conceptually okay security-wise. My reasoning is that the message to proceed through a security interstitial already comes from the renderer, so a compromised renderer can bypass cert errors. It so happens that DevTools doesn't really work on security interstitials, so it may be the case that it's not currently possible for a compromised DevTools to click through a cert error for the page it's attached to -- but if that's the case, it's probably kind of by accident, not an intentional security boundary. I have a few higher-level comments that I interspersed below with some more minor nits, so I'll pull out the high-level comments here: 1.) Security: Is it possible for a malicious DevTools to send a message which will be handled by the SecurityHandler instance for some other DevTools instance? While it seems defensible that DevTools should be able to click through cert errors for the page it's attached to, it's less clear to me that it's okay for a malicious DevTools to send a message which clicks through an error for some other page. 2.) Also security: I don't think that errors bypassed by DevTools should be remembered the same way that interstitial click-throughs are (1 week). So if DevTools sends a CERTIFICATE_REQUEST_RESULT_TYPE_CONTINUE message, we probably don't want to call state_delegate->AllowCert in SSLManager::OnAllowCertificate as we do now. Instead, I guess we probably want to remember the exception just for the duration of the page load...? 3.) Is it necessary to expose subresource cert errors to DevTools, or could we get away with just exposing cert errors on top-level navigation requests? The latter is what is exposed to users/renderers today, so it would be a bit easier to reason about if we could expose only that to devtools. https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:165: frontend_->CertificateError(++last_cert_error_id_, Is it necessary to send the error even if |certificate_errors_overridden_| is false? https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:205: if (callbacks_.find(event_id) == callbacks_.end()) { Is it possible for one devtools process to send a message to the browser that will be routed to the SecurityHandler corresponding to another devtools instance? If so, I think that might be a security problem -- one devtools process probably shouldn't be able to bypass certificate errors in another unrelated renderer. https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:215: type = content::CERTIFICATE_REQUEST_RESULT_TYPE_CANCEL; Do we need to expose both CANCEL and DENY to devtools? I wonder if we could get away with just exposing CANCEL, since that's what clicking through an interstitial does. https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:35: Response Enable() override; optional nit: while you're here, if you wouldn't mind adding "// Security::Backend overrides" here and "// DevToolsDomainHandler overrides" above line 30, that would be very much appreciated! https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:41: using CertErrorCallback = per https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#..., this should be at the top of the 'public:' section https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:43: using CertErrorCallbackMap = std::unordered_map<int, CertErrorCallback>; this one could be private, right? https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:45: bool NotifyCertificateError(int cert_error, Please document (especially what the return value means) https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:51: void FlushPendingRequests(); nit: maybe FlushPendingCertificateErrorNotifications? That's kinda long I guess. But it's unclear from the current name that this is specific to certificate errors. https://codereview.chromium.org/2639203003/diff/240001/content/browser/ssl/ss... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/240001/content/browser/ssl/ss... content/browser/ssl/ssl_manager.cc:366: Note that this code won't fire for all cases of certificate errors; for example, if the SSLHostStateDelegate::QueryPolicy call in SSLManager::OnCertError decides that the error should be allowed, we won't reach this point. It sounds like that it probably okay for your use case, but might be worth documenting somewhere. https://codereview.chromium.org/2639203003/diff/240001/content/browser/ssl/ss... content/browser/ssl/ssl_manager.cc:372: !security_handler->NotifyCertificateError(cert_error, request_url, As I mentioned elsewhere, I think it would be better if this did not fire for subresources but only for top-level navigations. https://codereview.chromium.org/2639203003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:932: "description": "Handles a certificate error.", Does this handle all certificate errors, or only top-level navigations? It would be best if it were only the latter, since we don't give a way for the user to bypass cert errors on subresources right now.
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #12 (id:300001) has been deleted
Patchset #11 (id:280001) has been deleted
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
1) @pfeldman, @dgozman, Could you answer this please? As you are more familiar :) 2) Addressed the issue with remembering the 'continue' message, although I'm not sure this is the best way. Alternatively, I can add a bool to the CertErrorCallback. But wdyt? 3) Added check for this. A similar check is in ChromeContentBrowserClient::AllowCertificate error - should I remove that one? https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:165: frontend_->CertificateError(++last_cert_error_id_, On 2017/02/28 23:34:01, estark wrote: > Is it necessary to send the error even if |certificate_errors_overridden_| is > false? @pfeldman earlier commented that the error should be sent if the security domain is enabled. I don't think it's strictly necessary, although nice to have. It could be removed - unless Pavel has an objection? As it is, I can see it may be confusing as to whether a certificateError event requires handling. If we want to continue sending the event regardless of whether certificate_errors_overridden is set, then perhaps we should add a bool to indicate whether handling is required and/or set eventID = -1. WYDT? https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:205: if (callbacks_.find(event_id) == callbacks_.end()) { On 2017/02/28 23:34:01, estark wrote: > Is it possible for one devtools process to send a message to the browser that > will be routed to the SecurityHandler corresponding to another devtools > instance? If so, I think that might be a security problem -- one devtools > process probably shouldn't be able to bypass certificate errors in another > unrelated renderer. I'll defer this one to the devtools people: @dgozman, @pfeldman? https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:215: type = content::CERTIFICATE_REQUEST_RESULT_TYPE_CANCEL; On 2017/02/28 23:34:01, estark wrote: > Do we need to expose both CANCEL and DENY to devtools? I wonder if we could get > away with just exposing CANCEL, since that's what clicking through an > interstitial does. Done. https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:35: Response Enable() override; On 2017/02/28 23:34:01, estark wrote: > optional nit: while you're here, if you wouldn't mind adding "// > Security::Backend overrides" here and "// DevToolsDomainHandler overrides" above > line 30, that would be very much appreciated! Done. https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:41: using CertErrorCallback = On 2017/02/28 23:34:01, estark wrote: > per > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#..., > this should be at the top of the 'public:' section Done. https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:43: using CertErrorCallbackMap = std::unordered_map<int, CertErrorCallback>; On 2017/02/28 23:34:01, estark wrote: > this one could be private, right? Done. https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:45: bool NotifyCertificateError(int cert_error, On 2017/02/28 23:34:01, estark wrote: > Please document (especially what the return value means) Done. https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:51: void FlushPendingRequests(); On 2017/02/28 23:34:01, estark wrote: > nit: maybe FlushPendingCertificateErrorNotifications? That's kinda long I guess. > But it's unclear from the current name that this is specific to certificate > errors. Done. https://codereview.chromium.org/2639203003/diff/240001/content/browser/ssl/ss... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/240001/content/browser/ssl/ss... content/browser/ssl/ssl_manager.cc:366: On 2017/02/28 23:34:03, estark wrote: > Note that this code won't fire for all cases of certificate errors; for example, > if the SSLHostStateDelegate::QueryPolicy call in SSLManager::OnCertError decides > that the error should be allowed, we won't reach this point. > > It sounds like that it probably okay for your use case, but might be worth > documenting somewhere. Done. https://codereview.chromium.org/2639203003/diff/240001/content/browser/ssl/ss... content/browser/ssl/ssl_manager.cc:372: !security_handler->NotifyCertificateError(cert_error, request_url, On 2017/02/28 23:34:04, estark wrote: > As I mentioned elsewhere, I think it would be better if this did not fire for > subresources but only for top-level navigations. Done. https://codereview.chromium.org/2639203003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:932: "description": "Handles a certificate error.", On 2017/02/28 23:34:05, estark wrote: > Does this handle all certificate errors, or only top-level navigations? It would > be best if it were only the latter, since we don't give a way for the user to > bypass cert errors on subresources right now. Done. Added a comment to clarify that this is for responding to certificateError events when setOverrideCertificateErrors is set and the events should only trigger for top-level navigations. https://codereview.chromium.org/2639203003/diff/320001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2639203003/diff/320001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2295: if (!callback.is_null()) { Ignore brackets. (Will fix when back on workstation tomorrow - just want to send this out now to get comments on the rest.)
1.) Security: Is it possible for a malicious DevTools to send a message which will be handled by the SecurityHandler instance for some other DevTools instance? There is no such thing as "malicious DevTools", this is essentially "malicious browser"
PTAL
Sorry for the delay. Have been discussing this more generally with the security team. I don't think it's necessary to block this CL on it, but 'There is no such thing as "malicious DevTools", this is essentially "malicious browser"' is more-or-less news to most security people I've talked to about it. So that might be worth a broader security <-> devtools discussion. (For example, I think historically we've considered devtools XSS to be low or medium severity, whereas it definitely should be high if devtools XSS => browser compromise.) https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:165: frontend_->CertificateError(++last_cert_error_id_, On 2017/03/06 11:56:48, irisu wrote: > On 2017/02/28 23:34:01, estark wrote: > > Is it necessary to send the error even if |certificate_errors_overridden_| is > > false? > > @pfeldman earlier commented that the error should be sent if the security domain > is enabled. I don't think it's strictly necessary, although nice to have. It > could be removed - unless Pavel has an objection? > > As it is, I can see it may be confusing as to whether a certificateError event > requires handling. If we want to continue sending the event regardless of > whether certificate_errors_overridden is set, then perhaps we should add a bool > to indicate whether handling is required and/or set eventID = -1. WYDT? Either way sounds fine to me; I'm not too worried about the specific behavior, more that the code is a little confusing to read because it's not clear why we're sending the error if we're not giving devtools any way to do something about it. So, whatever you decide, please add a comment explaining why it is the way it is. https://codereview.chromium.org/2639203003/diff/320001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/2639203003/diff/320001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:47: // value indicates whether the error is expected to be handled by a nit: "Returns true if the error is expected to be handled by a corresponding HandleCertificateError command, and false otherwise." https://codereview.chromium.org/2639203003/diff/320001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:67: CertErrorCallbackMap callbacks_; nit: perhaps call this |cert_error_callback_| to be more explicit https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ss... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ss... content/browser/ssl/ssl_manager.cc:69: state_delegate->AllowCert(handler->request_url().host(), I'm not sure that this will work; have you tested it on a page that has subresources? (like a cert error on www.blah.com that loads an image www.blah.com/foo.jpeg) I think that in order for that to work reliably, you'll either need to expose cert errors on subresources to devtools (undoing my previous suggestion), or you'll need to change the SSLHostStateDelegate so that it stores cert error decisions differently for when devtools bypasses them. Of those two options, the first is probably way simpler. So I guess I led you astray on subresources -- if we want devtools to be able to load a full page after clicking through a cert error without remembering the decision for a week as we do for normal interstitials, then devtools needs to be able to bypass cert errors on subresources. (I assume that asking devtools to bypass a cert error on every subresource could have performance implications, but I'm not familiar enough with this project to determine if that's a blocker or not.) https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ss... content/browser/ssl/ssl_manager.cc:376: if (resource_type != RESOURCE_TYPE_MAIN_FRAME) { Per my comment above, I think we need to go back on this and actually expose subresource cert errors to devtools if it needs to be able to load a full page (not just the main resource) without relying on cert decisions being remembered.
>> So that might be worth a broader security <-> devtools discussion. Sure, we have these roughly once in two years, probably time for another one :)
On 2017/03/09 00:32:25, pfeldman wrote: > >> So that might be worth a broader security <-> devtools discussion. > > Sure, we have these roughly once in two years, probably time for another one :) Is it documented anywhere? If this keeps coming up, maybe we just need to write it down somewhere so we don't have to keep bothering you :)
> Is it documented anywhere? If this keeps coming up, maybe we just need to write > it down somewhere so we don't have to keep bothering you :) That's a good idea, following up offline.
https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ss... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ss... content/browser/ssl/ssl_manager.cc:69: state_delegate->AllowCert(handler->request_url().host(), On 2017/03/09 00:24:31, estark wrote: > I'm not sure that this will work; have you tested it on a page that has > subresources? (like a cert error on http://www.blah.com that loads an image > http://www.blah.com/foo.jpeg) > > I think that in order for that to work reliably, you'll either need to expose > cert errors on subresources to devtools (undoing my previous suggestion), or > you'll need to change the SSLHostStateDelegate so that it stores cert error > decisions differently for when devtools bypasses them. > > Of those two options, the first is probably way simpler. So I guess I led you > astray on subresources -- if we want devtools to be able to load a full page > after clicking through a cert error without remembering the decision for a week > as we do for normal interstitials, then devtools needs to be able to bypass cert > errors on subresources. > > (I assume that asking devtools to bypass a cert error on every subresource could > have performance implications, but I'm not familiar enough with this project to > determine if that's a blocker or not.) For use in headless, I think we'll be happy with having to handle errors for every subresource. May not be ideal, but given the effort required for the alternative, it sounds like the better choice :)
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:165: frontend_->CertificateError(++last_cert_error_id_, On 2017/03/09 00:24:30, estark wrote: > On 2017/03/06 11:56:48, irisu wrote: > > On 2017/02/28 23:34:01, estark wrote: > > > Is it necessary to send the error even if |certificate_errors_overridden_| > is > > > false? > > > > @pfeldman earlier commented that the error should be sent if the security > domain > > is enabled. I don't think it's strictly necessary, although nice to have. It > > could be removed - unless Pavel has an objection? > > > > As it is, I can see it may be confusing as to whether a certificateError event > > requires handling. If we want to continue sending the event regardless of > > whether certificate_errors_overridden is set, then perhaps we should add a > bool > > to indicate whether handling is required and/or set eventID = -1. WYDT? > > Either way sounds fine to me; I'm not too worried about the specific behavior, > more that the code is a little confusing to read because it's not clear why > we're sending the error if we're not giving devtools any way to do something > about it. So, whatever you decide, please add a comment explaining why it is the > way it is. Done. https://codereview.chromium.org/2639203003/diff/320001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/2639203003/diff/320001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:47: // value indicates whether the error is expected to be handled by a On 2017/03/09 00:24:31, estark wrote: > nit: "Returns true if the error is expected to be handled by a corresponding > HandleCertificateError command, and false otherwise." Done. https://codereview.chromium.org/2639203003/diff/320001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:67: CertErrorCallbackMap callbacks_; On 2017/03/09 00:24:31, estark wrote: > nit: perhaps call this |cert_error_callback_| to be more explicit Done. https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ss... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ss... content/browser/ssl/ssl_manager.cc:69: state_delegate->AllowCert(handler->request_url().host(), On 2017/03/09 00:24:31, estark wrote: > I'm not sure that this will work; have you tested it on a page that has > subresources? (like a cert error on http://www.blah.com that loads an image > http://www.blah.com/foo.jpeg) Added an image to the test html, let me know if that's not what you had in mind - but it seems to be working without needing an extra certificateError event for the image. https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ss... content/browser/ssl/ssl_manager.cc:376: if (resource_type != RESOURCE_TYPE_MAIN_FRAME) { On 2017/03/09 00:24:31, estark wrote: > Per my comment above, I think we need to go back on this and actually expose > subresource cert errors to devtools if it needs to be able to load a full page > (not just the main resource) without relying on cert decisions being remembered. Done.
Thanks, looking good! Just a few comments inline about the tests. Also, a high-level question that I should have asked a while ago: can you give me some examples of how this functionality will be used? (I guess, what are the use cases for headless chrome in general? Is there a design doc somewhere that I can read?) Just want to make sure that we're exposing the right information to devtools so that the use cases are safe. https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ss... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ss... content/browser/ssl/ssl_manager.cc:69: state_delegate->AllowCert(handler->request_url().host(), On 2017/03/13 01:56:56, irisu wrote: > On 2017/03/09 00:24:31, estark wrote: > > I'm not sure that this will work; have you tested it on a page that has > > subresources? (like a cert error on http://www.blah.com that loads an image > > http://www.blah.com/foo.jpeg) > > Added an image to the test html, let me know if that's not what you had in mind > - but it seems to be working without needing an extra certificateError event for > the image. By "it seems to be working", do you mean that the test works, or did you manually test loading a page with an image and see that it works without an event for the subresource? If the latter, there might be some connection pooling or some such -- the subresource might not need a new connection. Could you please add a browser test for the subresource case? For example you could add a test where the main page does not trigger a cert error but a subresource on the page does. I think you can probably adapt a test like https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser... to set that up. https://codereview.chromium.org/2639203003/diff/340001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2639203003/diff/340001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1440: SendCommand("Security.handleCertificateError", std::move(command_params), Hmm, this test doesn't currently check that the command is actually handled correctly, IIUC. It seems to me that the expected state is that a navigation is in progress before the handleCertificateError command is sent, and then is stopped. Is that correct? And if so, is there any way to use TestNavigationObserver to test for that? I'm imagining something like this (in pseudocode): TestNavigationObserver cancel_observer; shell()->LoadUrl(...); WaitForNotification("Security.certificateError", ...); EXPECT_EQ(test_url, shell()->web_contents()->GetController().GetTransientEntry()->GetURL()); ... SendCommand("Security.handleCertificateError"...); cancel_observer.Wait(); EXPECT_FALSE(shell()->web_contents()->GetController().GetPendingEntry()); EXPECT_EQ(GURL("about://blank"), shell()->web_contents()->GetController().GetLastCommittedEntry()->GetURL())); something like that, maybe? https://codereview.chromium.org/2639203003/diff/340001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1456: continue_observer.Wait(); Maybe add an expectation that you ended up on the expected page?
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ss... File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ss... content/browser/ssl/ssl_manager.cc:69: state_delegate->AllowCert(handler->request_url().host(), On 2017/03/14 01:22:50, estark wrote: > On 2017/03/13 01:56:56, irisu wrote: > > On 2017/03/09 00:24:31, estark wrote: > > > I'm not sure that this will work; have you tested it on a page that has > > > subresources? (like a cert error on http://www.blah.com that loads an image > > > http://www.blah.com/foo.jpeg) > > > > Added an image to the test html, let me know if that's not what you had in > mind > > - but it seems to be working without needing an extra certificateError event > for > > the image. > > By "it seems to be working", do you mean that the test works, or did you > manually test loading a page with an image and see that it works without an > event for the subresource? If the latter, there might be some connection pooling > or some such -- the subresource might not need a new connection. > > Could you please add a browser test for the subresource case? For example you > could add a test where the main page does not trigger a cert error but a > subresource on the page does. > > I think you can probably adapt a test like > https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser... > to set that up. Done. I added the subresource case and figured out the issue with my previous approach. https://codereview.chromium.org/2639203003/diff/340001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2639203003/diff/340001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1440: SendCommand("Security.handleCertificateError", std::move(command_params), On 2017/03/14 01:22:50, estark wrote: > Hmm, this test doesn't currently check that the command is actually handled > correctly, IIUC. > > It seems to me that the expected state is that a navigation is in progress > before the handleCertificateError command is sent, and then is stopped. Is that > correct? And if so, is there any way to use TestNavigationObserver to test for > that? I'm imagining something like this (in pseudocode): > > TestNavigationObserver cancel_observer; > shell()->LoadUrl(...); > WaitForNotification("Security.certificateError", ...); > EXPECT_EQ(test_url, > shell()->web_contents()->GetController().GetTransientEntry()->GetURL()); > ... > SendCommand("Security.handleCertificateError"...); > cancel_observer.Wait(); > EXPECT_FALSE(shell()->web_contents()->GetController().GetPendingEntry()); > EXPECT_EQ(GURL("about://blank"), > shell()->web_contents()->GetController().GetLastCommittedEntry()->GetURL())); > > something like that, maybe? Done. https://codereview.chromium.org/2639203003/diff/340001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1456: continue_observer.Wait(); On 2017/03/14 01:22:50, estark wrote: > Maybe add an expectation that you ended up on the expected page? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, lgtm!
@pfeldman, are you happy with this?
lgtm % nits https://codereview.chromium.org/2639203003/diff/360001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/360001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:166: // Send certificateError to devtools frontend to inform, but do not I would always generate the id, regardless on whether certificate errors are overridden. Client knows whether it needs to perform any action. https://codereview.chromium.org/2639203003/diff/360001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:176: cert_error_callbacks_[last_cert_error_id_] = handler; But still put it into the map conditionally. https://codereview.chromium.org/2639203003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:934: { "name": "eventID", "type": "integer", "description": "The ID of the event."}, nit: eventId, we don't capitalize abbreviations.
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
https://codereview.chromium.org/2639203003/diff/360001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/360001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:166: // Send certificateError to devtools frontend to inform, but do not On 2017/03/22 00:47:32, pfeldman_slow wrote: > I would always generate the id, regardless on whether certificate errors are > overridden. Client knows whether it needs to perform any action. Done. https://codereview.chromium.org/2639203003/diff/360001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:176: cert_error_callbacks_[last_cert_error_id_] = handler; On 2017/03/22 00:47:32, pfeldman_slow wrote: > But still put it into the map conditionally. Done. https://codereview.chromium.org/2639203003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:934: { "name": "eventID", "type": "integer", "description": "The ID of the event."}, On 2017/03/22 00:47:32, pfeldman wrote: > nit: eventId, we don't capitalize abbreviations. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by irisu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eseckler@chromium.org, estark@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2639203003/#ps380001 (title: "Fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 380001, "attempt_start_ts": 1490159564400690, "parent_rev": "99e1c588417175adad9d67179695591ba69383a5", "commit_rev": "8452ddd64771d3573cd7083f3e6e263cf67fbe1b"}
Message was sent while issue was closed.
Description was changed from ========== Add certificate error handling to devtools. This is necessary as headless chrome cannot show a UI warning for SSL certificate errors. Instead, we can expose the errors as DevTools events and control the action to take through a DevTools command. BUG=659662 ========== to ========== Add certificate error handling to devtools. This is necessary as headless chrome cannot show a UI warning for SSL certificate errors. Instead, we can expose the errors as DevTools events and control the action to take through a DevTools command. BUG=659662 Review-Url: https://codereview.chromium.org/2639203003 Cr-Commit-Position: refs/heads/master@{#458673} Committed: https://chromium.googlesource.com/chromium/src/+/8452ddd64771d3573cd7083f3e6e... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:380001) as https://chromium.googlesource.com/chromium/src/+/8452ddd64771d3573cd7083f3e6e... |