- Notifications
You must be signed in to change notification settings - Fork673
fix: handle tags like debian/2%2.6-21 as identifiers#1336
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
gitlab/utils.py Outdated
| @@ -55,15 +55,15 @@ def copy_dict(dest: Dict[str, Any], src: Dict[str, Any]) -> None: | |||
| dest[k] = v | |||
| def clean_str_id(id: str) -> str: | |||
| return id.replace("/", "%2F").replace("#", "%23") | |||
| def clean_str_id(id): | |||
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.
Can the type-hints not be removed?
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.
Uuuuh, absolutely yes, sorry. Fixing it now!
Overall looks good to me. Please put the type-hints back in. Also change "fix: Handle..." to "fix: handle" to fix the lint issue. Thanks. |
f213a90 to41dc7bfComparecodecov-io commentedMar 5, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #1336 +/- ##======================================= Coverage 80.21% 80.21% ======================================= Files 73 73 Lines 3801 3801 ======================================= Hits 3049 3049 Misses 752 752
Flags with carried forward coverage won't be shown.Click here to find out more.
Continue to review full report at Codecov.
|
Git refnames are relatively free-form and can contain all sort forspecial characters, not just `/` and `#`, seehttp://git-scm.com/docs/git-check-ref-formatIn particular, Debian's DEP-14 standard for storing packaging in gitrepositories mandates the use of the `%` character in tags in somecases like `debian/2%2.6-21`.Unfortunately python-gitlab currently only escapes `/` to `%2F` and insome cases `#` to `%23`. This means that when using the commit API toretrieve information about the `debian/2%2.6-21` tag only the slash isescaped before being inserted in the URL path and the `%` is leftuntouched, resulting in something like`/api/v4/projects/123/repository/commits/debian%2F2%2.6-21`. Whenurllib3 seees that it detects the invalid `%` escape and then urlencodesthe whole string, resulting in`/api/v4/projects/123/repository/commits/debian%252F2%252.6-21`, wherethe original `/` got escaped twice and produced `%252F`.To avoid the issue, fully urlencode identifiers and parameters to avoidthe urllib3 auto-escaping in all cases.Signed-off-by: Emanuele Aina <emanuele.aina@collabora.com>
41dc7bf tob4dac5cCompareThere 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.
Thanks @em-! I'm now questioning why there was any custom URL encoding in the first place, but I've opened a follow-up in#1356 :)
Uh oh!
There was an error while loading.Please reload this page.
Git refnames are relatively free-form and can contain all sort for
special characters, not just
/and#, seehttp://git-scm.com/docs/git-check-ref-format
In particular, Debian's DEP-14 standard for storing packaging in git
repositories mandates the use of the
%character in tags in somecases like
debian/2%2.6-21.Unfortunately python-gitlab currently only escapes
/to%2Fand insome cases
#to%23. This means that when using the commit API toretrieve information about the
debian/2%2.6-21tag only the slash isescaped before being inserted in the URL path and the
%is leftuntouched, resulting in something like
/api/v4/projects/123/repository/commits/debian%2F2%2.6-21. Whenurllib3 sees that, it detects the invalid
%escape and then urlencodesthe whole string, resulting in
/api/v4/projects/123/repository/commits/debian%252F2%252.6-21, wherethe original
/got escaped twice and produced%252F.To avoid the issue, fully urlencode identifiers and parameters to avoid
the urllib3 auto-escaping in all cases.
I've run the unit tests but not the integration ones, so in theory this does
not break anything but I can't really confirm it. All I can firmly say is that it
fixes the issue I hit. :D