Bug 37097 - [Qt] Fix infinite redirection loop in QNetworkReplyHandler
Summary: [Qt] Fix infinite redirection loop in QNetworkReplyHandler
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords: Qt
Depends on:
Blocks: 35784 36690
  Show dependency treegraph
 
Reported: 2010-04-05 10:33 PDT by Robert Hogan
Modified: 2010-05-07 14:10 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.83 KB, patch)
2010-04-05 10:36 PDT, Robert Hogan
kenneth: review-
Details | Formatted Diff | Diff
Updated Patch (3.88 KB, patch)
2010-04-05 10:45 PDT, Robert Hogan
kenneth: review+
robert: commit-queue-
Details | Formatted Diff | Diff
User Counters (4.80 KB, patch)
2010-04-05 11:30 PDT, Robert Hogan
kenneth: review-
Details | Formatted Diff | Diff
Updated Counters Patch (4.25 KB, patch)
2010-04-05 11:39 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Updated Patch (4.49 KB, patch)
2010-04-05 12:04 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Updated Patch (4.50 KB, patch)
2010-04-05 12:12 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2010-04-05 10:33:38 PDT
Qt enters an infinite loop if a redirect response redirects to itself.

Fixes http/tests/connection-error-sync.html
Comment 1 Robert Hogan 2010-04-05 10:36:35 PDT
Created attachment 52540 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2010-04-05 10:42:37 PDT
Comment on attachment 52540 [details]
Patch


> +        QUrl newUrl = m_reply->url().resolved(redirection);
> +        if (newUrl == m_reply->url()) { // Infinite redirection loop!

 // avoid redirecting to the same url as it causes infinite recursion


> +            ResourceError error(newUrl.host(), 400 /*bad request*/,
> +                                newUrl.toString(),
> +                                QCoreApplication::translate("QWebPage", "Ininite Loop in Redirect Request"));

Spelling issue. "Infinite recursion in redirection request"

> +            client->didFail(m_resourceHandle, error);
> +            return;
> +        }
> +
>          m_redirected = true;
>  
> -        QUrl newUrl = m_reply->url().resolved(redirection);
>          ResourceRequest newRequest = m_resourceHandle->request();
>          newRequest.setURL(newUrl);
>  
> -- 
> 1.6.3.3
>
Comment 3 Robert Hogan 2010-04-05 10:45:31 PDT
Created attachment 52541 [details]
Updated Patch

Updated per Kenneth's comments
Comment 4 Robert Hogan 2010-04-05 10:58:42 PDT
Manually Committed r57085.
Comment 5 Dawit A. 2010-04-05 11:03:25 PDT
This fix will break sites that purposefully redirect to the very same url. IMHO
to detect and avoid infinite recursion to the same url, you have to have
additional checks such as a counter as it is done in KDE's KIO:

   // Some websites keep redirecting to themselves where each redirection
    // acts as the stage in a state-machine. We define "endless redirections"
    // as 5 redirections to the same URL.
    if (d->m_redirectionList.count(url) > 5)
    {
       kDebug(7007) << "CYCLIC REDIRECTION!";
       setError( ERR_CYCLIC_LINK );
       setErrorText( d->m_url.pathOrUrl() );
    }


See lines #1014-1022 of kio/kio/job.cpp:

http://websvn.kde.org/trunk/KDE/kdelibs/kio/kio/job.cpp?revision=1108333&view=markup
Comment 6 Robert Hogan 2010-04-05 11:30:47 PDT
Created attachment 52546 [details]
User Counters
Comment 7 Kenneth Rohde Christiansen 2010-04-05 11:35:11 PDT
Comment on attachment 52546 [details]
User Counters

I did a bit of research and Mozilla doesn't verify where the redirect comes from, and your patch won't handles cases as page1 -> page2 -> page1!

Let's introduce a max redirection count (default 10 as Mozilla) and count it down for each redirect (not just the case of newUrl == m_reply->url()).

When it reaches 0, we fail with an error such as "Redirection limit reached!" or similar. Later we can add a setting for this.

Info: http://kb.mozillazine.org/Network.http.redirection-limit (look at the bug report)
Comment 8 Robert Hogan 2010-04-05 11:39:41 PDT
Created attachment 52550 [details]
Updated Counters Patch
Comment 9 WebKit Review Bot 2010-04-05 11:41:56 PDT
Attachment 52550 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/qt/QNetworkReplyHandler.cpp:342:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Robert Hogan 2010-04-05 12:04:06 PDT
Created attachment 52554 [details]
Updated Patch
Comment 11 WebKit Review Bot 2010-04-05 12:06:19 PDT
Attachment 52554 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/qt/QNetworkReplyHandler.cpp:343:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Robert Hogan 2010-04-05 12:12:37 PDT
Created attachment 52557 [details]
Updated Patch
Comment 13 WebKit Review Bot 2010-04-05 12:13:43 PDT
Attachment 52557 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/qt/QNetworkReplyHandler.cpp:343:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 WebKit Commit Bot 2010-04-05 20:33:39 PDT
Comment on attachment 52557 [details]
Updated Patch

Clearing flags on attachment: 52557

Committed r57117: <http://trac.webkit.org/changeset/57117>
Comment 15 Simon Hausmann 2010-05-07 14:10:07 PDT
Revision r57085 cherry-picked into qtwebkit-2.0 with commit ca03a15b28a427cd9a0e9d8844780f01a7a2ad13
Revision r57117 cherry-picked into qtwebkit-2.0 with commit e830618c892e2e48ad038fc8ecc826c04c513905