Bug 129653 - [soup] Propagate TLS error information for resource requests
Summary: [soup] Propagate TLS error information for resource requests
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 129654
  Show dependency treegraph
 
Reported: 2014-03-03 20:01 PST by Ben Boeckel
Modified: 2017-03-11 10:52 PST (History)
9 users (show)

See Also:


Attachments
Patch (27 bytes, patch)
2014-03-03 20:12 PST, Ben Boeckel
ap: review-
Details | Formatted Diff | Diff
Set-the-TLS-error-information-if-a-TLS-error-occurred (2.40 KB, patch)
2014-03-04 22:39 PST, Ben Boeckel
no flags Details | Formatted Diff | Diff
Set-the-TLS-error-information-if-a-TLS-error-occurred (2) (2.50 KB, patch)
2014-03-05 00:32 PST, Ben Boeckel
gns: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Boeckel 2014-03-03 20:01:11 PST
Currently, if a TLS error occurs (particularly certificate errors), the certificate and libsoup flags are lost.
Comment 1 Ben Boeckel 2014-03-03 20:12:58 PST
Created attachment 225732 [details]
Patch
Comment 2 WebKit Commit Bot 2014-03-03 20:19:32 PST
Attachment 225732 [details] 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.
Comment 3 Alexey Proskuryakov 2014-03-04 09:55:45 PST
Comment on attachment 225732 [details]
Patch

The patch is empty.
Comment 4 Ben Boeckel 2014-03-04 10:04:03 PST
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.
Comment 5 Ben Boeckel 2014-03-04 22:39:45 PST
Created attachment 225858 [details]
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".
Comment 6 WebKit Commit Bot 2014-03-04 22:40:54 PST
Attachment 225858 [details] 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.
Comment 7 Ben Boeckel 2014-03-05 00:32:27 PST
Created attachment 225865 [details]
Set-the-TLS-error-information-if-a-TLS-error-occurred (2)

Style errors fixed.
Comment 8 Gustavo Noronha (kov) 2014-07-16 11:14:31 PDT
Comment on attachment 225865 [details]
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.
Comment 9 Ben Boeckel 2014-07-16 11:44:12 PDT
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.