Bug 24804 – [GTK] 401 responses cause rogue content to be loaded

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Gustavo Noronha (kov)]

Since my change to unspecial-case most responses >= 400, when first loading a page that requires HTTP basic authentication, the error message will be loaded along with the content. This bug is here to track fixing this regression.

[Gustavo Noronha (kov)]

Created attachment 28934 [details]
2009-03-25  Gustavo Noronha Silva  <[e-mail]>


        Reviewed by NOBODY (OOPS!).

	https://bugs.webkit.org/show_bug.cgi?id=24804
	[GTK] 401 responses cause rogue content to be loaded

        Our soup code handles 401 responses itself, so we should not feed
        the headers and data of those responses to the loader.

        * platform/network/soup/ResourceHandleSoup.cpp:
        (WebCore::gotHeadersCallback):
        (WebCore::gotChunkCallback):
---
 WebCore/ChangeLog                                  |   14 ++++++++++++++
 .../platform/network/soup/ResourceHandleSoup.cpp   |    8 +++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

[Gustavo Noronha (kov)]

Created attachment 28937 [details]
2009-03-25  Gustavo Noronha Silva  <[e-mail]>


        Reviewed by NOBODY (OOPS!).

        https://bugs.webkit.org/show_bug.cgi?id=24804
        [GTK] 401 responses cause rogue content to be loaded

        Our soup code handles 401 responses itself, so we should not feed
        the headers and data of those responses to the loader.

        * platform/network/soup/ResourceHandleSoup.cpp:
        (WebCore::gotHeadersCallback):
        (WebCore::gotChunkCallback):
---
 WebCore/ChangeLog                                  |   14 ++++++++
 .../platform/network/soup/ResourceHandleSoup.cpp   |   33 +++++++++++++++++---
 2 files changed, 42 insertions(+), 5 deletions(-)

[Holger Freyther]

Comment on attachment 28937 [details]
2009-03-25  Gustavo Noronha Silva  <[e-mail]>

> a5cf3ffc4ba04bd9ca5be3cf646f112aee3fe2db
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 03b7af7..a27bbf0 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,17 @@
> [phone]  Gustavo Noronha Silva  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +	https://bugs.webkit.org/show_bug.cgi?id=24804
> +	[GTK] 401 responses cause rogue content to be loaded
> +
> +        Our soup code handles 401 responses itself, so we should not feed
> +        the headers and data of those responses to the loader.


tabs vs. spaces here? anyway the pre-commit hook would catch these :)

> +    // For 401, we will accumulate the resource body, and only use it
> +    // in case authentication with the soup feature doesn't happen
> +    if (msg->status_code == SOUP_STATUS_UNAUTHORIZED) {
> +        soup_message_body_set_accumulate(msg->response_body, TRUE);
> +        return;
> +    }
> +
> +    // For all the other responses, we handle each chunk ourselves,
> +    // and we don't need msg->response_body to contain all of the data
> +    // we got, when we finish downloading.
> +    soup_message_body_set_accumulate(msg->response_body, FALSE);
> +
>      // The 304 status code (SOUP_STATUS_NOT_MODIFIED) needs to be fed
>      // into WebCore, as opposed to other kinds of redirections, which
>      // are handled by soup directly, so we special-case it here and in
>      // gotChunk.
>      if (SOUP_STATUS_IS_TRANSPORT_ERROR(msg->status_code)
> -        || (SOUP_STATUS_IS_REDIRECTION(msg->status_code) && (msg->status_code != SOUP_STATUS_NOT_MODIFIED)))
> +        || (SOUP_STATUS_IS_REDIRECTION(msg->status_code) && (msg->status_code != SOUP_STATUS_NOT_MODIFIED))
> +        || (msg->status_code == SOUP_STATUS_UNAUTHORIZED))
>          return;

okay, with status->code == SOUP_STATUS_UNAU... we will never reach here... unless soup_message_body_set_accumulate changes the status_code. So can this be removed?


>  
>      // We still don't know anything about Content-Type, so we will try
> @@ -268,7 +281,8 @@ static void gotHeadersCallback(SoupMessage* msg, gpointer data)
>  static void gotChunkCallback(SoupMessage* msg, SoupBuffer* chunk, gpointer data)
>  {
>      if (SOUP_STATUS_IS_TRANSPORT_ERROR(msg->status_code)
> -        || (SOUP_STATUS_IS_REDIRECTION(msg->status_code) && (msg->status_code != SOUP_STATUS_NOT_MODIFIED)))
> +        || (SOUP_STATUS_IS_REDIRECTION(msg->status_code) && (msg->status_code != SOUP_STATUS_NOT_MODIFIED))
> +        || (msg->status_code == SOUP_STATUS_UNAUTHORIZED))
>          return;

what about an ASSERT(msg->status_code != SOUP_STATUS_UNAUTHORIZED)? Or will gotChunkCallback be called regardles of the body accumulate?


otherwise, it looks pretty good.

[Holger Freyther]

Comment on attachment 28937 [details]
2009-03-25  Gustavo Noronha Silva  <[e-mail]>

Okay. There is agreement on the first hunk (needles return statement) and uncertainty about the ASSERT. I'm saying r=+ based on this, please try the assert first if that does not work go for the current code in gotChunkCallback.

[Gustavo Noronha (kov)]

Landed as r42001; soup will always emit got-chunk at least once, from my reading of the docs/code, so no assert for us.