Repository navigation
RFE: We should catch all 301, 302 redirects #1485
Description
Currently we raise an error if there is a 301, 302 redirect from an http URL to an httpS URL for any non GET methods.
But we don't raise an error for any other redirects.
This causes two problems:
PUTrequests that are redirected get changed toGETrequests which don't perform the correct action but raise no error. This is because theGETresponse succeeds but since it wasn't aPUTit doesn't update. This can be seen by updating a user status. It raises no errors but the status does not change. See issue Changing milestone of issue does not work #1432POSTrequests blow up with hard to debug tracebacks. See below. Also see issue Not possible to create note in issue #1477
An example of attempting to upload an SSH key on a URL that redirects with 302 for the POST request:
Traceback (most recent call last):
File "/home/jlvillal/.local/lib/python3.9/site-packages/gitlab/base.py", line 77, in __getattr__
return self.__dict__["_updated_attrs"][name]
KeyError: '_managers'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/jlvillal/sources/local/py-gitlab-testing/./tt.py", line 109, in <module>
sys.exit(main())
File "/home/jlvillal/sources/local/py-gitlab-testing/./tt.py", line 82, in main
k = user.keys.create({"title": "My key", "key": SSH_KEY})
File "/home/jlvillal/.local/lib/python3.9/site-packages/gitlab/exceptions.py", line 287, in wrapped_f
return f(*args, **kwargs)
File "/home/jlvillal/.local/lib/python3.9/site-packages/gitlab/mixins.py", line 325, in create
return self._obj_cls(self, server_data)
File "/home/jlvillal/.local/lib/python3.9/site-packages/gitlab/base.py", line 62, in __init__
self._create_managers()
File "/home/jlvillal/.local/lib/python3.9/site-packages/gitlab/base.py", line 145, in _create_managers
managers = getattr(self, "_managers", None)
File "/home/jlvillal/.local/lib/python3.9/site-packages/gitlab/base.py", line 80, in __getattr__
value = self.__dict__["_attrs"][name]
TypeError: list indices must be integers or slices, not str
Metadata
Metadata
Assignees
Labels
No labels
Activity
max-wittig commentedon May 31, 2021
Maybe we should disallow any redirects then, so we fail early instead of confusing the user? So even though
GETmight work, we should maybe fail anyway when receiving 301 and explain that to the user?JohnVillalovos commentedon May 31, 2021
I'm okay doing that. Would take a little reworking of the patch. Seems reasonable.
@nejch Do you have an opinion on this?
nejch commentedon May 31, 2021
I'm fine with that too, it would definitely reduce the number of issues we get where people think it's a bug in the library.
Although then I'd maybe wait until 3.0.0 for this one too, because I suspect this might surprise/break things for a lot of people who didn't know they had a problem just because they were using it in small scripts for mostly read-only API stuff and it was all GET. We have a few PRs already that would make sense for a 3.0.0, I think I started a milestone just to track which ones are breaking. Does that make sense? (3.0.0 doesnt need to be that far, we can aim for this month if I clean up my old PR's 😁 )
JohnVillalovos commentedon May 31, 2021
That makes sense to me. We could do the current patch that allows the
GETand then for 3.0.0 we could change it to not allow aGETeither.nejch commentedon May 31, 2021
Lol also took another look at this in
requests, thanks browsers! 🤦 (it's the same in httpx)https://github.com/psf/requests/blob/c45a4dfe6bfc6017d4ea7e9f051d6cc30972b310/requests/sessions.py#L324-L332
JohnVillalovos commentedon May 31, 2021
Yeah. I was reading that mentioned bug report yesterday and was like 😲
But with that behavior it for sure breaks people for
PUT/POSTrequests. Silently or the list index error.Cynerd commentedon Jun 1, 2021
May I suggest at least adding note to documentation as soon as possible? It doesn't matter which solution is chosen but clearly stating that URL has to be without redirect is enough for now.
JohnVillalovos commentedon Jun 1, 2021
That's a great idea! Thanks.
nejch commentedon Jun 1, 2021
Also just based on a bit of testing I did, it would be impossible to get the base URL to suggest to the user right?
The target will be something in
/api/v4/projects/123/resourceso it won't be super clear but I don't think we can reliably get what the server URL should really be.JohnVillalovos commentedon Jun 1, 2021
In the proposed patch the final URL is shown. So can see that it went from
gitlab.labs.nic.cztogitlab.nic.cz(nolabs)Note: I manually word-wrapped it above so it would be easier to read.
Here is raw output from a test run:
nejch commentedon Jun 1, 2021
I meant more if we could hint to the user the base URL that they should configure in
Gitlab(), not the final request URL, just based on some of the issues I've seen here some people may still be confused 😁I guess you could so something like "we think you should use
target.split(f"/api/v{api_version}")[0]as the GitLab server URL" but now that I see it, it's a bit convoluted, never mind 😀