-
Notifications
You must be signed in to change notification settings - Fork 963
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
Make filename check more strict #14027
Conversation
No more, no less. Previously this allowed filenames that started with the project name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typos suggested for fixin' and a couple of non-blocking questions.
filename.partition("-")[0] | ||
if filename.endswith(".whl") | ||
# For source releases, we know that the version should not contain any | ||
# hypens, so we can split on the last hypen to get the project name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# hypens, so we can split on the last hypen to get the project name. | |
# hyphens, so we can split on the last hyphen to get the project name. |
@@ -1208,10 +1208,20 @@ def file_upload(request): | |||
# Ensure the filename doesn't contain any characters that are too 🌶️spicy🥵 | |||
_validate_filename(filename) | |||
|
|||
# Extract the project name from the filename and normalize it. | |||
filename_prefix = pkg_resources.safe_name( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: this is using the deprecated pkg_resources
package - should we take the time now to replace, or do you want for forge ahead and let this get bundled into #13991 (or another)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd like it to get bunded into the fix for #7811, this is sort of orthogonal.
|
||
db_request.POST = MultiDict( | ||
{ | ||
"metadata_version": "1.2", | ||
"name": project.name, | ||
"version": release.version, | ||
"filetype": "sdist", | ||
"md5_digest": "nope!", | ||
"md5_digest": _TAR_GZ_PKG_MD5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: now that we're paramtrizing .whl files in the test case, does the same TAR_GZ behavior name apply to those files as well? Or is this 100% a testing-only behavior and we're unlikely to hit a whl-specific condition on prod? I suspect it's fine, but wanted to ask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file contents don't really matter here as long as the hashes match, but I agree this could be a little confusing to future travelers.
* Add a failing test * Make the test fail all the way to DID NOT RAISE * Refactor the test a bit * Add another failing test case * More test cases * Ensure filenames only have the project name No more, no less. Previously this allowed filenames that started with the project name.
This ensures that the filenames of uploaded files only have the actual project name where the project name should be. Previously, this allowed filenames that started with the project name, so a file
no-way-{version}.tar.gz
could be uploaded to theno
project, effectively making the filename unusable for theno-way
project due to our policy on filename reuse.Towards #7811, but doesn't totally fix it.