Add a function to check the security of symbolic links.#13550
Conversation
|
Revise the description of the test case.
|
notatallshaw
left a comment
There was a problem hiding this comment.
As I understand it, the underlying issue should no longer happen on Python 3.14+?
If so could you check for less than Python 3.14 and make sure the relevant tests still pass.
This will allow us to clearly identify it as code that can removed once we drop support for Python 3.14.
Add deprecation note for _untar_without_filter for future removal
This issue should not happen in Python 3.12. I've added a NOTE to the _untar_without_filter function indicating it can be removed once pip's minimum supported Python version is 3.12. |
| @@ -0,0 +1,2 @@ | |||
| Pip will now raise an installation error for a source distribution when it includes a symlink that | |||
There was a problem hiding this comment.
I know "pip" is lowercased usually, is this also the case for news? We also need to note that this is only an issue for Python versions that don't implement PEP 706 (maybe including the version ranges starting at the minimum version pip supports).
There was a problem hiding this comment.
I don’t believe there is a formal policy that pip should be lowercase. I certainly don’t do so. I think some maintainers take that view but there’s no actual consensus. So I’m fine with leaving this as it is.
There was a problem hiding this comment.
Hi, I've updated the NEWS entry to highlight that these changes are specifically for Python versions that don't support PEP 706.
Also, there's no need to dwell on the capitalization of "pip" anymore.
There was a problem hiding this comment.
Yeah, that's how I normally avoid conflicts over this :-)
| with pytest.raises(InstallationError) as e: | ||
| untar_file(tar_filepath, extract_path) | ||
|
|
||
| assert "trying to install outside target directory" in str(e.value) |
There was a problem hiding this comment.
Since we know the full paths, can we not template to do an exact match of the complete message in both negative cases?
There was a problem hiding this comment.
The modifications have been adjusted based on your suggestions.
sethmlarson
left a comment
There was a problem hiding this comment.
Had one nit, but otherwise all my comments are resolved.
| linkname = os.path.join(os.path.dirname(tarinfo.name), tarinfo.linkname) | ||
|
|
||
| linkname = os.path.normpath(linkname) | ||
| if "\\" in linkname: |
There was a problem hiding this comment.
In theory this does a full-scan of linkname already, which .replace() would also need to do? Is this any faster than calling linkname.replace() unconditionally?
There was a problem hiding this comment.
A very detailed question. Redundant conditional checks have been removed.
notatallshaw
left a comment
There was a problem hiding this comment.
@dkjsone thanks for your work here and responding to reviews, the code is in a significantly higher quality state.
I'll leave this open for a couple more weeks to give anyone else time to review or object (it seems most maintainers have been busy with other stuff this summer), otherwise I will merge.
|
Okay, thank you all for the guidance. |
|
@dkjsone thanks for your contribution to pip. |
Fixes #13537
Add the _check_link_targetfunction to validate the file pointed to by a symlink. If path traversal is detected, it should raise an InstallationErrorexception, similar to is_within_directory.
Add test cases for symlink: