Bug 129653 – [soup] Propagate TLS error information for resource requests

• Component: WebCore Misc.
• Status: NEW

[Ben Boeckel]

Currently, if a TLS error occurs (particularly certificate errors), the certificate and libsoup flags are lost.

[Ben Boeckel]

Created attachment 225732 [details]
Patch

[WebKit Commit Bot]

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.

[Alexey Proskuryakov]

Comment on attachment 225732 [details]
Patch

The patch is empty.

[Ben Boeckel]

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.

[Ben Boeckel]

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".

[WebKit Commit Bot]

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.

[Ben Boeckel]

Created attachment 225865 [details]
Set-the-TLS-error-information-if-a-TLS-error-occurred (2)

Style errors fixed.

[Gustavo Noronha (kov)]

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.

[Ben Boeckel]

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.