Closed drakkan closed 1 year ago
Sounds probably good.
uhm ... the problem here is that we should also return an error so we cannot reuse the current interface.
š Yeah, looking back at it now, it should have always retuned an error.
If we cannot change the RealPath interface, I think I have to wait for a v2 to fix this issue.
I don't want to maintain a custom branch, I think this is a minor issue, we don't violate the specs. Do you have other (non-ugly) ideas?
The current code base never returns an error because it does not resolve symbolic links, so no errors can occur
Well right now, the interface isnāt used for anything right? And weāre supposed to just be using it as an extension interface anyways, right?
So, switching the interface shouldnāt actually break anything? š¤
Well right now, the interface isnāt used for anything right? And weāre supposed to just be using it as an extension interface anyways, right?
So, switching the interface shouldnāt actually break anything? thinking
If we could change the RealPathFileLister
like this
type RealPathFileLister interface {
FileLister
RealPath(string) (string, error)
}
would be perfect. I think it is unlikely that anyone is using this interface as it was added for the start directory feature but it basically doesn't work.
Another solution is to add a new extension interface but this way we have to check for 2 interfaces here. This doesn't seem ideal
A type switch at that point wouldnāt be the worst thing ever, and I donāt see that it would be any particularly performance critical path either.
So, I kind of see two different options:
RealPathWithErrorFileLister
and leave the RealPathFileLister
interface alone, this requires a type switch where mentionedRealPathFileLister
to return an error, which has a small chance that someone might possibly be using it explicitly, and which might break things silently if we donāt also define a [Ll]egacyRealPathFileListener
(not exporting it would be more ideal), that would catch the previous interface and account for it. The later would still require a type switch exactly the same as the first, just with different type names.Forgot to mention: I did a search on Github for RealPathFileLister
and the only results that come up are our code and vendoring of our code.
But then the worry is always that you might break closed source, right? The one you cannot account for ahead of time.
Ok, do you have any preference? Maybe the second option will avoid a backward incompatible change in the future
Yeah, Iām partial to the second option. Thereās a higher possibility of source-code-level breakage, but that risk was always going to be fairly low, and the fix is relatively easy as well, just add an error return and return a nil
.
It would make sense though to also document that the legacy form is still supported, even though it might not be exported. Just being clear about what kind of behavior exists there (since for anyone coming in new, it might seem astonishing), and why it exists.
Hi,
OpenSSH resolves symlinks for
SSH_FXP_REALPATH
calls (without failing if the symlink is broken), see here.We simply return a cleaned path without resolving any symbolic links. This is what the v3 specs literally say (v6 has more details):
If we want to behave like OpenSSH I think we can use
os.Lstat
andos.Readlink
for our server implementation.For request-server it is more tricky, we would have to generate
Lstat
andReadlink
calls ourself. Maybe we can just document our realpath behaviour and remove the depracation notice from RealPathFileLister. Request server users can implement RealPathFileLister if they want to resolve symlinks. What do you think about? Thanks