distutils crashes when uploading to PyPI having only the username (no pw) defined

[jgehrcke]

BPO	18454
Nosy	@loewis, @jaraco, @ncoghlan, @giampaolo, @tarekziade, @merwok, @jgehrcke, @Wooble, @zooba, @dstufft, @driesdesmet, @pi314
Files	issue18454_py27_prompt.patch issue18454_py27_prompt_test.patch issue18454_py35_prompt.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/merwok'
closed_at = <Date 2021-02-03.18:19:31.001>
created_at = <Date 2013-07-14.17:24:48.605>
labels = ['type-bug', 'library']
title = 'distutils crashes when uploading to PyPI having only the username (no pw) defined'
updated_at = <Date 2021-02-03.18:19:31.000>
user = 'https://github.com/jgehrcke'

bugs.python.org fields:

activity = <Date 2021-02-03.18:19:31.000>
actor = 'steve.dower'
assignee = 'eric.araujo'
closed = True
closed_date = <Date 2021-02-03.18:19:31.001>
closer = 'steve.dower'
components = ['Distutils']
creation = <Date 2013-07-14.17:24:48.605>
creator = 'jgehrcke'
dependencies = []
files = ['38005', '38006', '42817']
hgrepos = []
issue_num = 18454
keywords = ['patch']
message_count = 8.0
messages = ['193062', '202384', '221688', '235143', '235364', '260473', '265320', '386341']
nosy_count = 13.0
nosy_names = ['loewis', 'jaraco', 'ncoghlan', 'techtonik', 'giampaolo.rodola', 'tarek', 'eric.araujo', 'jgehrcke', 'geoffreyspear', 'steve.dower', 'dstufft', 'dries_desmet', '\xe6\xb1\xa0\xe6\x98\x8c\xe8\xa8\x80']
pr_nums = []
priority = 'normal'
resolution = 'out of date'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue18454'
versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

[jgehrcke]

When updating an existing project on PyPI via distutils using the upload command, I observe erroneous behavior regarding the credentials when I do not want to store my password in clear text in the pypirc file:

(1) When running

python setup.py sdist upload

without having the pypirc file in place, I get the error

Upload failed (401): You must be identified to edit package information

(2) When running the same command as above with the pypirc file in place but without having the 'password' option in the 'pypi' section defined, I get a TypeError exception.

In both cases and at least in the second case I expect to be prompted for my credentials. This is what the 2.7.5 docs are saying about the contents of the pypirc file (http://docs.python.org/2.7/distutils/packageindex.html#the-pypirc-file):

"password, that will be used to authenticate. If omitted the user will be prompt to type it when needed."

I have seen http://bugs.python.org/issue5187 saying "distutils is feature frozen" but the current situation is buggy. Either there is a documentation mistake (it clearly says that the user is prompted for the password) or there is an error in the code (*, see below), or both.

• Regarding the TypeError mentioned above:

In distutils/command/upload.py, finalize_options(), the configuration dictionary is retrieved from _read_pypirc() (distutils/config.py). There, the value for the password key in the config dictionary is set to None if not defined in the pypirc configuration file. The password value is not modified/updated in finalize_options() if self.distribution.password is not set. I think the latter is only set when the 'register' command is used. Hence, when the user for example simply runs

python setup.py sdist upload

and did not set the password in the pypirc file, the password value stays None.

Nevertheless, in distutils/command/upload.py, upload_file(), password is treated as string:

    auth = "Basic " + standard_b64encode(self.username + ":" + self.password)

This obviously leads to

TypeError: cannot concatenate 'str' and 'NoneType' objects

I would be happy to work on a patch if we agree on what the proper behavior should be. Me, as a user of PyPI, would vote for being prompted in both cases outlined above. I do not like to store my PyPI password in clear text in the file system.

And after specifying how distutils should behave in case (2) I think we all agree that distutils/tests/test_upload.py should provide a test for this case. An example configuration file with username but without password is already defined via PYPIRC_NOPASSWORD. Currently, this config is only tested within an edge-case in test_saved_password() with dist.password = 'xxx', simulating the simultaneous usage of 'register' with 'upload' if I understood correctly. Register probably is used less frequently than upload alone.

Looking forward to your input,

Jan-Philip

[giampaolo]

Same problem here. I'm currently uploading .exe files for psutil by hand.
Interestingly the problem occurs with certain versions of python only (2.4, 2.5, 2.7, 3.2).

[driesdesmet]

I confirm, using python 2.7 on Mac.

[ncoghlan]

I agree the documentation is correct here, and the stdlib should be updated to implement the expected behaviour.

While distutils is indeed feature frozen, that's aimed at changing how we approach the larger scale evolution of the packaging ecosystem, it doesn't mean we won't fix bugs and usability issues. (While we may be slow sometimes, that's a general core developer availability issue, rather than a distutils specific problem)

