Conversation
Follow-up to nextcloud#63161. The port field of an external storage holds whatever the admin typed, so `is_numeric()` still let through values that are not usable TCP ports: "21.5" and "1e3" were silently truncated by the int cast, and "0", "-2121" or "65536" were passed on to the connection as-is. Add PortHelper::parsePort(), which only accepts an integer or a digit-only string within the valid TCP port range of 1-65535 and otherwise returns the given fallback. Use it for both FTP and SFTP, including the port that SFTP parses out of the host field, and replace the hardcoded default ports with class constants. Signed-off-by: bahman026 <bahman026@gmail.com>
come-nc
left a comment
There was a problem hiding this comment.
I have mixed feelings about this, especially the use of a static class for it, and the fact that it silently falls back to default port when invalid values are given.
All the callsite with parse_url do not need this complexity as parse_url will always return an int for port. Is it really needed to check the port is valid? What happens if it’s not? I expect connection will simply fail.
Address review feedback on the previous commit: - Remove the PortHelper static class. filter_var() with FILTER_VALIDATE_INT and a 1-65535 range covers the same cases in one stdlib call, so both constructors do it inline. - Drop the validation from splitHost(). parse_url() already returns the port as an int and fails outright for anything outside 1-65535, so the check there could never fall back. - Log a warning instead of silently ignoring a configured port, so a misconfiguration is visible rather than falling back unnoticed. Leading zeros are stripped before validating, so "0022" keeps resolving to 22 as it did before. This also restores the master behaviour for "+2121", " 2121 " and 2121.0, which the previous commit rejected. Signed-off-by: bahman026 <bahman026@gmail.com>
|
@come-nc I removed the Invalid configured ports now trigger a warning instead of silently falling back. I kept the I also handled leading zeros so values like |
|
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.) |
Follow-up to #63161, addressing the review suggestion there to validate that the port is an integer within the valid TCP range.
Summary
#63161 made
FTP::__construct()fall back to the default port unless the configured value is numeric, mirroring the SFTP guard from #58350:That fixes the empty-string crash, but
is_numeric()still accepts values that are not usable TCP ports, and the cast then silently turns them into something else:"21.5"21"1e3"1000"0"0"-2121"-2121"65536"65536The port field of an external storage is a free-text parameter holding whatever the administrator typed, so all of these are reachable through the UI. FTP and SFTP are the only backends that read
['port'].The cases that merely fail to connect are not really the problem — the ones that matter are
"21.5"and"1e3", where the storage silently connects to a different valid port than the one configured.Fix
Both constructors validate the configured value inline with
filter_var(), and log a warning when an invalid value is ignored rather than falling back unnoticed:The hardcoded
21/22defaults are replaced with aDEFAULT_PORTclass constant.FILTER_VALIDATE_INTrejects leading zeros, so they are stripped first and"0022"keeps resolving to22exactly as it does on master.For SFTP the port can also be part of the host field (
somehost:2222).parse_url()already returns that as an int and fails outright for anything outside 1–65535, so it needs no validation of its own — it simply remains the fallback when the port field is empty or invalid, sosomehost:2222with an empty port still connects to 2222.Valid configurations are unaffected: an integer, or a numeric string such as
"2121", resolves exactly as before, as do"+2121"," 2121 "and2121.0.Checklist