Skip to content

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.

Sign up for GitHub

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

Open
flymarq wants to merge 6 commits into
base: master
Choose a base branch
from
+15 −0
Open

Uri in xml response must be url encoded. #1591

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0ab236f
Uri in xml response must be url encoded.
flymarq Jul 25, 2025
03940b8
Multiple unlimited uploads with progress bar.
flymarq Jul 25, 2025
9d58022
Merge branch 'sabre-io:master' into master
flymarq Nov 21, 2025
3aca3d5
Revert "Multiple unlimited uploads with progress bar."
Nov 21, 2025
e33db5e
Add test for locking urls with spaces.
Nov 21, 2025
863d6c3
Add check for lock token.
Dec 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/DAV/Locks/Plugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ public function httpLock(RequestInterface $request, ResponseInterface $response)
}

$this->lockNode($uri, $lockInfo);
$lockInfo->uri = urlencode($lockInfo->uri);
Copy link
Contributor

@alecpl alecpl Dec 1, 2025 •
edited
Loading

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.

Copy link
Author

@flymarq flymarq Dec 2, 2025

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.


$response->setHeader('Content-Type', 'application/xml; charset=utf-8');
$response->setHeader('Lock-Token', '<opaquelocktoken:'.$lockInfo->token.'>');
Expand Down
10 changes: 10 additions & 0 deletions tests/Sabre/DAV/Locks/PluginTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,4 +920,14 @@ public function testGetTimeoutHeaderInvalid()
$this->server->httpRequest = $request;
$this->locksPlugin->getTimeoutHeader();
}

public function testLockWithSpaces()
{
$request = new HTTP\Request('LOCK', '/test .txt', [
'Timeout' => 'second-5, infinite',
]);
$this->server->httpRequest = $request;
$this->server->exec();
self::assertEquals(201, $this->response->status);
Copy link
Contributor

@alecpl alecpl Dec 1, 2025

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.

Copy link
Author

@flymarq flymarq Dec 2, 2025

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.

Copy link
Contributor

@alecpl alecpl Dec 2, 2025

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.

Copy link
Author

@flymarq flymarq Dec 2, 2025

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

Copy link
Contributor

@alecpl alecpl Dec 2, 2025

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().

Copy link
Contributor

@alecpl alecpl Dec 2, 2025 •
edited
Loading

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.

Copy link
Author

@flymarq flymarq Dec 2, 2025 •
edited
Loading

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.

Copy link
Author

@flymarq flymarq Dec 2, 2025

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.

}
}