D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[David Faure]

dfaure added a comment.


  I forgot to link the fix here: D24880 

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D24316

To: vkrause, dfaure
Cc: meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[Méven Car]

meven added a comment.


  The new bug https://bugs.kde.org/show_bug.cgi?id=413117 mentions this as well 
:
  
  > kio-5.63 makes kdav-19.08.2 fail with HTTP error (400)
  > 
  > STEPS TO REPRODUCE
  >  Install the mentioned packages. Try to connect to a davical server via 
https.
  > 
  > RESULT
  >  HTTP error (400).
  > 
  > Works with kio-5.62.
  > 
  > Reverting commit 9713ea02e49eda11d72e1ac605417dac0dab8c37 

  >  "Consider the usage of WebDAV methods sufficient for assuming WebDAV" in 
kio-5.63 makes kdav work again.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D24316

To: vkrause, dfaure
Cc: meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[Volker Krause]

vkrause added a comment.


  In D24316#552541 , @dfaure wrote:
  
  > Can I just ignore the Depth from the custom http headers instead?
  >
  >   --- i/src/ioslaves/http/http.cpp
  >   +++ w/src/ioslaves/http/http.cpp
  >   @@ -172,7 +172,8 @@ static QString sanitizeCustomHTTPHeader(const QString 
&_header)
  >if (!header.contains(QLatin1Char(':')) ||
  >header.startsWith(QLatin1String("host"), 
Qt::CaseInsensitive) ||
  >header.startsWith(QLatin1String("proxy-authorization"), 
Qt::CaseInsensitive) ||
  >   -header.startsWith(QLatin1String("via"), 
Qt::CaseInsensitive)) {
  >   +header.startsWith(QLatin1String("via"), 
Qt::CaseInsensitive) ||
  >   +header.startsWith(QLatin1String("depth"), 
Qt::CaseInsensitive)) {
  >continue;
  >}
  >   
  
  
  Indeed, that should work too, especially considering Depth has its own 
meta-data field anyway.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D24316

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[David Faure]

dfaure added a comment.


  Can I just ignore the Depth from the custom http headers instead?
  
--- i/src/ioslaves/http/http.cpp
+++ w/src/ioslaves/http/http.cpp
@@ -172,7 +172,8 @@ static QString sanitizeCustomHTTPHeader(const QString 
&_header)
 if (!header.contains(QLatin1Char(':')) ||
 header.startsWith(QLatin1String("host"), 
Qt::CaseInsensitive) ||
 header.startsWith(QLatin1String("proxy-authorization"), 
Qt::CaseInsensitive) ||
-header.startsWith(QLatin1String("via"), 
Qt::CaseInsensitive)) {
+header.startsWith(QLatin1String("via"), 
Qt::CaseInsensitive) ||
+header.startsWith(QLatin1String("depth"), 
Qt::CaseInsensitive)) {
 continue;
 }

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D24316

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[Volker Krause]

vkrause added a comment.


  In D24316#552528 , @dfaure wrote:
  
  > I'm on the Applications/19.08 git branch, which shouldn't get broken by a 
newer KF5 release.
  >
  > Your patch works for Content-Type, but the duplicated Depth still leads to 
"Bad request".
  >
  > Where does the second Depth come from? I see one at line 2411, I guess the 
other comes from kdav via metadata, but not from line 2599 according to my 
debug output.
  
  
  The second Depth is likely also in customHTTPHeader meta data, which 