[jgehrcke]

First, I want to address the situation in 2.7. Please have a look at my patch and my reasoning.

This is my setup.py test file content:

    from distutils.core import setup
    setup(name='foo', version='1.0', packages=['testpackage'])

This is the current situation for 2.7 head, in summary:

1) Without a proper .pypirc file in place, uploading fails with
   "Upload failed (401): You must be identified to edit
    package information"

2) With a valid .pypirc in place which does not define a password
   this exception is raised:
   TypeError: cannot concatenate 'str' and 'NoneType' objects

(1) happens due to the fact that the default username/password strings are empty, but the request is still fired against PyPI. This obviously fails. This could be fixed by prompting for both, username and password or by aborting before sending the request. However, this would be quite a significant change in behavior. I think we should not do this, because AFAIK the 2.7 documentation does not specify what the behavior should be *without* existing .pypirc file. So, I think there might not be enough reason to consider such a change a bug fix. What do you think?

(2) violates the docs and I guess this is what Nick meant when he said that the stdlib should be updated. I propose a patch so that what the docs say ("If omitted, the user will be prompt to type it when needed") is what actually happens. The patch consists of two parts:

Part A: issue18454_py27_prompt.patch
------------------------------------

This uses getpass/sys.stdin.readline() for retrieving the password, depending on whether the process is attached to a tty or not, respectively. The config parsing and value propagation is a little messy in the command/upload.py module, so I have also added comments for clarity and simplified the logic a little.

Part B: issue18454_py27_prompt_test.patch
-----------------------------------------

Here it gets messy. Normally, we want to have a new test case that fails before applying patch A, and succeeds after applying it. However, here we actually have to deal with an already existing bogus test case which succeeds before and fails after A gets applied. A very bad sign, usually. But in this case I am of the strong opinion that we can and have to change the test implementation, because:

- the test actually makes sure that `cmd.password` is set to None when
  `cmd.finalize_options()` has done its work. However,`cmd.password` being
  None leads to the TypeError exception described above. That is, this test
  case basically ensures the very existance of the bug that we want to fix.

- it has a suspiciously useless name. It is called `test_saved_password`,
  but actually tests the case when no password is saved in .pypirc.
  
- this test actually has quite low code coverage. AFAIK it *only* tests
  the behavior of `cmd.finalize_options()`. We want to change this behavior,
  so we have to consider changing the test.

My patch modifies this test case so that it has a proper name and makes sure that cmd.finalize_options() obtains a password from stdin. The involvement of stdin makes testing a little complicated. In a proper testing environment, we'd probably spawn a subprocess and provide it with some real stdin data. The distutils upload command test structure seems not to be prepared for this endeavor, so I have decided to temporarily/locally patch sys.stdin, and ensure restoration with a try/finally clause.

With A and B in place, all distutils unit tests validate. I used two methods of invocation, for covering the two cases (with and without attached tty):

    $ nohup python test_distutils.py 2>&1 > out.log &
    $ python test_distutils.py

[jaraco]

I've recently taken a different tack to this issue with setuptools 20.1, which will resolve the password from a keyring if available.

[pi314]

Patch for Python 3.5, using getpass module

[zooba]

Distutils is now deprecated (see PEP-632) and all tagged issues are being closed. From now until removal, only release blocking issues will be considered for distutils.

If this issue does not relate to distutils, please remove the component and reopen it. If you believe it still requires a fix, most likely the issue should be re-reported at https://github.com/pypa/setuptools