Bug 37097 – [Qt] Fix infinite redirection loop in QNetworkReplyHandler

• Component: WebKit Qt
• Status: CLOSED FIXED

[Robert Hogan]

Qt enters an infinite loop if a redirect response redirects to itself.

Fixes http/tests/connection-error-sync.html

[Robert Hogan]

Created attachment 52540 [details]
Patch

[Kenneth Rohde Christiansen]

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
>

[Robert Hogan]

Created attachment 52541 [details]
Updated Patch

Updated per Kenneth's comments

[Robert Hogan]

Manually Committed r57085.

[Dawit A.]

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

[Robert Hogan]

Created attachment 52546 [details]
User Counters

[Kenneth Rohde Christiansen]

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)

[Robert Hogan]

Created attachment 52550 [details]
Updated Counters Patch

[WebKit Review Bot]

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.

[Robert Hogan]

Created attachment 52554 [details]
Updated Patch

[WebKit Review Bot]

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.

[Robert Hogan]

Created attachment 52557 [details]
Updated Patch

[WebKit Review Bot]

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.

[WebKit Commit Bot]

Comment on attachment 52557 [details]
Updated Patch

Clearing flags on attachment: 52557

Committed r57117: <http://trac.webkit.org/changeset/57117>

[Simon Hausmann]

Revision r57085 cherry-picked into qtwebkit-2.0 with commit ca03a15b28a427cd9a0e9d8844780f01a7a2ad13
Revision r57117 cherry-picked into qtwebkit-2.0 with commit e830618c892e2e48ad038fc8ecc826c04c513905