currently isn't available for checking when generating the Depth header. Moving 
this further up and doing a similar check in 2411 might be enough.
  
  > I assume that if we revert this (or rather #ifdef KF6), kdav master needs a 
revert (#ifdef) too?
  
  Yes.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D24316

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[David Faure]

dfaure added a comment.


  I'm on the Applications/19.08 git branch, which shouldn't get broken by a 
newer KF5 release.
  
  Your patch works for Content-Type, but the duplicated Depth still leads to 
"Bad request".
  
  Where does the second Depth come from? I see one at line 2411, I guess the 
other comes from kdav via metadata, but not from line 2599 according to my 
debug output.
  
  I assume that if we revert this (or rather #ifdef KF6), kdav master needs a 
revert (#ifdef) too?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D24316

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[Volker Krause]

vkrause added a comment.


  Overall we probably have two options, revert this change (and the 
corresponding change in KDAV) and defer it to KF6, or harden this code further 
against duplicates in custom headers. The below might work for Content-Type (no 
way to test here atm), a similar fix for Depth would require a bit more code 
reshuffling I think.
  
diff --git a/src/ioslaves/http/http.cpp b/src/ioslaves/http/http.cpp
index e3bad6cc..88c13999 100644
--- a/src/ioslaves/http/http.cpp
+++ b/src/ioslaves/http/http.cpp
@@ -2599,7 +2599,7 @@ bool HTTPProtocol::sendQuery()
 davHeader += metaData(QStringLiteral("davHeader"));
 
 // Set content type of webdav data
-if (hasDavData) {
+if (hasDavData && 
!header.contains(QLatin1String("Content-Type: "))) {
 davHeader += QStringLiteral("Content-Type: text/xml; 
charset=utf-8\r\n");
 }

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D24316

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[Volker Krause]

vkrause added a comment.


  Which version of KDAV do you have? master or some older release?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D24316

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[David Faure]

dfaure added a comment.


  `Depth: 1` is duplicated too.
  
  I tried adding a contentType.isEmpty() check, but it is actually empty. Needs 
more debugging

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D24316

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[David Faure]

dfaure added a comment.


  This commit completely breaks the davgroupware resource for me (to access 
kolab calendars via CALDAV).
  
  REPORT requests get a 400 Bad Request error from the server.
  
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
 Sending Header:
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"REPORT /calendars/dfa...@kdab.com/0a9636a30e83-8c02805b740e-0a0dee75/ HTTP/1.1"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"Host: caldav.kdab.com"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"Connection: keep-alive"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/534.34 (KHTML, like 
Gecko) akonadi_davgroupware_resource_11/5.12.2 Safari/534.34"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"Pragma: no-cache"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"Cache-control: no-cache"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"Accept: text/html, text/*;q=0.9, image/jpeg;q=0.9, image/png;q=0.9, 
image/*;q=0.9, */*;q=0.8"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"Accept-Encoding: gzip, deflate, x-gzip, x-deflate"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"Accept-Charset: utf-8,*;q=0.5"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"Accept-Language: en-US,en;q=0.9"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"Content-Type: text/xml"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"Depth: 1"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"Authorization: Basic [snipped"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"Depth: 1"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendQuery: 
"Content-Type: text/xml; charset=utf-8"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http HTTPProtocol::sendBody: 
"\r\n\n \n  \n  \n \n \n  \n   \n  \n 
\n"
  2019-10-22T12:35:15 kio_http(12004)/kf5.kio.kio_http 
HTTPProtocol::readResponseHeader: 
  2019-10-22T12:35:17 kio_http(12004)/kf5.kio.kio_http 
HTTPProtocol::readResponseHeader:  Received Status Response:
  2019-10-22T12:35:17 kio_http(12004)/kf5.kio.kio_http 
HTTPProtocol::readResponseHeader: "HTTP/1.1 400 Bad Request"
  
  I think the problem is that the request has two Content-Type headers. The 
second one is added by the code we are now entering, line 2603, because of the 
modified if().

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D24316

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[Volker Krause]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:9713ea02e49e: Consider the usage of WebDAV methods 
sufficient for assuming WebDAV (authored by vkrause).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24316?vs=67079=67143

REVISION DETAIL
  https://phabricator.kde.org/D24316

AFFECTED FILES
  src/ioslaves/http/http.cpp

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Ah I see, `the hacks in KDAV::DavManager` is the context :)

REPOSITORY
  R241 KIO

BRANCH
  next

REVISION DETAIL
  https://phabricator.kde.org/D24316

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[David Faure]

dfaure added a comment.


  Test Plan: ?
  
  Did you see this in a specific application? Just to have some context about 
why/when this occurs.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D24316

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24316: Consider the usage of WebDAV methods sufficient for assuming WebDAV

[Volker Krause]

vkrause created this revision.
vkrause added a reviewer: dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
vkrause requested review of this revision.

REVISION SUMMARY
  So far WebDAV only really works when using webdav[s]: URLs, which is an
  unexpected KIO-specific detail that does not seem to be understood by all
  applications (see e.g. the hacks in KDAV::DavManager). With this change
  at least the WebDAV methods also emit the WebDAV specific headers when
  using http[s] URLs.

REPOSITORY
  R241 KIO

BRANCH
  next

REVISION DETAIL
  https://phabricator.kde.org/D24316

AFFECTED FILES
  src/ioslaves/http/http.cpp

To: vkrause, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns