-
Notifications
You must be signed in to change notification settings - Fork 362
Uri in xml response must be url encoded. #1591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
The Uri is derived from the request with the method getPath, which generates a valid path fragment suitable for file and directory names. In the XML-response to a lock-request the uri is used for lockroot element. If the uri contains a space or german umlauts, a strict client like libneon, which is used in davfs2, refuses the lock reponse and files cannot be copied.
- allow upload of multipe files - uploads are not limited by php upload size using PUT - progress bar for each file
|
I see the second commit here is unrelated and should be removed. Also, it would be great to have a test added in tests/Sabre/DAV/Locks/PluginTest.php. |
|
My fault, the second commit does not belong to this issue. I can try to add a test, but I'm afraid it won't work unless a strict client like libneon performs the test. |
|
Done:
Before the patch we received a http status value 423. |
| ]); | ||
| $this->server->httpRequest = $request; | ||
| $this->server->exec(); | ||
| self::assertEquals(201, $this->response->status); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should assert the activelock/lockroot/href property in the XML response. That's where the encoding problem would be visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a check for the lock token, in the error case no lock token is visiblr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not make sense. Your change adds uri property encoding, this is not Lock-Token. Maybe I don't understand something here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My starting point is this line in the logs of the webserver:
"LOCK /F%20G/F%20G%20H%20J.txt HTTP/1.1" 423 327 "-" "davfs2/1.7.2 neon/0.35.0"
and on the terminal I get
touch: '/home/flynn/media/flynnux/F G/F G H J.txt' kann nicht berührt werden: Eingabe-/Ausgabefehler
In the error case the XML-response contains an exception:
<s:exception>Sabre\DAV\Exception\Conflict no-conflicting-lock>
On success the the token:
<d:locktoken><d:href>opaquelocktoken:c546f02c-5fe7-4a9f-bf78-3bd7c57ef72a</d:href></d:locktoken>
This is, what I try test, tell me if I'm wrong ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConflictingLock exceptions are thrown in line 170 and 178. Your change is after that, so I don't see how that fixes anything, especially after lockNode().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, in the initial pull request description you mention problem with lockroot, so why now you test something different? Seems there might be two different issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is, that the used client (libneon) has strict checking of urls.
The uri in the LOCK request and the url in the xml response differ:
- the request is url encoded
- the response before this patch is plain/not url encoded
I already mentioned, that a test is maybe difficult as long as no strict client is used.
Maybe the error above is only consequential error of this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not find a method to use the original uri in the lock xml-response, that is maybe the real problem.
| } | ||
|
|
||
| $this->lockNode($uri, $lockInfo); | ||
| $lockInfo->uri = urlencode($lockInfo->uri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do this here you might have it fixed for LOCK request, but what about PROPFIND?
I think the fix should rather go to Sabre\DAV\Xml\Property\LockDiscovery::xmlSerialize(), but I don't really know this code yet. There \Sabre\HTTP\encodePath() should be used, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not have any problmes or errors in PROPFIND calls, why should I change anything?
I can create directories with spaces in the name and inside this directory I can create a file with spaces in the name.
From my experience only LOCK is broken/affected.
|
I was talking about this alecpl@7a549bc I must say that I have no clue if that does not break other clients. Seems all tests still work. |
The Uri is derived from the request with the method getPath, which generates a valid path fragment suitable for file and directory names.
In the XML-response to a lock-request the uri is used for lockroot element. If the uri contains a space or german umlauts, a strict client like libneon, which is used in davfs2, refuses the lock reponse and files cannot be copied.