Skip to content

Commit 3742c39

Browse files
leliaclaude
andcommitted
Determine SCM type from config instead of sniffing the Socket report URL
CodeQL flagged `"github.com" in diff_url` as incomplete URL substring sanitization. Looking at what diff_url actually holds makes the finding more interesting than a sanitization gap. diff_url is always a Socket dashboard link, built in Core as `https://socket.dev/dashboard/org/{org_slug}/diff/...` (or the equivalent sbom URL). Its host is always socket.dev and it carries no SCM information -- which is exactly what the comment three lines below the check already said. The only variable part is the org slug, so the sniff could only ever fire when a Socket org slug happened to contain "github", "gitlab" or "bitbucket". Such an org got a link to a repository host it may not use; everyone else fell through to the Socket file view. The branch was also almost unreachable: CliConfig declares `scm` with a default of "api", so `hasattr(config, "scm")` is true for every real config and the elif never runs. It was observable only for a config object carrying `repo` but no `scm`, since the URL builders all require a truthy config -- with `config=None` the sniffed value was computed and then discarded. Replaced with `getattr(config, "scm", None) or "api"`, which handles a missing config, a config without the attribute, and an empty value. Adds tests for get_manifest_file_url, which had none: GitHub, GitHub Enterprise, GitLab, self-hosted GitLab, Bitbucket, the Socket fallback, build-agent prefix stripping, and multi-manifest paths. The three org-slug cases are regression guards, confirmed to fail against the old implementation. Removing the dead branch drops the function under the complexity limit, so RUF100 required its `# noqa: C901` be removed. The backlog is now 19. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent a3acbc8 commit 3742c39

2 files changed

Lines changed: 138 additions & 13 deletions

File tree

socketsecurity/core/messages.py

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ def map_severity_to_sarif(severity: str) -> str:
3434
return severity_mapping.get(severity.lower(), "note")
3535

