Having a new check to avoid redirects from PyPI #809

dukecat0 · 2021-09-12T09:48:04Z

Fixes #310

We should also update the redirect test as it is using https://test.pypi.org/legacy for the test, which is replaced by the exception InvalidPyPIUploadURLinstead of RedirectDetected currently. Any suggestions? (I think we need a new URL to test.)

twine/tests/test_upload.py

Lines 251 to 278 in f543114

    
           def test_exception_for_redirect(make_settings): 
        
               # Not using fixtures because this setup is significantly different 
        
               upload_settings = make_settings( 
        
                   """ 
        
                   [pypi] 
        
                   repository: https://test.pypi.org/legacy 
        
                   username:foo 
        
                   password:bar 
        
                   """ 
        
               ) 
        
               stub_response = pretend.stub( 
        
                   is_redirect=True, 
        
                   status_code=301, 
        
                   headers={"location": "https://test.pypi.org/legacy/"}, 
        
               ) 
        
               stub_repository = pretend.stub( 
        
                   upload=lambda package: stub_response, close=lambda: None 
        
               ) 
        
               upload_settings.create_repository = lambda: stub_repository 
        
               with pytest.raises(exceptions.RedirectDetected) as err: 
        
                   upload.upload(upload_settings, [helpers.WHEEL_FIXTURE]) 
        
               assert "https://test.pypi.org/legacy/" in err.value.args[0]

Old:

