Skip to content

Commit f16525f

Browse files
leliaclaude
andcommitted
fix(ci): validate CI-supplied server URLs before building a link
GITHUB_SERVER_URL and CI_SERVER_URL were composed into the pull request link verbatim, while the sibling repository URLs read from the same environment already went through a scheme/netloc check. The result is sent to the API as a diff scan's external_href, so route all of them through one validator. Standard runners set these themselves, so this is defense in depth rather than a live hole. An unusable value now falls back to github.com for GitHub; GitLab has no public default host, so the link is dropped and the scan keeps its number. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 977a8fb commit f16525f

2 files changed

Lines changed: 69 additions & 6 deletions

File tree

socketsecurity/core/pull_request.py

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,14 +27,28 @@ def parse_pull_request_number(value) -> int:
2727
return parsed if parsed > 0 else 0
2828

2929

30+
def _http_url(value: Optional[str]) -> Optional[str]:
31+
"""Return ``value`` if it is an http(s) URL with a host, else ``None``.
32+
33+
Every URL fragment read out of the CI environment goes through here before it
34+
is composed into a link, because the result is sent to the API as a diff scan's
35+
``external_href``. Standard runners set these variables themselves, so this is
36+
defense in depth rather than a live hole.
37+
"""
38+
if not value:
39+
return None
40+
url = value.strip().rstrip("/")
41+
parsed = urlparse(url)
42+
return url if parsed.scheme in ("http", "https") and parsed.netloc else None
43+
44+
3045
def _repository_url(value: Optional[str]) -> Optional[str]:
3146
if not value:
3247
return None
3348
url = value.strip().rstrip("/")
3449
if url.endswith(".git"):
3550
url = url[:-4]
36-
parsed = urlparse(url)
37-
return url if parsed.scheme in ("http", "https") and parsed.netloc else None
51+
return _http_url(url)
3852

3953

4054
def _github_number(env: Mapping[str, str]) -> int:
@@ -52,8 +66,11 @@ def _github_url(number: int, repo: Optional[str], env: Mapping[str, str]) -> Opt
5266
repository = env.get("GITHUB_REPOSITORY") or remote_path or repo
5367
if not repository or "/" not in repository:
5468
return None
55-
server = env.get("GITHUB_SERVER_URL") or (f"https://{remote_host}" if remote_host else "")
56-
server = (server or "https://github.com").rstrip("/")
69+
server = (
70+
_http_url(env.get("GITHUB_SERVER_URL"))
71+
or (_http_url(f"https://{remote_host}") if remote_host else None)
72+
or "https://github.com"
73+
)
5774
return f"{server}/{repository.strip('/')}/pull/{number}"
5875

5976

@@ -62,8 +79,10 @@ def _gitlab_url(number: int, repo: Optional[str], env: Mapping[str, str]) -> Opt
6279
if not project_url:
6380
remote_host, remote_path = parse_git_remote(env.get("BUILDKITE_REPO"))
6481
project_path = env.get("CI_PROJECT_PATH") or remote_path or repo
65-
server = env.get("CI_SERVER_URL") or (f"https://{remote_host}" if remote_host else "")
66-
server = server.rstrip("/")
82+
server = (
83+
_http_url(env.get("CI_SERVER_URL"))
84+
or (_http_url(f"https://{remote_host}") if remote_host else None)
85+
)
6786
if server and project_path and "/" in project_path:
6887
project_url = f"{server}/{project_path.strip('/')}"
6988
return f"{project_url}/-/merge_requests/{number}" if project_url else None

tests/unit/test_pull_request_context.py

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,50 @@ def test_buildkite_github_enterprise_host_is_taken_from_the_remote():
196196
assert context.url == "https://github.example.com/acme/widgets/pull/42"
197197

198198

199+
@pytest.mark.parametrize(
200+
"server",
201+
["javascript:alert(1)", "notaurl", "ftp://example.com", "https://", ""],
202+
)
203+
def test_unusable_github_server_url_falls_back_to_the_default(server):
204+
"""The result becomes a diff scan's external_href, so validate before composing."""
205+
context = resolve_pull_request_context(
206+
"github",
207+
"42",
208+
"acme/widgets",
209+
configured_explicit=True,
210+
env={"GITHUB_SERVER_URL": server, "GITHUB_REPOSITORY": "acme/widgets"},
211+
)
212+
213+
assert context.url == "https://github.com/acme/widgets/pull/42"
214+
215+
216+
@pytest.mark.parametrize("server", ["javascript:alert(1)", "notaurl", "ftp://example.com"])
217+
def test_unusable_gitlab_server_url_yields_no_link(server):
218+
"""GitLab has no public default host to fall back to, so the link is dropped."""
219+
context = resolve_pull_request_context(
220+
"gitlab",
221+
"42",
222+
"acme/widgets",
223+
configured_explicit=True,
224+
env={"CI_SERVER_URL": server, "CI_PROJECT_PATH": "acme/widgets"},
225+
)
226+
227+
assert context.number == 42
228+
assert context.url is None
229+
230+
231+
def test_self_hosted_server_urls_are_still_honored():
232+
assert resolve_pull_request_context(
233+
"github", "42", None, configured_explicit=True,
234+
env={"GITHUB_SERVER_URL": "https://github.example.com", "GITHUB_REPOSITORY": "acme/widgets"},
235+
).url == "https://github.example.com/acme/widgets/pull/42"
236+
237+
assert resolve_pull_request_context(
238+
"gitlab", "42", None, configured_explicit=True,
239+
env={"CI_SERVER_URL": "http://gitlab.internal", "CI_PROJECT_PATH": "acme/platform/widgets"},
240+
).url == "http://gitlab.internal/acme/platform/widgets/-/merge_requests/42"
241+
242+
199243
def test_github_actions_environment_wins_over_the_checkout_remote():
200244
context = resolve_pull_request_context(
201245
"github",

0 commit comments

Comments
 (0)