Fix url encoding of strings with slashes in URLs (#3263)

Fixes #3262.

This changes the URL encoding function used for strings in URLs.
Currently, this uses `urllib.parse.quote`, which makes an exclusion of
URL-encoding the `/` character:
> urllib.parse.quote(string, safe='/', encoding=None, errors=None)
Replace special characters in string using the %xx escape. Letters,
digits, and the characters '_.-~' are never quoted. By default, this
function is intended for quoting the path section of a URL. The optional
safe parameter specifies additional ASCII characters that should not be
quoted — its default value is '/'.

The GitHub APIs expect that `/` _should_ be URL encoded. For example, if
my environment name contains `/` I get a 404 from the GitHub API.

I have modified the call to remove this exclusion for the `/` character,
which causes the environment name to be correctly encoded. I have
updated the test cases to provide coverage for this change.

---------

Co-authored-by: Enrico Minack <[email protected]>

Author: OscarVanL

github/Repository.py

@@ -1268,7 +1268,7 @@ def add_to_collaborators(self, collaborator: str | NamedUser, permission: Opt[st
         if isinstance(collaborator, github.NamedUser.NamedUser):
             collaborator = collaborator._identity
         else:
-            collaborator = urllib.parse.quote(collaborator)
+            collaborator = urllib.parse.quote(collaborator, safe="")
 
         if is_defined(permission):
             put_parameters = {"permission": permission}
@@ -1294,7 +1294,7 @@ def get_collaborator_permission(self, collaborator: str | NamedUser) -> str:
         if isinstance(collaborator, github.NamedUser.NamedUser):
             collaborator = collaborator._identity
         else:
-            collaborator = urllib.parse.quote(collaborator)
+            collaborator = urllib.parse.quote(collaborator, safe="")
         headers, data = self._requester.requestJsonAndCheck(
             "GET",
             f"{self.url}/collaborators/{collaborator}/permission",
@@ -1949,7 +1949,7 @@ def create_secret(
         assert isinstance(unencrypted_value, str), unencrypted_value
         assert secret_type in ["actions", "dependabot"], "secret_type should be actions or dependabot"
 
-        secret_name = urllib.parse.quote(secret_name)
+        secret_name = urllib.parse.quote(secret_name, safe="")
         public_key = self.get_public_key(secret_type=secret_type)
         payload = public_key.encrypt(unencrypted_value)
         put_parameters = {
@@ -1997,7 +1997,7 @@ def get_secret(self, secret_name: str, secret_type: str = "actions") -> github.S
         assert isinstance(secret_name, str), secret_name
         assert secret_type in ["actions", "dependabot"], "secret_type should be actions or dependabot"
 
-        secret_name = urllib.parse.quote(secret_name)
+        secret_name = urllib.parse.quote(secret_name, safe="")
         return github.Secret.Secret(
             requester=self._requester,
             headers={},
@@ -2047,7 +2047,7 @@ def get_variable(self, variable_name: str) -> github.Variable.Variable:
         :rtype: github.Variable.Variable
         """
         assert isinstance(variable_name, str), variable_name
-        variable_name = urllib.parse.quote(variable_name)
+        variable_name = urllib.parse.quote(variable_name, safe="")
         return github.Variable.Variable(
             requester=self._requester,
             headers={},
@@ -2064,7 +2064,7 @@ def delete_secret(self, secret_name: str, secret_type: str = "actions") -> bool:
         """
         assert isinstance(secret_name, str), secret_name
         assert secret_type in ["actions", "dependabot"], "secret_type should be actions or dependabot"
-        secret_name = urllib.parse.quote(secret_name)
+        secret_name = urllib.parse.quote(secret_name, safe="")
         status, headers, data = self._requester.requestJson("DELETE", f"{self.url}/{secret_type}/secrets/{secret_name}")
         return status == 204
 
@@ -2075,7 +2075,7 @@ def delete_variable(self, variable_name: str) -> bool:
         :rtype: bool
         """
         assert isinstance(variable_name, str), variable_name
-        variable_name = urllib.parse.quote(variable_name)
+        variable_name = urllib.parse.quote(variable_name, safe="")
         status, headers, data = self._requester.requestJson("DELETE", f"{self.url}/actions/variables/{variable_name}")
         return status == 204
 
@@ -2226,11 +2226,11 @@ def get_archive_link(self, archive_format: str, ref: Opt[str] = NotSet) -> str:
         :rtype: string
         """
         assert isinstance(archive_format, str), archive_format
-        archive_format = urllib.parse.quote(archive_format)
+        archive_format = urllib.parse.quote(archive_format, safe="")
         assert is_optional(ref, str), ref
         url = f"{self.url}/{archive_format}"
         if is_defined(ref):
-            ref = urllib.parse.quote(ref)
+            ref = urllib.parse.quote(ref, safe="")
             url += f"/{ref}"
         headers, data = self._requester.requestJsonAndCheck("GET", url)
         return headers["location"]
@@ -3010,7 +3010,7 @@ def get_git_matching_refs(self, ref: str) -> PaginatedList[GitRef]:
         :rtype: :class:`PaginatedList` of :class:`github.GitRef.GitRef`
         """
         assert isinstance(ref, str), ref
-        ref = urllib.parse.quote(ref)
+        ref = urllib.parse.quote(ref, safe="")
         return PaginatedList(
             github.GitRef.GitRef,
             self._requester,
@@ -3025,7 +3025,7 @@ def get_git_tag(self, sha: str) -> GitTag:
         :rtype: :class:`github.GitTag.GitTag`
         """
         assert isinstance(sha, str), sha
-        sha = urllib.parse.quote(sha)
+        sha = urllib.parse.quote(sha, safe="")
         headers, data = self._requester.requestJsonAndCheck("GET", f"{self.url}/git/tags/{sha}")
         return github.GitTag.GitTag(self._requester, headers, data, completed=True)
 
@@ -3256,7 +3256,9 @@ def get_label(self, name: str) -> Label:
         :rtype: :class:`github.Label.Label`
         """
         assert isinstance(name, str), name
-        headers, data = self._requester.requestJsonAndCheck("GET", f"{self.url}/labels/{urllib.parse.quote(name)}")
+        headers, data = self._requester.requestJsonAndCheck(
+            "GET", f"{self.url}/labels/{urllib.parse.quote(name, safe='')}"
+        )
         return github.Label.Label(self._requester, headers, data, completed=True)
 
     def get_labels(self) -> PaginatedList[Label]:
@@ -3607,7 +3609,7 @@ def get_release(self, id: int | str) -> GitRelease:
             headers, data = self._requester.requestJsonAndCheck("GET", f"{self.url}/releases/{id}")
             return github.GitRelease.GitRelease(self._requester, headers, data, completed=True)
         elif isinstance(id, str):
-            id = urllib.parse.quote(id)
+            id = urllib.parse.quote(id, safe="")
             headers, data = self._requester.requestJsonAndCheck("GET", f"{self.url}/releases/tags/{id}")
             return github.GitRelease.GitRelease(self._requester, headers, data, completed=True)
 
@@ -3665,7 +3667,7 @@ def get_workflow(self, id_or_file_name: str | int) -> Workflow:
         :rtype: :class:`github.Workflow.Workflow`
         """
         assert isinstance(id_or_file_name, (int, str)), id_or_file_name
-        id_or_file_name = urllib.parse.quote(str(id_or_file_name))
+        id_or_file_name = urllib.parse.quote(str(id_or_file_name), safe="")
         headers, data = self._requester.requestJsonAndCheck("GET", f"{self.url}/actions/workflows/{id_or_file_name}")
         return github.Workflow.Workflow(self._requester, headers, data, completed=True)
 
@@ -3755,7 +3757,7 @@ def has_in_assignees(self, assignee: str | NamedUser) -> bool:
         if isinstance(assignee, github.NamedUser.NamedUser):
             assignee = assignee._identity
         else:
-            assignee = urllib.parse.quote(assignee)
+            assignee = urllib.parse.quote(assignee, safe="")
 
         status, headers, data = self._requester.requestJson("GET", f"{self.url}/assignees/{assignee}")
         return status == 204
@@ -3771,7 +3773,7 @@ def has_in_collaborators(self, collaborator: str | NamedUser) -> bool:
         if isinstance(collaborator, github.NamedUser.NamedUser):
             collaborator = collaborator._identity
         else:
-            collaborator = urllib.parse.quote(collaborator)
+            collaborator = urllib.parse.quote(collaborator, safe="")
 
         status, headers, data = self._requester.requestJson("GET", f"{self.url}/collaborators/{collaborator}")
         return status == 204
@@ -3803,7 +3805,7 @@ def legacy_search_issues(self, state: str, keyword: str) -> list[Issue]:
         assert isinstance(keyword, str), keyword
         headers, data = self._requester.requestJsonAndCheck(
             "GET",
-            f"/legacy/issues/search/{self.owner.login}/{self.name}/{state}/{urllib.parse.quote(keyword)}",
+            f"/legacy/issues/search/{self.owner.login}/{self.name}/{state}/{urllib.parse.quote(keyword, safe='')}",
         )
         return [
             github.Issue.Issue(
@@ -3979,7 +3981,7 @@ def remove_from_collaborators(self, collaborator: str | NamedUser) -> None:
         if isinstance(collaborator, github.NamedUser.NamedUser):
             collaborator = collaborator._identity
         else:
-            collaborator = urllib.parse.quote(collaborator)
+            collaborator = urllib.parse.quote(collaborator, safe="")
 
         headers, data = self._requester.requestJsonAndCheck("DELETE", f"{self.url}/collaborators/{collaborator}")
 
@@ -4083,7 +4085,7 @@ def _hub(self, mode: str, event: str, callback: str, secret: Opt[str]) -> None:
         assert isinstance(event, str), event
         assert isinstance(callback, str), callback
         assert is_optional(secret, str), secret
-        event = urllib.parse.quote(event)
+        event = urllib.parse.quote(event, safe="")
 
         post_parameters = collections.OrderedDict()
         post_parameters["hub.callback"] = callback
@@ -4235,7 +4237,7 @@ def get_environment(self, environment_name: str) -> Environment:
         :rtype: :class:`github.Environment.Environment`
         """
         assert isinstance(environment_name, str), environment_name
-        environment_name = urllib.parse.quote(environment_name)
+        environment_name = urllib.parse.quote(environment_name, safe="")
         headers, data = self._requester.requestJsonAndCheck("GET", f"{self.url}/environments/{environment_name}")
         data["environments_url"] = f"/repositories/{self.id}/environments"
         return Environment(self._requester, headers, data, completed=True)
@@ -4271,7 +4273,7 @@ def create_environment(
             )
             or deployment_branch_policy is None
         )
-        environment_name = urllib.parse.quote(environment_name)
+        environment_name = urllib.parse.quote(environment_name, safe="")
 
         put_parameters = {
             "wait_timer": wait_timer,
@@ -4293,7 +4295,7 @@ def delete_environment(self, environment_name: str) -> None:
         :rtype: None
         """
         assert isinstance(environment_name, str), environment_name
-        environment_name = urllib.parse.quote(environment_name)
+        environment_name = urllib.parse.quote(environment_name, safe="")
 
         headers, data = self._requester.requestJsonAndCheck("DELETE", f"{self.url}/environments/{environment_name}")
 

github/Team.py

@@ -283,7 +283,7 @@ def get_team_membership(self, member: str | NamedUser) -> Membership:
         if isinstance(member, github.NamedUser.NamedUser):
             member = member._identity
         else:
-            member = urllib.parse.quote(member)
+            member = urllib.parse.quote(member, safe="")
         headers, data = self._requester.requestJsonAndCheck("GET", f"{self.url}/memberships/{member}")
         return github.Membership.Membership(self._requester, headers, data, completed=True)
 
@@ -302,7 +302,7 @@ def get_repo_permission(self, repo: Repository) -> Permissions | None:
         if isinstance(repo, github.Repository.Repository):
             repo = repo._identity  # type: ignore
         else:
-            repo = urllib.parse.quote(repo)
+            repo = urllib.parse.quote(repo, safe="")
         try:
             headers, data = self._requester.requestJsonAndCheck(
                 "GET",
@@ -343,7 +343,7 @@ def update_team_repository(self, repo: Repository, permission: str) -> bool:
         if isinstance(repo, github.Repository.Repository):
             repo_url_param = repo._identity
         else:
-            repo_url_param = urllib.parse.quote(repo)
+            repo_url_param = urllib.parse.quote(repo, safe="")
         put_parameters = {
             "permission": permission,
         }

tests/Environment.py

@@ -114,17 +114,17 @@ def testGetEnvironments(self):
         self.assertEqual(environments[0].name, "dev")
 
     def testCreateEnvironment(self):
-        environment = self.repo.create_environment("test")
-        self.assertEqual(environment.name, "test")
+        environment = self.repo.create_environment("test/env")
+        self.assertEqual(environment.name, "test/env")
         self.assertEqual(environment.id, 470015651)
         self.assertEqual(environment.node_id, "EN_kwDOHKhL9c4cA96j")
         self.assertEqual(
             environment.url,
-            "https://api.github.com/repos/alson/PyGithub/environments/test",
+            "https://api.github.com/repos/alson/PyGithub/environments/test/env",
         )
         self.assertEqual(
             environment.html_url,
-            "https://github.com/alson/PyGithub/deployments/activity_log?environments_filter=test",
+            "https://github.com/alson/PyGithub/deployments/activity_log?environments_filter=test%2Fenv",
         )
         self.assertEqual(
             environment.created_at,
@@ -139,7 +139,7 @@ def testCreateEnvironment(self):
 
     def testUpdateEnvironment(self):
         environment = self.repo.create_environment(
-            "test",
+            "test/env",
             wait_timer=42,
             reviewers=[github.EnvironmentProtectionRuleReviewer.ReviewerParams(type_="User", id_=19245)],
             prevent_self_review=True,
@@ -152,11 +152,11 @@ def testUpdateEnvironment(self):
         self.assertEqual(environment.node_id, "EN_kwDOHKhL9c4cA96j")
         self.assertEqual(
             environment.url,
-            "https://api.github.com/repos/alson/PyGithub/environments/test",
+            "https://api.github.com/repos/alson/PyGithub/environments/test/env",
         )
         self.assertEqual(
             environment.html_url,
-            "https://github.com/alson/PyGithub/deployments/activity_log?environments_filter=test",
+            "https://github.com/alson/PyGithub/deployments/activity_log?environments_filter=test%2Fenv",
         )
         self.assertEqual(
             environment.created_at,

tests/ReplayData/Environment.testCreateEnvironment.txt

@@ -2,9 +2,9 @@ https
 PUT
 api.github.com
 None
-/repos/alson/PyGithub/environments/test
+/repos/alson/PyGithub/environments/test%2Fenv
 {'Authorization': 'token private_token_removed', 'User-Agent': 'PyGithub/Python', 'Content-Type': 'application/json'}
 {"wait_timer": 0, "reviewers": [], "deployment_branch_policy": null, "prevent_self_review": false}
 200
 [('Server', 'GitHub.com'), ('Date', 'Tue, 19 Apr 2022 14:04:32 GMT'), ('Content-Type', 'application/json; charset=utf-8'), ('Transfer-Encoding', 'chunked'), ('Cache-Control', 'private, max-age=60, s-maxage=60'), ('Vary', 'Accept, Authorization, Cookie, X-GitHub-OTP, Accept-Encoding, Accept, X-Requested-With'), ('ETag', 'W/"551b8efc5f64e50c7aea535a16d55561640ea5649a4e1d349ed09f953cae96da"'), ('X-OAuth-Scopes', 'repo'), ('X-Accepted-OAuth-Scopes', ''), ('github-authentication-token-expiration', '2022-05-13 15:01:13 UTC'), ('X-GitHub-Media-Type', 'github.v3; format=json'), ('X-RateLimit-Limit', '5000'), ('X-RateLimit-Remaining', '4993'), ('X-RateLimit-Reset', '1650380634'), ('X-RateLimit-Used', '7'), ('X-RateLimit-Resource', 'core'), ('Access-Control-Expose-Headers', 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset'), ('Access-Control-Allow-Origin', '*'), ('Strict-Transport-Security', 'max-age=31536000; includeSubdomains; preload'), ('X-Frame-Options', 'deny'), ('X-Content-Type-Options', 'nosniff'), ('X-XSS-Protection', '0'), ('Referrer-Policy', 'origin-when-cross-origin, strict-origin-when-cross-origin'), ('Content-Security-Policy', "default-src 'none'"), ('Content-Encoding', 'gzip'), ('X-GitHub-Request-Id', 'C759:D172:956268:97BB8A:625EC170')]
-{"id":470015651,"node_id":"EN_kwDOHKhL9c4cA96j","name":"test","url":"https://api.github.com/repos/alson/PyGithub/environments/test","html_url":"https://github.com/alson/PyGithub/deployments/activity_log?environments_filter=test","created_at":"2022-04-19T14:04:32Z","updated_at":"2022-04-19T14:04:32Z","protection_rules":[],"deployment_branch_policy":null}
+{"id":470015651,"node_id":"EN_kwDOHKhL9c4cA96j","name":"test/env","url":"https://api.github.com/repos/alson/PyGithub/environments/test/env","html_url":"https://github.com/alson/PyGithub/deployments/activity_log?environments_filter=test%2Fenv","created_at":"2022-04-19T14:04:32Z","updated_at":"2022-04-19T14:04:32Z","can_admins_bypass":true,"protection_rules":[],"deployment_branch_policy":null}