Repository navigation
RFE: We should catch all 301, 302 redirects #1485
Description
Activity
Maybe we should disallow any redirects then, so we fail early instead of confusing the user? So even though GET might work, we should maybe fail anyway when receiving 301 and explain that to the user?
Maybe we should disallow any redirects then, so we fail early instead of confusing the user? So even though GET might work, we should maybe fail anyway when receiving 301 and explain that to the user?
I'm okay doing that. Would take a little reworking of the patch. Seems reasonable.
@nejch Do you have an opinion on this?
Maybe we should disallow any redirects then, so we fail early instead of confusing the user? So even though GET might work, we should maybe fail anyway when receiving 301 and explain that to the user?
I'm okay doing that. Would take a little reworking of the patch. Seems reasonable.
@nejch Do you have an opinion on this?
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 😁 )
That makes sense to me. We could do the current patch that allows the GET and then for 3.0.0 we could change it to not allow a GET either.
Lol also took another look at this in requests, thanks browsers! 🤦 (it's the same in httpx)
Yeah. I was reading that mentioned bug report yesterday and was like 😲
But with that behavior it for sure breaks people for PUT/POST requests. Silently or the list index error.
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.
Reacted by Nejc HabjanMay 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.
That's a great idea! Thanks.
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/resource so it won't be super clear but I don't think we can reliably get what the server URL should really be.
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?
In the proposed patch the final URL is shown. So can see that it went from gitlab.labs.nic.cz to gitlab.nic.cz (no labs)
python-gitlab detected a 302 ('Moved Temporarily') redirection. You must update your GitLab URL to the correct URL to avoid issues. The redirection was from: 'https://gitlab.labs.nic.cz/api/v4/user/status' to 'https://gitlab.nic.cz/api/v4/user/status'Note: I manually word-wrapped it above so it would be easier to read.
Here is raw output from a test run:
gitlab.exceptions.RedirectError: python-gitlab detected a 302 ('Moved Temporarily') redirection. You must update your GitLab URL to the correct URL to avoid issues. The redirection was from: 'https://gitlab.labs.nic.cz/api/v4/user/status' to 'https://gitlab.nic.cz/api/v4/user/status'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 😀
- locked as resolved and limited conversation to collaborators on Sep 12, 2022
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:
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