3636
@staticmethod
37-
def get_manifest_file_url(diff: Diff, manifest_path: str, config=None) -> str: # noqa: C901
37+
def get_manifest_file_url(diff: Diff, manifest_path: str, config=None) -> str:
3838
"""
3939
Generate proper URL for manifest file based on the repository type and diff URL.
4040
@@ -71,18 +71,16 @@ def get_manifest_file_url(diff: Diff, manifest_path: str, config=None) -> str:
7171
# Remove leading slashes
7272
clean_path = clean_path.lstrip("/")
7373

74-
# Determine SCM type from config or diff_url
75-
scm_type = "api" # Default to API
76-
if config and hasattr(config, "scm"):
77-
scm_type = config.scm.lower()
78-
elif hasattr(diff, "diff_url") and diff.diff_url:
79-
diff_url = diff.diff_url.lower()
80-
if "github.com" in diff_url or "github" in diff_url:
81-
scm_type = "github"
82-
elif "gitlab" in diff_url:
83-
scm_type = "gitlab"
84-
elif "bitbucket" in diff_url:
85-
scm_type = "bitbucket"
74+
# Determine SCM type from config.
75+
#
76+
# diff.diff_url is deliberately not consulted. It is always a Socket
77+
# dashboard link -- https://socket.dev/dashboard/org/<org>/diff/... , as
78+
# the note below says -- so it carries no SCM information at all. The
79+
# substring sniff that used to live here ("github" in diff_url) could
80+
# therefore only fire when the Socket *org slug* happened to contain
81+
# "github", "gitlab" or "bitbucket", mislabelling the SCM for those orgs
82+
# and doing nothing for everyone else.
83+
scm_type = (getattr(config, "scm", None) or "api").lower()
8684

8785
# Generate URL based on SCM type using config information
8886
# NEVER use diff.diff_url for SCM URLs - those are Socket URLs for "View report" links
Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
"""Tests for Messages.get_manifest_file_url.
2+
3+
The SCM type comes from config alone. It used to fall back to sniffing
4+
`diff.diff_url` for the substrings "github" / "gitlab" / "bitbucket", which
5+
CodeQL flagged as incomplete URL sanitization. The deeper problem was that
6+
diff_url is always a Socket dashboard link, so the only variable part it
7+
contains is the org slug -- meaning the sniff mislabelled the SCM for any org
8+
whose slug happened to contain one of those words, and did nothing otherwise.
9+
"""
10+
11+
from dataclasses import dataclass
12+
13+
import pytest
14+
15+
from socketsecurity.core.classes import Diff
16+
from socketsecurity.core.messages import Messages
17+
18+
SOCKET_REPORT = "https://socket.dev/dashboard/org/acme/sbom/abc123"
19+
20+
21+
@dataclass
22+
class _Config:
23+
scm: str = "api"
24+
repo: str = "acme/widgets"
25+
branch: str = "main"
26+
27+
28+
def _diff(diff_url: str = "https://socket.dev/dashboard/org/acme/diff/h1/abc123") -> Diff:
29+
return Diff(id="abc123", diff_url=diff_url, report_url=SOCKET_REPORT)
30+
31+
32+
def test_github_url_is_built_from_config_not_diff_url():
33+
url = Messages.get_manifest_file_url(_diff(), "package.json", _Config(scm="github"))
34+
assert url == "https://github.com/acme/widgets/blob/main/package.json"
35+
36+
37+
def test_github_enterprise_honours_server_env(monkeypatch):
38+
monkeypatch.setenv("GITHUB_SERVER_URL", "https://github.mycorp.com")
39+
url = Messages.get_manifest_file_url(_diff(), "package.json", _Config(scm="github"))
40+
assert url == "https://github.mycorp.com/acme/widgets/blob/main/package.json"
41+
42+
43+
def test_gitlab_uses_blob_path_and_server_env(monkeypatch):
44+
monkeypatch.setenv("CI_SERVER_URL", "https://gitlab.mycorp.com")
45+
url = Messages.get_manifest_file_url(_diff(), "go.mod", _Config(scm="gitlab", branch="dev"))
46+
assert url == "https://gitlab.mycorp.com/acme/widgets/-/blob/dev/go.mod"
47+
48+
49+
def test_bitbucket_uses_src_path():
50+
url = Messages.get_manifest_file_url(_diff(), "pom.xml", _Config(scm="bitbucket"))
51+
assert url == "https://bitbucket.org/acme/widgets/src/main/pom.xml"
52+
53+
54+
def test_scm_is_case_insensitive():
55+
url = Messages.get_manifest_file_url(_diff(), "package.json", _Config(scm="GitHub"))
56+
assert url.startswith("https://github.com/")
57+
58+
59+
@pytest.mark.parametrize("scm", ["api", "", "unknown"])
60+
def test_non_scm_types_fall_back_to_the_socket_file_view(scm):
61+
url = Messages.get_manifest_file_url(_diff(), "src/package.json", _Config(scm=scm))
62+
assert url == f"{SOCKET_REPORT}?tab=files&file=src%2Fpackage.json"
63+
64+
65+
def test_missing_config_falls_back_to_the_socket_file_view():
66+
url = Messages.get_manifest_file_url(_diff(), "package.json", None)
67+
assert url == f"{SOCKET_REPORT}?tab=files&file=package.json"
68+
69+
70+
class _ConfigWithoutScm:
71+
"""A config that carries repo/branch but no `scm` attribute.
72+
73+
This is the shape that made the old diff_url sniff observable: it is truthy
74+
and has `repo`, so a sniffed scm_type actually reached the URL builders.
75+
With `config=None` the sniffed value was computed and then discarded,
76+
because every branch also required a truthy config.
77+
"""
78+
79+
repo = "acme/widgets"
80+
branch = "main"
81+
82+
83+
def test_config_without_an_scm_attribute_falls_back_to_socket():
84+
url = Messages.get_manifest_file_url(_diff(), "package.json", _ConfigWithoutScm())
85+
assert url == f"{SOCKET_REPORT}?tab=files&file=package.json"
86+
87+
88+
@pytest.mark.parametrize(
89+
"diff_url",
90+
[
91+
"https://socket.dev/dashboard/org/github-tools/diff/h1/abc123",
92+
"https://socket.dev/dashboard/org/our-gitlab-org/diff/h1/abc123",
93+
"https://socket.dev/dashboard/org/bitbucket-team/diff/h1/abc123",
94+
],
95+
)
96+
def test_org_slug_containing_an_scm_name_does_not_change_the_url(diff_url):
97+
"""Regression guard: diff_url must not influence SCM detection.
98+
99+
Each of these Socket org slugs embeds an SCM name. The old substring sniff
100+
read that as the repository's SCM and emitted a GitHub/GitLab/Bitbucket
101+
link for orgs that may use none of them. Uses a config without `scm` so the
102+
sniffed value would actually be reached.
103+
"""
104+
url = Messages.get_manifest_file_url(_diff(diff_url), "package.json", _ConfigWithoutScm())
105+
assert url == f"{SOCKET_REPORT}?tab=files&file=package.json"
106+
107+
108+
def test_first_manifest_is_used_when_several_are_joined():
109+
url = Messages.get_manifest_file_url(_diff(), "a/package.json;b/yarn.lock", _Config(scm="github"))
110+
assert url == "https://github.com/acme/widgets/blob/main/a/package.json"
111+
112+
113+
@pytest.mark.parametrize(
114+
"raw",
115+
[
116+
"home/runner/work/widgets/widgets/src/package.json",
117+
"/home/runner/work/widgets/widgets/src/package.json",
118+
"opt/buildagent/work/abc123/widgets/src/package.json",
119+
],
120+
)
121+
def test_build_agent_prefixes_are_stripped(raw):
122+
url = Messages.get_manifest_file_url(_diff(), raw, _Config(scm="github"))
123+
assert url == "https://github.com/acme/widgets/blob/main/src/package.json"
124+
125+
126+
def test_empty_manifest_path_returns_empty_string():
127+
assert Messages.get_manifest_file_url(_diff(), "", _Config(scm="github")) == ""

0 commit comments

Comments
 (0)