fix(ftp): handle empty port parameter and ensure strictly numeric ports - #63161
Conversation
|
/backport to stable34 |
02f4daf to
f803781
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
/backport to stable35 |
joshtrichards
left a comment
There was a problem hiding this comment.
Thanks for including test coverage!
A later followup might be to validate that the port is an integer and within the valid TCP range (1–65535)?
This need not block this fix if though since the existing SFTP behavior is similar..
|
Please fix the unconventional commits |
The external storage settings submit an empty string when the Port field is left blank. `??` only substitutes null or missing values, so `""` was assigned to $port and passed on to FtpConnection::__construct(), whose $port parameter is typed int. PHP rejects a non-numeric string there, so adding an FTP storage without a port ended in a TypeError and an HTTP 500 - after the configuration had already been saved. Fall back to the default port of 21 unless the configured value is numeric, mirroring the guard added for SFTP in nextcloud#58350. Casting without the check would not work, because (int)"" is 0 rather than 21. Co-authored-by: Carl Schwan <carl@carlschwan.eu> Signed-off-by: bahman026 <bahman026@gmail.com>
f803781 to
7d76f9d
Compare
Updated the commit history as requested. |
Summary
Creating an FTP/FTPS external storage with an empty Port field ends in an HTTP 500.
FTP::__construct()read the port like this:??only substitutesnullor a missing key. The external storage settings submit anempty string when the field is left blank, so
""was assigned to$this->portandpassed on to
FtpConnection::__construct(bool $secure, string $hostname, int $port, ...).PHP rejects a non-numeric string for an
intparameter, so the request died with aTypeError:A numeric string such as
"2121"is coerced normally, so explicitly configured portswere never affected — only an empty (or otherwise non-numeric) value.
Two details make the failure more confusing for users:
UserStoragesController::create()saves the configuration first and calls
updateStorageStatus()afterwards, and thecrash happens in that status check. The user gets a 500 and no visible storage, then
finds the storage after reloading the page.
FTP::getConnection(),MountConfig::getBackendStatus()andStoragesController::updateStorageStatus()all catch\Exception. ATypeErroris an\Error, so it passes through them and surfaces as a hard 500 instead of a"storage not available" status.
SFTP had exactly the same problem and it was fixed in #58350
(reported in #58324, #58293, #58507 and #58833).
FTP.phpnever received the equivalentguard. FTP and SFTP are the only backends that read
['port'].Steps to reproduce
are available (for example global credentials).
TypeErrorshown above.The same steps with SFTP work, thanks to the guard from #58350.
Fix
Fall back to the default port unless the configured value is numeric, mirroring the SFTP
guard:
21stays the default, so nothing changes for existing configurations. Casting withoutthe check would not work, because
(int)""is0rather than21.Tests
apps/files_external/tests/FtpTest.phpcovers the constructor for a missing,empty,
null, non-numeric, numeric-string and integer port. The FTP connection is onlyopened lazily, so no FTP server is needed. The test is placed in
tests/rather thantests/Storage/becausetests/phpunit-autotest-external.xmlexcludes the latterdirectory, and a test there would not run in CI.
I verified the whole
files_externalsuite as well as theStorage/FtpTest.phpbackendtests against a real FTP server, and confirmed the new test fails without this change.
Side note
The
catch (\Exception $e)blocks mentioned above could be\Throwable, so that thisclass of error degrades to a storage status instead of a 500. I left that out to keep
this change minimal, and I am happy to open a separate pull request for it.
Checklist