Skip to content

Commit

Permalink
Ignore case when verifying GitLab/GitHub URLs (#16899)
Browse files Browse the repository at this point in the history
* Fixes #16666.

* Also consider pages on github.io .

* Fix test name

* Remove global variable for OIDCMixin.

* Update warehouse/oidc/models/gitlab.py

Simplify code.

Co-authored-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>

* Add missing test

* Update warehouse/oidc/models/gitlab.py

Co-authored-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>

* Fix linting issue

* Fix linting issues

---------

Co-authored-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
  • Loading branch information
3 people authored Nov 14, 2024
1 parent acef92b commit 77ec5bb
Show file tree
Hide file tree
Showing 6 changed files with 45 additions and 2 deletions.
16 changes: 16 additions & 0 deletions tests/unit/oidc/models/test_activestate.py
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,22 @@ def test_activestate_publisher_sub(
check(expected, actual, signed_claims)
assert str(e.value) == error_msg

@pytest.mark.parametrize(
("url", "expected"),
[
("https://platform.activestate.com/repository_name/project_name", True),
("https://platform.activestate.com/repository_name/PrOjECt_NaMe", False),
],
)
def test_activestate_publisher_verify_url(self, url, expected):
publisher = ActiveStatePublisher(
organization="repository_name",
activestate_project_name="project_name",
actor_id=ACTOR_ID,
actor=ACTOR,
)
assert publisher.verify_url(url) == expected


class TestPendingActiveStatePublisher:
def test_reify_does_not_exist_yet(self, db_request):
Expand Down
5 changes: 5 additions & 0 deletions tests/unit/oidc/models/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,11 @@ def test_supports_attestations(self):
"https://github.com",
False,
),
( # Default verification is case-sensitive
"https://publisher.com/owner/project",
"https://publisher.com/owner/PrOjeCt",
False,
),
],
)
def test_verify_url(self, monkeypatch, url, publisher_url, expected):
Expand Down
2 changes: 2 additions & 0 deletions tests/unit/oidc/models/test_github.py
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,8 @@ def test_github_publisher_duplicates_cant_be_created(self, db_request):
("https://repository_owner.github.io/repository_name/../malicious", False),
("https://repository_owner.github.io/", False),
("https://repository_owner.github.io/unrelated_name/", False),
("https://github.com/RePosItory_OwNeR/rePository_Name.git", True),
("https://repository_owner.github.io/RePoSiToRy_NaMe/subpage", True),
],
)
def test_github_publisher_verify_url(self, url, expected):
Expand Down
2 changes: 2 additions & 0 deletions tests/unit/oidc/models/test_gitlab.py
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,8 @@ def test_gitlab_publisher_duplicates_cant_be_created(self, db_request):
("https://gitlab.com/repository_owner/repository_name.git", True),
("https://gitlab.com/repository_owner/repository_name.git/", True),
("https://gitlab.com/repository_owner/repository_name.git/issues", False),
("https://gitlab.com/repository_OwNeR/RePoSiToRy_Name.git/", True),
("https://gitlab.com/another_owner/repository_name/issues", False),
],
)
def test_gitlab_publisher_verify_url(self, url, expected):
Expand Down
15 changes: 13 additions & 2 deletions warehouse/oidc/models/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -361,13 +361,24 @@ def verify_url(self, url: str):
The suffix `.git` in repo URLs is ignored, since `github.com/org/repo.git`
always redirects to `github.com/org/repo`. This does not apply to subpaths,
like `github.com/org/repo.git/issues`, which do not redirect to the correct URL.
GitHub uses case-insensitive owner/repo slugs - so we perform a case-insensitive
comparison.
"""
url_for_generic_check = url.removesuffix("/").removesuffix(".git")
docs_url = (
f"https://{self.repository_owner}.github.io/{self.repository_name}".lower()
)

normalized_url_prefixes = (self.publisher_base_url.lower(), docs_url)
for prefix in normalized_url_prefixes:
if url.lower().startswith(prefix):
url = prefix + url[len(prefix) :]
break

url_for_generic_check = url.removesuffix("/").removesuffix(".git")
if super().verify_url(url_for_generic_check):
return True

docs_url = f"https://{self.repository_owner}.github.io/{self.repository_name}"
docs_uri = rfc3986.api.uri_reference(docs_url).normalize()
user_uri = rfc3986.api.uri_reference(url).normalize()

Expand Down
7 changes: 7 additions & 0 deletions warehouse/oidc/models/gitlab.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,14 @@ def verify_url(self, url: str):
in repo URLs, since `gitlab.com/org/repo.git` always redirects to
`gitlab.com/org/repo`. This does not apply to subpaths like
`gitlab.com/org/repo.git/issues`, which do not redirect to the correct URL.
GitLab uses case-insensitive owner/repo slugs - so we perform a case-insensitive
comparison.
"""
lowercase_base_url = self.publisher_base_url.lower()
if url.lower().startswith(lowercase_base_url):
url = lowercase_base_url + url[len(lowercase_base_url) :]

url_for_generic_check = url.removesuffix("/").removesuffix(".git")
return super().verify_url(url_for_generic_check)

Expand Down

0 comments on commit 77ec5bb

Please sign in to comment.