Skip to content
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

[ftp] Extend path handling #1433

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

th3hamm0r
Copy link
Contributor

@th3hamm0r th3hamm0r commented Jul 15, 2024

Currently, the FTPStorage does not allow to work in the default directory of the connection. It only allows the following:

  • ftp://foo:b@r@localhost:2121/: Adding a "/" is required, but this will execute a CWD to the root directory "/", which often fails on multi-tenancy services, where you only get access to a subfolder.
  • ftp://foo:b@r@localhost:2121/test: This will execute a CWD to the directory "test" relative to the connections PWD.
  • ftp://foo:b@r@localhost:2121//root/test: This will execute a CWD to the absolute path "/root/test".

I would like to propose the change, that you can omit the path completely, which leaves you in the directory that gets opened by default. So with the new handling both ftp://foo:b@r@localhost:2121 and ftp://foo:b@r@localhost:2121/ won't execute a CWD.
If you want to start in the root directory, you have to use ftp://foo:b@r@localhost:2121//.

What do you think?

@jschneier
Copy link
Owner

Makes sense, can you comment on backwards compat?

@th3hamm0r
Copy link
Contributor Author

Do you mean here, or somewhere in the code? :)

The only breaking change is, that if one has used an URL with the pattern {scheme}://{user}:{passwd}@{host}:{port}/, so no path (like in the docs-example), it has to be changed to {scheme}://{user}:{passwd}@{host}:{port}// (an extra / needs to be added) to get the same result (cwd to the root directory).
All other patterns work as before, additionally you can now skip the path.

@jschneier
Copy link
Owner

Thanks, basically just need to figure out how to message or deal with the backwards incompat change. Any ideas?

@th3hamm0r
Copy link
Contributor Author

I would suggest to merge this change into the next 1.X release due to the breaking change, but I'm also not sure how to formulate this in an understandable way, but I would add something like the following to a "breaking changes"-section:

In previous releases the location-URL required to contain at least a slash at the end, which resulted in a CWD to the root of the FTP. The "path" of the ftp-location can be skipped now, which results in no CWD being executed (example: {scheme}://{user}:{passwd}@{host}:{port}). The minimal location URL, with only a slash at the end, is now equal to no path.
Upgrade considerations: With this change, storage-URLs in the format of "{scheme}://{user}:{passwd}@{host}:{port}/", only containing a slash as the path, must be changed to "{scheme}://{user}:{passwd}@{host}:{port}//" (adding an extra slash).

After writing the above, I'm thinking of extending the documentation for the location-URL with more examples, what do you think?

@jschneier
Copy link
Owner

Yes, quite agree on the docs change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants