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

Do not quote paths for ssh >= 9 #6826

Merged

Conversation

yarikoptic
Copy link
Member

This is the work by @christian-monch in #6812 with 2 little final tune ups to get it working and robustify against case where version is None for some reason (avoid quoting). And also submitted against maint.

I would appreciate quite review/merge since that test kept failing all our CI runs, bringing fatigue.

christian-monch and others added 10 commits July 7, 2022 05:58
This commit adds a very rough ssh-version detection
to deal with different filename quoting requirements
in ssh version <= 8 and in ssh version 9.

This fixes the ssh copy tests error that is
described in issue datalad#6806.

This is a rough first draft to verify the overall
viability and to test the waters in CI. The version
parsing will have to be improved.
This commit adds a "# nosec" comment to an application
of 'md5' in order to skip insecure-hash checks by
'codeclimate'. The hash is not security critical, since
it is only used as an "abbreviation" of the unique
connection property string.
This commit adds '<' and '>' to the characters that
can be put into an obscure filename.
Thie commit adds a regular expression to determine
the version, sub-version and patch-number of the used
ssh-client.

It currently only recognizes "OpenSSH" versions.
This commit removes the custom ssh-version detection
code from sshconnector.py in favor of the existing
version detection code in external_versions.py
This commit refactors the external version
detection for ssh to include the ssh type.
Currently only OpenSSH is recognized, all
other types are considered to be unknown.
This commit removes the ssh type signaling in the
external versions object because it exhibited too
many incompatibilities and we do not really have
a strategy to deal with ssh types other than
OpenSSH.
now we use external_versions to get version and it does not
store implementation. So compare just against the [0]
Swapped the logic for version comparison so that if we have not
figured out ssh version, we do not quote (since we do not know
if we should).
@codecov
Copy link

codecov bot commented Jul 9, 2022

Codecov Report

Merging #6826 (dc2f1b7) into maint (65ef546) will increase coverage by 0.96%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##            maint    #6826      +/-   ##
==========================================
+ Coverage   90.24%   91.21%   +0.96%     
==========================================
  Files         354      354              
  Lines       46056    46064       +8     
==========================================
+ Hits        41563    42017     +454     
+ Misses       4493     4047     -446     
Impacted Files Coverage Δ
datalad/support/external_versions.py 95.73% <80.00%> (-0.55%) ⬇️
datalad/support/sshconnector.py 87.70% <100.00%> (+0.85%) ⬆️
datalad/support/tests/test_sshconnector.py 98.77% <100.00%> (ø)
datalad/tests/utils_pytest.py 89.72% <100.00%> (ø)
datalad/runner/tests/test_threadsafety.py 100.00% <0.00%> (ø)
datalad/runner/tests/test_nonasyncrunner.py 100.00% <0.00%> (ø)
datalad/local/tests/test_add_archive_content.py 99.62% <0.00%> (+<0.01%) ⬆️
datalad/distributed/tests/test_ria_basics.py 98.20% <0.00%> (+0.25%) ⬆️
datalad/distributed/ora_remote.py 80.46% <0.00%> (+0.31%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65ef546...dc2f1b7. Read the comment docs.

@yarikoptic
Copy link
Member Author

Oh you are so green, 💚 it

@christian-monch
Copy link
Contributor

@yarikoptic Thanks for fixing the last bugs. Niche surprise! Just sat down to fix and: voila, it's already done. Thx

Copy link
Contributor

@christian-monch christian-monch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thx for fixing the name-comparison bug

@christian-monch christian-monch merged commit 20ddd65 into datalad:maint Jul 10, 2022
@yarikoptic yarikoptic deleted the issue-6806-ssh-quoting-fix-maint branch July 11, 2022 19:53
@github-actions
Copy link

🚀 PR was released in 0.17.1 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants