Review Request 116570: Ask user for confirmation before doing POST -> POST redirection in KIO

[Andrea Iacovitti]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116570/#review53047
-----------------------------------------------------------


First of all it's worth to mention about th proposed update for rfc2616 (http://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-26) that makes asking user confirmation optional (see http://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-26#page-53 and subsequents). It states: "...the user agent MAY automatically redirect its request to the URI referenced by the Location field value..".
I tested some other clients and it seems that only Firefox prompts the user about redirection when method of subsequent request is considered unsafe (namely DELETE, POST, PUT).

About the patch i can see two issues.
1) I think "real method string" sent in the request should be checked instead, as for example http_post + CustomHTTPMethod can have been used.
2) When the user do not approve the redirection no data is sent back to client (data == server's redirection response payload).

May be prompting the user *before* the redirection handling code take place (and only if the server have sent back a valid a Location header) is a way to solve these issues.
What i mean is something like that (proof of concept patch):

diff --git a/kioslave/http/http.cpp b/kioslave/http/http.cpp
index e4f1eba..33fbda6 100644
--- a/kioslave/http/http.cpp
+++ b/kioslave/http/http.cpp
@@ -2925,6 +2925,7 @@ try_again:
     char buffer[maxHeaderSize];
     bool cont = false;
     bool bCanResume = false;
+    bool askRedirectionConfirm = false;
 
     if (!isConnected()) {
         kDebug(7113) << "No connection.";
@@ -3105,6 +3106,8 @@ try_again:
               case 302: // Found
                 if (m_request.sentMethodString == "POST") {
                   setMetaData(QLatin1String("redirect-to-get"), QLatin1String("true"));
+                } else if (m_request.sentMethodString == "DELETE" || m_request.sentMethodString == "PUT") {
+                  askRedirectionConfirm = true;
                 }
                 break;
               case 303: // See Other
@@ -3112,8 +3115,16 @@ try_again:
                   setMetaData(QLatin1String("redirect-to-get"), QLatin1String("true"));
                 }
                 break;
+              case 307:
+                if (m_request.sentMethodString == "POST" || m_request.sentMethodString == "DELETE" || m_request.sentMethodString == "PUT") {
+                  askRedirectionConfirm = true;
+                }
+                break;
               case 308: // Permanent Redirect
                 setMetaData(QLatin1String("permanent-redirect"), QLatin1String("true"));
+                if (m_request.sentMethodString == "POST" || m_request.sentMethodString == "DELETE" || m_request.sentMethodString == "PUT") {
+                  askRedirectionConfirm = true;
+                }
                 break;
               default:
                 break;
@@ -3484,8 +3495,18 @@ endParsing:
         if (tIt.hasNext() && m_request.responseCode > 299 && m_request.responseCode < 400) {
             locationStr = QString::fromUtf8(tIt.next().trimmed());
         }
-        // We need to do a redirect
-        if (!locationStr.isEmpty())
+
+        // Should we redirect?
+        bool shouldRedirect = !locationStr.isEmpty();
+
+        if (shouldRedirect && askRedirectionConfirm) {
+            const int userWish = messageBox(WarningYesNo, i18nc("@info redir", "redir"), i18nc("@title:window", "Confirm Redir"));
+            if (userWish == KMessageBox::No) {
+                shouldRedirect = false;
+            }
+        }
+
+        if (shouldRedirect)
         {
             KUrl u(m_request.url, locationStr);
             if(!u.isValid())


- Andrea Iacovitti


On March 7, 2014, 6:07 a.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116570/
> -----------------------------------------------------------
> 
> (Updated March 7, 2014, 6:07 a.m.)
> 
> 
> Review request for kdelibs, Andrea Iacovitti and David Faure.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> This patch is a companion to the recent POST->POST redirection implementation in KIO, https://git.reviewboard.kde.org/r/116017/. It prompts the user to approve the redirection as explicitly required in sections 10.3.[2|3] of RFC 2616:
> 
>    If the 301 status code is received in response to a request other
>    than GET or HEAD, the user agent MUST NOT automatically redirect the
>    request unless it can be confirmed by the user, since this might
>    change the conditions under which the request was issued.
> 
> Please note that this patch only prompts the user for confirmation on POST->POST redirections. It can be expanded to include redirections for other requests such as PUT.
> 
> There is also an issue of whether this patch should be part of the 4.13 release? Since we are in a freeze and the patch has both message changes as well as a new API, I have simply marked it for inclusion in master branch, i.e. 4.14.
> 
> 
> Diffs
> -----
> 
>   kio/kio/job.cpp 50b4afb 
>   kio/kio/jobuidelegate.h 17fd554 
>   kio/kio/jobuidelegate.cpp 5aff330 
> 
> Diff: https://git.reviewboard.kde.org/r/116570/diff/
> 
> 
> Testing
> -------
> 
> http://greenbytes.de/tech/tc/httpredirects/t307methods.html
> 
> 
> File Attachments
> ----------------
> 
> POST redirection confirm dialog
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/03/07/e77dd03e-cb37-49bb-8554-cca991c8c546__post_redirection_confirmation.png
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20140316/ff6d1469/attachment.htm>