$ twine upload -r pypitest dist/*
[...]
RedirectDetected: https://test.pypi.org/legacy attempted to redirect to https://test.pypi.org/legacy/.
If you trust these URLs, set https://test.pypi.org/legacy/ as your repository URL.
Aborting.

New:

$ twine upload -r pypitest dist/*
InvalidPyPIUploadURL: The configured repository https://test.pypi.org/legacy is not a known PyPI repository.
Did you mean https://upload.pypi.org/legacy/ or https://test.pypi.org/legacy/ ?
Check your --repository-url value or modify the url in ~/.pypirc (if you are using -r or --repository),
so twine can upload your distributions properly.
See https://packaging.python.org/specifications/pypirc/ for more details.

sigmavirus24 · 2021-09-12T13:15:44Z

twine/settings.py

+                f"or {utils.TEST_REPOSITORY} ?\n"
+                f"Check your --repository-url value or "
+                f"modify the url in {self.config_file} "
+                f"(if you are using -r or --repository),\n"


I don't think the parenthetical is necessary or helpful as written

Using --repository-url https://test.pypi.org/legacy will also raise InvalidPyPIUploadURL. Probably folks who use --repository-url won't know what is ~/.pypirc and makes confusion to them?

twine/settings.py

bhrutledge

I haven't looked at the failing test yet, but that should be fixed, and probably a new one added for this behavior.

bhrutledge · 2021-09-12T15:22:22Z

twine/settings.py

+        if "pypi.org" in repository_url and repository_url not in [
+            utils.DEFAULT_REPOSITORY,
+            utils.TEST_REPOSITORY,
+        ]:
+            raise exceptions.InvalidPyPIUploadURL(
+                f"The configured repository {repository_url} is not a known "
+                f"PyPI repository.\n"
+                f"You probably want {utils.DEFAULT_REPOSITORY} "
+                f"or {utils.TEST_REPOSITORY}.\n"
+                f"Check your --repository-url value "
+                f"or the repository configuration in {self.config_file}.\n"
+                f"See https://packaging.python.org/specifications/pypirc/ "
+                f"for more details."
+            )


Following up on #809 (comment), I generalized the logic to catch more cases, and tweaked the message. Here's what it looks like:

% twine upload --repository-url https://upload.pypi.org/legacy dist/* InvalidPyPIUploadURL: The configured repository https://upload.pypi.org/legacy is not a known PyPI repository. You probably want https://upload.pypi.org/legacy/ or https://test.pypi.org/legacy/. Check your --repository-url value or the repository configuration in ~/.pypirc. See https://packaging.python.org/specifications/pypirc/ for more details.

In the process, I noted that the logic and messages is very similar to twine.utils.check_status_code. I'd really like to refactor that, maybe by moving the messages into the exception definitions. But I'm guessing that should be a follow-up PR, related to #586.

dukecat0 · 2021-09-12T15:35:32Z

I haven't looked at the failing test yet, but that should be fixed, and probably a new one added for this behavior.

Yes, and the failing test is about the redirecting test. Probably we need a new link to pass the test.

#809 (comment)

bhrutledge · 2021-09-14T01:21:02Z

twine/settings.py

+                f"See https://packaging.python.org/specifications/pypirc/ "
+                f"for more details."
+            )
+


I fixed the failing test and adding coverage for the new behavior in 414625b, but looking at the drop in coverage via Codecov emphasized my point in #809 (comment), i.e. that this is similar to utils.check_status_code.

Taking a closer look at that, plus the comments in #310, plus my initial attempt in #460 to offer a better error message, I'm now wondering if it's needlessly redundant, with the primary difference being that this executes before an upload, while check_status_code executes afterwards. Instead, maybe we could improve the post-upload RedirectDetected message by tweaking the language, possibly with a special message for a trailing slash, regardless of whether or not it's a pypi.org hostname.

In other words, this feels like a case of "Look Before You Leap" vs "It's Easter to Ask Forgiveness than Permission", and with this PR, I think we're doing both. I'm going to mull this over a bit, and maybe put up another PR that improves the EAFP approach.

@meowmeowmeowcat Apologies for the confusion here; I should have taken a closer look at the existing behavior.

@bhrutledge I think the improved message in #812 would be also helpful:

RedirectDetected: https://upload.pypi.org/legacy attempted to redirect to https://upload.pypi.org/legacy/. Your repository URL is missing a trailing slash. Please add it and try again.

But I also think that having this check will save folks' time. They will know there is an error in the config or --repository-url before twine tries to upload their distributions, although this check is similar to utils.check_status_code.

It's true that it will save a little bit of time, but I'm not sure it's worth the added complexity and technical debt of duplicated logic. It's also not as robust as the repository-independent implementation in #812. Furthermore, if PyPI ever supports a different URL, then this implementation will block people from uploading to it at all.

I see. Probably this PR can be closed? The improved message in #812 is enough.

I'd like to get @sigmavirus24's feedback on #812 first.

bhrutledge · 2021-09-16T12:42:06Z

Closing in favor of #812 (which I'll merge once I've added a changelog entry). Thanks again for the effort, @meowmeowmeowcat.

dukecat0 added 3 commits September 12, 2021 17:41

Having a new check to avoid redirects from PyPI

776712a

Add a link to docs of .pypirc

dcec394

Fix linting error

a806125

sigmavirus24 reviewed Sep 12, 2021

View reviewed changes

bhrutledge self-requested a review September 12, 2021 13:29

Rewrite logic and message

074d5f1

dukecat0 commented Sep 12, 2021

View reviewed changes

twine/settings.py Outdated Show resolved Hide resolved

Tweak message

651853a

bhrutledge requested changes Sep 12, 2021

View reviewed changes

bhrutledge added 2 commits September 13, 2021 20:27

Update tests

414625b

Use safer URL parsing

4d27ee5

bhrutledge reviewed Sep 14, 2021

View reviewed changes

bhrutledge mentioned this pull request Sep 14, 2021

Improve redirect error message for trailing slash #812

Merged

bhrutledge closed this Sep 16, 2021

dukecat0 deleted the new-check branch September 16, 2021 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Having a new check to avoid redirects from PyPI #809

Having a new check to avoid redirects from PyPI #809

dukecat0 commented Sep 12, 2021 •

edited

Loading

sigmavirus24 Sep 12, 2021

dukecat0 Sep 12, 2021 •

edited

Loading

bhrutledge left a comment

bhrutledge Sep 12, 2021

dukecat0 commented Sep 12, 2021 •

edited

Loading

bhrutledge Sep 14, 2021

dukecat0 Sep 14, 2021 •

edited

Loading

bhrutledge Sep 14, 2021

dukecat0 Sep 14, 2021 •

edited

Loading

bhrutledge Sep 14, 2021 •

edited

Loading

bhrutledge commented Sep 16, 2021

	def test_exception_for_redirect(make_settings):
	# Not using fixtures because this setup is significantly different

	upload_settings = make_settings(
	"""
	[pypi]
	repository: https://test.pypi.org/legacy
	username:foo
	password:bar
	"""
	)

	stub_response = pretend.stub(
	is_redirect=True,
	status_code=301,
	headers={"location": "https://test.pypi.org/legacy/"},
	)

	stub_repository = pretend.stub(
	upload=lambda package: stub_response, close=lambda: None
	)

	upload_settings.create_repository = lambda: stub_repository

	with pytest.raises(exceptions.RedirectDetected) as err:
	upload.upload(upload_settings, [helpers.WHEEL_FIXTURE])

	assert "https://test.pypi.org/legacy/" in err.value.args[0]

Having a new check to avoid redirects from PyPI #809

Having a new check to avoid redirects from PyPI #809

Conversation

dukecat0 commented Sep 12, 2021 • edited Loading

sigmavirus24 Sep 12, 2021

Choose a reason for hiding this comment

dukecat0 Sep 12, 2021 • edited Loading

Choose a reason for hiding this comment

bhrutledge left a comment

Choose a reason for hiding this comment

bhrutledge Sep 12, 2021

Choose a reason for hiding this comment

dukecat0 commented Sep 12, 2021 • edited Loading

bhrutledge Sep 14, 2021

Choose a reason for hiding this comment

dukecat0 Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

bhrutledge Sep 14, 2021

Choose a reason for hiding this comment

dukecat0 Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

bhrutledge Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

bhrutledge commented Sep 16, 2021

dukecat0 commented Sep 12, 2021 •

edited

Loading

dukecat0 Sep 12, 2021 •

edited

Loading

dukecat0 commented Sep 12, 2021 •

edited

Loading

dukecat0 Sep 14, 2021 •

edited

Loading

dukecat0 Sep 14, 2021 •

edited

Loading

bhrutledge Sep 14, 2021 •

edited

Loading