Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(180)

Issue 2639203003: Add certificate error handling to devtools. (Closed)

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.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+278 lines, -9 lines) Patch
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M content/browser/devtools/protocol/devtools_protocol_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +115 lines, -0 lines 0 comments Download
M content/browser/devtools/protocol/security_handler.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +26 lines, -2 lines 0 comments Download
M content/browser/devtools/protocol/security_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +59 lines, -0 lines 0 comments Download
M content/browser/devtools/render_frame_devtools_agent_host.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M content/browser/ssl/ssl_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +30 lines, -5 lines 0 comments Download
A content/test/data/devtools/image.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +6 lines, -0 lines 0 comments Download
A content/test/data/devtools/test.jpg View 1 2 3 4 5 6 7 8 9 10 11 Binary file 0 comments Download
M third_party/WebKit/Source/core/inspector/browser_protocol.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +30 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 131 (82 generated)
irisu
3 years, 11 months ago (2017-01-19 10:43:03 UTC) #7
Eric Seckler
Thanks, Iris - This looks great! :) I think we can also loop in one ...
3 years, 11 months ago (2017-01-19 11:42:29 UTC) #8
irisu
https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtools/protocol/devtools_protocol_browsertest.cc File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2639203003/diff/20001/content/browser/devtools/protocol/devtools_protocol_browsertest.cc#newcode1284 content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1284: SendCommand("Security.enable", nullptr, true); On 2017/01/19 11:42:28, Eric Seckler wrote: ...
3 years, 10 months ago (2017-02-07 23:30:20 UTC) #9
Eric Seckler
https://codereview.chromium.org/2639203003/diff/40001/content/browser/devtools/protocol/devtools_protocol_browsertest.cc File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2639203003/diff/40001/content/browser/devtools/protocol/devtools_protocol_browsertest.cc#newcode1424 content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1424: Attach(); Looks like loading a blank page first and ...
3 years, 10 months ago (2017-02-08 02:40:35 UTC) #10
irisu
https://codereview.chromium.org/2639203003/diff/40001/content/browser/devtools/protocol/devtools_protocol_browsertest.cc File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2639203003/diff/40001/content/browser/devtools/protocol/devtools_protocol_browsertest.cc#newcode1424 content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1424: Attach(); On 2017/02/08 02:40:35, Eric Seckler wrote: > Looks ...
3 years, 10 months ago (2017-02-08 03:53:24 UTC) #11
irisu
3 years, 10 months ago (2017-02-08 03:53:54 UTC) #13
Sami
This looks great! Just some minor nits. https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtools/protocol/security_handler.cc File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtools/protocol/security_handler.cc#newcode190 content/browser/devtools/protocol/security_handler.cc:190: if (callbacks_.find(event_id) ...
3 years, 10 months ago (2017-02-08 14:58:44 UTC) #15
pfeldman
Did you just want auto-responder here? That might be easier to implement altogether. https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtools/protocol/security_handler.cc File ...
3 years, 10 months ago (2017-02-08 18:24:00 UTC) #16
Eric Seckler
https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2639203003/diff/60001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode949 third_party/WebKit/Source/core/inspector/browser_protocol.json:949: { "name": "eventID", "type": "integer", "description": "The ID of ...
3 years, 10 months ago (2017-02-08 18:53:10 UTC) #17
irisu
https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtools/protocol/security_handler.cc File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/devtools/protocol/security_handler.cc#newcode144 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 ...
3 years, 10 months ago (2017-02-14 05:46:14 UTC) #18
Eric Seckler
https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl_manager.cc File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl_manager.cc#newcode364 content/browser/ssl/ssl_manager.cc:364: if (!web_contents->NotifyCertificateError(callback)) { On 2017/02/14 05:46:14, irisu wrote: > ...
3 years, 10 months ago (2017-02-14 18:42:25 UTC) #19
pfeldman
https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl_manager.cc File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl_manager.cc#newcode364 content/browser/ssl/ssl_manager.cc:364: if (!web_contents->NotifyCertificateError(callback)) { Something like that would do, I ...
3 years, 10 months ago (2017-02-14 19:00:24 UTC) #20
irisu
https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl_manager.cc File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/60001/content/browser/ssl/ssl_manager.cc#newcode364 content/browser/ssl/ssl_manager.cc:364: if (!web_contents->NotifyCertificateError(callback)) { On 2017/02/14 19:00:24, pfeldman wrote: > ...
3 years, 10 months ago (2017-02-16 00:24:30 UTC) #22
pfeldman
https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtools/protocol/security_handler.cc File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtools/protocol/security_handler.cc#newcode64 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/devtools/protocol/security_handler.cc#newcode162 content/browser/devtools/protocol/security_handler.cc:162: frontend_->CertificateError(++last_cert_error_id_, ...
3 years, 10 months ago (2017-02-16 01:35:34 UTC) #27
Eric Seckler
https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtools/protocol/security_handler.cc File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtools/protocol/security_handler.cc#newcode163 content/browser/devtools/protocol/security_handler.cc:163: net::ErrorToShortString(cert_error), On 2017/02/16 01:35:34, pfeldman wrote: > cert_error is ...
3 years, 10 months ago (2017-02-16 02:01:07 UTC) #28
irisu
https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtools/protocol/security_handler.cc File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/120001/content/browser/devtools/protocol/security_handler.cc#newcode64 content/browser/devtools/protocol/security_handler.cc:64: last_cert_error_id_(0), On 2017/02/16 01:35:34, pfeldman wrote: > you no ...
3 years, 10 months ago (2017-02-16 04:47:32 UTC) #29
pfeldman
>> Shall we add an enum type to the Security domain that covers the "Certificate ...
3 years, 10 months ago (2017-02-16 22:18:22 UTC) #31
irisu
On 2017/02/16 at 22:18:22, pfeldman wrote: > >> Shall we add an enum type to ...
3 years, 10 months ago (2017-02-17 02:48:10 UTC) #34
irisu
I've hit another problem with the Network Domain events. I need to run the test ...
3 years, 10 months ago (2017-02-20 07:08:27 UTC) #49
Sami
Camille, can you comment on the enable-browser-side-navigation (PlzNavigate) question here? Is the behavior change expected, ...
3 years, 10 months ago (2017-02-20 16:53:05 UTC) #51
pfeldman
+dgozman who was making sure network domain behaves with PlzNavigate.
3 years, 10 months ago (2017-02-21 18:51:09 UTC) #53
pfeldman
one of the comments i forgot to send out previously. https://codereview.chromium.org/2639203003/diff/160001/content/browser/devtools/protocol/security_handler.cc File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/160001/content/browser/devtools/protocol/security_handler.cc#newcode145 ...
3 years, 10 months ago (2017-02-21 18:51:41 UTC) #54
dgozman
> Firstly, for the 'cancel' action, no network events are received after > cancelling, including ...
3 years, 10 months ago (2017-02-21 19:34:05 UTC) #55
irisu
On 2017/02/21 19:34:05, dgozman wrote: > > Firstly, for the 'cancel' action, no network events ...
3 years, 10 months ago (2017-02-23 03:28:55 UTC) #56
irisu
https://codereview.chromium.org/2639203003/diff/160001/content/browser/devtools/protocol/security_handler.cc File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/160001/content/browser/devtools/protocol/security_handler.cc#newcode145 content/browser/devtools/protocol/security_handler.cc:145: void SecurityHandler::DidFinishLoad(RenderFrameHost* render_frame_host, On 2017/02/21 18:51:41, pfeldman wrote: > ...
3 years, 10 months ago (2017-02-23 03:29:08 UTC) #57
irisu
Fixed the tests to work with PlzNavigate.
3 years, 9 months ago (2017-02-28 05:26:52 UTC) #62
Eric Seckler
small nits, otherwise lgtm. Adding palmer@ as one of the security owners/reviewers, since they should ...
3 years, 9 months ago (2017-02-28 08:41:17 UTC) #64
Ryan Sleevi
Going to ask estark to look at this, as she's probably got the most knowledge ...
3 years, 9 months ago (2017-02-28 18:51:34 UTC) #66
Ryan Sleevi
Also, can you please update the description to be more descriptive. http://chris.beams.io/posts/git-commit/ is a useful ...
3 years, 9 months ago (2017-02-28 18:53:04 UTC) #67
irisu
On 2017/02/28 18:53:04, Ryan Sleevi (overloaded) wrote: > Also, can you please update the description ...
3 years, 9 months ago (2017-02-28 23:26:41 UTC) #69
irisu
https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtools/protocol/devtools_protocol_browsertest.cc File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtools/protocol/devtools_protocol_browsertest.cc#newcode1428 content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1428: SendCommand("Network.enable", nullptr, false); On 2017/02/28 08:41:17, Eric Seckler wrote: ...
3 years, 9 months ago (2017-02-28 23:30:25 UTC) #71
estark
I *think* that this is conceptually okay security-wise. My reasoning is that the message to ...
3 years, 9 months ago (2017-02-28 23:34:08 UTC) #72
irisu
1) @pfeldman, @dgozman, Could you answer this please? As you are more familiar :) 2) ...
3 years, 9 months ago (2017-03-06 11:56:49 UTC) #87
pfeldman
1.) Security: Is it possible for a malicious DevTools to send a message which will ...
3 years, 9 months ago (2017-03-06 17:57:34 UTC) #88
irisu
PTAL
3 years, 9 months ago (2017-03-08 23:14:37 UTC) #89
estark
Sorry for the delay. Have been discussing this more generally with the security team. I ...
3 years, 9 months ago (2017-03-09 00:24:31 UTC) #90
pfeldman
>> So that might be worth a broader security <-> devtools discussion. Sure, we have ...
3 years, 9 months ago (2017-03-09 00:32:25 UTC) #91
estark
On 2017/03/09 00:32:25, pfeldman wrote: > >> So that might be worth a broader security ...
3 years, 9 months ago (2017-03-09 00:34:36 UTC) #92
pfeldman
> Is it documented anywhere? If this keeps coming up, maybe we just need to ...
3 years, 9 months ago (2017-03-09 00:47:07 UTC) #93
Eric Seckler
https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ssl_manager.cc File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ssl_manager.cc#newcode69 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 ...
3 years, 9 months ago (2017-03-09 11:34:53 UTC) #94
irisu
https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtools/protocol/security_handler.cc File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/240001/content/browser/devtools/protocol/security_handler.cc#newcode165 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 ...
3 years, 9 months ago (2017-03-13 01:56:56 UTC) #103
estark
Thanks, looking good! Just a few comments inline about the tests. Also, a high-level question ...
3 years, 9 months ago (2017-03-14 01:22:50 UTC) #104
irisu
https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ssl_manager.cc File content/browser/ssl/ssl_manager.cc (right): https://codereview.chromium.org/2639203003/diff/320001/content/browser/ssl/ssl_manager.cc#newcode69 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 ...
3 years, 9 months ago (2017-03-16 03:40:18 UTC) #107
estark
Thanks, lgtm!
3 years, 9 months ago (2017-03-17 05:26:07 UTC) #114
irisu
@pfeldman, are you happy with this?
3 years, 9 months ago (2017-03-20 23:32:05 UTC) #115
pfeldman
lgtm % nits https://codereview.chromium.org/2639203003/diff/360001/content/browser/devtools/protocol/security_handler.cc File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/360001/content/browser/devtools/protocol/security_handler.cc#newcode166 content/browser/devtools/protocol/security_handler.cc:166: // Send certificateError to devtools frontend ...
3 years, 9 months ago (2017-03-22 00:47:33 UTC) #116
irisu
https://codereview.chromium.org/2639203003/diff/360001/content/browser/devtools/protocol/security_handler.cc File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/2639203003/diff/360001/content/browser/devtools/protocol/security_handler.cc#newcode166 content/browser/devtools/protocol/security_handler.cc:166: // Send certificateError to devtools frontend to inform, but ...
3 years, 9 months ago (2017-03-22 02:48:59 UTC) #118
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2639203003/380001
3 years, 9 months ago (2017-03-22 05:13:36 UTC) #128
commit-bot: I haz the power
3 years, 9 months ago (2017-03-22 07:47:11 UTC) #131
Message was sent while issue was closed.
Committed patchset #14 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/8452ddd64771d3573cd7083f3e6e...

Powered by Google App Engine
This is Rietveld 408576698