[Bug 43078] New: File transport SFTP backend returns inconsistent list() structure compared to FTP/Local backends
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 Bug ID: 43078 Summary: File transport SFTP backend returns inconsistent list() structure compared to FTP/Local backends Initiative type: --- Sponsorship --- status: Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@openfifth.co.uk QA Contact: testopia@bugs.koha-community.org Target Milestone: --- The polymorphic file transport classes (FTP, SFTP, Local) are intended to provide a consistent API across transport backends, wrapping existing CPAN modules (Net::FTP, Net::SFTP::Foreign) alongside a local filesystem implementation. The list() method on the FTP and Local backends returns an array of flat hashrefs, each containing file metadata (filename, size, modification time, etc.). The SFTP backend, however, returns metadata partially nested inside Net::SFTP::Foreign::Attributes objects, as this is the native return structure from Net::SFTP::Foreign. This means callers of list() currently need to know which backend they're talking to in order to correctly extract file metadata, which defeats the purpose of the shared API. Steps to reproduce: Instantiate the FTP or Local transport class and call list() on a directory Instantiate the SFTP transport class and call list() on the same logical directory Compare the structure of returned metadata — SFTP results include nested attribute objects; FTP/Local do not Expected behaviour: All three backends return metadata in the same flat hashref structure for a given list() call. Suggested fix: Flatten the relevant fields out of Net::SFTP::Foreign::Attributes (size, mtime, permissions, etc.) into the same flat hash shape used by the other two backends, within the SFTP wrapper class. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk Depends on| |42992 Status|NEW |ASSIGNED Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42992 [Bug 42992] FTP file transport list_files() corrupts filenames -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|--- |Small patch Sponsorship status|--- |Unsponsored -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 201960 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201960&action=edit Bug 43078: Unify file transport list_files() across FTP, Local and SFTP Net::SFTP::Foreign's ls() returned file metadata with size, permissions and modification time nested inside a Net::SFTP::Foreign::Attributes object, while FTP and Local returned flat hash keys and silently dropped directories. Callers of list_files() needed backend-specific parsing to get at file metadata or to know a directory even existed, defeating the purpose of the shared transport API. All three transports now return the same flat hashref shape per entry: filename, longname, size, perms, mtime, type ('file'/'directory'/ 'other'). Directories are included everywhere; '.' and '..' remain excluded everywhere. FTP's MLSD path prefers the UNIX.mode fact for exact Unix permissions, falling back to an approximation from the RFC 3659 perm capability fact when UNIX.mode isn't available, and parses the modify fact into mtime; its NLST fallback (servers without MLSD) probes each entry with SIZE/MDTM to recover size, mtime and a best-effort type. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com, | |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43087 Blocks| |43088 Blocks| |43092 Attachment #201960|0 |1 is obsolete| | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43087 [Bug 43087] Koha::File::Transport lacks a consistent current_directory() accessor across FTP/SFTP/Local https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 [Bug 43088] Koha::File::Transport lacks a consistent current_directory() accessor across FTP/SFTP/Local https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43092 [Bug 43092] Koha::File::Transport::change_directory(undef) means three different things across FTP, SFTP and Local -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 --- Comment #2 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 202015 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202015&action=edit Bug 43078: Unify file transport list_files() across FTP, Local and SFTP Net::SFTP::Foreign's ls() returned file metadata with size, permissions and modification time nested inside a Net::SFTP::Foreign::Attributes object, while FTP and Local returned flat hash keys and silently dropped directories. Callers of list_files() needed backend-specific parsing to get at file metadata or to know a directory even existed, defeating the purpose of the shared transport API. All three transports now return the same flat hashref shape per entry: filename, longname, size, perms, mtime, type ('file'/'directory'/ 'other'). Directories are included everywhere; '.' and '..' remain excluded everywhere. FTP's MLSD path prefers the UNIX.mode fact for exact Unix permissions, falling back to an approximation from the RFC 3659 perm capability fact when UNIX.mode isn't available, and parses the modify fact into mtime; its NLST fallback (servers without MLSD) probes each entry with SIZE/MDTM to recover size, mtime and a best-effort type. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 --- Comment #3 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 202016 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202016&action=edit Bug 43078: Include longname and perms keys in FTP's NLST fallback listing The MLSD path, SFTP, and Local always return all six documented list_files() keys (filename, longname, size, perms, mtime, type) - undef where the value is unknown, but the key always present. The NLST fallback path (servers without MLSD support) only set filename, size, mtime and type, omitting longname and perms entirely rather than setting them to undef. Functionally harmless for consumers reading $file->{perms} (Perl returns undef for a missing hash key too), but it breaks the "same flat shape" contract this bug established, and would trip up any consumer using exists() instead of defined() to check for a key. Test plan: 1. prove t/db_dependent/Koha/File/Transport/FTP.t => All tests successful, including the new assertion that longname and perms keys are present (as undef) on the NLST fallback path. 2. koha-qa.pl -v 2 --more-tests => [PASS], no critic/pod warnings. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #202015|0 |1 is obsolete| | Attachment #202016|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 --- Comment #4 from David Nind <david@davidnind.com> --- Created attachment 202124 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202124&action=edit Bug 43078: Unify file transport list_files() across FTP, Local and SFTP Net::SFTP::Foreign's ls() returned file metadata with size, permissions and modification time nested inside a Net::SFTP::Foreign::Attributes object, while FTP and Local returned flat hash keys and silently dropped directories. Callers of list_files() needed backend-specific parsing to get at file metadata or to know a directory even existed, defeating the purpose of the shared transport API. All three transports now return the same flat hashref shape per entry: filename, longname, size, perms, mtime, type ('file'/'directory'/ 'other'). Directories are included everywhere; '.' and '..' remain excluded everywhere. FTP's MLSD path prefers the UNIX.mode fact for exact Unix permissions, falling back to an approximation from the RFC 3659 perm capability fact when UNIX.mode isn't available, and parses the modify fact into mtime; its NLST fallback (servers without MLSD) probes each entry with SIZE/MDTM to recover size, mtime and a best-effort type. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 202125 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202125&action=edit Bug 43078: Include longname and perms keys in FTP's NLST fallback listing The MLSD path, SFTP, and Local always return all six documented list_files() keys (filename, longname, size, perms, mtime, type) - undef where the value is unknown, but the key always present. The NLST fallback path (servers without MLSD support) only set filename, size, mtime and type, omitting longname and perms entirely rather than setting them to undef. Functionally harmless for consumers reading $file->{perms} (Perl returns undef for a missing hash key too), but it breaks the "same flat shape" contract this bug established, and would trip up any consumer using exists() instead of defined() to check for a key. Test plan: 1. prove t/db_dependent/Koha/File/Transport/FTP.t => All tests successful, including the new assertion that longname and perms keys are present (as undef) on the NLST fallback path. 2. koha-qa.pl -v 2 --more-tests => [PASS], no critic/pod warnings. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 --- Comment #6 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 202205 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202205&action=edit Bug 43078: Exclude '.' and '..' entries from SFTP file listings The FTP and Local transports both explicitly filter out '.' and '..' pseudo-entries when listing a directory, but the SFTP transport never did - it relied on the assumption that Net::SFTP::Foreign's ls() already excludes them. It doesn't: ls() returns exactly whatever entries the remote server includes in its SSH_FXP_READDIR response, with no default filtering. Whether '.'/'..' show up therefore depends entirely on the remote SFTP server implementation, producing inconsistent results between otherwise identically configured SFTP accounts. Test plan: 1. prove t/db_dependent/Koha/File/Transport/SFTP.t => fails before this patch (listing includes '.' and '..') => passes after this patch 2. prove -r t/db_dependent/Koha/File/Transport/ t/db_dependent/Koha/File/Transports.t => all still pass -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |andrew@bywatersolutions.com |y.org | CC| |lisette@bywatersolutions.co | |m -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #202124|0 |1 is obsolete| | Attachment #202125|0 |1 is obsolete| | Attachment #202205|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 --- Comment #7 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 202827 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202827&action=edit Bug 43078: Unify file transport list_files() across FTP, Local and SFTP Net::SFTP::Foreign's ls() returned file metadata with size, permissions and modification time nested inside a Net::SFTP::Foreign::Attributes object, while FTP and Local returned flat hash keys and silently dropped directories. Callers of list_files() needed backend-specific parsing to get at file metadata or to know a directory even existed, defeating the purpose of the shared transport API. All three transports now return the same flat hashref shape per entry: filename, longname, size, perms, mtime, type ('file'/'directory'/ 'other'). Directories are included everywhere; '.' and '..' remain excluded everywhere. FTP's MLSD path prefers the UNIX.mode fact for exact Unix permissions, falling back to an approximation from the RFC 3659 perm capability fact when UNIX.mode isn't available, and parses the modify fact into mtime; its NLST fallback (servers without MLSD) probes each entry with SIZE/MDTM to recover size, mtime and a best-effort type. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 --- Comment #8 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 202828 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202828&action=edit Bug 43078: Include longname and perms keys in FTP's NLST fallback listing The MLSD path, SFTP, and Local always return all six documented list_files() keys (filename, longname, size, perms, mtime, type) - undef where the value is unknown, but the key always present. The NLST fallback path (servers without MLSD support) only set filename, size, mtime and type, omitting longname and perms entirely rather than setting them to undef. Functionally harmless for consumers reading $file->{perms} (Perl returns undef for a missing hash key too), but it breaks the "same flat shape" contract this bug established, and would trip up any consumer using exists() instead of defined() to check for a key. Test plan: 1. prove t/db_dependent/Koha/File/Transport/FTP.t => All tests successful, including the new assertion that longname and perms keys are present (as undef) on the NLST fallback path. 2. koha-qa.pl -v 2 --more-tests => [PASS], no critic/pod warnings. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 --- Comment #9 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 202829 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202829&action=edit Bug 43078: Exclude '.' and '..' entries from SFTP file listings The FTP and Local transports both explicitly filter out '.' and '..' pseudo-entries when listing a directory, but the SFTP transport never did - it relied on the assumption that Net::SFTP::Foreign's ls() already excludes them. It doesn't: ls() returns exactly whatever entries the remote server includes in its SSH_FXP_READDIR response, with no default filtering. Whether '.'/'..' show up therefore depends entirely on the remote SFTP server implementation, producing inconsistent results between otherwise identically configured SFTP accounts. Test plan: 1. prove t/db_dependent/Koha/File/Transport/SFTP.t => fails before this patch (listing includes '.' and '..') => passes after this patch 2. prove -r t/db_dependent/Koha/File/Transport/ t/db_dependent/Koha/File/Transports.t => all still pass Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|43092 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43092 [Bug 43092] Koha::File::Transport::change_directory(undef) means three different things across FTP, SFTP and Local -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |26.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 --- Comment #10 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Thanks everyone! Pushed to main for 26.11! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org