----------------------------------------------------------- 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 > >
