diff --git a/binderhub/repoproviders.py b/binderhub/repoproviders.py index 56998c68c..bf8b7aefe 100644 --- a/binderhub/repoproviders.py +++ b/binderhub/repoproviders.py @@ -1054,11 +1054,13 @@ async def get_resolved_ref(self): if hasattr(self, "resolved_ref"): return self.resolved_ref + # Encode the ref (safe="" so its slashes can't inject extra path + # segments) to prevent traversal, see GHSA-q276-fxp7-xhx3. api_url = "{api_base_path}/repos/{user}/{repo}/commits/{ref}".format( api_base_path=self.api_base_path.format(hostname=self.hostname), user=self.user, repo=self.repo, - ref=self.unresolved_ref, + ref=urllib.parse.quote(self.unresolved_ref, safe=""), ) self.log.debug("Fetching %s", api_url) cached = self.cache.get(api_url) diff --git a/binderhub/tests/test_repoproviders.py b/binderhub/tests/test_repoproviders.py index a594a8302..297f88763 100644 --- a/binderhub/tests/test_repoproviders.py +++ b/binderhub/tests/test_repoproviders.py @@ -1,6 +1,6 @@ import re from unittest import TestCase -from urllib.parse import quote +from urllib.parse import quote, urlparse import pytest from tornado.ioloop import IOLoop @@ -417,6 +417,26 @@ def test_github_missing_ref(): assert ref is None +async def test_github_ref_is_url_encoded(monkeypatch): + """An attacker-controlled ref must not inject extra GitHub API path segments.""" + ref = "HEAD/../../../../owner/private/contents/README.md" + provider = GitHubRepoProvider(spec=f"user/repo/{ref}") + + captured = {} + + async def fake_request(api_url, etag=None): + captured["url"] = api_url + return None # short-circuit before any network call + + monkeypatch.setattr(provider, "github_api_request", fake_request) + await provider.get_resolved_ref() + + path = urlparse(captured["url"]).path + # the ref's slashes are encoded, so the ".." cannot climb out of /commits/ + assert path == f"/repos/user/repo/commits/{quote(ref, safe='')}" + assert "/../" not in path + + class TestSpecErrorHandling(TestCase): def test_too_short_spec(self): spec = "nothing_to_split"