Use certificate error codes for failed certificate error requests instead of ERR_INSECURE_RESPONSE |
||||
Issue descriptionWhen //chrome cancels a request with a certificate error, it uses ERR_INSECURE_RESPONSE. I'd like to use the original certificate error code (e.g. ERR_CERT_DATE_INVALID) instead. [1] This will work better for Committed Interstitials: we'll be committing a net error page for requests with certificate errors, so it makes sense for the net error code to reflect the actual error that the URLRequest encountered. It will also be convenient in cases where we only have the error code available (and not supplementary information like the SSLInfo), e.g. [2] I can't unearth any reason why ERR_INSECURE_RESPONSE was used originally; it dates back to the original commit and I couldn't find any context it. [1] https://cs.chromium.org/chromium/src/content/browser/ssl/ssl_error_handler.cc?q=ssl_error_handler.cc&sq=package:chromium&l=76 [2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp?q=FrameFetchContext::DispatchDidFail&sq=package:chromium&l=605
,
Nov 29 2017
Sorry, I don't remember.
,
Dec 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/03607654694d1a98b251f36c280bc4ff558d0e8b commit 03607654694d1a98b251f36c280bc4ff558d0e8b Author: Emily Stark <estark@google.com> Date: Fri Dec 01 04:14:03 2017 Preserve cert error code instead of transforming to ERR_INSECURE_RESPONSE When denying a request with a certificate error, we've historically cancelled it with a net error code of net::ERR_INSECURE_RESPONSE in content/browser/ssl/ssl_error_handler.cc, instead of the actual error code that was encountered (e.g. net::ERR_CERT_DATE_INVALID). This is a little awkward for committed interstitials [1] because we won't be able to determine from the error code along whether an error page is for a certificate error, and there are places where we have only the error code available, not supplemental info like the SSLInfo or CertStatus. (See for example NetErrorHelperCore::ErrorPageInfo.) This comes up in other contexts as well where we only have the net error code available (see bug for details). Along the way, this change fixes a bug in ServiceWorkerScriptURLLoader. This loader was using response_head.cert_status to detect certificate errors, but that field was not being populated by URLLoader (and was only populated by ResourceLoader when devtools was open). So this CL populates cert_status unconditionally to avoid this problem. The corresponding test was buggily passing: it was expecting ERR_INSECURE_RESPONSE due to a cert error, but was actually producing ERR_INSECURE_RESPONSE due to an unrelated error (unexpected mime type). Converting certificate errors to use the more specific certificate error codes instead of ERR_INSECURE_RESPONSE should prevent this test from regressing again. Finally, fixing the Service Worker certificate checking revealed a number of other SW tests which were expecting certificate errors to be ignored via --ignore-certificate-errors, which broke once the SW bug had been fixed. This CL adds --ignore-certificate-errors support to the network service, so that those tests pass again. [1] https://docs.google.com/document/d/1rEBpw5V-Nn1UIi8CIFa5ZZvwlR08SkY3CogvWE2UMFs/edit Bug: 789720 , 789682 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: I963b172dcfa400ef256e89c7c94d3f91b8bc5697 Reviewed-on: https://chromium-review.googlesource.com/797536 Commit-Queue: Emily Stark <estark@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/heads/master@{#520850} [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/frame_host/navigation_handle_impl_browsertest.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/loader/resource_loader.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/loader/resource_loader_unittest.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/service_worker/service_worker_script_url_loader.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/service_worker/service_worker_script_url_loader_unittest.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/service_worker/service_worker_version.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/service_worker/service_worker_write_to_cache_job.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/ssl/ssl_error_handler.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/utility_process_host_impl.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/network/url_loader.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/network/url_loader_unittest.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/public/browser/certificate_request_result_type.h [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/public/common/resource_response_info.h
,
Dec 2 2017
Requesting a merge for #3. This change just missed branch and is a dependent CL for a metric (https://bugs.chromium.org/p/chromium/issues/detail?id=772534#c5) that it would be really nice to have in M64 due to the upcoming Certificate Transparency enforcement date in April (https://groups.google.com/a/chromium.org/forum/#!topic/ct-policy/sz_3W_xKBNY). The CL changes an error code but doesn't otherwise have functional changes, and it's covered by automated tests.
,
Dec 3 2017
Your change meets the bar and is auto-approved for M64. Please go ahead and merge the CL to branch 3282 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/de7cb87f71cb13598225f75d3a96c49bc260b54a commit de7cb87f71cb13598225f75d3a96c49bc260b54a Author: Emily Stark <estark@google.com> Date: Mon Dec 04 19:02:33 2017 Preserve cert error code instead of transforming to ERR_INSECURE_RESPONSE When denying a request with a certificate error, we've historically cancelled it with a net error code of net::ERR_INSECURE_RESPONSE in content/browser/ssl/ssl_error_handler.cc, instead of the actual error code that was encountered (e.g. net::ERR_CERT_DATE_INVALID). This is a little awkward for committed interstitials [1] because we won't be able to determine from the error code along whether an error page is for a certificate error, and there are places where we have only the error code available, not supplemental info like the SSLInfo or CertStatus. (See for example NetErrorHelperCore::ErrorPageInfo.) This comes up in other contexts as well where we only have the net error code available (see bug for details). Along the way, this change fixes a bug in ServiceWorkerScriptURLLoader. This loader was using response_head.cert_status to detect certificate errors, but that field was not being populated by URLLoader (and was only populated by ResourceLoader when devtools was open). So this CL populates cert_status unconditionally to avoid this problem. The corresponding test was buggily passing: it was expecting ERR_INSECURE_RESPONSE due to a cert error, but was actually producing ERR_INSECURE_RESPONSE due to an unrelated error (unexpected mime type). Converting certificate errors to use the more specific certificate error codes instead of ERR_INSECURE_RESPONSE should prevent this test from regressing again. Finally, fixing the Service Worker certificate checking revealed a number of other SW tests which were expecting certificate errors to be ignored via --ignore-certificate-errors, which broke once the SW bug had been fixed. This CL adds --ignore-certificate-errors support to the network service, so that those tests pass again. [1] https://docs.google.com/document/d/1rEBpw5V-Nn1UIi8CIFa5ZZvwlR08SkY3CogvWE2UMFs/edit Bug: 789720 , 789682 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: I963b172dcfa400ef256e89c7c94d3f91b8bc5697 Reviewed-on: https://chromium-review.googlesource.com/797536 Commit-Queue: Emily Stark <estark@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#520850}(cherry picked from commit 03607654694d1a98b251f36c280bc4ff558d0e8b) Reviewed-on: https://chromium-review.googlesource.com/806315 Reviewed-by: Emily Stark <estark@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#15} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/frame_host/navigation_handle_impl_browsertest.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/loader/resource_loader.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/loader/resource_loader_unittest.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/service_worker/service_worker_script_url_loader.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/service_worker/service_worker_script_url_loader_unittest.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/service_worker/service_worker_version.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/service_worker/service_worker_write_to_cache_job.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/ssl/ssl_error_handler.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/utility_process_host_impl.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/network/url_loader.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/network/url_loader_unittest.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/public/browser/certificate_request_result_type.h [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/public/common/resource_response_info.h |
||||
►
Sign in to add a comment |
||||
Comment 1 by est...@chromium.org
, Nov 29 2017