Fix: Remove glob character escaping for SCP RCP protocol#7335
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7335 +/- ##
==========================================
- Coverage 80.28% 80.28% -0.00%
==========================================
Files 577 577
Lines 45545 45545
==========================================
- Hits 36562 36560 -2
- Misses 8983 8985 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Problem: - When using OpenSSH < 9.0, scp uses the RCP protocol - RCP protocol passes paths to remote shell for glob expansion - Escaping glob characters (*, ?, []) breaks file pattern matching - This causes SCP to fail with returncode=1 when copying files with glob paths Solution: - Remove ALL glob characters from the special character escape set in _escape_for_rcp() - Glob characters should be passed to remote shell for proper expansion - All three glob metacharacters are preserved: * (asterisk) - matches any sequence of characters ? (question mark) - matches any single character [] (square brackets) - matches character classes/ranges Changes: 1. Modified _escape_for_rcp() to not escape glob characters (*, ?, []) 2. Updated docstring in _escape_for_rcp() to explain why glob chars are preserved 3. Enhanced docstring in _escape_for_scp() to clearly document version-specific behavior: - OpenSSH 9.0+ uses SFTP mode (no escaping needed) - OpenSSH < 9.0 uses RCP mode (preserve glob chars for shell expansion) Testing: - Verified on OpenSSH 8.9 (Ubuntu 22.04) - Successfully copies remote files with glob patterns (*.txt, etc.) - OpenSSH 9.0+ unaffected (uses SFTP protocol) Compatibility: - Fully compatible with both OpenSSH < 9.0 and >= 9.0 - Version detection is based on is_openssh_9_or_higher flag
8a77ba6 to
4d74294
Compare
khsrali
left a comment
There was a problem hiding this comment.
Thanks a lot @stillfast
|
I thought we merged this, still here 😬 |
|
Sorry, I will fix it as soon as possible. Let me try again @khsrali |
|
@stillfast I think it's all good, the failing test might have been flaky. let's give it another 20 min, once passed I'll merge it |
|
OK, I used to think we should change the code according to the failing test. It is fine if it pass. |
|
Thanks a lot @stillfast for finding the bug and for your contribution! |
Problem:
Solution:
Changes:
Testing:
Compatibility: