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

Fix bug at ssh.py:get() with relative path #2214

Merged
merged 8 commits into from
Jul 9, 2023
Merged

Conversation

goreil
Copy link
Contributor

@goreil goreil commented Jul 6, 2023

This fixes a bug in ssh.py that previously prevented downloading a file from a relative path.

Previously the following snippet resulted in an error:

s = ssh("exploitme.example.com")
s.get("somefile")

TypeError:** Can't mix strings and bytes in path components

Since the get function casts the remote variable to bytes, but the self.cwd is always str, this function failed no matter whether bytes or str was provided.

This fixes the bug in ssh.py:download_file by casting both parts of the join to bytes

@peace-maker
Copy link
Member

Nice, thank you! Would you mind adding a test for this please? We're using doctests, so adding an Examples section to the method documentation in the code similar to the download_data function above this one should do.

It looks like the download_data function would need the same treatment - which makes me wonder if we even need to join the remote path with cwd? It is the cwd after all and relative paths should just be relative to that already. Maybe the correct fix would be to just remove the os.path.join(self.cwd, remote). Still, tests would be great to make sure this works in the future!

* doctest for download and download_file
* remove os.path.join, since _download_raw already seem
  handle remote paths
@peace-maker
Copy link
Member

Maybe still create the file in /tmp but do s.cwd = '/tmp' before downloading to fix the test? In the test environment "example.pwnme" points to localhost, that's how the other tests work, but the cwd of the test user might be different to where the tests are run from.

@goreil
Copy link
Contributor Author

goreil commented Jul 7, 2023

I see, thanks for the info! Hopefully this passes the checks now.

@Arusekk
Copy link
Member

Arusekk commented Jul 7, 2023

FWIW if you do not want to waste your time waiting for the automated CI checks, you can run the tests locally with:

$ python -m sphinx -b doctest docs/source docs/build/doctest docs/source/tubes/ssh.rst

(this requires some setup described in TESTING.md, though)

I think we should one day rework the testing to make it easier and more reliable for the contributors.

@Arusekk Arusekk enabled auto-merge (squash) July 9, 2023 12:50
@Arusekk Arusekk merged commit c397626 into Gallopsled:stable Jul 9, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants