Summary: | [GTK] 401 responses cause rogue content to be loaded | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gustavo Noronha (kov) <gustavo> | ||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | Keywords: | Gtk, Soup | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Gustavo Noronha (kov)
2009-03-25 10:18:46 PDT
Created attachment 28934 [details] 2009-03-25 Gustavo Noronha Silva <gustavo.noronha@collabora.co.uk> 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(-) Created attachment 28937 [details] 2009-03-25 Gustavo Noronha Silva <gustavo.noronha@collabora.co.uk> 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(-) Comment on attachment 28937 [details] 2009-03-25 Gustavo Noronha Silva <gustavo.noronha@collabora.co.uk> > a5cf3ffc4ba04bd9ca5be3cf646f112aee3fe2db > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > index 03b7af7..a27bbf0 100644 > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,17 @@ > +2009-03-25 Gustavo Noronha Silva <gustavo.noronha@collabora.co.uk> > + > + 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. Comment on attachment 28937 [details] 2009-03-25 Gustavo Noronha Silva <gustavo.noronha@collabora.co.uk> 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. |