The RootedFileSystemProvider.resolveLocalPath() does not return normilized path any more

[artembilan]

Version

2.10.0

Bug description

In the previous 2.9.2 version the RootedFileSystemProvider.resolveLocalPath() returns a normalized resolved path:

        Path resolved = root.resolve(subPath);
        resolved = resolved.normalize();
        resolved = resolved.toAbsolutePath();
        if (log.isTraceEnabled()) {
            log.trace("resolveLocalPath({}): {}", absPath, resolved);
        }

        /*
         * This can happen for Windows since we represent its paths as /C:/some/path, so substring(1) yields
         * C:/some/path - which is resolved as an absolute path (which we don't want).
         */
        if (!resolved.startsWith(root)) {
            throw new InvalidPathException(r, "Not under root");
        }
        return resolved;

But in the current 2.10.0 it is like this:

        Path resolved = IoUtils.chroot(root, path);

        /*
         * This can happen for Windows since we represent its paths as /C:/some/path, so substring(1) yields
         * C:/some/path - which is resolved as an absolute path (which we don't want).
         *
         * This also is a security assertion to protect against unknown attempts to break out of the chroot jail
         */
        if (!resolved.normalize().startsWith(root)) {
            throw new InvalidPathException(root.toString(), "Not under root");
        }
        return resolved;

Actual behavior

It even works well on Windows: looks it is forgiving for the trailing .. On Linux we fails with error like this:

2023-07-11 14:26:23,978 DEBUG [sshd-SftpSubsystem-36168-thread-1] [org.apache.sshd.sftp.server.SftpSubsystem] - process(ServerSessionImpl[foo@/127.0.0.1:44840])[length=19, type=SSH_FXP_MKDIR, id=106] processing
2023-07-11 14:26:23,979 TRACE [sshd-SftpSubsystem-36168-thread-1] [org.apache.sshd.common.file.root.RootedFileSystem] - getPath(/foo/., []): /foo/.
2023-07-11 14:26:23,979 TRACE [sshd-SftpSubsystem-36168-thread-1] [org.apache.sshd.sftp.server.SftpSubsystem] - resolveFile(ServerSessionImpl[foo@/127.0.0.1:44840]) /foo/ => /foo/.
2023-07-11 14:26:23,979 DEBUG [sshd-SftpSubsystem-36168-thread-1] [org.apache.sshd.sftp.server.SftpSubsystem] - doMakeDirectory(ServerSessionImpl[foo@/127.0.0.1:44840])[id=106] SSH_FXP_MKDIR (path=/foo/[/foo/.], attrs={})
2023-07-11 14:26:23,979 TRACE [sshd-SftpSubsystem-36168-thread-1] [org.apache.sshd.common.file.root.RootedFileSystemProvider] - readAttributes(/foo/.)[/tmp/junit1261919202368611685/source/foo/.] type=BasicFileAttributes
2023-07-11 14:26:23,980 DEBUG [sshd-SftpSubsystem-36168-thread-1] [org.apache.sshd.sftp.server.SftpSubsystem] - doSendStatus[ServerSessionImpl[foo@/127.0.0.1:44840]][id=106,cmd=14] exception
java.nio.file.NoSuchFileException: /foo
	at org.apache.sshd.common.file.root.RootedFileSystemProvider.translateIoException(RootedFileSystemProvider.java:585) ~[sshd-common-2.10.0.jar:2.10.0]
	at org.apache.sshd.common.file.root.RootedFileSystemProvider.createDirectory(RootedFileSystemProvider.java:249) ~[sshd-common-2.10.0.jar:2.10.0]
	at java.nio.file.Files.createDirectory(Files.java:700) ~[?:?]
	at org.apache.sshd.sftp.server.SftpFileSystemAccessor.createDirectory(SftpFileSystemAccessor.java:487) ~[sshd-sftp-2.10.0.jar:2.10.0]
	at org.apache.sshd.sftp.server.AbstractSftpSubsystemHelper.doMakeDirectory(AbstractSftpSubsystemHelper.java:1681) [sshd-sftp-2.10.0.jar:2.10.0]
	at org.apache.sshd.sftp.server.AbstractSftpSubsystemHelper.doMakeDirectory(AbstractSftpSubsystemHelper.java:1632) [sshd-sftp-2.10.0.jar:2.10.0]
	at org.apache.sshd.sftp.server.AbstractSftpSubsystemHelper.doProcess(AbstractSftpSubsystemHelper.java:386) [sshd-sftp-2.10.0.jar:2.10.0]
	at org.apache.sshd.sftp.server.SftpSubsystem.doProcess(SftpSubsystem.java:327) [sshd-sftp-2.10.0.jar:2.10.0]
	at org.apache.sshd.sftp.server.AbstractSftpSubsystemHelper.process(AbstractSftpSubsystemHelper.java:344) [sshd-sftp-2.10.0.jar:2.10.0]
	at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:303) [sshd-sftp-2.10.0.jar:2.10.0]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) [?:?]
	at java.util.concurrent.FutureTask.run(FutureTask.java:264) [?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) [?:?]

