Fix WebDAV directory renaming if KeepAlive is off
ClosedPublic

Authored by hoffmannrobert on Mar 15 2019, 2:06 PM.

Details

Summary

Reset m_request.isKeepAlive to true if responseCode == 301
during renaming a directory. Otherwise the connection is gone
and renaming fails, if the server is configured 'KeepAlive off'.

After sending the not quite correct request 'MOVE dirurl'
without trailing '/', an Apache webserver answers with a
redirection (301) providing the correct dirurl with trailing
'/'. But in this case the session is not reset, so if the
server sets isKeepAlive to false the connection is ended
too early.

Test Plan
  • Configure an Apache http server providing WebDAV and set KeepAlive to 'off'.
  • In dolphin, navigate to webdav://webdavserver/ and try to rename a directory there.
  • Without this patch the connection to webdavserver is broken (error message) and renaming fails, with this patch applied it works.

Diff Detail

Repository
R241 KIO
Branch
fix_webdav_rename_directory
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9945
Build 9963: arc lint + arc unit
hoffmannrobert created this revision.Mar 15 2019, 2:06 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 15 2019, 2:06 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
hoffmannrobert requested review of this revision.Mar 15 2019, 2:06 PM

+1. Seems to make sense, but I don't really know this stuff.

setting server to keepalive=false? what is this? the 90's? :O

wouldn't it be simpler to finishDav and recurse rename with the redirected url? the code inside the conditional block is basically a code copy of the code above, so why not just throw away the duplicated code and start the request
from the top? it'd solve this issue and prevent future issues at the cost of performance when/if renames are carried out without trailing slash but be 100% more reliable. or at the very least call resetSessionSettings before the new request is set?

your diff looks good to me but I am not so sure about the original code there

  • Call resetSessionSettings()

setting server to keepalive=false? what is this? the 90's? :O

Perhaps they are low on memory? I don't know, I can't change that.

wouldn't it be simpler to finishDav and recurse rename with the redirected url? the code inside the conditional block is basically a code copy of the code above, so why not just throw away the duplicated code and start the request
from the top? it'd solve this issue and prevent future issues at the cost of performance when/if renames are carried out without trailing slash but be 100% more reliable. or at the very least call resetSessionSettings before the new request is set?

Calling davFinished() isn't possible here, it calls SlaveBase::finished() and that prints a warning "finished() called twice! Please fix...". I think, it would have to be dispatch()ed to change the d->m_state to d->InsideMethod.

I changed it to call resetSessionSettings() now.

sitter accepted this revision.Mar 22 2019, 9:47 AM

Fair enough. LGTM :)

Do you need sponsoring or do you have commit access?

This revision is now accepted and ready to land.Mar 22 2019, 9:47 AM

No, thanks, I have a sponsor, but I don't have commit access. Please push it for me.

This revision was automatically updated to reflect the committed changes.