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

Support OPTS commands for querying configured policy for resumed uploads, downloads #1676

Closed
martinprikryl opened this issue May 25, 2023 · 8 comments
Assignees
Milestone

Comments

@martinprikryl
Copy link

martinprikryl commented May 25, 2023

Repeatedly I got reports from WinSCP users having problem with uploading to ProFTPD server that has AllowStoreRestart disabled (the default, I believe). One of the reports.

When the upload is interrupted for whatever reason, WinSCP cannot recover it automatically.
ProFTPD reports REST STREAM in FEAT, so WinSCP believes it can resume the transfer. But it does not work, as ProFTPD fails it with

451 Append/Restart not permitted, try again

I'm wondering how to improve the user experience here. But there does not seem much WinSCP can do to detect the situation.

  • As mentioned, FEAT contains REST STREAM.
  • The code 451 is too generic.
  • The error response comes after STOR command, not REST (I understand that's it because at the REST command, the server does not know yet, if it's gonna be for upload or download – and the permissions for those are separate).

Could you:

  • Not send REST STREAM, at least when AllowStoreRestart is disabled globally (I guess that it's the most common scenario, as it's the default)? (I understand that the directive can be set per-folder).
  • Use more unique error code?
  • Other ideas? :)

Thanks.

@Castaglia Castaglia self-assigned this May 25, 2023
@Castaglia
Copy link
Member

The particular response codes used here are from RFC 959 (not the best guide, but many clients adhere to it strictly, sadly):

REST
500, 501, 502, 421, 530
350

And, as you point out, at time of REST command we don't know whether the next data transfer (assuming there is one) will be a file upload, download, or even a directory listing (although what REST would mean for a directory list is undefined, as far as I know).

We could return 503 "Bad sequence of commands" for a REST followed by a STOR, but this too is not explicitly called out in RFC 959.

The REST STREAM presence in the FEAT response applies equally to file uploads and downloads; AllowStoreRestart only pertains to uploads. So removing the FEAT entry if AllowStoreRestart off is configured seems overkill, as it would prevent FEAT-honoring clients from restarting downloads as well.

Obviously the "fix" is remove the AllowStoreRestart off setting from the ProFTPD configuration. But as your report highlights, it is often new users, unfamiliar with all of the details and just wanting to "make things work" who encounter these problems and are frustrated. I could add some logging in ProFTPD to try to indicate the issue better -- but that, too, assumes that users know where/how to look at the ProFTPD logs in such cases.

I guess my next idea might be some custom SITE command, that WinSCP could use to discover this particular ProFTPD configuration a priori. FEAT isn't specific enough for this particular case.

@martinprikryl
Copy link
Author

SITE command would do (something like SITE RESTUPLOADALLOWED path to check if restartable upload is possible for given path?). I'm just not sure if it is not an overkill for this. I'll leave that on you :) Thanks for your feedback.

@Castaglia
Copy link
Member

Castaglia commented Jun 3, 2023

I was just re-reading RFC 2389 Section #4, and thought of another possibility: an OPTS REST STOR command. That is, can we use the OPTS command to effectively query whether the REST + STOR would work? ProFTPD, in the presence of AllowStoreRestart off, might respond with a 451 response code.

Currently ProFTPD would reject such an OPTS REST command, but I could add this pretty easily. This approach feels slightly less cumbersome than a custom SITE command. Thoughts?

@martinprikryl
Copy link
Author

Sounds good!

@Castaglia Castaglia changed the title Handling disabled AllowStoreRestart on client side Support OPTS commands for querying configured policy for resumed uploads, downloads Jun 6, 2023
Castaglia added a commit that referenced this issue Jun 6, 2023
…nticated clients can use for querying the configured policy for allowing resumed uploads, download.
@Castaglia Castaglia added this to the 1.3.9 milestone Jun 6, 2023
Castaglia added a commit that referenced this issue Jun 6, 2023
Issue #1676: Implement support for an `OPTS REST` command, that authe…
@Castaglia
Copy link
Member

Castaglia commented Jun 6, 2023

Support for an OPTS REST STOR (and OPTS REST RETR, for symmetry) has been merged to master. Note that ProFTPD expects that the client have authenticated first, before trying out these OPTS REST commands.

Let me know if it works, or if you find any issues. Thanks!

@martinprikryl
Copy link
Author

Thanks! I'll try to test it within few days.

@martinprikryl
Copy link
Author

martinprikryl commented Jun 7, 2023

Thanks. Seems to be working fine.
I've implemented the WinSCP part:
https://winscp.net/tracker/2194


Do I understand right that the OPTS REST command evaluates only global (or per-user) configuration (not the per-directory configuration)?

@Castaglia
Copy link
Member

Do I understand right that the OPTS REST command evaluates only global (or per-user) configuration (not the per-directory configuration)?

Currently, yes. I understand that there may be per-directory edge cases with this feature; I want to tackle those separately, going forward, as those use cases arise. Honoring the global-level configurations should hopefully handle a majority of use cases.

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

No branches or pull requests

2 participants