Expected behavior

Of course no error and restored behavior like in previous version. In my project I have a workaround like this: https://github.com/spring-projects/spring-integration/pull/8676

Relevant log output

No response

Other information

No response

[tomaswolf]

I'm rather confused by this dot. Where does that come from? What path did the client pass originally in its mkdir command?

Evidently the log excerpt above comes from some JUnit test. Can you point me to the source of that test?

[artembilan]

Well, that's really the point. Our unit test has this piece of code:

		template.execute(session -> {
			session.mkdir("/foo/");
			return session.mkdir("/foo/bar/");
		});

That session is a wrapper around SftpClient. According to my debugging investigation, the dot is added here in the SelectorUtils:

  /*
         * At this point we know for sure that 'path' contains only SINGLE slashes. According to section 4.11 - Pathname
         * resolution
         *
         * A pathname that contains at least one non-slash character and that ends with one or more trailing slashes
         * shall be resolved as if a single dot character ( '.' ) were appended to the pathname.
         */
        if ((path.length() > 1) && (path.charAt(path.length() - 1) == sepChar)) {
            return path + ".";
        } else {
            return path;
        }

I see that if I remove trailing slash from my remote dir to create, the dot is not added:

2023-07-17 10:19:25,424 TRACE [sshd-SftpSubsystem-8948-thread-1] [org.apache.sshd.common.file.root.RootedFileSystem] - getPath(/foo, []): /foo
2023-07-17 10:19:25,424 TRACE [sshd-SftpSubsystem-8948-thread-1] [org.apache.sshd.sftp.server.SftpSubsystem] - resolveFile(ServerSessionImpl[foo@/127.0.0.1:52418]) /foo => /foo

Is it really what recommended now? Why a trailing slash is considered to by as some extra signal to do something else?

Thanks

[artembilan]

Even if you say that trailing slash is not legit anymore, why just don't return a normalized path from RootedFileSystemProvider.resolveLocalPath() as it was before?

[Pungero]

I also stumbled into some issues with this added dot, although in a different context. In my case I was calling doRemoveDirectory with a path with a trailing /, which had worked flawlessly in the previous version.

Now the nasty part in my case was that when the dotted path gets passed on to UnixFileSystemProvider#delete (yes I'm using mac) it eventually get passed on to a native method call, which fails with a really vague exception: sun.nio.fs.UnixException: Invalid argument. This exception does eventually get converted into a FileSystemException and thrown, but in the Exception's file parameter every second directory is missing from the file path, alongside with the trailing dot. This mutilation seems to be done in fixExceptionFileName method.

With the returned exception it took me a while to realize what in earth was going on

[tomaswolf]

At first I was inclined to agree that the normalization should be re-added. But actually that's not right either. If it was re-added, it would be possible to do sftp.mkdir("/does_not_exist/../somedir") and /somedir would be created. Normalization would remove "/does_not_exist/..".

So I now think that the current behavior is nearly correct, but commands should check earlier whether the path ends with a slash, and remove it if it is not significant for the operation. Unfortunately that depends on the operation. In Unix:

• mkdir foo/ is the same as mkdir foo, but not as mkdir foo/. (fails) and mkdir does_not_exist/../foo fails, too.
• ls -al foo and ls -al foo/ are not the same if foo is a symlink pointing to a directory. The first shows information about the symlink, the second lists the directory contents. (As does ls -al foo/.)

So fixing the problem reported needs to be done much more carefully.

[jylinv0]

I bumped into this issue too.

AbstractSftpSubsystemHelper.doRemoveDirectory takes valid input path a/b/c/d/ and calls SftpFileSystemAccessor.resolveLocalFilePath, where a/b/c/d/ is converted to a/b/c/d/..

In old version, RootedFileSystemProvider.resolveLocalPath will normalize a/b/c/d/. and doRemoveDirectory will complete successfully. From user's point of view, passing a valid path a/b/c/d/ as input and only get a vague error message saying invalid argument is quite confusing.