129653 2014-03-03 20:01:11 -0800 [soup] Propagate TLS error information for resource requests 2017-03-11 10:52:17 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified NEW P2 Normal --- 129654 1 mathstuf webkit-unassigned berto bugs-noreply cdumez cgarcia commit-queue danw darin gustavo mrobinson oldest_to_newest 986649 0 mathstuf 2014-03-03 20:01:11 -0800 Currently, if a TLS error occurs (particularly certificate errors), the certificate and libsoup flags are lost. 986653 1 225732 mathstuf 2014-03-03 20:12:58 -0800 Created attachment 225732 Patch 986658 2 commit-queue 2014-03-03 20:19:32 -0800 Attachment 225732 did not pass style-queue: Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style. 986851 3 225732 ap 2014-03-04 09:55:45 -0800 Comment on attachment 225732 Patch The patch is empty. 986856 4 mathstuf 2014-03-04 10:04:03 -0800 Oops. Seems I botched the Tools/Scripts tool I used (on a different machine). I'm using the git repo; what's the proper command to use for submitting a single patch from a branch (I think I used webkit-patch -g HEAD~ 129653 patch-name)? I'll respin once I get home. 987135 5 225858 mathstuf 2014-03-04 22:39:45 -0800 Created attachment 225858 Set-the-TLS-error-information-if-a-TLS-error-occurred FWIW, here's the command I used to attach to the bug: "Tools/Scripts/webkit-patch attach-to-bug -g HEAD -m Patch 129653 TLS-error-propagation.patch". 987137 6 commit-queue 2014-03-04 22:40:54 -0800 Attachment 225858 did not pass style-queue: ERROR: Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:61: Declaration has space between type name and * in GTlsCertificate *certificate [whitespace/declaration] [3] ERROR: Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:64: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:57: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 3 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 987170 7 225865 mathstuf 2014-03-05 00:32:27 -0800 Created attachment 225865 Set-the-TLS-error-information-if-a-TLS-error-occurred (2) Style errors fixed. 1022776 8 225865 gustavo 2014-07-16 11:14:31 -0700 Comment on attachment 225865 Set-the-TLS-error-information-if-a-TLS-error-occurred (2) View in context: https://bugs.webkit.org/attachment.cgi?id=225865&action=review What does this fix or how does this improve things? Are you fixing a specific bug? > Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:57 > + if (message && SOUP_STATUS_IS_TRANSPORT_ERROR(message->status_code)) { This should be turned into an early return, so reverse the check and return generic error immediately. > Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:61 > + ResourceError error = transportError(request, message->status_code, > String::fromUTF8(message->reason_phrase)); > - else > - return genericGError(error, request); > + if (message->status_code == SOUP_STATUS_SSL_FAILED) { > + GTlsCertificate* certificate; Nit: an empty line before the if will make this more readable I think. 1022788 9 mathstuf 2014-07-16 11:44:12 -0700 Thanks for the review. (In reply to comment #8) > What does this fix or how does this improve things? Are you fixing a specific bug? That's filed in bugzilla? No. The problem is that when an SSL error occurs, there's no information available for a browser to say "here's the certificate that failed; do you want to ignore the error?". Currently, there's nothing there since the generic error doesn't contain such information. Of course, things may have changed since I last checked; I'll see if I can get my WebKit repository up to speed tonight and double check the state of things (i.e., whether the patch is necessary and still provides the correct information). > > Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:57 > > + if (message && SOUP_STATUS_IS_TRANSPORT_ERROR(message->status_code)) { > > This should be turned into an early return, so reverse the check and return generic error immediately. Sounds good. > > Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:61 > > + ResourceError error = transportError(request, message->status_code, > > String::fromUTF8(message->reason_phrase)); > > - else > > - return genericGError(error, request); > > + if (message->status_code == SOUP_STATUS_SSL_FAILED) { > > + GTlsCertificate* certificate; > > Nit: an empty line before the if will make this more readable I think. Will change. 225732 2014-03-03 20:12:58 -0800 2014-03-04 22:39:45 -0800 Patch bug-129653-20140303231257.txt text/plain 27 mathstuf VExTLWVycm9yLXByb3BhZ2F0aW9uLnBhdGNo 225858 2014-03-04 22:39:45 -0800 2014-03-05 00:32:27 -0800 Set-the-TLS-error-information-if-a-TLS-error-occurred 0001-Set-the-TLS-error-information-if-a-TLS-error-occurre.patch text/plain 2455 mathstuf RnJvbSBkNmViZmYzMTNkYTNhNTU3NGMyMzE1N2Y5NTc2MzZiNzcyNTU4ODM0IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBCZW4gQm9lY2tlbCA8bWF0aHN0dWZAZ21haWwuY29tPgpEYXRl OiBNb24sIDMgTWFyIDIwMTQgMDA6Mzg6NTUgLTA1MDAKU3ViamVjdDogW1BBVENIIDEvMl0gU2V0 IHRoZSBUTFMgZXJyb3IgaW5mb3JtYXRpb24gaWYgYSBUTFMgZXJyb3Igb2NjdXJyZWQKCi0tLQog U291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICB8 IDEzICsrKysrKysrKysrKysKIC4uLi9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvc291cC9SZXNv dXJjZUVycm9yU291cC5jcHAgfCAxNyArKysrKysrKysrKysrKy0tLQogMiBmaWxlcyBjaGFuZ2Vk LCAyNyBpbnNlcnRpb25zKCspLCAzIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCAxMDQ4M2Nl Li5lYzRjMTAzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTQtMDMtMDMgIEJlbiBC b2Vja2VsICA8bWF0aHN0dWZAZ21haWwuY29tPgorCisgICAgICAgIFNldCB0aGUgVExTIGVycm9y IGluZm9ybWF0aW9uIGlmIGEgVExTIGVycm9yIG9jY3VycmVkCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjk2NTMKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVzdHMsIG5vIGJlaGF2aW9yIGNo YW5nZXMuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9uZXR3b3JrL3NvdXAvUmVzb3VyY2VFcnJvclNv dXAuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVzb3VyY2VFcnJvcjo6aHR0cEVycm9yKTogQ2hl Y2sgZm9yIFRMUyBlcnJvcnMgYW5kIGZpbGwgaW4KKyAgICAgICAgdGhlIFJlc291cmNlRXJyb3Ig ZmllbGRzIGFwcHJvcHJpYXRlbHkuCisKIDIwMTMtMTEtMTQgIEd5dXlvdW5nIEtpbSAgPGd5dXlv dW5nLmtpbUBzYW1zdW5nLmNvbT4KIAogICAgICAgICBHZW5lcmF0ZSB0b1NWR1BvbHlsaW5lfGdv bkVsZW1lbnQoKSB0byByZXBsYWNlIHN0YXRpY19jYXN0PD4KZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvc291cC9SZXNvdXJjZUVycm9yU291cC5jcHAgYi9Tb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL3NvdXAvUmVzb3VyY2VFcnJvclNvdXAuY3BwCmlu ZGV4IGJhODY1N2IuLjk2MmI3NWIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L25ldHdvcmsvc291cC9SZXNvdXJjZUVycm9yU291cC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291cmNlRXJyb3JTb3VwLmNwcApAQCAtNTQsMTAgKzU0 LDIxIEBAIFJlc291cmNlRXJyb3IgUmVzb3VyY2VFcnJvcjo6dHJhbnNwb3J0RXJyb3IoU291cFJl cXVlc3QqIHJlcXVlc3QsIGludCBzdGF0dXNDb2RlCiAKIFJlc291cmNlRXJyb3IgUmVzb3VyY2VF cnJvcjo6aHR0cEVycm9yKFNvdXBNZXNzYWdlKiBtZXNzYWdlLCBHRXJyb3IqIGVycm9yLCBTb3Vw UmVxdWVzdCogcmVxdWVzdCkKIHsKLSAgICBpZiAobWVzc2FnZSAmJiBTT1VQX1NUQVRVU19JU19U UkFOU1BPUlRfRVJST1IobWVzc2FnZS0+c3RhdHVzX2NvZGUpKQotICAgICAgICByZXR1cm4gdHJh bnNwb3J0RXJyb3IocmVxdWVzdCwgbWVzc2FnZS0+c3RhdHVzX2NvZGUsCisgICAgaWYgKG1lc3Nh Z2UgJiYgU09VUF9TVEFUVVNfSVNfVFJBTlNQT1JUX0VSUk9SKG1lc3NhZ2UtPnN0YXR1c19jb2Rl KSkgeworICAgICAgICBSZXNvdXJjZUVycm9yIGVycm9yID0gdHJhbnNwb3J0RXJyb3IocmVxdWVz dCwgbWVzc2FnZS0+c3RhdHVzX2NvZGUsCiAgICAgICAgICAgICBTdHJpbmc6OmZyb21VVEY4KG1l c3NhZ2UtPnJlYXNvbl9waHJhc2UpKTsKLSAgICBlbHNlCisgICAgICAgIGlmIChtZXNzYWdlLT5z dGF0dXNfY29kZSA9PSBTT1VQX1NUQVRVU19TU0xfRkFJTEVEKSB7CisgICAgICAgICAgICBHVGxz Q2VydGlmaWNhdGUgKmNlcnRpZmljYXRlOworICAgICAgICAgICAgR1Rsc0NlcnRpZmljYXRlRmxh Z3MgZmxhZ3M7CisKKyAgICAgICAgICAgIHNvdXBfbWVzc2FnZV9nZXRfaHR0cHNfc3RhdHVzICht ZXNzYWdlLCAmY2VydGlmaWNhdGUsICZmbGFncyk7CisKKyAgICAgICAgICAgIGVycm9yLnNldFRM U0Vycm9ycyhmbGFncyk7CisgICAgICAgICAgICBlcnJvci5zZXRDZXJ0aWZpY2F0ZShjZXJ0aWZp Y2F0ZSk7CisgICAgICAgIH0KKworICAgICAgICByZXR1cm4gZXJyb3I7CisgICAgfSBlbHNlCiAg ICAgICAgIHJldHVybiBnZW5lcmljR0Vycm9yKGVycm9yLCByZXF1ZXN0KTsKIH0KIAotLSAKMS45 LjAKCg== 225865 2014-03-05 00:32:27 -0800 2014-07-16 11:14:31 -0700 Set-the-TLS-error-information-if-a-TLS-error-occurred (2) 0001-Set-the-TLS-error-information-if-a-TLS-error-occurre.patch text/plain 2563 mathstuf RnJvbSA4ZTNhMjEzNjEwOTc0OTJhMTkzODIyMTk4ODliM2VkN2FlNTg5M2Y2IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBCZW4gQm9lY2tlbCA8bWF0aHN0dWZAZ21haWwuY29tPgpEYXRl OiBNb24sIDMgTWFyIDIwMTQgMDA6Mzg6NTUgLTA1MDAKU3ViamVjdDogW1BBVENIIDEvMl0gU2V0 IHRoZSBUTFMgZXJyb3IgaW5mb3JtYXRpb24gaWYgYSBUTFMgZXJyb3Igb2NjdXJyZWQKCi0tLQog U291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nICAgICAgICAgICAgICAgICAgICAgICAgICAgICB8IDEz ICsrKysrKysrKysrKysKIC4uLi9wbGF0Zm9ybS9uZXR3b3JrL3NvdXAvUmVzb3VyY2VFcnJvclNv dXAuY3BwICAgICAgfCAyMCArKysrKysrKysrKysrKysrLS0tLQogMiBmaWxlcyBjaGFuZ2VkLCAy OSBpbnNlcnRpb25zKCspLCA0IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJD b3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCAxMDQ4M2NlLi5l YzRjMTAzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNl L1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTQtMDMtMDMgIEJlbiBCb2Vj a2VsICA8bWF0aHN0dWZAZ21haWwuY29tPgorCisgICAgICAgIFNldCB0aGUgVExTIGVycm9yIGlu Zm9ybWF0aW9uIGlmIGEgVExTIGVycm9yIG9jY3VycmVkCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjk2NTMKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVzdHMsIG5vIGJlaGF2aW9yIGNoYW5n ZXMuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9uZXR3b3JrL3NvdXAvUmVzb3VyY2VFcnJvclNvdXAu Y3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVzb3VyY2VFcnJvcjo6aHR0cEVycm9yKTogQ2hlY2sg Zm9yIFRMUyBlcnJvcnMgYW5kIGZpbGwgaW4KKyAgICAgICAgdGhlIFJlc291cmNlRXJyb3IgZmll bGRzIGFwcHJvcHJpYXRlbHkuCisKIDIwMTMtMTEtMTQgIEd5dXlvdW5nIEtpbSAgPGd5dXlvdW5n LmtpbUBzYW1zdW5nLmNvbT4KIAogICAgICAgICBHZW5lcmF0ZSB0b1NWR1BvbHlsaW5lfGdvbkVs ZW1lbnQoKSB0byByZXBsYWNlIHN0YXRpY19jYXN0PD4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL25ldHdvcmsvc291cC9SZXNvdXJjZUVycm9yU291cC5jcHAgYi9Tb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL3NvdXAvUmVzb3VyY2VFcnJvclNvdXAuY3BwCmluZGV4 IGJhODY1N2IuLjI5MDUyNDAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25l dHdvcmsvc291cC9SZXNvdXJjZUVycm9yU291cC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vbmV0d29yay9zb3VwL1Jlc291cmNlRXJyb3JTb3VwLmNwcApAQCAtNTQsMTEgKzU0LDIz IEBAIFJlc291cmNlRXJyb3IgUmVzb3VyY2VFcnJvcjo6dHJhbnNwb3J0RXJyb3IoU291cFJlcXVl c3QqIHJlcXVlc3QsIGludCBzdGF0dXNDb2RlCiAKIFJlc291cmNlRXJyb3IgUmVzb3VyY2VFcnJv cjo6aHR0cEVycm9yKFNvdXBNZXNzYWdlKiBtZXNzYWdlLCBHRXJyb3IqIGVycm9yLCBTb3VwUmVx dWVzdCogcmVxdWVzdCkKIHsKLSAgICBpZiAobWVzc2FnZSAmJiBTT1VQX1NUQVRVU19JU19UUkFO U1BPUlRfRVJST1IobWVzc2FnZS0+c3RhdHVzX2NvZGUpKQotICAgICAgICByZXR1cm4gdHJhbnNw b3J0RXJyb3IocmVxdWVzdCwgbWVzc2FnZS0+c3RhdHVzX2NvZGUsCisgICAgaWYgKG1lc3NhZ2Ug JiYgU09VUF9TVEFUVVNfSVNfVFJBTlNQT1JUX0VSUk9SKG1lc3NhZ2UtPnN0YXR1c19jb2RlKSkg eworICAgICAgICBSZXNvdXJjZUVycm9yIGVycm9yID0gdHJhbnNwb3J0RXJyb3IocmVxdWVzdCwg bWVzc2FnZS0+c3RhdHVzX2NvZGUsCiAgICAgICAgICAgICBTdHJpbmc6OmZyb21VVEY4KG1lc3Nh Z2UtPnJlYXNvbl9waHJhc2UpKTsKLSAgICBlbHNlCi0gICAgICAgIHJldHVybiBnZW5lcmljR0Vy cm9yKGVycm9yLCByZXF1ZXN0KTsKKyAgICAgICAgaWYgKG1lc3NhZ2UtPnN0YXR1c19jb2RlID09 IFNPVVBfU1RBVFVTX1NTTF9GQUlMRUQpIHsKKyAgICAgICAgICAgIEdUbHNDZXJ0aWZpY2F0ZSog Y2VydGlmaWNhdGU7CisgICAgICAgICAgICBHVGxzQ2VydGlmaWNhdGVGbGFncyBmbGFnczsKKwor ICAgICAgICAgICAgc291cF9tZXNzYWdlX2dldF9odHRwc19zdGF0dXMobWVzc2FnZSwgJmNlcnRp ZmljYXRlLCAmZmxhZ3MpOworCisgICAgICAgICAgICBlcnJvci5zZXRUTFNFcnJvcnMoZmxhZ3Mp OworICAgICAgICAgICAgZXJyb3Iuc2V0Q2VydGlmaWNhdGUoY2VydGlmaWNhdGUpOworICAgICAg ICB9CisKKyAgICAgICAgcmV0dXJuIGVycm9yOworICAgIH0KKworICAgIHJldHVybiBnZW5lcmlj R0Vycm9yKGVycm9yLCByZXF1ZXN0KTsKIH0KIAogUmVzb3VyY2VFcnJvciBSZXNvdXJjZUVycm9y OjphdXRoZW50aWNhdGlvbkVycm9yKFNvdXBNZXNzYWdlKiBtZXNzYWdlKQotLSAKMS45LjAKCg==