From 950694b92e3c4d678e10c7ef4077c6ed64f5ee95 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:44:44 -0700 Subject: [PATCH 01/27] feat(output): show patched versions in security findings --- socketsecurity/core/messages.py | 17 +++++++ tests/unit/test_messages.py | 81 +++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+) create mode 100644 tests/unit/test_messages.py diff --git a/socketsecurity/core/messages.py b/socketsecurity/core/messages.py index d968c14b..d4e8c9e8 100644 --- a/socketsecurity/core/messages.py +++ b/socketsecurity/core/messages.py @@ -4,6 +4,7 @@ import re import uuid from datetime import datetime, timezone +from html import escape from pathlib import Path from mdutils import MdUtils @@ -15,6 +16,13 @@ class Messages: + @staticmethod + def get_patched_version(alert: Issue) -> str: + """Return the first patched version exposed by an alert, if any.""" + props = getattr(alert, "props", {}) or {} + value = props.get("firstPatchedVersionIdentifier") + return str(value) if value not in (None, "") else "" + @staticmethod def map_severity_to_sarif(severity: str) -> str: """ @@ -949,6 +957,12 @@ def security_comment_template(diff: Diff, config=None) -> str: severity_icon = Messages.get_severity_icon(alert.severity) action = "Block" if alert.error else "Warn" details_open = "" + patched_version = Messages.get_patched_version(alert) + patched_version_html = ( + "

Patched version: " + f"{escape(Messages.inline_html_text(patched_version))}

" + if patched_version else "" + ) # Generate proper manifest URL manifest_url = Messages.get_manifest_file_url(diff, alert.manifests, config) # Generate a table row for each alert @@ -969,6 +983,7 @@ def security_comment_template(diff: Diff, config=None) -> str:
{alert.pkg_name}@{alert.pkg_version} - {Messages.inline_html_text(alert.title)}

Note: {Messages.inline_html_text(alert.description)}

+ {patched_version_html}

Source: Manifest File

ℹ️ Read more on: This package | @@ -1332,6 +1347,7 @@ def create_console_security_alert_table(diff: Diff) -> PrettyTable: [ "Alert", "Package", + "Patched Version", "url", "Introduced by", "Manifest File", @@ -1352,6 +1368,7 @@ def create_console_security_alert_table(diff: Diff) -> PrettyTable: row = [ alert.title, alert.purl, + Messages.get_patched_version(alert), alert.url, source_str, manifest_str, diff --git a/tests/unit/test_messages.py b/tests/unit/test_messages.py new file mode 100644 index 00000000..ce6b696e --- /dev/null +++ b/tests/unit/test_messages.py @@ -0,0 +1,81 @@ +from socketsecurity.core.classes import Diff, Issue +from socketsecurity.core.messages import Messages + + +def _issue(**kwargs): + values = { + "pkg_type": "npm", + "pkg_name": "example-lib", + "pkg_version": "1.4.2", + "type": "highCVE", + "severity": "high", + "title": "High CVE", + "description": "A vulnerable dependency.", + "suggestion": "Upgrade to a patched release.", + "purl": "pkg:npm/example-lib@1.4.2", + "url": "https://socket.dev/npm/package/example-lib/overview/1.4.2", + "manifests": "package-lock.json", + "introduced_by": [["example-lib", "package-lock.json"]], + "error": True, + } + values.update(kwargs) + return Issue(**values) + + +def test_console_security_alert_table_includes_patched_version(): + diff = Diff( + new_alerts=[ + _issue(props={"firstPatchedVersionIdentifier": "1.5.0"}), + ] + ) + + table = Messages.create_console_security_alert_table(diff) + + assert table.field_names == [ + "Alert", + "Package", + "Patched Version", + "url", + "Introduced by", + "Manifest File", + "CI Status", + ] + assert table.rows[0][2] == "1.5.0" + + +def test_console_security_alert_table_leaves_missing_patched_version_blank(): + diff = Diff( + new_alerts=[ + _issue(), + _issue(props={}), + _issue(props={"firstPatchedVersionIdentifier": None}), + ] + ) + + table = Messages.create_console_security_alert_table(diff) + + assert [row[2] for row in table.rows] == ["", "", ""] + + +def test_security_comment_includes_patched_version_when_available(): + diff = Diff( + new_alerts=[ + _issue(props={"firstPatchedVersionIdentifier": "1.5.0"}), + ], + diff_url="https://socket.dev/dashboard/org/acme/diff/before/after", + ) + + comment = Messages.security_comment_template(diff) + + assert "Patched version: 1.5.0" in comment + + +def test_security_comment_omits_missing_patched_version(): + diff = Diff( + new_alerts=[_issue(props={})], + diff_url="https://socket.dev/dashboard/org/acme/diff/before/after", + ) + + comment = Messages.security_comment_template(diff) + + assert "Patched version:" not in comment From 7c1ec81409098ab71fa5899866336cdfdde31075 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:44:51 -0700 Subject: [PATCH 02/27] feat(ci): preserve pull request context in scan metadata --- docs/ci-cd.md | 111 +++++++++++- docs/cli-reference.md | 4 +- socketsecurity/config.py | 37 +++- socketsecurity/core/__init__.py | 23 ++- socketsecurity/core/pull_request.py | 145 +++++++++++++++ socketsecurity/socketcli.py | 65 ++++++- tests/core/test_diff_scan_polling.py | 8 + tests/unit/test_cli_config.py | 54 ++++++ tests/unit/test_pull_request_context.py | 228 ++++++++++++++++++++++++ tests/unit/test_socketcli.py | 9 + workflows/buildkite.yml | 21 ++- 11 files changed, 673 insertions(+), 32 deletions(-) create mode 100644 socketsecurity/core/pull_request.py create mode 100644 tests/unit/test_pull_request_context.py diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 968799b5..fc94e942 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -2,6 +2,10 @@ Use this guide for pipeline-focused CLI usage across platforms. +The shell commands in the recommended patterns are CI-provider neutral. Buildkite +pipeline equivalents and provider-specific considerations are called out alongside +the relevant guidance below. + ## Recommended patterns ### Dashboard-style reachable SARIF @@ -27,6 +31,27 @@ socketcli \ --strict-blocking ``` +### Buildkite: retain SARIF as a build artifact + +Either recommended pattern can run directly in a Buildkite command step. When the +scan writes SARIF, add +[`artifact_paths`](https://buildkite.com/docs/pipelines/configure/artifacts#upload-artifacts-with-a-command-step) +so developers can download the report from the build after the command finishes: + +```yaml +steps: + - label: ":socket: Socket reachable diff" + command: | + socketcli \ + --reach \ + --sarif-file results.sarif \ + --sarif-scope diff \ + --sarif-reachability reachable \ + --strict-blocking + artifact_paths: + - "results.sarif" +``` + ## Config file usage in CI Use `--config .socketcli.toml` or `--config .socketcli.json` to keep pipeline commands small. @@ -60,6 +85,9 @@ Equivalent JSON: } ``` +The Buildkite examples below use the same checked-in `.socketcli.toml` file; no +Buildkite-specific config-file format is required. + ## Platform examples ### GitHub Actions @@ -306,14 +334,33 @@ initial timeout signal or 137 if `SIGKILL` is involved. ### Buildkite +This example assumes a GitHub-hosted repository. Change +`SOCKET_SCM_INTEGRATION` to `gitlab` for a GitLab-hosted repository, or `api` +when provider association is not wanted. The doubled dollar signs defer +Buildkite variable expansion until the command runs on an agent. + ```yaml +env: + SOCKET_SCM_INTEGRATION: "github" + steps: - label: "Socket scan" - command: "socketcli --config .socketcli.toml --target-path ." - env: - SOCKET_SECURITY_API_TOKEN: "${SOCKET_SECURITY_API_TOKEN}" + command: | + socketcli \ + --config .socketcli.toml \ + --target-path . \ + --integration "$${SOCKET_SCM_INTEGRATION:-api}" \ + --pr-number "$${BUILDKITE_PULL_REQUEST:-0}" + secrets: + - SOCKET_SECURITY_API_TOKEN ``` +The `secrets` block expects a +[Buildkite secret](https://buildkite.com/docs/pipelines/security/secrets/buildkite-secrets) +named `SOCKET_SECURITY_API_TOKEN`. If your organization uses an external secrets +plugin or an agent hook instead, remove that block and inject the same environment +variable through your existing mechanism. Do not store the token in pipeline YAML. + The CLI reads Buildkite's native `BUILDKITE_COMMIT`, `BUILDKITE_BRANCH`, `BUILDKITE_PULL_REQUEST`, and `BUILDKITE_PULL_REQUEST_BASE_BRANCH` variables. For pull-request builds, ensure the checkout contains the base branch and the @@ -385,6 +432,18 @@ socket_scan: SOCKET_SECURITY_API_TOKEN: $SOCKET_SECURITY_API_TOKEN ``` +### Azure Pipelines + +```yaml +- script: | + socketcli \ + --integration azure \ + --enable-diff \ + --target-path "$(Build.SourcesDirectory)" + env: + SOCKET_SECURITY_API_TOKEN: $(SOCKET_SECURITY_API_TOKEN) +``` + ### Bitbucket Pipelines ```yaml @@ -395,6 +454,44 @@ pipelines: - socketcli --config .socketcli.toml --target-path . ``` +## Pull request and Dashboard association + +The CLI sends the resolved pull request number with each full scan and attaches +the pull request URL to diff scans so the Socket Dashboard can associate the +report with its originating change. If `--pr-number` is supplied, it wins; +passing `--pr-number 0` explicitly disables automatic association. + +Without an explicit value, the CLI recognizes: + +- GitHub Actions: `PR_NUMBER`, then the PR number in `GITHUB_REF`. +- GitLab CI: `CI_MERGE_REQUEST_IID`. +- Azure Pipelines: `SYSTEM_PULLREQUEST_PULLREQUESTNUMBER` for GitHub-hosted + repositories, otherwise `SYSTEM_PULLREQUEST_PULLREQUESTID` for Azure Repos. + +### Buildkite PR context + +Buildkite is SCM-provider neutral, so the CLI does not infer a provider or consume +its PR variable automatically. Pass Buildkite's +[`BUILDKITE_PULL_REQUEST`](https://buildkite.com/docs/pipelines/configure/environment-variables#BUILDKITE_PULL_REQUEST) +value to +`--pr-number` and identify the repository host with `--integration`, as shown in +the Buildkite platform example above. Buildkite sets `BUILDKITE_PULL_REQUEST` to +`false` outside PR builds; the CLI treats that value as no PR. + +Use `--integration github` for GitHub-hosted repositories and `--integration gitlab` +for GitLab-hosted ones. In both cases the CLI reads the repository slug and host from +[`BUILDKITE_REPO`](https://buildkite.com/docs/pipelines/configure/environment-variables#BUILDKITE_REPO) +to build the pull request or merge request link, so github.com, GitLab.com, and +self-hosted installations all work without extra configuration. Setting +`CI_PROJECT_URL` still overrides the derived GitLab project URL. Keep `--scm api` +unless you also intend to configure an existing GitHub or GitLab comment adapter and +its provider token. + +`--scm github` and `--scm gitlab` also imply the matching scan integration for +Dashboard metadata unless `--integration` was explicitly supplied. PR comments +remain limited to the existing GitHub and GitLab SCM adapters; Azure receives +console output and Dashboard association but does not post a PR comment. + ## Workflow templates Prebuilt examples in this repo: @@ -411,3 +508,11 @@ Prebuilt examples in this repo: - `--sarif-grouping alert` currently applies to `--sarif-scope full`. - Diff-based SARIF can validly be empty when there are no matching net-new alerts. - Keep API tokens in secret stores (`SOCKET_SECURITY_API_TOKEN`), not in config files. +- In Buildkite pipeline YAML, follow its + [runtime interpolation](https://buildkite.com/docs/pipelines/configure/environment-variables#runtime-variable-interpolation) + guidance and use `$$` for variables that must expand when the command runs rather + than when the pipeline is uploaded. +- Security findings with `props.firstPatchedVersionIdentifier` show that value in + the console table, including native Buildkite job logs, and in GitHub/GitLab + security comments when that SCM adapter is configured. Findings without a known + patched release leave the console cell blank and omit the comment field. diff --git a/docs/cli-reference.md b/docs/cli-reference.md index f64de267..d5d102fd 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -238,7 +238,7 @@ If you don't want to provide the Socket API Token every time then you can use th | `--repo` | False | *auto* | Repository name in owner/repo format (auto-detected from git remote) | | `--workspace` | False | | The Socket workspace to associate the scan with (e.g. `my-org` in `my-org/my-repo`). See note below. | | `--repo-is-public` | False | False | If set, flags a new repository creation as public. Defaults to false. | -| `--integration` | False | api | Integration type (api, github, gitlab, azure, bitbucket) | +| `--integration` | False | api | Integration type (api, github, gitlab, azure, bitbucket). When omitted, `--scm github` or `--scm gitlab` implies the matching integration. | | `--owner` | False | | Name of the integration owner, defaults to the socket organization slug | | `--branch` | False | *auto* | Branch name (auto-detected from git) | | `--committers` | False | *auto* | Committer(s) to filter by (auto-detected from git commit) | @@ -252,7 +252,7 @@ If you don't want to provide the Socket API Token every time then you can use th #### Pull Request and Commit | Parameter | Required | Default | Description | |:-----------------|:---------|:--------|:-----------------------------------------------| -| `--pr-number` | False | "0" | Pull request number | +| `--pr-number` | False | *auto* | Pull request number. Auto-detected in GitHub Actions, GitLab CI, and Azure Pipelines; explicitly passing `0` disables detection. | | `--commit-message` | False | *auto* | Commit message (auto-detected from git) | | `--commit-sha` | False | *auto* | Commit SHA (auto-detected from git) | | `--base-scan-id` | False | | Full scan ID to diff against, overriding the repository's head scan as the baseline. Mutually exclusive with `--base-commit-sha` | diff --git a/socketsecurity/config.py b/socketsecurity/config.py index 35904976..645e5145 100644 --- a/socketsecurity/config.py +++ b/socketsecurity/config.py @@ -115,6 +115,7 @@ class CliConfig: branch: str = "" committers: Optional[List[str]] = None pr_number: str = "0" + pr_number_explicit: bool = False commit_message: Optional[str] = None default_branch: bool = False target_path: str = "./" @@ -208,10 +209,10 @@ def from_args(cls, args_list: Optional[List[str]] = None) -> 'CliConfig': pre_parser.add_argument("--config", dest="config_file", default=None) pre_args, _ = pre_parser.parse_known_args(args_list) + normalized_defaults = {} if pre_args.config_file: config_defaults = load_cli_config_file(pre_args.config_file) valid_dests = {action.dest for action in parser._actions if action.dest != "help"} - normalized_defaults = {} for key, value in config_defaults.items(): dest = str(key).replace("-", "_") if dest in valid_dests: @@ -219,6 +220,17 @@ def from_args(cls, args_list: Optional[List[str]] = None) -> 'CliConfig': parser.set_defaults(**normalized_defaults) args = parser.parse_args(args_list) + integration_explicit = hasattr(args, "integration") + pr_number_explicit = hasattr(args, "pr_number") + + integration_type = getattr(args, "integration", "api") + pr_number = getattr(args, "pr_number", "0") + if ( + not integration_explicit and + integration_type == "api" and + args.scm in ("github", "gitlab") + ): + integration_type = args.scm if args.reach_exclude_paths: logging.warning( @@ -262,7 +274,8 @@ def from_args(cls, args_list: Optional[List[str]] = None) -> 'CliConfig': 'repo': args.repo, 'branch': args.branch, 'committers': args.committers, - 'pr_number': args.pr_number, + 'pr_number': pr_number, + 'pr_number_explicit': pr_number_explicit, 'commit_message': commit_message, 'default_branch': args.default_branch, 'target_path': os.path.expanduser(args.target_path), @@ -294,7 +307,7 @@ def from_args(cls, args_list: Optional[List[str]] = None) -> 'CliConfig': 'disable_ignore': args.disable_ignore, 'upload_logs': args.upload_logs, 'strict_blocking': args.strict_blocking, - 'integration_type': args.integration, + 'integration_type': integration_type, 'pending_head': args.pending_head, 'timeout': args.timeout, 'exit_code_on_api_error': args.exit_code_on_api_error, @@ -519,8 +532,12 @@ def create_argument_parser() -> argparse.ArgumentParser: "--integration", choices=INTEGRATION_TYPES, metavar="", - help="Integration type of api, github, gitlab, azure, or bitbucket. Defaults to api", - default="api" + help=( + "Integration type of api, github, gitlab, azure, or bitbucket. " + "Defaults to api; --scm github/gitlab implies the matching integration " + "when this option is omitted" + ), + default=argparse.SUPPRESS ) integration_group.add_argument( "--owner", @@ -535,13 +552,17 @@ def create_argument_parser() -> argparse.ArgumentParser: "--pr-number", dest="pr_number", metavar="", - help="Pull request number", - default="0" + help=( + "Pull request number. Auto-detected in supported CI environments when omitted; " + "pass 0 explicitly to disable detection" + ), + default=argparse.SUPPRESS ) pr_group.add_argument( "--pr_number", dest="pr_number", - help=argparse.SUPPRESS + help=argparse.SUPPRESS, + default=argparse.SUPPRESS ) pr_group.add_argument( "--commit-message", diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index b1b1d65b..6f20d5b9 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -1640,7 +1640,8 @@ def get_license_text_via_purl(self, packages: dict[str, Package], batch_size: in def get_diff_scan_artifacts( self, head_full_scan_id: str, - new_full_scan_id: str + new_full_scan_id: str, + external_href: Optional[str] = None ) -> DiffArtifacts: """Compare two full scans via the diff-scans endpoints, polling for the result. @@ -1663,6 +1664,8 @@ def get_diff_scan_artifacts( Args: head_full_scan_id: The before/base full scan ID new_full_scan_id: The after/head full scan ID + external_href: Optional pull request or merge request URL to associate + with the diff scan in the Socket Dashboard Returns: DiffArtifacts with the added/removed/unchanged/replaced/updated lists @@ -1672,6 +1675,8 @@ def get_diff_scan_artifacts( "after": new_full_scan_id, "description": f"Socket Security CLI v{__version__} scan comparison", } + if external_href: + create_params["external_href"] = external_href try: result = self.sdk.diffscans.create_from_ids(self.config.org_slug, create_params) diff_scan = result.get("diff_scan") or {} @@ -1820,7 +1825,8 @@ def get_added_and_removed_packages( self, head_full_scan_id: str, new_full_scan_id: str, - include_license_details: bool = False + include_license_details: bool = False, + external_href: Optional[str] = None ) -> Tuple[Dict[str, Package], Dict[str, Package], Dict[str, Package]]: """ Get packages that were added and removed between scans. @@ -1853,6 +1859,8 @@ def get_added_and_removed_packages( is retained as an explicit override seam, not wired to the ``--exclude-license-details`` user flag (which still governs the human-facing dashboard report URL). + external_href: Optional pull request or merge request URL to associate + with the primary diff-scan resource Returns: Tuple of (added_packages, removed_packages) dictionaries @@ -1864,7 +1872,8 @@ def get_added_and_removed_packages( try: diff_artifacts = self.get_diff_scan_artifacts( head_full_scan_id, - new_full_scan_id + new_full_scan_id, + external_href=external_href, ) except Exception as error: # SDK error messages can span many lines (path + response headers); the @@ -1980,7 +1989,8 @@ def create_new_diff( save_files_list_path: Optional[str] = None, save_manifest_tar_path: Optional[str] = None, base_paths: Optional[List[str]] = None, - explicit_files: Optional[List[str]] = None + explicit_files: Optional[List[str]] = None, + external_href: Optional[str] = None ) -> Diff: """Create a new diff using the Socket SDK. @@ -1992,6 +2002,8 @@ def create_new_diff( save_manifest_tar_path: Optional path to save manifest files tar.gz archive base_paths: List of base paths for the scan (optional) explicit_files: Optional list of explicit files to use instead of discovering files + external_href: Optional pull request or merge request URL to associate + with the diff scan """ log.debug(f"starting create_new_diff with no_change: {no_change}") if no_change: @@ -2126,7 +2138,8 @@ def create_new_diff( ) = self.get_added_and_removed_packages( head_full_scan_id, new_full_scan.id, - include_license_details=False + include_license_details=False, + external_href=external_href, ) # Separate unchanged packages from added/removed for --strict-blocking support diff --git a/socketsecurity/core/pull_request.py b/socketsecurity/core/pull_request.py new file mode 100644 index 00000000..cff0c184 --- /dev/null +++ b/socketsecurity/core/pull_request.py @@ -0,0 +1,145 @@ +import re +from dataclasses import dataclass +from typing import Mapping, Optional +from urllib.parse import urlparse + + +@dataclass(frozen=True) +class PullRequestContext: + number: int = 0 + url: Optional[str] = None + + +def _positive_int(value) -> int: + try: + parsed = int(value) + except (TypeError, ValueError): + return 0 + return parsed if parsed > 0 else 0 + + +def _repository_url(value: Optional[str]) -> Optional[str]: + if not value: + return None + url = value.strip().rstrip("/") + if url.endswith(".git"): + url = url[:-4] + parsed = urlparse(url) + return url if parsed.scheme in ("http", "https") and parsed.netloc else None + + +# git@host:owner/repo - the scp-like syntax urlparse cannot handle. The negative +# lookahead keeps scheme-prefixed URLs (https://, ssh://) out of this branch. +_SCP_LIKE_REMOTE = re.compile(r"^(?:[^@/]+@)?([^:/]+):(?!//)(.+)$") + + +def _parse_remote(value: Optional[str]) -> tuple[Optional[str], Optional[str]]: + """Split a git remote URL into its host and its ``owner/repo`` path. + + Providers expose the checkout URL rather than a slug on CI systems that are + not tied to a single SCM (Buildkite's ``BUILDKITE_REPO``, for example), so + the slug the URL builders need has to be recovered from it. The path is + returned whole because GitLab projects can be nested under subgroups. + """ + if not value: + return None, None + url = value.strip().rstrip("/") + if url.endswith(".git"): + url = url[:-4] + + match = _SCP_LIKE_REMOTE.match(url) + if match: + return match.group(1), match.group(2).strip("/") + + parsed = urlparse(url) + if parsed.scheme in ("http", "https", "ssh", "git") and parsed.hostname: + return parsed.hostname, parsed.path.strip("/") + return None, None + + +def _github_number(env: Mapping[str, str]) -> int: + number = _positive_int(env.get("PR_NUMBER")) + if number: + return number + match = re.match(r"^refs/pull/(\d+)/", env.get("GITHUB_REF", "")) + return _positive_int(match.group(1)) if match else 0 + + +def _github_url(number: int, repo: Optional[str], env: Mapping[str, str]) -> Optional[str]: + remote_host, remote_path = _parse_remote(env.get("BUILDKITE_REPO")) + # config.repo is only ever a bare repository name, so it cannot produce a + # slug on its own; it is kept last for callers that pass a full owner/repo. + repository = env.get("GITHUB_REPOSITORY") or remote_path or repo + if not repository or "/" not in repository: + return None + server = env.get("GITHUB_SERVER_URL") or (f"https://{remote_host}" if remote_host else "") + server = (server or "https://github.com").rstrip("/") + return f"{server}/{repository.strip('/')}/pull/{number}" + + +def _gitlab_url(number: int, repo: Optional[str], env: Mapping[str, str]) -> Optional[str]: + project_url = _repository_url(env.get("CI_PROJECT_URL")) + if not project_url: + remote_host, remote_path = _parse_remote(env.get("BUILDKITE_REPO")) + project_path = env.get("CI_PROJECT_PATH") or remote_path or repo + server = env.get("CI_SERVER_URL") or (f"https://{remote_host}" if remote_host else "") + server = server.rstrip("/") + if server and project_path and "/" in project_path: + project_url = f"{server}/{project_path.strip('/')}" + return f"{project_url}/-/merge_requests/{number}" if project_url else None + + +def _azure_url(number: int, env: Mapping[str, str], github_pr: bool) -> Optional[str]: + repository_url = _repository_url( + env.get("BUILD_REPOSITORY_URI") or + env.get("SYSTEM_PULLREQUEST_SOURCEREPOSITORYURI") + ) + if not repository_url: + return None + github_pr = github_pr or "github" in urlparse(repository_url).netloc.lower() + path = "pull" if github_pr else "pullrequest" + return f"{repository_url}/{path}/{number}" + + +def resolve_pull_request_context( + integration_type: str, + configured_number, + repo: Optional[str], + *, + configured_explicit: bool = False, + env: Optional[Mapping[str, str]] = None, +) -> PullRequestContext: + """Resolve PR metadata without making provider API calls. + + Explicit CLI/config values win, including an explicit zero used to disable + association. Otherwise the provider's standard CI environment is used. + """ + environment = env or {} + provider = str(integration_type or "api").lower() + number = _positive_int(configured_number) + + if not configured_explicit and not number: + if provider == "github": + number = _github_number(environment) + elif provider == "gitlab": + number = _positive_int(environment.get("CI_MERGE_REQUEST_IID")) + elif provider == "azure": + number = ( + _positive_int(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTNUMBER")) or + _positive_int(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTID")) + ) + + if not number: + return PullRequestContext() + + if provider == "github": + url = _github_url(number, repo, environment) + elif provider == "gitlab": + url = _gitlab_url(number, repo, environment) + elif provider == "azure": + github_pr = bool(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTNUMBER")) + url = _azure_url(number, environment, github_pr) + else: + url = None + + return PullRequestContext(number=number, url=url) diff --git a/socketsecurity/socketcli.py b/socketsecurity/socketcli.py index 9dd3bb1e..aafed013 100644 --- a/socketsecurity/socketcli.py +++ b/socketsecurity/socketcli.py @@ -18,6 +18,7 @@ from socketsecurity.core.git_interface import Git from socketsecurity.core.logging import initialize_logging, set_debug_mode from socketsecurity.core.messages import Messages +from socketsecurity.core.pull_request import resolve_pull_request_context from socketsecurity.core.scm_comments import Comments from socketsecurity.core.socket_config import SocketConfig, module_folder_dirs from socketsecurity.core.streaming import StreamingLogs @@ -133,6 +134,10 @@ def should_write_comment(disabled: bool, has_findings: bool, update_existing: bo return update_existing return True +def _select_pull_request_provider(integration_type: str, scm_type: str) -> str: + """Prefer an active comment adapter when resolving pull request context.""" + return scm_type if scm_type in ("github", "gitlab") else integration_type + def build_socket_sdk(config: CliConfig) -> socketdev: cli_user_agent_string = f"SocketPythonCLI/{config.version}" @@ -599,10 +604,26 @@ def main_code(): core.config.repo_visibility = "public" integration_type = config.integration_type integration_org_slug = config.integration_org_slug or org_slug - try: - pr_number = int(config.pr_number) - except (ValueError, TypeError): - pr_number = 0 + pr_provider = _select_pull_request_provider(integration_type, config.scm) + pr_context = resolve_pull_request_context( + pr_provider, + config.pr_number, + config.repo, + configured_explicit=config.pr_number_explicit, + env=os.environ, + ) + pr_number = pr_context.number + if pr_number: + config.pr_number = str(pr_number) + if scm is not None: + if hasattr(scm.config, "pr_number"): + scm.config.pr_number = str(pr_number) + elif hasattr(scm.config, "mr_iid"): + scm.config.mr_iid = str(pr_number) + log.debug( + f"Resolved {pr_provider} pull request context: " + f"number={pr_number}, url={pr_context.url or 'unavailable'}" + ) # Determine if this should be treated as default branch # Priority order: @@ -721,7 +742,16 @@ def _is_unprocessed(c): log.info("Push initiated flow") if scm.check_event_type() == "diff": log.info("Starting comment logic for PR/MR event") - diff = core.create_new_diff(scan_paths, params, no_change=should_skip_scan, save_files_list_path=config.save_submitted_files_list, save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, explicit_files=scan_explicit_files) + diff = core.create_new_diff( + scan_paths, + params, + no_change=should_skip_scan, + save_files_list_path=config.save_submitted_files_list, + save_manifest_tar_path=config.save_manifest_tar, + base_paths=base_paths, + explicit_files=scan_explicit_files, + external_href=pr_context.url, + ) comments = scm.get_comments_for_pr() # FIXME: this overwrites diff.new_alerts, which was previously populated by Core.create_issue_alerts @@ -843,14 +873,32 @@ def _is_unprocessed(c): ) else: log.info("Starting non-PR/MR flow") - diff = core.create_new_diff(scan_paths, params, no_change=should_skip_scan, save_files_list_path=config.save_submitted_files_list, save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, explicit_files=scan_explicit_files) + diff = core.create_new_diff( + scan_paths, + params, + no_change=should_skip_scan, + save_files_list_path=config.save_submitted_files_list, + save_manifest_tar_path=config.save_manifest_tar, + base_paths=base_paths, + explicit_files=scan_explicit_files, + external_href=pr_context.url, + ) output_handler.handle_output(diff) elif (config.enable_diff or force_diff_mode) and not force_api_mode: # New logic: --enable-diff or force_diff_mode (from --ignore-commit-files in git repos) forces diff mode log.info("Diff mode enabled without SCM integration") - diff = core.create_new_diff(scan_paths, params, no_change=should_skip_scan, save_files_list_path=config.save_submitted_files_list, save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, explicit_files=scan_explicit_files) + diff = core.create_new_diff( + scan_paths, + params, + no_change=should_skip_scan, + save_files_list_path=config.save_submitted_files_list, + save_manifest_tar_path=config.save_manifest_tar, + base_paths=base_paths, + explicit_files=scan_explicit_files, + external_href=pr_context.url, + ) output_handler.handle_output(diff) elif (config.enable_diff or force_diff_mode) and force_api_mode: @@ -917,7 +965,8 @@ def _is_unprocessed(c): save_files_list_path=config.save_submitted_files_list, save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, - explicit_files=scan_explicit_files + explicit_files=scan_explicit_files, + external_href=pr_context.url, ) output_handler.handle_output(diff) diff --git a/tests/core/test_diff_scan_polling.py b/tests/core/test_diff_scan_polling.py index d8c0e396..df95f04f 100644 --- a/tests/core/test_diff_scan_polling.py +++ b/tests/core/test_diff_scan_polling.py @@ -160,6 +160,14 @@ def test_eager_list_artifacts_do_not_bypass_filtered_get(core, diff_scan_get_res params={"cached": "true", "omit_unchanged": "true"}, ) +def test_diff_scan_is_associated_with_pull_request_url(core): + external_href = "https://dev.azure.com/acme/platform/_git/widgets/pullrequest/17" + + core.get_diff_scan_artifacts("head", "new", external_href=external_href) + + create_params = core.sdk.diffscans.create_from_ids.call_args.args[1] + assert create_params["external_href"] == external_href + def test_fallback_to_streaming_diff_on_failure(core): """If the diff-scans flow fails (e.g. token missing the diff-scans scopes), diff --git a/tests/unit/test_cli_config.py b/tests/unit/test_cli_config.py index 6aa9bf1a..f70cda2b 100644 --- a/tests/unit/test_cli_config.py +++ b/tests/unit/test_cli_config.py @@ -68,6 +68,60 @@ def test_default_values(self): assert config.target_path == "./" assert config.files == "[]" + @pytest.mark.parametrize("scm", ["github", "gitlab"]) + def test_scm_infers_scan_integration_when_integration_is_not_explicit(self, scm): + config = CliConfig.from_args(["--api-token", "test", "--scm", scm]) + + assert config.integration_type == scm + + def test_explicit_api_integration_wins_over_scm_inference(self): + config = CliConfig.from_args([ + "--api-token", "test", + "--scm", "github", + "--integration", "api", + ]) + + assert config.integration_type == "api" + + def test_abbreviated_integration_is_still_treated_as_explicit(self): + config = CliConfig.from_args([ + "--api-token", "test", + "--scm", "github", + "--integ", "api", + ]) + + assert config.integration_type == "api" + + def test_pr_number_tracks_whether_it_was_explicit(self): + inferred = CliConfig.from_args(["--api-token", "test"]) + explicit = CliConfig.from_args([ + "--api-token", "test", "--pr-number", "0", + ]) + + assert inferred.pr_number_explicit is False + assert explicit.pr_number_explicit is True + + def test_abbreviated_pr_number_is_still_treated_as_explicit(self): + config = CliConfig.from_args([ + "--api-token", "test", "--pr-n", "0", + ]) + + assert config.pr_number == "0" + assert config.pr_number_explicit is True + + def test_config_file_values_are_treated_as_explicit(self, tmp_path): + config_path = tmp_path / "socketcli.json" + config_path.write_text( + '{"socketcli":{"scm":"github","integration":"api","pr_number":"0"}}' + ) + + config = CliConfig.from_args([ + "--api-token", "test", "--config", str(config_path), + ]) + + assert config.integration_type == "api" + assert config.pr_number_explicit is True + @pytest.mark.parametrize("flag,attr", [ ("--enable-debug", "enable_debug"), ("--disable-blocking", "disable_blocking"), diff --git a/tests/unit/test_pull_request_context.py b/tests/unit/test_pull_request_context.py new file mode 100644 index 00000000..5ad12903 --- /dev/null +++ b/tests/unit/test_pull_request_context.py @@ -0,0 +1,228 @@ +from socketsecurity.core.pull_request import resolve_pull_request_context + + +def test_explicit_pr_number_wins_over_detected_context(): + context = resolve_pull_request_context( + "github", + "42", + "acme/widgets", + configured_explicit=True, + env={ + "GITHUB_REF": "refs/pull/99/merge", + "GITHUB_REPOSITORY": "acme/widgets", + }, + ) + + assert context.number == 42 + assert context.url == "https://github.com/acme/widgets/pull/42" + + +def test_explicit_zero_disables_pr_auto_detection(): + context = resolve_pull_request_context( + "github", + "0", + "acme/widgets", + configured_explicit=True, + env={"GITHUB_REF": "refs/pull/99/merge"}, + ) + + assert context.number == 0 + assert context.url is None + + +def test_buildkite_non_pr_sentinel_is_treated_as_no_pull_request(): + context = resolve_pull_request_context( + "github", + "false", + "acme/widgets", + configured_explicit=True, + env={}, + ) + + assert context.number == 0 + assert context.url is None + + +def test_github_context_is_detected_from_actions_environment(): + context = resolve_pull_request_context( + "github", + "0", + None, + env={ + "GITHUB_REF": "refs/pull/123/merge", + "GITHUB_REPOSITORY": "acme/widgets", + "GITHUB_SERVER_URL": "https://github.example.com", + }, + ) + + assert context.number == 123 + assert context.url == "https://github.example.com/acme/widgets/pull/123" + + +def test_gitlab_context_is_detected_from_merge_request_environment(): + context = resolve_pull_request_context( + "gitlab", + "0", + None, + env={ + "CI_MERGE_REQUEST_IID": "81", + "CI_PROJECT_URL": "https://gitlab.example.com/acme/widgets", + }, + ) + + assert context.number == 81 + assert context.url == "https://gitlab.example.com/acme/widgets/-/merge_requests/81" + + +def test_azure_repos_context_uses_pull_request_id(): + context = resolve_pull_request_context( + "azure", + "0", + None, + env={ + "SYSTEM_PULLREQUEST_PULLREQUESTID": "17", + "BUILD_REPOSITORY_URI": "https://dev.azure.com/acme/platform/_git/widgets", + }, + ) + + assert context.number == 17 + assert context.url == "https://dev.azure.com/acme/platform/_git/widgets/pullrequest/17" + + +def test_azure_fork_context_uses_target_repository_url(): + context = resolve_pull_request_context( + "azure", + "0", + None, + env={ + "SYSTEM_PULLREQUEST_PULLREQUESTID": "17", + "BUILD_REPOSITORY_URI": "https://dev.azure.com/acme/platform/_git/widgets", + "SYSTEM_PULLREQUEST_SOURCEREPOSITORYURI": ( + "https://dev.azure.com/contributor/forks/_git/widgets" + ), + }, + ) + + assert context.number == 17 + assert context.url == "https://dev.azure.com/acme/platform/_git/widgets/pullrequest/17" + + +def test_azure_pipeline_with_github_repo_uses_pull_request_number(): + context = resolve_pull_request_context( + "azure", + "0", + None, + env={ + "SYSTEM_PULLREQUEST_PULLREQUESTNUMBER": "23", + "SYSTEM_PULLREQUEST_PULLREQUESTID": "98765", + "BUILD_REPOSITORY_URI": "https://github.com/acme/widgets.git", + }, + ) + + assert context.number == 23 + assert context.url == "https://github.com/acme/widgets/pull/23" + + +def test_explicit_azure_github_pr_number_still_uses_github_url_shape(): + context = resolve_pull_request_context( + "azure", + "23", + None, + configured_explicit=True, + env={"BUILD_REPOSITORY_URI": "https://github.com/acme/widgets.git"}, + ) + + assert context.number == 23 + assert context.url == "https://github.com/acme/widgets/pull/23" + + +def test_non_pr_run_has_no_context(): + context = resolve_pull_request_context("azure", "0", "acme/widgets", env={}) + + assert context.number == 0 + assert context.url is None + + +# --------------------------------------------------------------------------- +# Provider-neutral CI (Buildkite). The provider comes from --integration and the +# PR number from --pr-number; only the repository slug has to be recovered from +# the checkout URL, because config.repo is a bare repository name with no owner. +# --------------------------------------------------------------------------- + + +def test_buildkite_github_repo_url_is_derived_from_the_checkout_remote(): + context = resolve_pull_request_context( + "github", + "42", + "widgets", + configured_explicit=True, + env={"BUILDKITE_REPO": "git@github.com:acme/widgets.git"}, + ) + + assert context.number == 42 + assert context.url == "https://github.com/acme/widgets/pull/42" + + +def test_buildkite_github_enterprise_host_is_taken_from_the_remote(): + context = resolve_pull_request_context( + "github", + "42", + "widgets", + configured_explicit=True, + env={"BUILDKITE_REPO": "https://github.example.com/acme/widgets.git"}, + ) + + assert context.url == "https://github.example.com/acme/widgets/pull/42" + + +def test_github_actions_environment_wins_over_the_checkout_remote(): + context = resolve_pull_request_context( + "github", + "42", + "widgets", + configured_explicit=True, + env={ + "GITHUB_REPOSITORY": "acme/widgets", + "GITHUB_SERVER_URL": "https://github.example.com", + "BUILDKITE_REPO": "git@github.com:stale/mirror.git", + }, + ) + + assert context.url == "https://github.example.com/acme/widgets/pull/42" + + +def test_buildkite_gitlab_repo_url_keeps_nested_subgroups(): + context = resolve_pull_request_context( + "gitlab", + "81", + "widgets", + configured_explicit=True, + env={"BUILDKITE_REPO": "ssh://git@gitlab.example.com/acme/platform/widgets.git"}, + ) + + assert context.url == "https://gitlab.example.com/acme/platform/widgets/-/merge_requests/81" + + +def test_gitlab_ci_project_url_wins_over_the_checkout_remote(): + context = resolve_pull_request_context( + "gitlab", + "81", + "widgets", + configured_explicit=True, + env={ + "CI_PROJECT_URL": "https://gitlab.example.com/acme/widgets", + "BUILDKITE_REPO": "git@gitlab.example.com:stale/mirror.git", + }, + ) + + assert context.url == "https://gitlab.example.com/acme/widgets/-/merge_requests/81" + + +def test_bare_repository_name_alone_yields_no_url(): + """config.repo has no owner segment, so it cannot stand in for a slug.""" + context = resolve_pull_request_context( + "github", "42", "widgets", configured_explicit=True, env={} + ) + + assert context.number == 42 + assert context.url is None diff --git a/tests/unit/test_socketcli.py b/tests/unit/test_socketcli.py index d8f661aa..103cfff8 100644 --- a/tests/unit/test_socketcli.py +++ b/tests/unit/test_socketcli.py @@ -65,6 +65,15 @@ def test_keyboard_interrupt_still_exits_2(monkeypatch): assert code == 2 +@pytest.mark.parametrize("scm", ["github", "gitlab"]) +def test_pr_context_provider_prefers_active_scm_adapter(scm): + assert socketcli._select_pull_request_provider("api", scm) == scm + + +def test_pr_context_provider_uses_integration_without_comment_adapter(): + assert socketcli._select_pull_request_provider("azure", "api") == "azure" + + # --------------------------------------------------------------------------- # Buildkite-aware infrastructure error formatting. # --------------------------------------------------------------------------- diff --git a/workflows/buildkite.yml b/workflows/buildkite.yml index a2f8e452..3657f283 100644 --- a/workflows/buildkite.yml +++ b/workflows/buildkite.yml @@ -1,13 +1,22 @@ # Socket Security Buildkite pipeline example -# Runs Socket CLI in a Buildkite step using repository-level environment variables. +# Runs Socket CLI in a Buildkite step. Set SOCKET_SCM_INTEGRATION below to github +# or gitlab for Dashboard PR association, or leave it as api when provider +# association is not wanted. The repository slug and host are read from +# BUILDKITE_REPO, so no further configuration is needed for either provider. + +env: + SOCKET_SCM_INTEGRATION: "api" steps: - label: "Socket Security Scan" command: | socketcli \ --target-path . \ - --scm api \ - --pr-number 0 - env: - # Configure this in Buildkite pipeline/repo settings. - SOCKET_SECURITY_API_TOKEN: "${SOCKET_SECURITY_API_TOKEN}" + --integration "$${SOCKET_SCM_INTEGRATION:-api}" \ + --pr-number "$${BUILDKITE_PULL_REQUEST:-0}" + secrets: + - SOCKET_SECURITY_API_TOKEN + + # This uses a Buildkite secret named SOCKET_SECURITY_API_TOKEN. If your + # organization uses an external secrets plugin or agent hook, remove the + # secrets block and inject that environment variable through your mechanism. From 7cf86822b4fe1feb13a46d71f1fc2f690e4004a0 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Wed, 2 Sep 2026 17:40:02 -0400 Subject: [PATCH 03/27] chore(release): bump version to 2.9.0 2.8.0 and 2.8.1 shipped from main while this branch was open, so the original 2.8.0 bump here is dead. This branch changes the behavior of existing flags rather than only fixing them -- --pr-number gains auto-detection, --scm github|gitlab implies --integration, and SCM branch pipelines switch from diff scans to full scans and stop returning a blocking exit code -- so it takes the minor bump per the repo's semver standard, not a patch. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 23 +++++++++++++++++++++++ pyproject.toml | 2 +- socketsecurity/__init__.py | 2 +- uv.lock | 2 +- 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6eebb26..540dc311 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,28 @@ # Changelog +## 2.9.0 + +### Added: patched versions in human-readable security output + +- The native console alert table now includes a `Patched Version` column, + populated from `props.firstPatchedVersionIdentifier` when the API provides it. +- GitHub pull request and GitLab merge request security comments now show the + patched version in each applicable alert's details. + +### Fixed: CLI scans retain pull request context in the Socket Dashboard + +- Pull request numbers are detected from standard GitHub Actions, GitLab CI, + and Azure Pipelines environments when `--pr-number` is not supplied. An + explicitly supplied value, including `0`, remains authoritative. +- The Buildkite workflow and CI/CD guide now forward `BUILDKITE_PULL_REQUEST` + explicitly and document provider selection for Dashboard PR association. With + `--integration github` or `--integration gitlab`, the repository slug and host + for the link are read from `BUILDKITE_REPO`, covering self-hosted installations. +- `--scm github` and `--scm gitlab` now imply the matching scan integration + unless `--integration` is explicitly supplied. +- Diff scans include the detected pull request or merge request URL as their + external link, allowing Dashboard reports to retain their CI change context. + ## 2.8.1 ### Changed: bump pinned @coana-tech/cli to 15.10.40 diff --git a/pyproject.toml b/pyproject.toml index 33d2d4df..334ff1b4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,7 +6,7 @@ build-backend = "hatchling.build" [project] name = "socketsecurity" -version = "2.8.1" +version = "2.9.0" requires-python = ">= 3.11" license = {"file" = "LICENSE"} dependencies = [ diff --git a/socketsecurity/__init__.py b/socketsecurity/__init__.py index 6cf31cd7..ce3e70ab 100644 --- a/socketsecurity/__init__.py +++ b/socketsecurity/__init__.py @@ -1,3 +1,3 @@ __author__ = 'socket.dev' -__version__ = '2.8.1' +__version__ = '2.9.0' USER_AGENT = f'SocketPythonCLI/{__version__}' diff --git a/uv.lock b/uv.lock index 69bb3127..470f920a 100644 --- a/uv.lock +++ b/uv.lock @@ -1293,7 +1293,7 @@ wheels = [ [[package]] name = "socketsecurity" -version = "2.8.1" +version = "2.9.0" source = { editable = "." } dependencies = [ { name = "beautifulsoup4" }, From fa6ec9ac8d2e541508de25c371a8c75f9796eb70 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Wed, 2 Sep 2026 17:51:28 -0400 Subject: [PATCH 04/27] refactor: share one git remote parser between Buildkite consumers The GitHub comment adapter and pull request link construction each parsed BUILDKITE_REPO independently. Consolidate on socketsecurity.core.git_remote, which also reports the remote host (needed for self-hosted GitHub Enterprise and GitLab) and preserves nested GitLab subgroup paths. Co-Authored-By: Claude Opus 5 (1M context) --- docs/ci-cd.md | 26 +++++++++-------- socketsecurity/core/git_remote.py | 43 +++++++++++++++++++++++++++++ socketsecurity/core/pull_request.py | 35 +++-------------------- socketsecurity/core/scm/github.py | 26 +++++------------ tests/unit/test_git_remote.py | 41 +++++++++++++++++++++++++++ 5 files changed, 109 insertions(+), 62 deletions(-) create mode 100644 socketsecurity/core/git_remote.py create mode 100644 tests/unit/test_git_remote.py diff --git a/docs/ci-cd.md b/docs/ci-cd.md index fc94e942..57b34f6e 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -368,11 +368,12 @@ checked-out head commit. The CLI uses those local refs first and performs a targeted fetch only when a required ref or its comparison history is missing; it does not fetch every remote ref and tag during startup. -When `--scm github` is used from Buildkite, the CLI also derives GitHub comment -context from `BUILDKITE_REPO`, `BUILDKITE_BUILD_CHECKOUT_PATH`, and the variables -above. Set `GH_API_TOKEN` to a GitHub token with the required repository access. -GitHub Enterprise users should also set `GITHUB_API_URL`; GitHub.com defaults to -`https://api.github.com`. +When `--scm github` is used from Buildkite, the CLI also posts GitHub PR comments. +It identifies the repository from `BUILDKITE_REPO` and takes the rest of the build +context from `BUILDKITE_BUILD_CHECKOUT_PATH` and the variables above — see +[Buildkite PR context](#buildkite-pr-context). Set `GH_API_TOKEN` to a GitHub token +with the required repository access. GitHub Enterprise users should also set +`GITHUB_API_URL`; GitHub.com defaults to `https://api.github.com`. #### Merge-base baselines in Buildkite (dynamic pipelines) @@ -479,13 +480,14 @@ the Buildkite platform example above. Buildkite sets `BUILDKITE_PULL_REQUEST` to `false` outside PR builds; the CLI treats that value as no PR. Use `--integration github` for GitHub-hosted repositories and `--integration gitlab` -for GitLab-hosted ones. In both cases the CLI reads the repository slug and host from -[`BUILDKITE_REPO`](https://buildkite.com/docs/pipelines/configure/environment-variables#BUILDKITE_REPO) -to build the pull request or merge request link, so github.com, GitLab.com, and -self-hosted installations all work without extra configuration. Setting -`CI_PROJECT_URL` still overrides the derived GitLab project URL. Keep `--scm api` -unless you also intend to configure an existing GitHub or GitLab comment adapter and -its provider token. +for GitLab-hosted ones. The CLI identifies the repository from +[`BUILDKITE_REPO`](https://buildkite.com/docs/pipelines/configure/environment-variables#BUILDKITE_REPO), +taking both the slug and the host from it, so github.com, GitLab.com, and self-hosted +installations all build a correct pull request or merge request link without extra +configuration. That same value identifies the repository for GitHub PR comments when +`--scm github` is set. `CI_PROJECT_URL` still overrides the derived GitLab project URL. +Keep `--scm api` unless you also intend to configure an existing GitHub or GitLab +comment adapter and its provider token. `--scm github` and `--scm gitlab` also imply the matching scan integration for Dashboard metadata unless `--integration` was explicitly supplied. PR comments diff --git a/socketsecurity/core/git_remote.py b/socketsecurity/core/git_remote.py new file mode 100644 index 00000000..9ca5dc57 --- /dev/null +++ b/socketsecurity/core/git_remote.py @@ -0,0 +1,43 @@ +"""Parsing for git remote URLs. + +CI systems that are not tied to a single SCM expose the checkout URL rather than +an ``owner/repo`` slug (Buildkite's ``BUILDKITE_REPO``, for example). Both the +GitHub comment adapter and pull request context resolution need to recover the +slug from it, so the parsing lives here rather than in either caller. +""" +import re +from typing import Optional, Tuple +from urllib.parse import urlparse + +# git@host:owner/repo - the scp-like syntax urlparse cannot handle. The negative +# lookahead keeps scheme-prefixed URLs (https://, ssh://) out of this branch. +_SCP_LIKE_REMOTE = re.compile(r"^(?:[^@/]+@)?([^:/]+):(?!//)(.+)$") + + +def parse_git_remote(value: Optional[str]) -> Tuple[Optional[str], Optional[str]]: + """Split a git remote URL into its host and its repository path. + + Returns ``(host, path)``, or ``(None, None)`` when the value is not a usable + remote. The path is returned whole rather than as ``owner``/``repo`` because + GitLab projects can be nested under subgroups; callers that only want the + last two segments can split it themselves. ``host`` is ``None`` for a bare + ``owner/repo`` path, which carries no host to report. + """ + if not value: + return None, None + url = value.strip().rstrip("/") + if url.endswith(".git"): + url = url[:-4] + + match = _SCP_LIKE_REMOTE.match(url) + if match: + return match.group(1), match.group(2).strip("/") + + parsed = urlparse(url) + if parsed.scheme in ("http", "https", "ssh", "git") and parsed.hostname: + return parsed.hostname, parsed.path.strip("/") + + # A bare owner/repo path, with no scheme and nothing to infer a host from. + if "/" in url: + return None, url.strip("/") + return None, None diff --git a/socketsecurity/core/pull_request.py b/socketsecurity/core/pull_request.py index cff0c184..60ad3965 100644 --- a/socketsecurity/core/pull_request.py +++ b/socketsecurity/core/pull_request.py @@ -3,6 +3,8 @@ from typing import Mapping, Optional from urllib.parse import urlparse +from socketsecurity.core.git_remote import parse_git_remote + @dataclass(frozen=True) class PullRequestContext: @@ -28,35 +30,6 @@ def _repository_url(value: Optional[str]) -> Optional[str]: return url if parsed.scheme in ("http", "https") and parsed.netloc else None -# git@host:owner/repo - the scp-like syntax urlparse cannot handle. The negative -# lookahead keeps scheme-prefixed URLs (https://, ssh://) out of this branch. -_SCP_LIKE_REMOTE = re.compile(r"^(?:[^@/]+@)?([^:/]+):(?!//)(.+)$") - - -def _parse_remote(value: Optional[str]) -> tuple[Optional[str], Optional[str]]: - """Split a git remote URL into its host and its ``owner/repo`` path. - - Providers expose the checkout URL rather than a slug on CI systems that are - not tied to a single SCM (Buildkite's ``BUILDKITE_REPO``, for example), so - the slug the URL builders need has to be recovered from it. The path is - returned whole because GitLab projects can be nested under subgroups. - """ - if not value: - return None, None - url = value.strip().rstrip("/") - if url.endswith(".git"): - url = url[:-4] - - match = _SCP_LIKE_REMOTE.match(url) - if match: - return match.group(1), match.group(2).strip("/") - - parsed = urlparse(url) - if parsed.scheme in ("http", "https", "ssh", "git") and parsed.hostname: - return parsed.hostname, parsed.path.strip("/") - return None, None - - def _github_number(env: Mapping[str, str]) -> int: number = _positive_int(env.get("PR_NUMBER")) if number: @@ -66,7 +39,7 @@ def _github_number(env: Mapping[str, str]) -> int: def _github_url(number: int, repo: Optional[str], env: Mapping[str, str]) -> Optional[str]: - remote_host, remote_path = _parse_remote(env.get("BUILDKITE_REPO")) + remote_host, remote_path = parse_git_remote(env.get("BUILDKITE_REPO")) # config.repo is only ever a bare repository name, so it cannot produce a # slug on its own; it is kept last for callers that pass a full owner/repo. repository = env.get("GITHUB_REPOSITORY") or remote_path or repo @@ -80,7 +53,7 @@ def _github_url(number: int, repo: Optional[str], env: Mapping[str, str]) -> Opt def _gitlab_url(number: int, repo: Optional[str], env: Mapping[str, str]) -> Optional[str]: project_url = _repository_url(env.get("CI_PROJECT_URL")) if not project_url: - remote_host, remote_path = _parse_remote(env.get("BUILDKITE_REPO")) + remote_host, remote_path = parse_git_remote(env.get("BUILDKITE_REPO")) project_path = env.get("CI_PROJECT_PATH") or remote_path or repo server = env.get("CI_SERVER_URL") or (f"https://{remote_host}" if remote_host else "") server = server.rstrip("/") diff --git a/socketsecurity/core/scm/github.py b/socketsecurity/core/scm/github.py index 7504a46c..9ec1e4ca 100644 --- a/socketsecurity/core/scm/github.py +++ b/socketsecurity/core/scm/github.py @@ -1,7 +1,6 @@ import json import os import sys -import urllib.parse from dataclasses import dataclass from git import Optional @@ -9,6 +8,7 @@ from socketsecurity import USER_AGENT from socketsecurity.core import log from socketsecurity.core.classes import Comment +from socketsecurity.core.git_remote import parse_git_remote from socketsecurity.core.scm_comments import Comments from socketsecurity.socketcli import CliClient @@ -38,24 +38,12 @@ class GithubConfig: @staticmethod def _repository_from_buildkite() -> tuple[str, str]: """Return ``(owner, repository)`` from Buildkite's Git repository URL.""" - repository_url = ( - # Comments and statuses belong to the pipeline/base repository, - # not a contributor's fork from BUILDKITE_PULL_REQUEST_REPO. - os.getenv("BUILDKITE_REPO") - or os.getenv("BUILDKITE_PULL_REQUEST_REPO") - or "" - ).strip() - if not repository_url: - return "", "" - - if "://" in repository_url: - repository_path = urllib.parse.urlparse(repository_url).path - elif ":" in repository_url: - # SCP-style SSH URL: git@github.com:owner/repository.git - repository_path = repository_url.split(":", 1)[1] - else: - repository_path = repository_url - parts = repository_path.strip("/").removesuffix(".git").split("/") + # Comments and statuses belong to the pipeline/base repository, not a + # contributor's fork from BUILDKITE_PULL_REQUEST_REPO. + _, repository_path = parse_git_remote( + os.getenv("BUILDKITE_REPO") or os.getenv("BUILDKITE_PULL_REQUEST_REPO") + ) + parts = repository_path.split("/") if repository_path else [] if len(parts) < 2: return "", "" return parts[-2], parts[-1] diff --git a/tests/unit/test_git_remote.py b/tests/unit/test_git_remote.py new file mode 100644 index 00000000..c589132e --- /dev/null +++ b/tests/unit/test_git_remote.py @@ -0,0 +1,41 @@ +"""Tests for the shared git remote parser. + +Both the GitHub comment adapter (`GithubConfig._repository_from_buildkite`) and +pull request URL construction depend on this, so the URL forms Buildkite and +self-hosted installations emit are pinned here rather than in either caller. +""" +import pytest + +from socketsecurity.core.git_remote import parse_git_remote + + +@pytest.mark.parametrize( + ("remote", "expected"), + [ + # The three forms BUILDKITE_REPO is observed to take. + ("git@github.com:acme/widgets.git", ("github.com", "acme/widgets")), + ("https://github.com/acme/widgets.git", ("github.com", "acme/widgets")), + ("ssh://git@github.com/acme/widgets.git", ("github.com", "acme/widgets")), + # Self-hosted hosts must survive: they decide the PR/MR link's origin. + ("git@github.example.com:acme/widgets.git", ("github.example.com", "acme/widgets")), + ("https://gitlab.example.com/acme/widgets", ("gitlab.example.com", "acme/widgets")), + # GitLab subgroups: the path is returned whole, not just the last two parts. + ( + "ssh://git@gitlab.example.com/acme/platform/widgets.git", + ("gitlab.example.com", "acme/platform/widgets"), + ), + ("git://github.com/acme/widgets.git", ("github.com", "acme/widgets")), + # Cosmetic variation callers should not have to normalise themselves. + (" https://github.com/acme/widgets/ ", ("github.com", "acme/widgets")), + # Credentials in the URL must not leak into the host. + ("https://user@github.com/acme/widgets", ("github.com", "acme/widgets")), + # A bare slug carries no host to report, but is still usable. + ("acme/widgets", (None, "acme/widgets")), + # Nothing usable. + ("not-a-repository", (None, None)), + ("", (None, None)), + (None, (None, None)), + ], +) +def test_parse_git_remote(remote, expected): + assert parse_git_remote(remote) == expected From 0bdac6c2ab6f0e800d8a9d745c73d24e27bda4ad Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:08:32 -0400 Subject: [PATCH 05/27] fix(ci): apply the pull request link to an already-compared scan pair external_href is only honored while a diff scan is being created, so a re-run over the same before/after pair left the Dashboard report with no link back to its pull request. Send on_duplicate=update alongside it, which applies the link to the existing diff scan and answers 200 with the same envelope as a create. The 409-and-resolve path is retained for runs with no pull request context and for deployments that predate on_duplicate=update. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 2 ++ socketsecurity/core/__init__.py | 21 +++++++++--- tests/core/test_diff_scan_polling.py | 49 +++++++++++++++++++++++++++- 3 files changed, 66 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 540dc311..e24b9d0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,8 @@ unless `--integration` is explicitly supplied. - Diff scans include the detected pull request or merge request URL as their external link, allowing Dashboard reports to retain their CI change context. + Re-running a comparison over an already-compared scan pair now applies the + link to the existing diff scan instead of leaving that report unassociated. ## 2.8.1 diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index 6f20d5b9..a9728c72 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -1677,6 +1677,14 @@ def get_diff_scan_artifacts( } if external_href: create_params["external_href"] = external_href + # external_href is only honored while a diff scan is being created, + # so re-running a comparison over an already-compared scan pair + # would otherwise leave the Dashboard report with no link back to + # the pull request. on_duplicate=update applies the link to the + # existing resource and answers 200 with the same {"diff_scan": ...} + # envelope as a create. Notably it is not on_duplicate=redirect, + # whose 302 the SDK follows into a GET without cached=true. + create_params["on_duplicate"] = "update" try: result = self.sdk.diffscans.create_from_ids(self.config.org_slug, create_params) diff_scan = result.get("diff_scan") or {} @@ -1685,11 +1693,14 @@ def get_diff_scan_artifacts( if error.status_code != 409: raise - # Do not use on_duplicate=redirect here. The SDK follows that 302 - # automatically with a GET that lacks cached=true, which can leave - # the connection idle while an existing diff scan is still computing. - # Resolve the duplicate resource explicitly so every result fetch - # continues through the bounded cached polling path below. + # Reached without on_duplicate=update (no pull request context to + # attach) and against deployments that predate it and still answer + # 409 regardless. Do not switch this to on_duplicate=redirect: the + # SDK follows that 302 automatically with a GET that lacks + # cached=true, which can leave the connection idle while an existing + # diff scan is still computing. Resolve the duplicate resource + # explicitly so every result fetch continues through the bounded + # cached polling path below. existing = self.sdk.diffscans.list( self.config.org_slug, params={ diff --git a/tests/core/test_diff_scan_polling.py b/tests/core/test_diff_scan_polling.py index df95f04f..d30bf19f 100644 --- a/tests/core/test_diff_scan_polling.py +++ b/tests/core/test_diff_scan_polling.py @@ -87,7 +87,8 @@ def test_duplicate_conflict_uses_cached_polling(core, diff_scan_get_response): artifacts = core.get_diff_scan_artifacts("head", "new") create_params = core.sdk.diffscans.create_from_ids.call_args.args[1] - assert "on_duplicate" not in create_params + # "redirect" is the unsafe value: its 302 is followed into an uncached GET. + assert create_params.get("on_duplicate") != "redirect" core.sdk.diffscans.list.assert_called_once_with( core.config.org_slug, params={ @@ -160,6 +161,7 @@ def test_eager_list_artifacts_do_not_bypass_filtered_get(core, diff_scan_get_res params={"cached": "true", "omit_unchanged": "true"}, ) + def test_diff_scan_is_associated_with_pull_request_url(core): external_href = "https://dev.azure.com/acme/platform/_git/widgets/pullrequest/17" @@ -167,6 +169,51 @@ def test_diff_scan_is_associated_with_pull_request_url(core): create_params = core.sdk.diffscans.create_from_ids.call_args.args[1] assert create_params["external_href"] == external_href + # Without this the link is dropped whenever the scan pair was compared before. + assert create_params["on_duplicate"] == "update" + + +def test_no_duplicate_handling_requested_without_a_pull_request_url(core): + """Runs with no PR context keep the plain 409-and-resolve path.""" + core.get_diff_scan_artifacts("head", "new") + + create_params = core.sdk.diffscans.create_from_ids.call_args.args[1] + assert "on_duplicate" not in create_params + assert "external_href" not in create_params + + +def test_updated_duplicate_is_polled_like_a_created_diff_scan(core, diff_scan_get_response): + """on_duplicate=update answers 200 with the create envelope, not a 409. + + The existing scan must then flow through the same cached-polling path, and + the duplicate-resolving list call must not be needed at all. + """ + core.sdk.diffscans.create_from_ids.return_value = { + "diff_scan": {"id": "existing-diff-scan"} + } + + artifacts = core.get_diff_scan_artifacts( + "head", "new", external_href="https://github.com/acme/widgets/pull/42" + ) + + core.sdk.diffscans.list.assert_not_called() + assert core.sdk.diffscans.get.call_args.args[1] == "existing-diff-scan" + assert len(artifacts.added) > 0 + + +def test_link_falls_back_to_resolving_the_duplicate_on_older_deployments(core): + """Deployments predating on_duplicate=update still answer 409; keep working.""" + core.sdk.diffscans.create_from_ids.side_effect = APIFailure( + "duplicate", status_code=409 + ) + core.sdk.diffscans.list.return_value = {"results": [{"id": "existing-diff-scan"}]} + + artifacts = core.get_diff_scan_artifacts( + "head", "new", external_href="https://github.com/acme/widgets/pull/42" + ) + + assert core.sdk.diffscans.get.call_args.args[1] == "existing-diff-scan" + assert len(artifacts.added) > 0 def test_fallback_to_streaming_diff_on_failure(core): From 54e4b2f856b592e068550b50caf2e5ee63b67f5e Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:09:43 -0400 Subject: [PATCH 06/27] fix(comments): make per-alert ignores round trip --- socketsecurity/core/messages.py | 6 ++--- socketsecurity/core/scm_comments.py | 32 +++++++++++++++---------- tests/unit/test_disable_ignore.py | 25 +++++++++++++++++++ tests/unit/test_pr_comment_rendering.py | 24 +++++++++++++++++++ 4 files changed, 72 insertions(+), 15 deletions(-) diff --git a/socketsecurity/core/messages.py b/socketsecurity/core/messages.py index d4e8c9e8..1d71642c 100644 --- a/socketsecurity/core/messages.py +++ b/socketsecurity/core/messages.py @@ -936,7 +936,7 @@ def security_comment_template(diff: Diff, config=None) -> str: > **Review the following alerts detected in dependencies.** > > According to your organization's policies, you **must** resolve all **"Block"** alerts before proceeding. It's recommended to resolve **"Warn"** alerts too. -> Learn more about [Socket for GitHub](https://socket.dev?utm_medium=gh). +> Learn more about [Socket](https://socket.dev). @@ -968,7 +968,7 @@ def security_comment_template(diff: Diff, config=None) -> str: # Generate a table row for each alert ignore_html = ( f"

Mark as acceptable risk: To ignore this alert only in this pull request, reply with:
" - f"@SocketSecurity ignore {alert.pkg_name}@{alert.pkg_version}
" + f"@SocketSecurity ignore {alert.pkg_type}/{alert.pkg_name}@{alert.pkg_version}
" f"Or ignore all future alerts with:
" f"@SocketSecurity ignore-all

" ) if show_ignore else "" @@ -1032,7 +1032,7 @@ def security_comment_template(diff: Diff, config=None) -> str: license_ignore_html = ( f"

Mark the package as acceptable risk: To ignore this alert only in this pull request, reply with the comment " - f"@SocketSecurity ignore {first_alert.pkg_name}@{first_alert.pkg_version}. " + f"@SocketSecurity ignore {first_alert.pkg_type}/{first_alert.pkg_name}@{first_alert.pkg_version}. " f"You can also ignore all packages with @SocketSecurity ignore-all. " f"To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

" ) if show_ignore else "" diff --git a/socketsecurity/core/scm_comments.py b/socketsecurity/core/scm_comments.py index 7c479b72..ea758ca1 100644 --- a/socketsecurity/core/scm_comments.py +++ b/socketsecurity/core/scm_comments.py @@ -37,10 +37,10 @@ def remove_alerts(comments: dict, new_alerts: list) -> list: if ignore_all: break else: - full_name = f"{alert.pkg_type}/{alert.pkg_name}" - purl = (full_name, alert.pkg_version) - purl_star = (full_name, "*") - if purl in ignore_commands or purl_star in ignore_commands: + if any( + Comments.is_ignore(alert.pkg_name, alert.pkg_version, name, version, alert.pkg_type) + for name, version in ignore_commands + ): log.info(f"Alerts for {alert.pkg_name}@{alert.pkg_version} ignored") else: log.info(f"Adding alert {alert.type} for {alert.pkg_name}@{alert.pkg_version}") @@ -66,8 +66,10 @@ def get_ignore_options(comments: dict) -> [bool, list]: ignore_all = True else: command = command.lstrip("ignore").strip() - name, version = command.split("@") - data = (name, version) + name, separator, version = command.rpartition("@") + if not separator or not name or not version: + raise ValueError("Expected package@version") + data = (name.strip(), version.strip()) ignore_commands.append(data) except Exception as error: log.error(f"Unable to process ignore command for {comment}") @@ -75,11 +77,17 @@ def get_ignore_options(comments: dict) -> [bool, list]: return ignore_all, ignore_commands @staticmethod - def is_ignore(pkg_name: str, pkg_version: str, name: str, version: str) -> bool: - result = False - if pkg_name == name and (pkg_version == version or version == "*"): - result = True - return result + def is_ignore( + pkg_name: str, pkg_version: str, name: str, version: str, + pkg_type: str = "" + ) -> bool: + package_names = {pkg_name} + if pkg_type: + package_names.add(f"{pkg_type}/{pkg_name}") + target_names = {name} + if not pkg_type and "/" in name: + target_names.add(name.split("/", 1)[1]) + return bool(package_names & target_names) and (pkg_version == version or version == "*") @staticmethod def is_heading_line(line) -> bool: @@ -187,7 +195,7 @@ def process_updated_security_comment( # Extract package name and version from the comment try: start_marker = stripped[len("" in body assert "" in body + def test_copy_is_provider_neutral(self): + body = Messages.security_comment_template( + _make_diff([_make_alert()]), _FakeConfig(scm="gitlab") + ) + assert "Socket for GitHub" not in body + assert "Learn more about [Socket]" in body + class TestSecurityCommentTemplateWithNoAlerts: def test_no_alerts_omits_the_empty_table(self): @@ -232,6 +239,23 @@ def test_ignoring_every_alert_individually_collapses_too(self): assert "No dependency alerts to report" in new_body + def test_qualified_scoped_package_ignore_matches_comment_marker(self): + security = _security_comment_with([ + _make_alert( + pkg_name="@socketsecurity/example", + purl="pkg:npm/@socketsecurity/example@4.17.21", + ) + ]) + comments = { + "security": security, + "ignore": [_make_comment( + "SocketSecurity ignore npm/@socketsecurity/example@4.17.21", + comment_id=2, + )], + } + + assert "No dependency alerts to report" in Comments.process_security_comment(security, comments) + def test_no_ignore_commands_leaves_alerts_in_place(self): security = self._two_alert_comment() comments = {"security": security, "ignore": []} From 90462b7cbb7bb533697c99d4ffd591b0096dd4c8 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:11:12 -0400 Subject: [PATCH 07/27] fix(comments): preserve dependency change types --- socketsecurity/core/__init__.py | 46 ++++++------ socketsecurity/core/alert_selection.py | 2 + socketsecurity/core/classes.py | 8 +++ socketsecurity/core/messages.py | 97 ++++++++++++++------------ socketsecurity/socketcli.py | 9 ++- tests/core/test_diff_generation.py | 13 ++++ tests/unit/test_dependency_overview.py | 18 +++++ 7 files changed, 126 insertions(+), 67 deletions(-) diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index a9728c72..51c622c3 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -2214,16 +2214,22 @@ def create_diff_report( alerts_in_removed_packages: Dict[str, List[Issue]] = {} alerts_in_unchanged_packages: Dict[str, List[Issue]] = {} - seen_new_packages = set() - seen_removed_packages = set() + seen_packages = { + "added": set(), + "updated": set(), + "removed": set(), + "replaced": set(), + } for package_id, package in added_packages.items(): purl = self.create_purl(package_id, added_packages) base_purl = f"{purl.ecosystem}/{purl.name}@{purl.version}" - if (not direct_only or package.direct) and base_purl not in seen_new_packages: - diff.new_packages.append(purl) - seen_new_packages.add(base_purl) + change_type = "updated" if package.diffType == "updated" else "added" + target = diff.updated_packages if change_type == "updated" else diff.new_packages + if (not direct_only or package.direct) and base_purl not in seen_packages[change_type]: + target.append(purl) + seen_packages[change_type].add(base_purl) self.add_package_alerts_to_collection( package=package, @@ -2235,9 +2241,11 @@ def create_diff_report( purl = self.create_purl(package_id, removed_packages) base_purl = f"{purl.ecosystem}/{purl.name}@{purl.version}" - if (not direct_only or package.direct) and base_purl not in seen_removed_packages: - diff.removed_packages.append(purl) - seen_removed_packages.add(base_purl) + change_type = "replaced" if package.diffType == "replaced" else "removed" + target = diff.replaced_packages if change_type == "replaced" else diff.removed_packages + if (not direct_only or package.direct) and base_purl not in seen_packages[change_type]: + target.append(purl) + seen_packages[change_type].add(base_purl) self.add_package_alerts_to_collection( package=package, @@ -2367,18 +2375,16 @@ def add_purl_capabilities(diff: Diff) -> None: Args: diff: Diff object to update with capability information """ - new_packages = [] - for purl in diff.new_packages: - if purl.id in diff.new_capabilities: - new_purl = Purl( - **{**purl.__dict__, - "capabilities": diff.new_capabilities[purl.id]} - ) - new_packages.append(new_purl) - else: - new_packages.append(purl) - - diff.new_packages = new_packages + for attribute in ("new_packages", "updated_packages"): + packages = [] + for purl in getattr(diff, attribute): + if purl.id in diff.new_capabilities: + purl = Purl( + **{**purl.__dict__, + "capabilities": diff.new_capabilities[purl.id]} + ) + packages.append(purl) + setattr(diff, attribute, packages) def add_package_alerts_to_collection(self, package: Package, alerts_collection: dict, packages: dict) -> dict: """ diff --git a/socketsecurity/core/alert_selection.py b/socketsecurity/core/alert_selection.py index ae5b4772..132be294 100644 --- a/socketsecurity/core/alert_selection.py +++ b/socketsecurity/core/alert_selection.py @@ -31,7 +31,9 @@ def clone_diff_with_selected_alerts(diff: Diff, selected_alerts: List[Issue]) -> removed_alerts=[], diff_url=getattr(diff, "diff_url", ""), new_packages=getattr(diff, "new_packages", []), + updated_packages=getattr(diff, "updated_packages", []), removed_packages=getattr(diff, "removed_packages", []), + replaced_packages=getattr(diff, "replaced_packages", []), packages=getattr(diff, "packages", {}), ) selected_diff.id = getattr(diff, "id", "") diff --git a/socketsecurity/core/classes.py b/socketsecurity/core/classes.py index db145221..d821d872 100644 --- a/socketsecurity/core/classes.py +++ b/socketsecurity/core/classes.py @@ -507,7 +507,9 @@ class Diff: """ new_packages: list[Purl] + updated_packages: list[Purl] removed_packages: list[Purl] + replaced_packages: list[Purl] packages: dict[str, Package] new_capabilities: Dict[str, List[str]] new_alerts: list[Issue] @@ -525,8 +527,12 @@ def __init__(self, **kwargs): setattr(self, key, value) if not hasattr(self, "new_packages"): self.new_packages = [] + if not hasattr(self, "updated_packages"): + self.updated_packages = [] if not hasattr(self, "removed_packages"): self.removed_packages = [] + if not hasattr(self, "replaced_packages"): + self.replaced_packages = [] if not hasattr(self, "new_alerts"): self.new_alerts = [] if not hasattr(self, "unchanged_alerts"): @@ -548,8 +554,10 @@ def to_dict(self) -> dict: """ return { "new_packages": [p.to_dict() for p in self.new_packages], + "updated_packages": [p.to_dict() for p in self.updated_packages], "new_capabilities": self.new_capabilities, "removed_packages": [p.to_dict() for p in self.removed_packages], + "replaced_packages": [p.to_dict() for p in self.replaced_packages], "new_alerts": [alert.__dict__ for alert in self.new_alerts], "unchanged_alerts": [alert.__dict__ for alert in self.unchanged_alerts] if hasattr(self, "unchanged_alerts") else [], "removed_alerts": [alert.__dict__ for alert in self.removed_alerts] if hasattr(self, "removed_alerts") else [], diff --git a/socketsecurity/core/messages.py b/socketsecurity/core/messages.py index 1d71642c..76047689 100644 --- a/socketsecurity/core/messages.py +++ b/socketsecurity/core/messages.py @@ -1268,51 +1268,58 @@ def create_added_table(diff: Diff, md: MdUtils) -> MdUtils: num_of_overview_columns = len(overview_table) count = 0 - for added in diff.new_packages: - added: Purl # Ensure `added` has scores and relevant attributes. - - package_url = f"[{added.purl}]({added.url})" - diff_badge = f"[![+](https://github-app-statics.socket.dev/diff-added.svg)]({added.url})" - - # Scores dynamically converted to badge URLs and linked - def score_to_badge(score): - score_percent = int(score * 100) # Convert to integer percentage - return f"[![{score_percent}](https://github-app-statics.socket.dev/score-{score_percent}.svg)]({added.url})" - - def get_score_for_badge(score_name: str) -> float: - scores = getattr(added, "scores", None) - if isinstance(scores, dict): - raw_score = scores.get(score_name) - else: - raw_score = getattr(scores, score_name, None) if scores is not None else None - - if raw_score is None: - return 1.0 - - score = float(raw_score) - if score > 1: - score = score / 100 - return max(0.0, min(score, 1.0)) - - # Generate badges for each score type - supply_chain_risk_badge = score_to_badge(get_score_for_badge("supplyChain")) - vulnerability_badge = score_to_badge(get_score_for_badge("vulnerability")) - quality_badge = score_to_badge(get_score_for_badge("quality")) - maintenance_badge = score_to_badge(get_score_for_badge("maintenance")) - license_badge = score_to_badge(get_score_for_badge("license")) - - # Add the row for this package - row = [ - diff_badge, - package_url, - supply_chain_risk_badge, - vulnerability_badge, - quality_badge, - maintenance_badge, - license_badge - ] - overview_table.extend(row) - count += 1 # Count total packages + changes = ( + ("Added", diff.new_packages), + ("Updated", diff.updated_packages), + ("Removed", diff.removed_packages), + ("Replaced", diff.replaced_packages), + ) + for change, packages in changes: + for package in packages: + package: Purl + + package_url = f"[{package.purl}]({package.url})" + diff_badge = f"**{change}**" + + # Scores dynamically converted to badge URLs and linked + def score_to_badge(score): + score_percent = int(score * 100) # Convert to integer percentage + return f"[![{score_percent}](https://github-app-statics.socket.dev/score-{score_percent}.svg)]({package.url})" + + def get_score_for_badge(score_name: str) -> float: + scores = getattr(package, "scores", None) + if isinstance(scores, dict): + raw_score = scores.get(score_name) + else: + raw_score = getattr(scores, score_name, None) if scores is not None else None + + if raw_score is None: + return 1.0 + + score = float(raw_score) + if score > 1: + score = score / 100 + return max(0.0, min(score, 1.0)) + + # Generate badges for each score type + supply_chain_risk_badge = score_to_badge(get_score_for_badge("supplyChain")) + vulnerability_badge = score_to_badge(get_score_for_badge("vulnerability")) + quality_badge = score_to_badge(get_score_for_badge("quality")) + maintenance_badge = score_to_badge(get_score_for_badge("maintenance")) + license_badge = score_to_badge(get_score_for_badge("license")) + + # Add the row for this package + row = [ + diff_badge, + package_url, + supply_chain_risk_badge, + vulnerability_badge, + quality_badge, + maintenance_badge, + license_badge + ] + overview_table.extend(row) + count += 1 # Calculate total rows for table num_of_overview_rows = count + 1 # Include header row diff --git a/socketsecurity/socketcli.py b/socketsecurity/socketcli.py index aafed013..a0353078 100644 --- a/socketsecurity/socketcli.py +++ b/socketsecurity/socketcli.py @@ -854,10 +854,15 @@ def _is_unprocessed(c): if not new_security_comment: log.debug("Security issue comment disabled, or no alerts and none to update") - # FIXME: diff.new_packages is never populated, neither is removed_packages + has_dependency_changes = any(( + diff.new_packages, + diff.updated_packages, + diff.removed_packages, + diff.replaced_packages, + )) new_overview_comment = should_write_comment( config.disable_overview, - len(diff.new_packages) > 0, + has_dependency_changes, update_old_overview_comment, ) if not new_overview_comment: diff --git a/tests/core/test_diff_generation.py b/tests/core/test_diff_generation.py index 477fd6bd..63dfb4af 100644 --- a/tests/core/test_diff_generation.py +++ b/tests/core/test_diff_generation.py @@ -95,6 +95,19 @@ def test_create_diff_report(core, diff_input): assert hasattr(dp3_purl, "capabilities") assert "Environment Variables" in dp3_purl.capabilities + +def test_create_diff_report_preserves_package_change_types(core, diff_input): + added, removed = diff_input + added["dp3"].diffType = "updated" + removed["dp2"].diffType = "replaced" + + diff = core.create_diff_report(added, removed) + + assert {package.id for package in diff.new_packages} == {"dp4"} + assert {package.id for package in diff.updated_packages} == {"dp3"} + assert diff.removed_packages == [] + assert {package.id for package in diff.replaced_packages} == {"dp2"} + def create_input(core): # Get two different scans to compare head_scan = core.get_full_scan("head") diff --git a/tests/unit/test_dependency_overview.py b/tests/unit/test_dependency_overview.py index 3afb8051..709fd659 100644 --- a/tests/unit/test_dependency_overview.py +++ b/tests/unit/test_dependency_overview.py @@ -65,3 +65,21 @@ def test_dependency_overview_template_defaults_missing_or_null_scores(tmp_path, assert "score-42.svg" in comment assert "score-100.svg" in comment assert "score-10000.svg" not in comment + + +def test_dependency_overview_labels_each_change_type(tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + diff = Diff( + id="test-diff", + diff_url="https://socket.dev/test-diff", + new_packages=[_make_purl("added", {})], + updated_packages=[_make_purl("updated", {})], + removed_packages=[_make_purl("removed", {})], + replaced_packages=[_make_purl("replaced", {})], + new_alerts=[], + ) + + comment = Messages.dependency_overview_template(diff) + + for change in ("Added", "Updated", "Removed", "Replaced"): + assert f"**{change}**" in comment From 004cae47f399486aa2847c981e57087ccf99e6ad Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:12:11 -0400 Subject: [PATCH 08/27] fix(ci): use full scans outside pull requests --- socketsecurity/socketcli.py | 22 +++++++++++++++++----- tests/unit/test_socketcli.py | 14 ++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/socketsecurity/socketcli.py b/socketsecurity/socketcli.py index a0353078..10c4795f 100644 --- a/socketsecurity/socketcli.py +++ b/socketsecurity/socketcli.py @@ -139,6 +139,14 @@ def _select_pull_request_provider(integration_type: str, scm_type: str) -> str: return scm_type if scm_type in ("github", "gitlab") else integration_type +def _should_create_scm_diff( + event_type: str, + enable_diff: bool = False, + force_diff_mode: bool = False, +) -> bool: + return event_type == "diff" or enable_diff or force_diff_mode + + def build_socket_sdk(config: CliConfig) -> socketdev: cli_user_agent_string = f"SocketPythonCLI/{config.version}" return socketdev( @@ -684,7 +692,8 @@ def _is_unprocessed(c): return False return True - if scm is not None and scm.check_event_type() == "comment": + scm_event_type = scm.check_event_type() if scm is not None else None + if scm_event_type == "comment": # FIXME: This entire flow should be a separate command called "filter_ignored_alerts_in_comments" # It's not related to scanning or diff generation - it just: # 1. Triggers on comments in GitHub/GitLab @@ -738,9 +747,13 @@ def _is_unprocessed(c): else: log.info("Ignore commands disabled (--disable-ignore), skipping comment processing") - elif scm is not None and scm.check_event_type() != "comment" and not force_api_mode: + elif scm is not None and not force_api_mode: log.info("Push initiated flow") - if scm.check_event_type() == "diff": + if _should_create_scm_diff( + scm_event_type, + enable_diff=config.enable_diff, + force_diff_mode=force_diff_mode, + ): log.info("Starting comment logic for PR/MR event") diff = core.create_new_diff( scan_paths, @@ -878,7 +891,7 @@ def _is_unprocessed(c): ) else: log.info("Starting non-PR/MR flow") - diff = core.create_new_diff( + diff = core.create_full_scan_with_report_url( scan_paths, params, no_change=should_skip_scan, @@ -886,7 +899,6 @@ def _is_unprocessed(c): save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, explicit_files=scan_explicit_files, - external_href=pr_context.url, ) output_handler.handle_output(diff) diff --git a/tests/unit/test_socketcli.py b/tests/unit/test_socketcli.py index 103cfff8..3d6b8a38 100644 --- a/tests/unit/test_socketcli.py +++ b/tests/unit/test_socketcli.py @@ -74,6 +74,20 @@ def test_pr_context_provider_uses_integration_without_comment_adapter(): assert socketcli._select_pull_request_provider("azure", "api") == "azure" +def test_scm_merge_request_event_creates_diff(): + assert socketcli._should_create_scm_diff("diff") is True + + +def test_scm_branch_event_defaults_to_full_scan(): + assert socketcli._should_create_scm_diff("main") is False + + +@pytest.mark.parametrize("override", ["enable_diff", "force_diff_mode"]) +def test_scm_branch_event_honors_diff_override(override): + options = {override: True} + assert socketcli._should_create_scm_diff("main", **options) is True + + # --------------------------------------------------------------------------- # Buildkite-aware infrastructure error formatting. # --------------------------------------------------------------------------- From 167554fe39c4011b778b81deb27d02986c0403a9 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:14:00 -0400 Subject: [PATCH 09/27] docs: update release notes for comment fixes --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e24b9d0f..89e1b336 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,17 @@ external link, allowing Dashboard reports to retain their CI change context. Re-running a comparison over an already-compared scan pair now applies the link to the existing diff scan instead of leaving that report unassociated. +- GitHub and GitLab branch pipelines now create full scans by default. Explicit + diff flags continue to opt non-PR runs into comparison mode. + +### Fixed: pull request and merge request comment accuracy + +- Per-alert ignore instructions now use ecosystem-qualified package names and + accept scoped packages while remaining compatible with older bare-name replies. +- Dependency overviews preserve added, updated, removed, and replaced package + classifications instead of presenting updates as new dependencies. +- Shared security comment copy no longer describes GitLab merge request output + as Socket for GitHub. ## 2.8.1 From 95483f35c7f834267e6ce28c6e02026abc7b0275 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Thu, 3 Sep 2026 18:46:18 -0400 Subject: [PATCH 10/27] fix(ci): restrict SCM diffs to pull requests --- socketsecurity/socketcli.py | 14 +++----------- tests/unit/test_socketcli.py | 8 +------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/socketsecurity/socketcli.py b/socketsecurity/socketcli.py index 10c4795f..350bb7ae 100644 --- a/socketsecurity/socketcli.py +++ b/socketsecurity/socketcli.py @@ -139,12 +139,8 @@ def _select_pull_request_provider(integration_type: str, scm_type: str) -> str: return scm_type if scm_type in ("github", "gitlab") else integration_type -def _should_create_scm_diff( - event_type: str, - enable_diff: bool = False, - force_diff_mode: bool = False, -) -> bool: - return event_type == "diff" or enable_diff or force_diff_mode +def _should_create_scm_diff(event_type: str) -> bool: + return event_type == "diff" def build_socket_sdk(config: CliConfig) -> socketdev: @@ -749,11 +745,7 @@ def _is_unprocessed(c): elif scm is not None and not force_api_mode: log.info("Push initiated flow") - if _should_create_scm_diff( - scm_event_type, - enable_diff=config.enable_diff, - force_diff_mode=force_diff_mode, - ): + if _should_create_scm_diff(scm_event_type): log.info("Starting comment logic for PR/MR event") diff = core.create_new_diff( scan_paths, diff --git a/tests/unit/test_socketcli.py b/tests/unit/test_socketcli.py index 3d6b8a38..6e0e42e8 100644 --- a/tests/unit/test_socketcli.py +++ b/tests/unit/test_socketcli.py @@ -78,16 +78,10 @@ def test_scm_merge_request_event_creates_diff(): assert socketcli._should_create_scm_diff("diff") is True -def test_scm_branch_event_defaults_to_full_scan(): +def test_scm_branch_event_always_uses_full_scan(): assert socketcli._should_create_scm_diff("main") is False -@pytest.mark.parametrize("override", ["enable_diff", "force_diff_mode"]) -def test_scm_branch_event_honors_diff_override(override): - options = {override: True} - assert socketcli._should_create_scm_diff("main", **options) is True - - # --------------------------------------------------------------------------- # Buildkite-aware infrastructure error formatting. # --------------------------------------------------------------------------- From 8f9e4029c26ac4621ebd631c42a2bdebddf6a133 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:08:13 -0400 Subject: [PATCH 11/27] fix(scans): keep the package list on full scans create_full_scan_with_report_url only fetched SBOM data when an alert-bearing output format was enabled, so --generate-license and --legal-format fossa saw an empty diff.packages and wrote an attribution file with zero packages. That is the list they enumerate, as _requires_unchanged_artifacts already documents for the comparison path. Fetch the SBOM for them too, and enrich it through the PURL endpoint the way the comparison path does. The full scan's package map is keyed by artifact id while get_license_text_via_purl keys off ecosystem/name@version, so pass a purl-keyed view over the same Package objects. Alert consolidation stays behind its own gate, so an alert-only run does not pay for the license lookup and a license-only run does not build an alert list. Co-Authored-By: Claude Opus 5 (1M context) --- socketsecurity/core/__init__.py | 62 +++++++++++++++----- tests/core/test_full_scan_outputs.py | 87 ++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 16 deletions(-) create mode 100644 tests/core/test_full_scan_outputs.py diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index 51c622c3..597818f6 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -1288,30 +1288,42 @@ def create_full_scan_with_report_url( or self.cli_config.enable_sarif ) ) + # --generate-license (and --legal-format fossa, which it gates) enumerates + # diff.packages rather than the alert list, so a full scan has to carry the + # package map even when no alert-bearing output format is enabled. Without + # this, an SCM branch pipeline writes an attribution file with zero packages. + # Keep in sync with _requires_unchanged_artifacts, which lists the same + # consumers for the comparison path. + needs_license_artifacts = ( + self.cli_config is not None and self.cli_config.generate_license + ) - if needs_alerts: - log.info("Output format requires alerts, fetching SBOM data for full scan") + if needs_alerts or needs_license_artifacts: + log.info("Output format requires SBOM data, fetching it for the full scan") sbom_start = time.time() sbom_artifacts_dict = self.get_sbom_data(new_full_scan.id) sbom_artifacts = self.get_sbom_data_list(sbom_artifacts_dict) packages = self._create_packages_dict_without_license_text(sbom_artifacts) + if needs_license_artifacts: + packages = self._add_license_details(packages) diff.packages = packages - all_alerts_collection: Dict[str, List[Issue]] = {} - for package_id, package in packages.items(): - self.add_package_alerts_to_collection( - package=package, - alerts_collection=all_alerts_collection, - packages=packages - ) + if needs_alerts: + all_alerts_collection: Dict[str, List[Issue]] = {} + for package_id, package in packages.items(): + self.add_package_alerts_to_collection( + package=package, + alerts_collection=all_alerts_collection, + packages=packages + ) - consolidated: Set[str] = set() - for alert_key, alerts in all_alerts_collection.items(): - for alert in alerts: - alert_str = f"{alert.purl},{alert.type}" - if (alert.error or alert.warn) and alert_str not in consolidated: - diff.new_alerts.append(alert) - consolidated.add(alert_str) + consolidated: Set[str] = set() + for alert_key, alerts in all_alerts_collection.items(): + for alert in alerts: + alert_str = f"{alert.purl},{alert.type}" + if (alert.error or alert.warn) and alert_str not in consolidated: + diff.new_alerts.append(alert) + consolidated.add(alert_str) sbom_end = time.time() log.info( @@ -1323,6 +1335,24 @@ def create_full_scan_with_report_url( return diff + def _add_license_details(self, packages: dict[str, Package]) -> dict[str, Package]: + """Populate licenseAttrib/licenseDetails on a full scan's package map. + + get_license_text_via_purl keys off ``ecosystem/name@version`` because that is + what the PURL endpoint echoes back, while a full scan's package map is keyed + by artifact id. Build a purl-keyed view over the same Package objects so the + enrichment lands on the map the caller keeps. + """ + batch_size = self.cli_config.max_purl_batch_size if self.cli_config else 5000 + self.get_license_text_via_purl( + { + f"{package.type}/{package.name}@{package.version}": package + for package in packages.values() + }, + batch_size=batch_size, + ) + return packages + def get_full_scan(self, full_scan_id: str) -> FullScan: """ Get a FullScan object for an existing full scan including sbom_artifacts and packages. diff --git a/tests/core/test_full_scan_outputs.py b/tests/core/test_full_scan_outputs.py new file mode 100644 index 00000000..e46a0b41 --- /dev/null +++ b/tests/core/test_full_scan_outputs.py @@ -0,0 +1,87 @@ +"""What a full scan has to carry for each enabled output. + +create_full_scan_with_report_url runs on every path with no baseline to compare +against: API mode, and (since 2.8.0) SCM branch pipelines. Fetching the SBOM is +the expensive part, so it is gated on the enabled outputs -- these pin which +outputs need it. +""" +import pytest +from socketdev.fullscans import FullScanParams + +from socketsecurity.config import CliConfig +from socketsecurity.core import Core +from socketsecurity.core.socket_config import SocketConfig + + +def _core(sdk, **cli_overrides): + config = CliConfig.from_args(["--api-token", "test"]) + for key, value in cli_overrides.items(): + setattr(config, key, value) + return Core(config=SocketConfig(api_key="test_key"), sdk=sdk, cli_config=config) + + +@pytest.fixture +def params(): + return FullScanParams(org_slug="test-org", repo="test", branch="main") + + +@pytest.fixture +def sdk(mock_sdk_with_responses): + # get_license_text_via_purl iterates the response; the shared fixture leaves + # purl.post as a bare MagicMock. + mock_sdk_with_responses.purl.post.return_value = [] + return mock_sdk_with_responses + + +def test_license_generation_gets_the_package_list(sdk, params): + """--generate-license enumerates diff.packages, not diff.new_alerts. + + Without this the attribution file for an SCM branch pipeline comes out empty. + """ + core = _core(sdk, generate_license=True) + + diff = core.create_full_scan_with_report_url( + ["."], params, explicit_files=["package.json"] + ) + + assert diff.packages + # No alert-bearing output format is enabled, so alerts stay unfetched. + assert diff.new_alerts == [] + + +def test_license_details_are_requested_for_the_scanned_packages(sdk, params): + core = _core(sdk, generate_license=True) + + core.create_full_scan_with_report_url( + ["."], params, explicit_files=["package.json"] + ) + + components = sdk.purl.post.call_args.kwargs["components"] + # Keyed the way the PURL endpoint echoes results back, not by artifact id. + assert all(component["purl"].startswith("pkg:/") for component in components) + assert any("@" in component["purl"] for component in components) + + +def test_alert_formats_still_fetch_the_sbom(sdk, params): + core = _core(sdk, enable_json=True) + + diff = core.create_full_scan_with_report_url( + ["."], params, explicit_files=["package.json"] + ) + + assert diff.packages + # Alert-only outputs do not pay for the license lookup. (The scan fixture's + # alerts carry no action, so none of them consolidate into new_alerts.) + sdk.purl.post.assert_not_called() + + +def test_console_only_run_skips_the_sbom_fetch(sdk, params): + core = _core(sdk) + + diff = core.create_full_scan_with_report_url( + ["."], params, explicit_files=["package.json"] + ) + + assert diff.packages == {} + assert diff.new_alerts == [] + sdk.fullscans.stream.assert_not_called() From 9e9bc58fb67b445f164903f4cae7f880d1c2640b Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:08:23 -0400 Subject: [PATCH 12/27] fix(ci): keep branch pipelines out of pull request handling Two ways an SCM branch build could still be treated like a pull request: Buildkite always sets BUILDKITE_PULL_REQUEST, to the string "false" on a branch build, so the documented --pr-number "$BUILDKITE_PULL_REQUEST" form delivers a truthy non-numeric value. resolve_pull_request_context read it as no PR but only wrote the normalized number back when one was found, so GithubConfig still saw "false", check_event_type returned "diff" for a push, and comment lookups went to issues/false/comments. Canonicalize config.pr_number before any adapter reads it. A branch run creating a full scan then blocked on diff.new_alerts, which a full scan cannot fill meaningfully: empty with no alert-bearing output format enabled, and every alert in the scan rather than the newly introduced ones with one. The exit code therefore depended on which output format was requested. Treat these runs the way a run with no supported manifest files is already treated and skip blocking, leaving pull request pipelines to enforce policy. Move the scan-type decision into create_scm_scan, which returns the diff and whether it came from a comparison, so the branch is exercised by tests rather than only its predicate. Document both the scan-type table and the blocking consequence in the CI/CD guide. Co-Authored-By: Claude Opus 5 (1M context) --- docs/ci-cd.md | 26 +++++- socketsecurity/core/pull_request.py | 21 +++-- socketsecurity/socketcli.py | 110 +++++++++++++++++------- tests/unit/test_pull_request_context.py | 23 ++++- tests/unit/test_socketcli.py | 87 ++++++++++++++++++- 5 files changed, 224 insertions(+), 43 deletions(-) diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 57b34f6e..3c819b13 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -455,12 +455,36 @@ pipelines: - socketcli --config .socketcli.toml --target-path . ``` +## Scan type by pipeline + +With `--scm github` or `--scm gitlab`, the detected event decides the scan type: + +| Event | Scan | Blocks the build | +|:------|:-----|:-----------------| +| Pull request / merge request | Diff scan against the repository's baseline | Yes, on newly introduced alerts | +| Any other pipeline, including default-branch pushes | Full scan | No | + +A full scan has no baseline, so it cannot tell a newly introduced alert from one +that was already there. Rather than block on a number that would mean something +different depending on which output format was enabled, those runs behave as if +`--disable-blocking` was supplied and report through the Dashboard instead. This +matches how the CLI already treats a run with no supported manifest files. + +The event type is authoritative once `--scm` is set: `--enable-diff` and +`--ignore-commit-files` do not turn a branch pipeline into a comparison. To diff +a branch build, drop `--scm` and use `--enable-diff` with `--integration`, which +runs the comparison without the PR comment adapter. + +`--generate-license` and `--legal-format fossa` work on both paths; a full scan +fetches the package list for them. + ## Pull request and Dashboard association The CLI sends the resolved pull request number with each full scan and attaches the pull request URL to diff scans so the Socket Dashboard can associate the report with its originating change. If `--pr-number` is supplied, it wins; -passing `--pr-number 0` explicitly disables automatic association. +passing `--pr-number 0` explicitly disables automatic association. Any value that +is not a positive integer, including Buildkite's `false`, means no pull request. Without an explicit value, the CLI recognizes: diff --git a/socketsecurity/core/pull_request.py b/socketsecurity/core/pull_request.py index 60ad3965..e38212d7 100644 --- a/socketsecurity/core/pull_request.py +++ b/socketsecurity/core/pull_request.py @@ -12,7 +12,14 @@ class PullRequestContext: url: Optional[str] = None -def _positive_int(value) -> int: +def parse_pull_request_number(value) -> int: + """Coerce a configured or CI-supplied pull request number to a positive int. + + Anything that is not a positive integer means "no pull request", including the + literal ``false`` that Buildkite puts in ``BUILDKITE_PULL_REQUEST`` on non-PR + builds. Callers that hand the value on to a comment adapter should store this + result rather than the raw string, which is truthy. + """ try: parsed = int(value) except (TypeError, ValueError): @@ -31,11 +38,11 @@ def _repository_url(value: Optional[str]) -> Optional[str]: def _github_number(env: Mapping[str, str]) -> int: - number = _positive_int(env.get("PR_NUMBER")) + number = parse_pull_request_number(env.get("PR_NUMBER")) if number: return number match = re.match(r"^refs/pull/(\d+)/", env.get("GITHUB_REF", "")) - return _positive_int(match.group(1)) if match else 0 + return parse_pull_request_number(match.group(1)) if match else 0 def _github_url(number: int, repo: Optional[str], env: Mapping[str, str]) -> Optional[str]: @@ -89,17 +96,17 @@ def resolve_pull_request_context( """ environment = env or {} provider = str(integration_type or "api").lower() - number = _positive_int(configured_number) + number = parse_pull_request_number(configured_number) if not configured_explicit and not number: if provider == "github": number = _github_number(environment) elif provider == "gitlab": - number = _positive_int(environment.get("CI_MERGE_REQUEST_IID")) + number = parse_pull_request_number(environment.get("CI_MERGE_REQUEST_IID")) elif provider == "azure": number = ( - _positive_int(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTNUMBER")) or - _positive_int(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTID")) + parse_pull_request_number(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTNUMBER")) or + parse_pull_request_number(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTID")) ) if not number: diff --git a/socketsecurity/socketcli.py b/socketsecurity/socketcli.py index 350bb7ae..0509fa3e 100644 --- a/socketsecurity/socketcli.py +++ b/socketsecurity/socketcli.py @@ -4,6 +4,7 @@ import sys import traceback from datetime import datetime, timezone +from typing import List, Optional, Tuple from uuid import uuid4 from dotenv import load_dotenv @@ -18,7 +19,10 @@ from socketsecurity.core.git_interface import Git from socketsecurity.core.logging import initialize_logging, set_debug_mode from socketsecurity.core.messages import Messages -from socketsecurity.core.pull_request import resolve_pull_request_context +from socketsecurity.core.pull_request import ( + parse_pull_request_number, + resolve_pull_request_context, +) from socketsecurity.core.scm_comments import Comments from socketsecurity.core.socket_config import SocketConfig, module_folder_dirs from socketsecurity.core.streaming import StreamingLogs @@ -139,8 +143,47 @@ def _select_pull_request_provider(integration_type: str, scm_type: str) -> str: return scm_type if scm_type in ("github", "gitlab") else integration_type -def _should_create_scm_diff(event_type: str) -> bool: - return event_type == "diff" +def create_scm_scan( + core: Core, + config: CliConfig, + scm_event_type: Optional[str], + *, + scan_paths: List[str], + params: FullScanParams, + no_change: bool, + base_paths: Optional[List[str]], + explicit_files: Optional[List[str]], + external_href: Optional[str], +) -> Tuple[Diff, bool]: + """Create the scan for an SCM-integrated run. + + Only a pull request or merge request event has a baseline to compare against, + so every other pipeline -- default-branch pushes included -- gets a full scan. + The detected event type is authoritative: API-only diff flags cannot turn an + ordinary branch pipeline into a comparison. + + Returns the diff and whether it came from a comparison. Callers need the second + value because a full scan carries no "new alerts" category to comment on or to + block a build with. + """ + scan_kwargs = { + "no_change": no_change, + "save_files_list_path": config.save_submitted_files_list, + "save_manifest_tar_path": config.save_manifest_tar, + "base_paths": base_paths, + "explicit_files": explicit_files, + } + if scm_event_type == "diff": + log.info("Starting comment logic for PR/MR event") + diff = core.create_new_diff( + scan_paths, params, external_href=external_href, **scan_kwargs + ) + return diff, True + + log.info("Starting non-PR/MR flow") + # No before/after pair here, so there is nothing for external_href to hang off. + diff = core.create_full_scan_with_report_url(scan_paths, params, **scan_kwargs) + return diff, False def build_socket_sdk(config: CliConfig) -> socketdev: @@ -507,6 +550,13 @@ def main_code(): log.info("Continuing with normal scan flow...") + # Canonicalize before any adapter reads it. Buildkite always sets + # BUILDKITE_PULL_REQUEST -- to the string "false" on non-PR builds -- so the + # documented --pr-number "$BUILDKITE_PULL_REQUEST" form delivers a truthy + # non-numeric value that GithubConfig would otherwise treat as a real PR, + # making a branch build look like a pull request event. + config.pr_number = str(parse_pull_request_number(config.pr_number)) + scm = None if config.scm == "github": from socketsecurity.core.scm.github import Github, GithubConfig @@ -689,6 +739,10 @@ def _is_unprocessed(c): return True scm_event_type = scm.check_event_type() if scm is not None else None + # Every branch below except the SCM full-scan one produces a comparison, or + # is already covered by force_api_mode. See the blocking guard after the + # scan for why this is tracked. + comparison_ran = True if scm_event_type == "comment": # FIXME: This entire flow should be a separate command called "filter_ignored_alerts_in_comments" # It's not related to scanning or diff generation - it just: @@ -745,18 +799,18 @@ def _is_unprocessed(c): elif scm is not None and not force_api_mode: log.info("Push initiated flow") - if _should_create_scm_diff(scm_event_type): - log.info("Starting comment logic for PR/MR event") - diff = core.create_new_diff( - scan_paths, - params, - no_change=should_skip_scan, - save_files_list_path=config.save_submitted_files_list, - save_manifest_tar_path=config.save_manifest_tar, - base_paths=base_paths, - explicit_files=scan_explicit_files, - external_href=pr_context.url, - ) + diff, comparison_ran = create_scm_scan( + core, + config, + scm_event_type, + scan_paths=scan_paths, + params=params, + no_change=should_skip_scan, + base_paths=base_paths, + explicit_files=scan_explicit_files, + external_href=pr_context.url, + ) + if comparison_ran: comments = scm.get_comments_for_pr() # FIXME: this overwrites diff.new_alerts, which was previously populated by Core.create_issue_alerts @@ -881,17 +935,6 @@ def _is_unprocessed(c): new_security_comment, new_overview_comment ) - else: - log.info("Starting non-PR/MR flow") - diff = core.create_full_scan_with_report_url( - scan_paths, - params, - no_change=should_skip_scan, - save_files_list_path=config.save_submitted_files_list, - save_manifest_tar_path=config.save_manifest_tar, - base_paths=base_paths, - explicit_files=scan_explicit_files, - ) output_handler.handle_output(diff) @@ -991,13 +1034,20 @@ def _is_unprocessed(c): ) _write_attribution_file(config, all_packages) - # If we forced API mode due to no supported files, behave as if --disable-blocking was set - if force_api_mode: + # A run that created a full scan instead of a comparison has no baseline, so + # diff.new_alerts is not a meaningful thing to block on: with no alert-bearing + # output format enabled it is empty, and with --enable-json/--sarif/ + # --enable-gitlab-security it holds every alert in the scan rather than the + # newly introduced ones. Blocking on it would make the exit code depend on + # which output format happened to be requested, so behave as if + # --disable-blocking was set. force_api_mode arrives here for the same reason + # (no supported manifest files, so nothing to compare). + if force_api_mode or not comparison_ran: if config.strict_blocking: log.warning("--strict-blocking is only supported in diff mode. " - "API mode (no diff) cannot evaluate existing violations.") + "A full scan (no diff) cannot evaluate existing violations.") if not config.disable_blocking: - log.debug("Temporarily enabling disable_blocking due to no supported manifest files") + log.debug("Temporarily enabling disable_blocking: this run created a full scan, not a comparison") config.disable_blocking = True # Post commit status to GitLab if enabled diff --git a/tests/unit/test_pull_request_context.py b/tests/unit/test_pull_request_context.py index 5ad12903..9f236859 100644 --- a/tests/unit/test_pull_request_context.py +++ b/tests/unit/test_pull_request_context.py @@ -1,4 +1,25 @@ -from socketsecurity.core.pull_request import resolve_pull_request_context +import pytest + +from socketsecurity.core.pull_request import ( + parse_pull_request_number, + resolve_pull_request_context, +) + + +@pytest.mark.parametrize( + "value", + # "false" is what Buildkite puts in BUILDKITE_PULL_REQUEST on a branch build. + # main_code stores this result rather than the raw value, because the string + # is truthy and GithubConfig would read it as a real pull request number. + ["false", "0", "", None, "-1", "not-a-number"], +) +def test_non_pull_request_values_canonicalize_to_zero(value): + assert parse_pull_request_number(value) == 0 + + +def test_pull_request_numbers_survive_canonicalization(): + assert parse_pull_request_number("42") == 42 + assert parse_pull_request_number(42) == 42 def test_explicit_pr_number_wins_over_detected_context(): diff --git a/tests/unit/test_socketcli.py b/tests/unit/test_socketcli.py index 6e0e42e8..db5e8582 100644 --- a/tests/unit/test_socketcli.py +++ b/tests/unit/test_socketcli.py @@ -3,6 +3,7 @@ import pytest from socketsecurity import socketcli +from socketsecurity.config import CliConfig from socketsecurity.core.classes import Diff, Package from socketsecurity.socketcli import ( build_license_artifact_payload, @@ -74,12 +75,90 @@ def test_pr_context_provider_uses_integration_without_comment_adapter(): assert socketcli._select_pull_request_provider("azure", "api") == "azure" -def test_scm_merge_request_event_creates_diff(): - assert socketcli._should_create_scm_diff("diff") is True +# --------------------------------------------------------------------------- +# SCM scan selection. +# +# Only a pull request or merge request event has a baseline, so every other +# pipeline gets a full scan. These drive create_scm_scan against a recording +# stub rather than asserting on the branch predicate, so swapping the call back +# to create_new_diff fails them. +# --------------------------------------------------------------------------- -def test_scm_branch_event_always_uses_full_scan(): - assert socketcli._should_create_scm_diff("main") is False +class _RecordingCore: + def __init__(self): + self.calls = [] + + def create_new_diff(self, *args, **kwargs): + self.calls.append(("create_new_diff", args, kwargs)) + return Diff(id="diff-scan") + + def create_full_scan_with_report_url(self, *args, **kwargs): + self.calls.append(("create_full_scan_with_report_url", args, kwargs)) + return Diff(id="full-scan") + + +def _run_scm_scan(scm_event_type, **overrides): + core = _RecordingCore() + config = CliConfig.from_args(["--api-token", "test"]) + diff, comparison_ran = socketcli.create_scm_scan( + core, + config, + scm_event_type, + **{ + "scan_paths": ["."], + "params": object(), + "no_change": False, + "base_paths": None, + "explicit_files": None, + "external_href": "https://github.com/acme/widgets/pull/42", + **overrides, + }, + ) + return core, diff, comparison_ran + + +def test_pull_request_event_creates_a_comparison_with_the_pr_link(): + core, diff, comparison_ran = _run_scm_scan("diff") + + method, _, kwargs = core.calls[0] + assert method == "create_new_diff" + assert kwargs["external_href"] == "https://github.com/acme/widgets/pull/42" + assert comparison_ran is True + assert diff.id == "diff-scan" + + +@pytest.mark.parametrize("scm_event_type", ["main", None]) +def test_branch_event_creates_a_full_scan(scm_event_type): + core, diff, comparison_ran = _run_scm_scan(scm_event_type) + + method, _, kwargs = core.calls[0] + assert method == "create_full_scan_with_report_url" + # A full scan has no before/after pair to associate the link with. + assert "external_href" not in kwargs + assert comparison_ran is False + assert diff.id == "full-scan" + + +def test_api_only_diff_flags_do_not_force_a_comparison_on_a_branch_build(): + """The detected event type is authoritative once an SCM adapter is active.""" + core = _RecordingCore() + config = CliConfig.from_args(["--api-token", "test", "--enable-diff"]) + + _, comparison_ran = socketcli.create_scm_scan( + core, + config, + "main", + scan_paths=["."], + params=object(), + no_change=False, + base_paths=None, + explicit_files=None, + external_href=None, + ) + + assert core.calls[0][0] == "create_full_scan_with_report_url" + assert comparison_ran is False # --------------------------------------------------------------------------- From 39403c77cdbd01a15e1882eb0380b89139848072 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:08:33 -0400 Subject: [PATCH 13/27] fix(comments): stop reading an npm scope as an ecosystem Ignore matching strips the ecosystem off a command so an ecosystem-qualified reply still matches the bare package name parsed out of a start-socket-alert marker. It stripped any leading path segment, and a scope sits in the same position, so "ignore @types/node@*" also suppressed alerts for a package named node. Only strip a leading segment that cannot be a scope. Co-Authored-By: Claude Opus 5 (1M context) --- socketsecurity/core/scm_comments.py | 15 ++++++++++++++- tests/unit/test_disable_ignore.py | 12 ++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/socketsecurity/core/scm_comments.py b/socketsecurity/core/scm_comments.py index ea758ca1..88197815 100644 --- a/socketsecurity/core/scm_comments.py +++ b/socketsecurity/core/scm_comments.py @@ -81,11 +81,24 @@ def is_ignore( pkg_name: str, pkg_version: str, name: str, version: str, pkg_type: str = "" ) -> bool: + """Match an alert's package against one parsed ignore command. + + Generated commands are ecosystem-qualified (``npm/lodash@4.17.21``) but + replies typed by hand, and commands written by older CLI versions, use the + bare package name, so both have to match. + + Callers that parse the package out of a ``start-socket-alert`` marker have no + pkg_type to compare against and instead strip the ecosystem off the command. + An npm scope looks the same as an ecosystem prefix there, so only strip when + the leading segment cannot be one: without the guard, + ``ignore @types/node@*`` would also silently ignore alerts for a package + literally named ``node``. + """ package_names = {pkg_name} if pkg_type: package_names.add(f"{pkg_type}/{pkg_name}") target_names = {name} - if not pkg_type and "/" in name: + if not pkg_type and "/" in name and not name.startswith("@"): target_names.add(name.split("/", 1)[1]) return bool(package_names & target_names) and (pkg_version == version or version == "*") diff --git a/tests/unit/test_disable_ignore.py b/tests/unit/test_disable_ignore.py index 62633cbd..93ae4e8c 100644 --- a/tests/unit/test_disable_ignore.py +++ b/tests/unit/test_disable_ignore.py @@ -101,6 +101,18 @@ def test_scoped_package_name_is_parsed_from_the_right(self): assert Comments.remove_alerts(comments, [alert]) == [] + def test_a_scope_is_not_mistaken_for_an_ecosystem(self): + """`ignore @types/node@*` must not also ignore the package named `node`. + + Callers with no pkg_type strip the ecosystem off the command, and a scope + occupies the same position; only a leading segment that cannot be a scope + may be stripped. + """ + assert not Comments.is_ignore("node", "1.0.0", "@types/node", "*") + assert Comments.is_ignore("@types/node", "1.0.0", "@types/node", "*") + # The real ecosystem prefix is still stripped. + assert Comments.is_ignore("node", "1.0.0", "npm/node", "*") + def test_alerts_preserved_when_no_ignore_comments(self): """With --disable-ignore the caller skips remove_alerts entirely, which is equivalent to passing empty comments.""" From 78199e2d0e89b10002afd4dcd280069b55d4be42 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:08:33 -0400 Subject: [PATCH 14/27] fix(comments): keep the diff badge where artwork exists Labelling every dependency overview row with bold text dropped the badge from added rows, which is the only category the overview rendered before. The badge host publishes diff-added.svg and diff-updated.svg but nothing for removed or replaced, so look the badge up per change type and fall back to the text label only where there is no image to render. Co-Authored-By: Claude Opus 5 (1M context) --- socketsecurity/core/messages.py | 19 ++++++++++++++++++- tests/unit/test_dependency_overview.py | 7 ++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/socketsecurity/core/messages.py b/socketsecurity/core/messages.py index 76047689..33681f97 100644 --- a/socketsecurity/core/messages.py +++ b/socketsecurity/core/messages.py @@ -1247,6 +1247,23 @@ def create_remove_line(diff: Diff, md: MdUtils) -> MdUtils: md.new_line(removed_line) return md + # Change types the shared badge host publishes an image for. Removed and + # replaced have no artwork, so they fall back to a bold text label rather than + # rendering a broken image; added and updated keep the badge the overview + # comment has always used. + DIFF_BADGES = { + "Added": "diff-added.svg", + "Updated": "diff-updated.svg", + } + + @staticmethod + def get_diff_badge(change: str, package_url: str) -> str: + """Return the Dependency Overview cell marking how a package changed.""" + badge = Messages.DIFF_BADGES.get(change) + if not badge: + return f"**{change}**" + return f"[![{change}](https://github-app-statics.socket.dev/{badge})]({package_url})" + @staticmethod def create_added_table(diff: Diff, md: MdUtils) -> MdUtils: """ @@ -1279,7 +1296,7 @@ def create_added_table(diff: Diff, md: MdUtils) -> MdUtils: package: Purl package_url = f"[{package.purl}]({package.url})" - diff_badge = f"**{change}**" + diff_badge = Messages.get_diff_badge(change, package.url) # Scores dynamically converted to badge URLs and linked def score_to_badge(score): diff --git a/tests/unit/test_dependency_overview.py b/tests/unit/test_dependency_overview.py index 709fd659..67895b2d 100644 --- a/tests/unit/test_dependency_overview.py +++ b/tests/unit/test_dependency_overview.py @@ -81,5 +81,10 @@ def test_dependency_overview_labels_each_change_type(tmp_path, monkeypatch): comment = Messages.dependency_overview_template(diff) - for change in ("Added", "Updated", "Removed", "Replaced"): + # The badge host publishes artwork for added and updated only, so the other + # two fall back to a text label rather than a broken image. + assert "diff-added.svg" in comment + assert "diff-updated.svg" in comment + for change in ("Removed", "Replaced"): assert f"**{change}**" in comment + assert f"diff-{change.lower()}.svg" not in comment From f869a52396d4812529c6072a39bdeef7955f4b69 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:08:33 -0400 Subject: [PATCH 15/27] refactor(config): scope the config-file defaults dict to its block normalized_defaults has no reader outside the branch that fills it. Co-Authored-By: Claude Opus 5 (1M context) --- socketsecurity/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/socketsecurity/config.py b/socketsecurity/config.py index 645e5145..63f0650c 100644 --- a/socketsecurity/config.py +++ b/socketsecurity/config.py @@ -209,10 +209,10 @@ def from_args(cls, args_list: Optional[List[str]] = None) -> 'CliConfig': pre_parser.add_argument("--config", dest="config_file", default=None) pre_args, _ = pre_parser.parse_known_args(args_list) - normalized_defaults = {} if pre_args.config_file: config_defaults = load_cli_config_file(pre_args.config_file) valid_dests = {action.dest for action in parser._actions if action.dest != "help"} + normalized_defaults = {} for key, value in config_defaults.items(): dest = str(key).replace("-", "_") if dest in valid_dests: From fc36c7617fb8274beea8bd4f170215c2e5207273 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:08:34 -0400 Subject: [PATCH 16/27] docs: correct the release notes for branch pipeline scans The entry still described the intermediate behavior where explicit diff flags opted a non-PR run into comparison mode; the detected event type has been authoritative since that was reverted. Record the blocking and license consequences alongside it, plus the ignore and overview fixes. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89e1b336..8a0dd4f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,15 +24,36 @@ external link, allowing Dashboard reports to retain their CI change context. Re-running a comparison over an already-compared scan pair now applies the link to the existing diff scan instead of leaving that report unassociated. -- GitHub and GitLab branch pipelines now create full scans by default. Explicit - diff flags continue to opt non-PR runs into comparison mode. +- A `--pr-number` value that is not a positive integer is now normalized to `0` + before the GitHub adapter reads it, so Buildkite's `false` on a branch build no + longer makes that build look like a pull request event. + +### Changed: GitHub and GitLab branch pipelines create full scans + +- With `--scm github` or `--scm gitlab`, only pull request and merge request + events create diff scans. Every other pipeline, including default-branch + pushes, creates a full scan. The detected event type is authoritative: + `--enable-diff` and `--ignore-commit-files` no longer opt an SCM branch run + into comparison mode. +- Those runs no longer set a blocking exit code. A full scan has no baseline, so + it cannot distinguish newly introduced alerts from pre-existing ones; the CLI + now behaves as if `--disable-blocking` was supplied, matching how it already + treats a run with no supported manifest files. Pull request and merge request + pipelines are unaffected and still block. +- `--generate-license` and `--legal-format fossa` fetch the package list on this + path, so attribution files generated from a branch pipeline are complete rather + than empty. ### Fixed: pull request and merge request comment accuracy - Per-alert ignore instructions now use ecosystem-qualified package names and accept scoped packages while remaining compatible with older bare-name replies. + A leading npm scope is no longer mistaken for an ecosystem, so + `ignore @types/node@*` no longer also ignores the package named `node`. - Dependency overviews preserve added, updated, removed, and replaced package - classifications instead of presenting updates as new dependencies. + classifications instead of presenting updates as new dependencies. Added and + updated rows keep their diff badge; removed and replaced, which have no + published badge, use a text label. - Shared security comment copy no longer describes GitLab merge request output as Socket for GitHub. From 63e938d00a8fededdb84e7eddce4aa26513d3467 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 11 Sep 2026 16:10:31 -0400 Subject: [PATCH 17/27] fix(comments): stop legacy comment updates crashing on scoped names process_original_security_comment split the package cell on every "@", so a scoped name carrying its own "@" unpacked into three values and raised an uncaught ValueError. Same bug class this branch already fixed one function over in process_updated_security_comment, just left in its sibling. Split from the right, and pass the ecosystem through as pkg_type rather than pre-concatenating it onto the package name. That makes the two comment formats agree: both now accept an ignore command for a scoped package in either the ecosystem-qualified or the bare form, where the legacy path previously matched only the qualified one. Co-Authored-By: Claude Opus 5 (1M context) --- socketsecurity/core/scm_comments.py | 7 +++--- tests/unit/test_pr_comment_rendering.py | 32 +++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/socketsecurity/core/scm_comments.py b/socketsecurity/core/scm_comments.py index 88197815..c1988985 100644 --- a/socketsecurity/core/scm_comments.py +++ b/socketsecurity/core/scm_comments.py @@ -152,13 +152,14 @@ def process_original_security_comment( details, _ = package.split("](") ecosystem, details = details.split("/", 1) ecosystem = ecosystem.lstrip("[") - pkg_name, pkg_version = details.split("@") - pkg_name = f"{ecosystem}/{pkg_name}" + # Split from the right: a scoped name carries its own "@", so + # split("@") unpacks into three parts and raises. + pkg_name, pkg_version = details.rsplit("@", 1) # ignore_all has to be checked outside the loop: an ignore-all # comment produces no ignore_commands, so a loop-internal check # never runs and every row was kept. ignore = ignore_all or any( - Comments.is_ignore(pkg_name, pkg_version, name, version) + Comments.is_ignore(pkg_name, pkg_version, name, version, ecosystem) for name, version in ignore_commands ) if not ignore: diff --git a/tests/unit/test_pr_comment_rendering.py b/tests/unit/test_pr_comment_rendering.py index a917064d..91fafbf2 100644 --- a/tests/unit/test_pr_comment_rendering.py +++ b/tests/unit/test_pr_comment_rendering.py @@ -289,6 +289,17 @@ def test_collapsed_body_is_stable_when_reprocessed(self): [View full report](https://socket.dev/report/legacy?action=error%2Cwarn) """ +SCOPED_LEGACY_COMMENT = """ + + +|Alert|Package|Introduced by|Manifest File|CI| +|:---|:---|:---|:---|:---| +|Known Malware|[npm/@socketsecurity/example@1.0.0](https://socket.dev/z)|example|package.json|:no_entry_sign:| + + +[View full report](https://socket.dev/report/legacy?action=error%2Cwarn) +""" + class TestProcessOriginalSecurityComment: def test_partial_ignore_keeps_remaining_row(self): @@ -316,6 +327,27 @@ def test_ignore_all_collapses_to_the_no_alerts_body(self): assert "No dependency alerts to report" in new_body assert "[View full report](https://socket.dev/report/legacy)" in new_body + def test_scoped_package_row_does_not_raise(self): + """A scoped name carries its own "@", so split("@") unpacked into three.""" + security = _make_comment(SCOPED_LEGACY_COMMENT) + comments = {"security": security, "ignore": []} + + new_body = Comments.process_security_comment(security, comments) + + assert "npm/@socketsecurity/example@1.0.0" in new_body + + def test_scoped_package_row_is_ignorable_both_ways(self): + for command in ( + "SocketSecurity ignore npm/@socketsecurity/example@1.0.0", + "SocketSecurity ignore @socketsecurity/example@1.0.0", + ): + security = _make_comment(SCOPED_LEGACY_COMMENT) + comments = {"security": security, "ignore": [_make_comment(command, comment_id=2)]} + + new_body = Comments.process_security_comment(security, comments) + + assert "No dependency alerts to report" in new_body, command + class TestExtractReportUrl: def test_strips_the_action_filter(self): From 977a8fb9ef79450b1dd3780d4450291f009039f0 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 11 Sep 2026 16:10:31 -0400 Subject: [PATCH 18/27] fix(comments): require write access to ignore an alert An @SocketSecurity ignore command suppresses a security finding, but the CLI honored one from any commenter. Comment.author_association was carried on the dataclass and never read, so nothing on the path from comment to suppressed alert asked whether the author could push to the repository. A drive-by ignore-all on an open pull request silenced every finding on it. Gate the ignore bucket in check_for_socket_comments, the one place every consumer goes through. A rejected command is logged with its author and is also absent from the ignore telemetry, which should record what was acted on. GitHub returns author_association with every comment, so the check is free and definitive: OWNER, MEMBER and COLLABORATOR only. GitLab notes carry no equivalent, so project membership is read once per run, and only when an ignore command is actually present. members/all is used rather than a per-user lookup because it answers non-membership with a 200 and an absent id -- CliClient collapses every HTTP error into APIFailure without a status code, so a per-user 404, exactly the outsider case, would be indistinguishable from a token that cannot read the endpoint and would have to fail open. When membership genuinely cannot be read -- a CI_JOB_TOKEN typically cannot -- the command is honored and a warning names the author, so this does not silently break pipelines already relying on ignore commands. Documented alongside the token requirement to get enforcement. Co-Authored-By: Claude Opus 5 (1M context) --- docs/cli-reference.md | 21 +++- socketsecurity/core/scm/github.py | 12 +- socketsecurity/core/scm/gitlab.py | 87 +++++++++++++- socketsecurity/core/scm_comments.py | 34 +++++- tests/unit/test_ignore_authorization.py | 144 ++++++++++++++++++++++++ 5 files changed, 294 insertions(+), 4 deletions(-) create mode 100644 tests/unit/test_ignore_authorization.py diff --git a/docs/cli-reference.md b/docs/cli-reference.md index d5d102fd..b78b45f6 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -431,7 +431,7 @@ The launcher can be tuned via the `SOCKET_CLI_COANA_LAUNCHER` environment variab |:-------------------------|:---------|:--------|:----------------------------------------------------------------------| | `--ignore-commit-files` | False | False | Ignore commit files | | `--disable-blocking` | False | False | Non-blocking CI mode: the CLI always exits **0**, even when blocking alerts are present (including with `--strict-blocking`). Also exits 0 on uncaught runtime errors and Socket API failures, so the job is treated as successful while findings and errors are still logged. Takes precedence over `--strict-blocking`. | -| `--disable-ignore` | False | False | Disable support for `@SocketSecurity ignore` commands in PR comments. When set, alerts cannot be suppressed via comments and ignore instructions are hidden from comment output. | +| `--disable-ignore` | False | False | Disable support for `@SocketSecurity ignore` commands in PR comments. When set, alerts cannot be suppressed via comments and ignore instructions are hidden from comment output. See [Who can ignore an alert](#who-can-ignore-an-alert). | | `--strict-blocking` | False | False | Fail on ANY security policy violations (blocking severity), not just new ones. Only works in diff mode. See [Strict Blocking Mode](#strict-blocking-mode) for details. | | `--enable-diff` | False | False | Enable diff mode even when using `--integration api` (forces diff mode without SCM integration) | | `--scm` | False | api | Source control management type | @@ -690,6 +690,25 @@ The CLI uses intelligent default branch detection with the following priority: Both `--default-branch` and `--pending-head` parameters are automatically synchronized to ensure consistent behavior. +## Who can ignore an alert + +`@SocketSecurity ignore /@` and +`@SocketSecurity ignore-all` suppress security findings, so the CLI honors them +only from a commenter with write access to the repository. A command from anyone +else is skipped, logged with the author's name, and the alerts it named stay +reported. `--disable-ignore` turns the feature off entirely. + +| Provider | How access is determined | If it cannot be determined | +|:---------|:-------------------------|:---------------------------| +| GitHub | The `author_association` returned with each comment. `OWNER`, `MEMBER` and `COLLABORATOR` are honored. | Treated as unauthorized. | +| GitLab | Project membership, read once per run when an ignore command is present. Developer (30) or above is honored. | The command is honored and a warning is logged. | + +GitLab notes carry no permission field, so the check needs a `GITLAB_TOKEN` that +can read `GET /projects/:id/members/all`. A `CI_JOB_TOKEN` generally cannot, and +in that case the CLI logs a warning and still honors the command rather than +breaking a pipeline that was already relying on it. Use a personal or group access +token with API read access to get enforcement. + ## GitLab Token Configuration GitLab token/auth behavior and CI examples are documented in [`ci-cd.md`](ci-cd.md). diff --git a/socketsecurity/core/scm/github.py b/socketsecurity/core/scm/github.py index 9ec1e4ca..35c63801 100644 --- a/socketsecurity/core/scm/github.py +++ b/socketsecurity/core/scm/github.py @@ -224,7 +224,17 @@ def get_comments_for_pr(self) -> dict: else: log.error(raw_comments) - return Comments.check_for_socket_comments(comments) + return Comments.check_for_socket_comments(comments, self.is_ignore_authorized) + + def is_ignore_authorized(self, comment: Comment) -> bool: + """Whether a commenter may suppress alerts with @SocketSecurity ignore. + + GitHub returns the author's relationship to the repository on every issue + comment, so this costs no extra request and is definitive. A missing value + is treated as unauthorized rather than trusted. + """ + association = (getattr(comment, "author_association", "") or "").upper() + return association in Comments.WRITE_ACCESS_ASSOCIATIONS def add_socket_comments( self, diff --git a/socketsecurity/core/scm/gitlab.py b/socketsecurity/core/scm/gitlab.py index 2c3947de..5b8aa3fe 100644 --- a/socketsecurity/core/scm/gitlab.py +++ b/socketsecurity/core/scm/gitlab.py @@ -126,9 +126,21 @@ def _get_auth_headers(token: str) -> dict: } class Gitlab: + # GitLab access levels: 30 Developer, 40 Maintainer, 50 Owner. Reporter (20) + # and Guest (10) cannot push, so they cannot suppress an alert either. + MIN_IGNORE_ACCESS_LEVEL = 30 + # Bounded so a project with a very large membership cannot stall a scan. Past + # the cap the answer is "undetermined", handled the same as a failed lookup. + MEMBER_PAGE_SIZE = 100 + MEMBER_PAGE_LIMIT = 10 + def __init__(self, client: CliClient, config: Optional[GitlabConfig] = None): self.config = config or GitlabConfig.from_env() self.client = client + # None until the first ignore comment forces a lookup; stays None when the + # members API cannot be read, which is the "undetermined" state. + self._member_access: Optional[dict] = None + self._member_lookup_attempted = False def _request_with_fallback(self, **kwargs): """ @@ -256,7 +268,80 @@ def get_comments_for_pr(self) -> dict: comment.body_list = comment.body.split("\n") else: log.error(raw_comments) - return Comments.check_for_socket_comments(comments) + return Comments.check_for_socket_comments(comments, self.is_ignore_authorized) + + def _load_member_access(self) -> Optional[dict]: + """Map project member user id -> access level, or None if unreadable. + + ``members/all`` is used rather than a per-user lookup because it answers + non-membership with a 200 and an absent id. CliClient collapses every HTTP + error into APIFailure without a status code, so a per-user 404 -- exactly + the outsider case this guards against -- would be indistinguishable from a + token that cannot read the endpoint, and would have to fail open. + """ + if self._member_lookup_attempted: + return self._member_access + self._member_lookup_attempted = True + if not self.config.mr_project_id: + return None + + access: dict = {} + for page in range(1, Gitlab.MEMBER_PAGE_LIMIT + 1): + path = ( + f"projects/{self.config.mr_project_id}/members/all" + f"?per_page={Gitlab.MEMBER_PAGE_SIZE}&page={page}" + ) + try: + response = self._request_with_fallback( + path=path, + headers=self.config.headers, + base_url=self.config.api_url + ) + members = response.json() + except Exception as error: + log.warning(f"Could not read GitLab project members: {error}") + return None + if not isinstance(members, list): + log.warning("Unexpected GitLab project members response") + return None + for member in members: + if isinstance(member, dict) and member.get("id") is not None: + access[member["id"]] = member.get("access_level") or 0 + if len(members) < Gitlab.MEMBER_PAGE_SIZE: + self._member_access = access + return access + + log.warning( + f"GitLab project has more than {Gitlab.MEMBER_PAGE_SIZE * Gitlab.MEMBER_PAGE_LIMIT} " + "members; cannot confirm ignore-command authorization" + ) + return None + + def is_ignore_authorized(self, comment: Comment) -> bool: + """Whether a commenter may suppress alerts with @SocketSecurity ignore. + + GitLab notes carry no permission field, so this costs one members lookup + per run (cached, and only when an ignore command is actually present). + + When membership can be read the answer is definitive. When it cannot -- a + CI_JOB_TOKEN generally cannot read the members API -- the command is + honored and a warning is logged, so turning this on does not silently break + pipelines that were already relying on ignore commands. Set a token with + API read access to get enforcement. + """ + access = self._load_member_access() + if access is None: + log.warning( + "Honoring @SocketSecurity ignore from " + f"{Comments.comment_author_name(comment)} without verifying write " + "access: GitLab project membership could not be read. Use a token " + "with API read access to enforce this." + ) + return True + + author = getattr(comment, "author", None) or {} + user_id = author.get("id") + return access.get(user_id, 0) >= Gitlab.MIN_IGNORE_ACCESS_LEVEL def add_socket_comments( self, diff --git a/socketsecurity/core/scm_comments.py b/socketsecurity/core/scm_comments.py index c1988985..e578b899 100644 --- a/socketsecurity/core/scm_comments.py +++ b/socketsecurity/core/scm_comments.py @@ -1,5 +1,6 @@ import json import re +from typing import Callable, Optional from requests import Response @@ -11,6 +12,17 @@ class Comments: VIEW_REPORT_PATTERN = re.compile(r"\[View full report\]\(([^)\s]+)\)") + # GitHub stamps every issue comment with the author's relationship to the + # repository. Only these three imply write access; CONTRIBUTOR, MANNEQUIN, + # MENTIONEE, FIRST_TIMER, FIRST_TIME_CONTRIBUTOR and NONE do not. + WRITE_ACCESS_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) + + @staticmethod + def comment_author_name(comment: Comment) -> str: + """Best-effort display name for a comment author, across providers.""" + user = getattr(comment, "user", None) or getattr(comment, "author", None) or {} + return user.get("login") or user.get("username") or "an unknown user" + @staticmethod def process_response(response: Response) -> dict: output = {} @@ -279,7 +291,20 @@ def extract_alert_details_from_row(row: str, ignore_all: bool, ignore_commands: @staticmethod - def check_for_socket_comments(comments: dict): + def check_for_socket_comments( + comments: dict, + is_authorized: Optional[Callable[[Comment], bool]] = None + ): + """Bucket a pull request's comments into the ones the CLI acts on. + + ``is_authorized`` gates the ignore bucket, and is the only place that gate + exists: an ``@SocketSecurity ignore`` command suppresses a security alert, + so it is honored only from someone with write access to the repository. + Filtering here rather than at each consumer means the rejected command is + also absent from the ignore telemetry, which should record what was acted + on. Both SCM adapters supply a predicate; omitting it trusts every + commenter and is only appropriate in tests. + """ socket_comments = {} for comment_id in comments: comment = comments[comment_id] @@ -289,6 +314,13 @@ def check_for_socket_comments(comments: dict): elif "socket-overview-comment-actions" in comment.body: socket_comments["overview"] = comment elif "SocketSecurity ignore".lower() in comment.body_list[0].lower(): + if is_authorized is not None and not is_authorized(comment): + log.warning( + "Skipping @SocketSecurity ignore command from " + f"{Comments.comment_author_name(comment)}: no write access " + "to this repository. Alerts remain reported." + ) + continue if "ignore" not in socket_comments: socket_comments["ignore"] = [] socket_comments["ignore"].append(comment) diff --git a/tests/unit/test_ignore_authorization.py b/tests/unit/test_ignore_authorization.py new file mode 100644 index 00000000..55143cc4 --- /dev/null +++ b/tests/unit/test_ignore_authorization.py @@ -0,0 +1,144 @@ +"""Who is allowed to suppress an alert with @SocketSecurity ignore. + +An ignore command silences a security finding, so it is honored only from someone +with write access to the repository. The gate lives in check_for_socket_comments, +so a rejected command never reaches the ignore parser, the alert filter, or the +ignore telemetry. +""" +from types import SimpleNamespace + +import pytest + +from socketsecurity.core.classes import Comment +from socketsecurity.core.scm.github import Github +from socketsecurity.core.scm.gitlab import Gitlab +from socketsecurity.core.scm_comments import Comments + + +def _comment(body="@SocketSecurity ignore npm/lodash@4.17.21", **fields): + return Comment(id=1, body=body, body_list=body.split("\n"), **fields) + + +# --- GitHub: author_association ships with the comment, no extra request ----- + + +@pytest.mark.parametrize("association", ["OWNER", "MEMBER", "COLLABORATOR"]) +def test_github_write_access_may_ignore(association): + github = Github.__new__(Github) + assert github.is_ignore_authorized(_comment(author_association=association)) is True + + +@pytest.mark.parametrize( + "association", + ["CONTRIBUTOR", "FIRST_TIME_CONTRIBUTOR", "FIRST_TIMER", "MANNEQUIN", "NONE", ""], +) +def test_github_without_write_access_may_not_ignore(association): + github = Github.__new__(Github) + assert github.is_ignore_authorized(_comment(author_association=association)) is False + + +def test_github_missing_association_is_not_trusted(): + """Absent field means unverified, which is not the same as authorized.""" + github = Github.__new__(Github) + assert github.is_ignore_authorized(_comment()) is False + + +def test_unauthorized_command_never_reaches_the_ignore_bucket(): + github = Github.__new__(Github) + outsider = _comment(author_association="NONE") + + bucketed = Comments.check_for_socket_comments( + {outsider.id: outsider}, github.is_ignore_authorized + ) + + assert "ignore" not in bucketed + # ...so the alert it named survives. + alert = SimpleNamespace( + pkg_name="lodash", pkg_version="4.17.21", pkg_type="npm", type="malware" + ) + assert Comments.remove_alerts(bucketed, [alert]) == [alert] + + +def test_ignore_all_from_an_outsider_is_rejected_too(): + """ignore-all is the more powerful command; it goes through the same gate.""" + github = Github.__new__(Github) + outsider = _comment(body="@SocketSecurity ignore-all", author_association="NONE") + + assert "ignore" not in Comments.check_for_socket_comments( + {outsider.id: outsider}, github.is_ignore_authorized + ) + + +# --- GitLab: notes carry no permission field, so membership is looked up ----- + + +def _gitlab(members_pages=None, raises=None): + gitlab = Gitlab.__new__(Gitlab) + gitlab.config = SimpleNamespace(mr_project_id="42", headers={}, api_url="https://gl/api/v4") + gitlab._member_access = None + gitlab._member_lookup_attempted = False + + calls = [] + + def fake_request(**kwargs): + calls.append(kwargs["path"]) + if raises: + raise raises + return SimpleNamespace(json=lambda: members_pages.pop(0)) + + gitlab._request_with_fallback = fake_request + gitlab.calls = calls + return gitlab + + +@pytest.mark.parametrize("access_level,expected", [(50, True), (40, True), (30, True), (20, False), (10, False)]) +def test_gitlab_requires_developer_access(access_level, expected): + gitlab = _gitlab([[{"id": 7, "access_level": access_level}]]) + comment = _comment(author={"id": 7, "username": "someone"}) + + assert gitlab.is_ignore_authorized(comment) is expected + + +def test_gitlab_non_member_may_not_ignore(): + """The outsider case: a 200 listing that simply does not contain them.""" + gitlab = _gitlab([[{"id": 7, "access_level": 40}]]) + comment = _comment(author={"id": 999, "username": "outsider"}) + + assert gitlab.is_ignore_authorized(comment) is False + + +def test_gitlab_membership_is_fetched_once_per_run(): + gitlab = _gitlab([[{"id": 7, "access_level": 40}]]) + + gitlab.is_ignore_authorized(_comment(author={"id": 7})) + gitlab.is_ignore_authorized(_comment(author={"id": 8})) + + assert len(gitlab.calls) == 1 + + +def test_gitlab_paginates_until_a_short_page(): + first = [{"id": i, "access_level": 30} for i in range(Gitlab.MEMBER_PAGE_SIZE)] + gitlab = _gitlab([first, [{"id": 999, "access_level": 40}]]) + + assert gitlab.is_ignore_authorized(_comment(author={"id": 999})) is True + assert len(gitlab.calls) == 2 + + +def test_gitlab_unreadable_membership_honors_the_command_with_a_warning(caplog): + """A CI_JOB_TOKEN usually cannot read members; that must not break pipelines.""" + gitlab = _gitlab(raises=Exception("403 Forbidden")) + + with caplog.at_level("WARNING", logger="socketcli"): + allowed = gitlab.is_ignore_authorized(_comment(author={"id": 7, "username": "dev"})) + + assert allowed is True + assert "without verifying write access" in caplog.text + + +def test_gitlab_oversized_membership_is_undetermined(): + full = [{"id": i, "access_level": 30} for i in range(Gitlab.MEMBER_PAGE_SIZE)] + gitlab = _gitlab([list(full) for _ in range(Gitlab.MEMBER_PAGE_LIMIT)]) + + # Undetermined falls back to honoring the command, same as an API failure. + assert gitlab.is_ignore_authorized(_comment(author={"id": 999})) is True + assert len(gitlab.calls) == Gitlab.MEMBER_PAGE_LIMIT From f16525f017ad25434810bee90f85921d8d9481f8 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 11 Sep 2026 16:10:43 -0400 Subject: [PATCH 19/27] 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) --- socketsecurity/core/pull_request.py | 31 +++++++++++++---- tests/unit/test_pull_request_context.py | 44 +++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/socketsecurity/core/pull_request.py b/socketsecurity/core/pull_request.py index e38212d7..2dcf77f1 100644 --- a/socketsecurity/core/pull_request.py +++ b/socketsecurity/core/pull_request.py @@ -27,14 +27,28 @@ def parse_pull_request_number(value) -> int: return parsed if parsed > 0 else 0 +def _http_url(value: Optional[str]) -> Optional[str]: + """Return ``value`` if it is an http(s) URL with a host, else ``None``. + + Every URL fragment read out of the CI environment goes through here before it + is composed into a link, because the result is sent to the API as a diff scan's + ``external_href``. Standard runners set these variables themselves, so this is + defense in depth rather than a live hole. + """ + if not value: + return None + url = value.strip().rstrip("/") + parsed = urlparse(url) + return url if parsed.scheme in ("http", "https") and parsed.netloc else None + + def _repository_url(value: Optional[str]) -> Optional[str]: if not value: return None url = value.strip().rstrip("/") if url.endswith(".git"): url = url[:-4] - parsed = urlparse(url) - return url if parsed.scheme in ("http", "https") and parsed.netloc else None + return _http_url(url) 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 repository = env.get("GITHUB_REPOSITORY") or remote_path or repo if not repository or "/" not in repository: return None - server = env.get("GITHUB_SERVER_URL") or (f"https://{remote_host}" if remote_host else "") - server = (server or "https://github.com").rstrip("/") + server = ( + _http_url(env.get("GITHUB_SERVER_URL")) + or (_http_url(f"https://{remote_host}") if remote_host else None) + or "https://github.com" + ) return f"{server}/{repository.strip('/')}/pull/{number}" @@ -62,8 +79,10 @@ def _gitlab_url(number: int, repo: Optional[str], env: Mapping[str, str]) -> Opt if not project_url: remote_host, remote_path = parse_git_remote(env.get("BUILDKITE_REPO")) project_path = env.get("CI_PROJECT_PATH") or remote_path or repo - server = env.get("CI_SERVER_URL") or (f"https://{remote_host}" if remote_host else "") - server = server.rstrip("/") + server = ( + _http_url(env.get("CI_SERVER_URL")) + or (_http_url(f"https://{remote_host}") if remote_host else None) + ) if server and project_path and "/" in project_path: project_url = f"{server}/{project_path.strip('/')}" return f"{project_url}/-/merge_requests/{number}" if project_url else None diff --git a/tests/unit/test_pull_request_context.py b/tests/unit/test_pull_request_context.py index 9f236859..0a59e4d4 100644 --- a/tests/unit/test_pull_request_context.py +++ b/tests/unit/test_pull_request_context.py @@ -196,6 +196,50 @@ def test_buildkite_github_enterprise_host_is_taken_from_the_remote(): assert context.url == "https://github.example.com/acme/widgets/pull/42" +@pytest.mark.parametrize( + "server", + ["javascript:alert(1)", "notaurl", "ftp://example.com", "https://", ""], +) +def test_unusable_github_server_url_falls_back_to_the_default(server): + """The result becomes a diff scan's external_href, so validate before composing.""" + context = resolve_pull_request_context( + "github", + "42", + "acme/widgets", + configured_explicit=True, + env={"GITHUB_SERVER_URL": server, "GITHUB_REPOSITORY": "acme/widgets"}, + ) + + assert context.url == "https://github.com/acme/widgets/pull/42" + + +@pytest.mark.parametrize("server", ["javascript:alert(1)", "notaurl", "ftp://example.com"]) +def test_unusable_gitlab_server_url_yields_no_link(server): + """GitLab has no public default host to fall back to, so the link is dropped.""" + context = resolve_pull_request_context( + "gitlab", + "42", + "acme/widgets", + configured_explicit=True, + env={"CI_SERVER_URL": server, "CI_PROJECT_PATH": "acme/widgets"}, + ) + + assert context.number == 42 + assert context.url is None + + +def test_self_hosted_server_urls_are_still_honored(): + assert resolve_pull_request_context( + "github", "42", None, configured_explicit=True, + env={"GITHUB_SERVER_URL": "https://github.example.com", "GITHUB_REPOSITORY": "acme/widgets"}, + ).url == "https://github.example.com/acme/widgets/pull/42" + + assert resolve_pull_request_context( + "gitlab", "42", None, configured_explicit=True, + env={"CI_SERVER_URL": "http://gitlab.internal", "CI_PROJECT_PATH": "acme/platform/widgets"}, + ).url == "http://gitlab.internal/acme/platform/widgets/-/merge_requests/42" + + def test_github_actions_environment_wins_over_the_checkout_remote(): context = resolve_pull_request_context( "github", From d7b01b0aa701e671672d76c069bac48af889134f Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 11 Sep 2026 16:10:43 -0400 Subject: [PATCH 20/27] docs: correct the add_purl_capabilities docstring The loop covers updated_packages as well as new_packages; the docstring still described only the latter. Co-Authored-By: Claude Opus 5 (1M context) --- socketsecurity/core/__init__.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index 597818f6..916da74b 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -2400,7 +2400,10 @@ def get_source_data(package: Package, packages: dict) -> list: @staticmethod def add_purl_capabilities(diff: Diff) -> None: """ - Adds capability information to each package in the diff's new_packages list. + Adds capability information to the diff's added and updated packages. + + Both lists are walked because an updated package is still newly present at + its new version, so its capabilities are as relevant as an added one's. Args: diff: Diff object to update with capability information From bd2a3c38f124defa85ef5df3a400263783446b69 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 11 Sep 2026 16:10:43 -0400 Subject: [PATCH 21/27] docs: record the review fixes in the 2.9.0 release notes Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a0dd4f6..8cec0c95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,24 @@ path, so attribution files generated from a branch pipeline are complete rather than empty. +### Changed: `@SocketSecurity ignore` requires write access + +- An ignore command suppresses a security alert, but the CLI honored one from any + commenter, including a drive-by comment from someone with no access to the + repository. Commands are now accepted only from an author with write access. +- On GitHub this is read from the `author_association` GitHub already returns with + each comment, so it costs no extra request: `OWNER`, `MEMBER` and `COLLABORATOR` + are honored, and `CONTRIBUTOR`, `FIRST_TIME_CONTRIBUTOR`, `MANNEQUIN` and `NONE` + are not. +- GitLab notes carry no equivalent field, so project membership is read once per + run (only when an ignore command is present) and Developer or above is required. + If that lookup cannot be answered — a `CI_JOB_TOKEN` generally cannot read the + members API — the command is still honored and a warning names the author, so + enabling this does not silently break pipelines that relied on ignore commands. + Use a `GITLAB_TOKEN` with API read access to get enforcement. +- A rejected command is logged and is also absent from the ignore telemetry, which + records what was acted on. + ### Fixed: pull request and merge request comment accuracy - Per-alert ignore instructions now use ecosystem-qualified package names and @@ -56,6 +74,14 @@ published badge, use a text label. - Shared security comment copy no longer describes GitLab merge request output as Socket for GitHub. +- Updating a security comment in the legacy table format no longer raises on a + scoped package name. That path split the package cell on every `@`, so a name + carrying its own `@` unpacked into three values; it now splits from the right, + matching the current comment format. Ignore commands for a scoped package are + accepted there in both the ecosystem-qualified and bare forms. +- Server URLs read from `GITHUB_SERVER_URL` and `CI_SERVER_URL` are validated as + http(s) URLs before being composed into a diff scan's external link, matching + the check already applied to the other repository URLs read from CI. ## 2.8.1 From 239c85fb6a15ad36dfe971cbca131fe7ac721ab3 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:51:23 -0400 Subject: [PATCH 22/27] docs: rewrite branch comments for the reader, not the author Sweep of every comment this branch adds, against the fourth-wall skill: - A test docstring stated the scan type "(since 2.8.0)", which was already wrong after the renumber to 2.9.0 and would rot again on the next one. Version stamps in comments describe a debut rather than the behavior. - Two docstrings narrated the failure the old parser produced instead of the invariant that makes rsplit correct. A scoped name carrying its own "@" is the whole reason; the traceback it used to raise is not. - The "do NOT use on_duplicate=redirect" landmine was explained twice, in full, at both call sites. Kept at the 409 fallback, where the temptation to add it lives; the create site now just says what update does. - A test section header justified its own design to a reviewer ("swapping the call back ... fails them"). Restated as what the test actually pins. - "out of this branch" in the remote-URL regex reads as a git branch in this repo; it means the regex case. 642 passed, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- socketsecurity/core/__init__.py | 24 ++++++++++-------------- socketsecurity/core/git_remote.py | 2 +- socketsecurity/core/scm_comments.py | 3 +-- tests/core/test_full_scan_outputs.py | 5 ++--- tests/unit/test_pr_comment_rendering.py | 2 +- tests/unit/test_socketcli.py | 4 ++-- 6 files changed, 17 insertions(+), 23 deletions(-) diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index 916da74b..fa6b004c 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -1708,12 +1708,9 @@ def get_diff_scan_artifacts( if external_href: create_params["external_href"] = external_href # external_href is only honored while a diff scan is being created, - # so re-running a comparison over an already-compared scan pair - # would otherwise leave the Dashboard report with no link back to - # the pull request. on_duplicate=update applies the link to the - # existing resource and answers 200 with the same {"diff_scan": ...} - # envelope as a create. Notably it is not on_duplicate=redirect, - # whose 302 the SDK follows into a GET without cached=true. + # so a re-run over an already-compared scan pair needs + # on_duplicate=update to apply the link to the existing resource. It + # answers 200 with the same {"diff_scan": ...} envelope as a create. create_params["on_duplicate"] = "update" try: result = self.sdk.diffscans.create_from_ids(self.config.org_slug, create_params) @@ -1723,14 +1720,13 @@ def get_diff_scan_artifacts( if error.status_code != 409: raise - # Reached without on_duplicate=update (no pull request context to - # attach) and against deployments that predate it and still answer - # 409 regardless. Do not switch this to on_duplicate=redirect: the - # SDK follows that 302 automatically with a GET that lacks - # cached=true, which can leave the connection idle while an existing - # diff scan is still computing. Resolve the duplicate resource - # explicitly so every result fetch continues through the bounded - # cached polling path below. + # Reached when there is no pull request context to attach, and on + # deployments that answer 409 regardless. Do NOT switch this to + # on_duplicate=redirect: the SDK follows that 302 automatically with + # a GET that lacks cached=true, which can leave the connection idle + # while an existing diff scan is still computing. Resolve the + # duplicate explicitly so every result fetch continues through the + # bounded cached polling path below. existing = self.sdk.diffscans.list( self.config.org_slug, params={ diff --git a/socketsecurity/core/git_remote.py b/socketsecurity/core/git_remote.py index 9ca5dc57..eb5b9c02 100644 --- a/socketsecurity/core/git_remote.py +++ b/socketsecurity/core/git_remote.py @@ -10,7 +10,7 @@ from urllib.parse import urlparse # git@host:owner/repo - the scp-like syntax urlparse cannot handle. The negative -# lookahead keeps scheme-prefixed URLs (https://, ssh://) out of this branch. +# lookahead keeps scheme-prefixed URLs (https://, ssh://) out of this case. _SCP_LIKE_REMOTE = re.compile(r"^(?:[^@/]+@)?([^:/]+):(?!//)(.+)$") diff --git a/socketsecurity/core/scm_comments.py b/socketsecurity/core/scm_comments.py index e578b899..16af560b 100644 --- a/socketsecurity/core/scm_comments.py +++ b/socketsecurity/core/scm_comments.py @@ -164,8 +164,7 @@ def process_original_security_comment( details, _ = package.split("](") ecosystem, details = details.split("/", 1) ecosystem = ecosystem.lstrip("[") - # Split from the right: a scoped name carries its own "@", so - # split("@") unpacks into three parts and raises. + # Split from the right: a scoped name carries its own "@". pkg_name, pkg_version = details.rsplit("@", 1) # ignore_all has to be checked outside the loop: an ignore-all # comment produces no ignore_commands, so a loop-internal check diff --git a/tests/core/test_full_scan_outputs.py b/tests/core/test_full_scan_outputs.py index e46a0b41..0823d682 100644 --- a/tests/core/test_full_scan_outputs.py +++ b/tests/core/test_full_scan_outputs.py @@ -1,9 +1,8 @@ """What a full scan has to carry for each enabled output. create_full_scan_with_report_url runs on every path with no baseline to compare -against: API mode, and (since 2.8.0) SCM branch pipelines. Fetching the SBOM is -the expensive part, so it is gated on the enabled outputs -- these pin which -outputs need it. +against: API mode and SCM branch pipelines. Fetching the SBOM is the expensive +part, so it is gated on the enabled outputs -- these pin which outputs need it. """ import pytest from socketdev.fullscans import FullScanParams diff --git a/tests/unit/test_pr_comment_rendering.py b/tests/unit/test_pr_comment_rendering.py index 91fafbf2..020b636e 100644 --- a/tests/unit/test_pr_comment_rendering.py +++ b/tests/unit/test_pr_comment_rendering.py @@ -328,7 +328,7 @@ def test_ignore_all_collapses_to_the_no_alerts_body(self): assert "[View full report](https://socket.dev/report/legacy)" in new_body def test_scoped_package_row_does_not_raise(self): - """A scoped name carries its own "@", so split("@") unpacked into three.""" + """A scoped name carries its own "@", so the split must come from the right.""" security = _make_comment(SCOPED_LEGACY_COMMENT) comments = {"security": security, "ignore": []} diff --git a/tests/unit/test_socketcli.py b/tests/unit/test_socketcli.py index db5e8582..765150e9 100644 --- a/tests/unit/test_socketcli.py +++ b/tests/unit/test_socketcli.py @@ -80,8 +80,8 @@ def test_pr_context_provider_uses_integration_without_comment_adapter(): # # Only a pull request or merge request event has a baseline, so every other # pipeline gets a full scan. These drive create_scm_scan against a recording -# stub rather than asserting on the branch predicate, so swapping the call back -# to create_new_diff fails them. +# stub rather than asserting on a predicate, so they fail if the branch stops +# reaching create_full_scan_with_report_url. # --------------------------------------------------------------------------- From 7a986c897f988df4548a06b06ac7e27c449e677c Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Fri, 11 Sep 2026 19:12:13 -0400 Subject: [PATCH 23/27] fix(comments): parse legacy alert rows defensively Each row of the legacy comment table was unpacked through four consecutive splits with no bounds checks: five cells, then the markdown link, then the ecosystem, then the version. The row comes back from the provider's API, so a cell carrying an extra "|", a package cell that is not a link, or a name with no version raised out of the comment rewrite and ended the run before it reported status. A scoped package name in Socket's own table reached the same place with nobody doing anything unusual. parse_alert_table_row returns None instead of raising for any row it cannot read, and an unreadable row keeps its alert reported -- the safe direction, since a row that cannot be parsed cannot be evaluated against the ignore commands either. Also pins change-type preservation against the real artifact conversion rather than a stubbed field. The existing test assigned diffType by hand, so it would have passed whether or not the conversion populated it; the new one runs real DiffArtifact objects through both response shapes, and fails if the field is dropped. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 11 ++++--- socketsecurity/core/scm_comments.py | 51 +++++++++++++++++++++++------ tests/core/test_diff_generation.py | 36 +++++++++++++++++++- 3 files changed, 83 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8cec0c95..5bfe795d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,10 +75,13 @@ - Shared security comment copy no longer describes GitLab merge request output as Socket for GitHub. - Updating a security comment in the legacy table format no longer raises on a - scoped package name. That path split the package cell on every `@`, so a name - carrying its own `@` unpacked into three values; it now splits from the right, - matching the current comment format. Ignore commands for a scoped package are - accepted there in both the ecosystem-qualified and bare forms. + malformed row. Each row was unpacked through four consecutive splits with no + bounds checks, so a cell carrying an extra `|`, a package cell that is not a + markdown link, or a name with no version ended the run before it reported + status — and a scoped package name in Socket's own table was enough to trigger + it. Rows are now parsed defensively, and a row that cannot be read keeps its + alert reported. Ignore commands for a scoped package are accepted there in both + the ecosystem-qualified and bare forms. - Server URLs read from `GITHUB_SERVER_URL` and `CI_SERVER_URL` are validated as http(s) URLs before being composed into a diff scan's external link, matching the check already applied to the other repository URLs read from CI. diff --git a/socketsecurity/core/scm_comments.py b/socketsecurity/core/scm_comments.py index 16af560b..5277654f 100644 --- a/socketsecurity/core/scm_comments.py +++ b/socketsecurity/core/scm_comments.py @@ -145,6 +145,35 @@ def process_security_comment(comment: Comment, comments) -> str: return new_body + @staticmethod + def parse_alert_table_row(line: str) -> Optional[tuple[str, str, str]]: + """Pull ``(ecosystem, package, version)`` out of a legacy alert table row. + + Returns None for any row that does not have the expected shape rather than + raising. The row comes back from the provider's API, so its contents are + outside this process's control: a cell carrying an extra ``|``, a package + cell that is not a markdown link, or a name with no version all used to + raise out of the comment rewrite and take the run down before it reported + status. A row that cannot be read is a row whose alert stays reported. + """ + cells = line.strip().lstrip("|").rstrip("|").split("|") + if len(cells) != 5: + return None + package = cells[1] + if "](" not in package: + return None + details = package.split("](", 1)[0].lstrip("[") + if "/" not in details: + return None + ecosystem, remainder = details.split("/", 1) + if "@" not in remainder: + return None + # Split from the right: a scoped name carries its own "@". + pkg_name, pkg_version = remainder.rsplit("@", 1) + if not pkg_name or not pkg_version: + return None + return ecosystem, pkg_name, pkg_version + @staticmethod def process_original_security_comment( comment: Comment, @@ -160,19 +189,21 @@ def process_original_security_comment( start = True lines.append(line) elif start and "end-socket-alerts-table" not in line and not Comments.is_heading_line(line) and line != '': - title, package, introduced_by, manifest, ci = line.lstrip("|").rstrip("|").split("|") - details, _ = package.split("](") - ecosystem, details = details.split("/", 1) - ecosystem = ecosystem.lstrip("[") - # Split from the right: a scoped name carries its own "@". - pkg_name, pkg_version = details.rsplit("@", 1) + parsed = Comments.parse_alert_table_row(line) # ignore_all has to be checked outside the loop: an ignore-all # comment produces no ignore_commands, so a loop-internal check # never runs and every row was kept. - ignore = ignore_all or any( - Comments.is_ignore(pkg_name, pkg_version, name, version, ecosystem) - for name, version in ignore_commands - ) + if parsed is None: + # An unparseable row cannot be evaluated against the ignore + # commands, so keep it: leaving an alert reported is the safe + # direction, and the comment body is not ours to discard. + ignore = ignore_all + else: + ecosystem, pkg_name, pkg_version = parsed + ignore = ignore_all or any( + Comments.is_ignore(pkg_name, pkg_version, name, version, ecosystem) + for name, version in ignore_commands + ) if not ignore: kept_alert = True lines.append(line) diff --git a/tests/core/test_diff_generation.py b/tests/core/test_diff_generation.py index 63dfb4af..5150e3fe 100644 --- a/tests/core/test_diff_generation.py +++ b/tests/core/test_diff_generation.py @@ -1,8 +1,9 @@ import json -from dataclasses import fields +from dataclasses import asdict, fields from pathlib import Path import pytest +from socketdev.fullscans import DiffArtifact from socketsecurity.core import Core from socketsecurity.core.classes import Package @@ -108,6 +109,39 @@ def test_create_diff_report_preserves_package_change_types(core, diff_input): assert diff.removed_packages == [] assert {package.id for package in diff.replaced_packages} == {"dp2"} + +def _diff_artifact(change_type: str, flattened: bool) -> dict: + """A DiffArtifact of the given change type, in one of the two response shapes. + + The API sends flattened artifacts; the older shape carries the dependency + context in a head/base ref instead, and Package.from_diff_artifact reads + diffType differently in each. + """ + raw = json.loads( + (Path(__file__).parent.parent / "data/fullscans/diff/stream_diff.json").read_text() + )["data"]["artifacts"]["added"][0] + artifact = dict(raw, diffType=change_type, head=None, base=None) + if not flattened: + link = {"topLevelAncestors": ["x"], "direct": True, "artifact": None, + "dependencies": [], "manifestFiles": []} + key = "head" if change_type in ("added", "updated") else "base" + artifact[key] = [link] + return asdict(DiffArtifact.from_dict(artifact)) + + +@pytest.mark.parametrize("flattened", [True, False], ids=["flattened", "ref-shaped"]) +@pytest.mark.parametrize("change_type", ["added", "updated", "removed", "replaced"]) +def test_change_type_survives_artifact_conversion(change_type, flattened): + """The classification reads Package.diffType, so the conversion must set it. + + create_diff_report buckets on this field alone. A conversion that dropped it + would silently report every update as an addition and every replacement as a + removal, which is the inaccuracy the change-type split exists to prevent. + """ + package = Package.from_diff_artifact(_diff_artifact(change_type, flattened)) + + assert package.diffType == change_type + def create_input(core): # Get two different scans to compare head_scan = core.get_full_scan("head") From adb56c668a166bbd254990cacb350cd54a7f08e6 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Sat, 12 Sep 2026 00:07:09 -0400 Subject: [PATCH 24/27] feat(comments): add --ignore-authorization The write-access gate had no escape hatch, and its GitLab behavior when project membership cannot be read -- honor the command with a warning -- was the one deliberate weakness in it. Both are now a choice: enforce (default) require write access; honor with a warning where the provider cannot report it strict reject in that case instead off perform no check enforce closes the hole wherever the provider can answer without breaking a pipeline whose token cannot read membership, which is why it is the default. strict closes it everywhere and will fail those pipelines. off restores the prior behavior for anyone who needs comment-driven ignores from unverified authors. Threaded through the adapter constructors as a keyword argument with a default, so existing call sites keep working. With off the predicate is never handed to check_for_socket_comments at all, so nothing is filtered and no rejection is logged, rather than a gate that silently approves everything. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 6 +++- docs/cli-reference.md | 20 ++++++++--- socketsecurity/config.py | 15 ++++++++ socketsecurity/core/scm/github.py | 11 ++++-- socketsecurity/core/scm/gitlab.py | 26 ++++++++++---- socketsecurity/socketcli.py | 4 +-- tests/unit/test_ignore_authorization.py | 47 ++++++++++++++++++++++++- 7 files changed, 113 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bfe795d..bc82c10f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,7 +60,11 @@ enabling this does not silently break pipelines that relied on ignore commands. Use a `GITLAB_TOKEN` with API read access to get enforcement. - A rejected command is logged and is also absent from the ignore telemetry, which - records what was acted on. + records what was acted on. No acknowledgement reaction is added to a comment that + was not honored. +- `--ignore-authorization` selects the policy: `enforce` (default) requires write + access and honors the command with a warning where the provider cannot report it, + `strict` rejects it in that case instead, and `off` performs no check. ### Fixed: pull request and merge request comment accuracy diff --git a/docs/cli-reference.md b/docs/cli-reference.md index b78b45f6..c4baf64e 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -432,6 +432,7 @@ The launcher can be tuned via the `SOCKET_CLI_COANA_LAUNCHER` environment variab | `--ignore-commit-files` | False | False | Ignore commit files | | `--disable-blocking` | False | False | Non-blocking CI mode: the CLI always exits **0**, even when blocking alerts are present (including with `--strict-blocking`). Also exits 0 on uncaught runtime errors and Socket API failures, so the job is treated as successful while findings and errors are still logged. Takes precedence over `--strict-blocking`. | | `--disable-ignore` | False | False | Disable support for `@SocketSecurity ignore` commands in PR comments. When set, alerts cannot be suppressed via comments and ignore instructions are hidden from comment output. See [Who can ignore an alert](#who-can-ignore-an-alert). | +| `--ignore-authorization` | False | enforce | Who may suppress alerts with `@SocketSecurity ignore`. `enforce` requires write access and honors the command with a warning when the provider cannot report it; `strict` rejects it in that case; `off` honors any commenter. See [Who can ignore an alert](#who-can-ignore-an-alert). | | `--strict-blocking` | False | False | Fail on ANY security policy violations (blocking severity), not just new ones. Only works in diff mode. See [Strict Blocking Mode](#strict-blocking-mode) for details. | | `--enable-diff` | False | False | Enable diff mode even when using `--integration api` (forces diff mode without SCM integration) | | `--scm` | False | api | Source control management type | @@ -704,10 +705,21 @@ reported. `--disable-ignore` turns the feature off entirely. | GitLab | Project membership, read once per run when an ignore command is present. Developer (30) or above is honored. | The command is honored and a warning is logged. | GitLab notes carry no permission field, so the check needs a `GITLAB_TOKEN` that -can read `GET /projects/:id/members/all`. A `CI_JOB_TOKEN` generally cannot, and -in that case the CLI logs a warning and still honors the command rather than -breaking a pipeline that was already relying on it. Use a personal or group access -token with API read access to get enforcement. +can read `GET /projects/:id/members/all`. A `CI_JOB_TOKEN` generally cannot. + +`--ignore-authorization` decides what happens when access cannot be determined: + +| Value | Verified write access | Access cannot be determined | +|:------|:----------------------|:----------------------------| +| `enforce` (default) | Honored | Honored, with a warning naming the author | +| `strict` | Honored | Rejected | +| `off` | Honored | Honored, no check performed | + +`enforce` closes the hole wherever the provider can answer, without breaking a +pipeline whose token cannot read membership. `strict` closes it everywhere, at the +cost of failing those pipelines. `off` restores the prior behavior and should be +paired with `--disable-ignore` unless you specifically need comment-driven ignores +from unverified authors. ## GitLab Token Configuration diff --git a/socketsecurity/config.py b/socketsecurity/config.py index 63f0650c..cfea4189 100644 --- a/socketsecurity/config.py +++ b/socketsecurity/config.py @@ -144,6 +144,7 @@ class CliConfig: ignore_commit_files: bool = False disable_blocking: bool = False disable_ignore: bool = False + ignore_authorization: str = "enforce" # Tri-state log-upload preference: True = --upload-logs, False = --no-upload-logs, # None = neither (server-side override decides). upload_logs: Optional[bool] = None @@ -305,6 +306,7 @@ def from_args(cls, args_list: Optional[List[str]] = None) -> 'CliConfig': 'ignore_commit_files': args.ignore_commit_files, 'disable_blocking': args.disable_blocking, 'disable_ignore': args.disable_ignore, + 'ignore_authorization': args.ignore_authorization, 'upload_logs': args.upload_logs, 'strict_blocking': args.strict_blocking, 'integration_type': integration_type, @@ -724,6 +726,19 @@ def create_argument_parser() -> argparse.ArgumentParser: action="store_true", help="If true, the new scan will be set as the branch's head scan" ) + config_group.add_argument( + "--ignore-authorization", + dest="ignore_authorization", + choices=["enforce", "strict", "off"], + default="enforce", + help=( + "Who may suppress alerts with @SocketSecurity ignore comments. " + "'enforce' (default) requires write access, and honors the command with " + "a warning when the provider cannot report the commenter's access. " + "'strict' rejects the command in that case instead. " + "'off' honors a command from any commenter." + ) + ) config_group.add_argument( "--pending_head", dest="pending_head", diff --git a/socketsecurity/core/scm/github.py b/socketsecurity/core/scm/github.py index 35c63801..0a32b970 100644 --- a/socketsecurity/core/scm/github.py +++ b/socketsecurity/core/scm/github.py @@ -154,9 +154,15 @@ def from_env(cls, pr_number: Optional[str] = None) -> 'GithubConfig': class Github: - def __init__(self, client: CliClient, config: Optional[GithubConfig] = None): + def __init__( + self, + client: CliClient, + config: Optional[GithubConfig] = None, + ignore_authorization: str = "enforce", + ): self.config = config or GithubConfig.from_env() self.client = client + self.ignore_authorization = ignore_authorization if not self.config.token: log.error("Unable to get Github API Token") @@ -224,7 +230,8 @@ def get_comments_for_pr(self) -> dict: else: log.error(raw_comments) - return Comments.check_for_socket_comments(comments, self.is_ignore_authorized) + gate = None if self.ignore_authorization == "off" else self.is_ignore_authorized + return Comments.check_for_socket_comments(comments, gate) def is_ignore_authorized(self, comment: Comment) -> bool: """Whether a commenter may suppress alerts with @SocketSecurity ignore. diff --git a/socketsecurity/core/scm/gitlab.py b/socketsecurity/core/scm/gitlab.py index 5b8aa3fe..ff4d07df 100644 --- a/socketsecurity/core/scm/gitlab.py +++ b/socketsecurity/core/scm/gitlab.py @@ -134,9 +134,15 @@ class Gitlab: MEMBER_PAGE_SIZE = 100 MEMBER_PAGE_LIMIT = 10 - def __init__(self, client: CliClient, config: Optional[GitlabConfig] = None): + def __init__( + self, + client: CliClient, + config: Optional[GitlabConfig] = None, + ignore_authorization: str = "enforce", + ): self.config = config or GitlabConfig.from_env() self.client = client + self.ignore_authorization = ignore_authorization # None until the first ignore comment forces a lookup; stays None when the # members API cannot be read, which is the "undetermined" state. self._member_access: Optional[dict] = None @@ -268,7 +274,8 @@ def get_comments_for_pr(self) -> dict: comment.body_list = comment.body.split("\n") else: log.error(raw_comments) - return Comments.check_for_socket_comments(comments, self.is_ignore_authorized) + gate = None if self.ignore_authorization == "off" else self.is_ignore_authorized + return Comments.check_for_socket_comments(comments, gate) def _load_member_access(self) -> Optional[dict]: """Map project member user id -> access level, or None if unreadable. @@ -331,11 +338,18 @@ def is_ignore_authorized(self, comment: Comment) -> bool: """ access = self._load_member_access() if access is None: + author = Comments.comment_author_name(comment) + if self.ignore_authorization == "strict": + log.warning( + f"Rejecting @SocketSecurity ignore from {author}: GitLab project " + "membership could not be read and --ignore-authorization is strict." + ) + return False log.warning( - "Honoring @SocketSecurity ignore from " - f"{Comments.comment_author_name(comment)} without verifying write " - "access: GitLab project membership could not be read. Use a token " - "with API read access to enforce this." + f"Honoring @SocketSecurity ignore from {author} without verifying " + "write access: GitLab project membership could not be read. Use a " + "token with API read access, or --ignore-authorization strict to " + "reject instead." ) return True diff --git a/socketsecurity/socketcli.py b/socketsecurity/socketcli.py index 0509fa3e..022f6ffa 100644 --- a/socketsecurity/socketcli.py +++ b/socketsecurity/socketcli.py @@ -563,11 +563,11 @@ def main_code(): # Only pass pr_number if it's not "0" (the default) pr_number = config.pr_number if config.pr_number != "0" else None github_config = GithubConfig.from_env(pr_number=pr_number) - scm = Github(client=client, config=github_config) + scm = Github(client=client, config=github_config, ignore_authorization=config.ignore_authorization) elif config.scm == 'gitlab': from socketsecurity.core.scm.gitlab import Gitlab, GitlabConfig gitlab_config = GitlabConfig.from_env() - scm = Gitlab(client=client, config=gitlab_config) + scm = Gitlab(client=client, config=gitlab_config, ignore_authorization=config.ignore_authorization) # Don't override config.default_branch if it was explicitly set via --default-branch flag # Only use SCM detection if --default-branch wasn't provided if scm is not None and not config.default_branch: diff --git a/tests/unit/test_ignore_authorization.py b/tests/unit/test_ignore_authorization.py index 55143cc4..cd7069bf 100644 --- a/tests/unit/test_ignore_authorization.py +++ b/tests/unit/test_ignore_authorization.py @@ -72,9 +72,10 @@ def test_ignore_all_from_an_outsider_is_rejected_too(): # --- GitLab: notes carry no permission field, so membership is looked up ----- -def _gitlab(members_pages=None, raises=None): +def _gitlab(members_pages=None, raises=None, policy="enforce"): gitlab = Gitlab.__new__(Gitlab) gitlab.config = SimpleNamespace(mr_project_id="42", headers={}, api_url="https://gl/api/v4") + gitlab.ignore_authorization = policy gitlab._member_access = None gitlab._member_lookup_attempted = False @@ -142,3 +143,47 @@ def test_gitlab_oversized_membership_is_undetermined(): # Undetermined falls back to honoring the command, same as an API failure. assert gitlab.is_ignore_authorized(_comment(author={"id": 999})) is True assert len(gitlab.calls) == Gitlab.MEMBER_PAGE_LIMIT + + +# --- --ignore-authorization --------------------------------------------------- + + +def test_strict_rejects_when_membership_cannot_be_read(caplog): + """strict closes the gap enforce leaves open, at the cost of breaking a + pipeline whose token cannot read members.""" + gitlab = _gitlab(raises=Exception("403 Forbidden"), policy="strict") + + with caplog.at_level("WARNING", logger="socketcli"): + allowed = gitlab.is_ignore_authorized(_comment(author={"id": 7, "username": "dev"})) + + assert allowed is False + assert "strict" in caplog.text + + +def test_strict_still_honors_a_verified_member(): + gitlab = _gitlab([[{"id": 7, "access_level": 40}]], policy="strict") + + assert gitlab.is_ignore_authorized(_comment(author={"id": 7})) is True + + +def test_off_skips_the_gate_entirely(): + """off restores the prior behavior: no predicate reaches the bucketing, so + nothing is filtered and no rejection is logged.""" + github = Github.__new__(Github) + github.ignore_authorization = "off" + github.config = SimpleNamespace(owner="o", repository="r", pr_number="1", + headers={}, api_url="https://api.github.com") + github.client = SimpleNamespace(request=lambda **kw: SimpleNamespace( + json=lambda: [{"id": 1, "body": "@SocketSecurity ignore npm/lodash@4.17.21", + "author_association": "NONE", "user": {"login": "outsider"}}], + text="")) + + bucketed = github.get_comments_for_pr() + + assert len(bucketed.get("ignore", [])) == 1 + + +def test_enforce_is_the_default_policy(): + from socketsecurity.config import CliConfig + + assert CliConfig.from_args(["--api-token", "t"]).ignore_authorization == "enforce" From b8502bb4549eb9afdff2ed120c1c036d1dfe67f5 Mon Sep 17 00:00:00 2001 From: lelia <2418071+lelia@users.noreply.github.com> Date: Sat, 12 Sep 2026 00:28:08 -0400 Subject: [PATCH 25/27] fix(comments): escape repository-derived values when rendering comments Manifest paths and sources are file paths inside the scanned repository, so anyone who can open a pull request controls them: a directory named with link or tag syntax, holding a manifest, put that markup into a comment posted by a trusted integration. Alert text comes from the API. Neither is markup the CLI authored, so both are escaped where they are interpolated -- text nodes with html.escape, href and src with quotes escaped too, since an unescaped quote closes the attribute and everything after it reads as more attributes. The alert markers are the exception: they are read back verbatim when a comment is rewritten, so they cannot be escaped without breaking the ignore round trip. They instead lose only the ability to terminate the comment early. plain and raw styles are untouched. Slack, Jira and the console do not render HTML, and escaping there would show entities to a human. Round-trip tests render a comment with each hostile path and feed it back through the parser, because the renderer and the parser are two halves of one loop: an escaping choice the parser cannot read would silently stop ignores working. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 6 + socketsecurity/core/messages.py | 87 ++++++++++---- tests/unit/test_pr_comment_rendering.py | 145 ++++++++++++++++++++++++ 3 files changed, 218 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc82c10f..f4831047 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,6 +89,12 @@ - Server URLs read from `GITHUB_SERVER_URL` and `CI_SERVER_URL` are validated as http(s) URLs before being composed into a diff scan's external link, matching the check already applied to the other repository URLs read from CI. +- Repository-derived values are escaped before they are rendered into a pull + request or merge request comment. Manifest paths and sources are file paths from + the scanned repository, and alert text comes from the API; neither is markup the + CLI authored, so both are now escaped at the point they are interpolated. The + alert markers can no longer be terminated early by a package name. Slack, Jira + and console output are unchanged, since none of them render HTML. ## 2.8.1 diff --git a/socketsecurity/core/messages.py b/socketsecurity/core/messages.py index 33681f97..a07fe961 100644 --- a/socketsecurity/core/messages.py +++ b/socketsecurity/core/messages.py @@ -837,6 +837,37 @@ def inline_html_text(value) -> str: return "" return " ".join(str(value).split()) + @staticmethod + def html_text(value) -> str: + """Flatten a value onto one line and escape it for an HTML text node. + + Manifest paths and sources come from the customer's repository, so any PR + author controls them: a directory named ``![x](https://host/p.png)`` or + carrying a raw tag would otherwise render as that markup inside a comment + posted by a trusted integration. Alert text comes from the API and is + escaped for the same reason, since neither is markup the CLI authored. + """ + return escape(Messages.inline_html_text(value)) + + @staticmethod + def html_attr(value) -> str: + """Escape a value for an HTML attribute, quotes included. + + Used for href and src, where an unescaped quote closes the attribute and + everything after it is read as more attributes. + """ + return escape(Messages.inline_html_text(value), quote=True) + + @staticmethod + def comment_marker_text(value) -> str: + """Neutralize an HTML comment terminator inside a marker value. + + The alert markers carry the package name so the comment can be rewritten + later, and the parser reads them back verbatim -- so this cannot escape the + value, only stop it ending the comment early. + """ + return str(value or "").replace("-->", "-->").replace(" + - + """ # Add license policy violation entries grouped by PURL @@ -1007,24 +1042,33 @@ def security_comment_template(diff: Diff, config=None) -> str: # Use orange diamond for license policy violations license_icon = "🔶" + license_label = Messages.html_text( + f"{first_alert.pkg_name}@{first_alert.pkg_version}" + ) + # The marker is read back verbatim when the comment is rewritten, so it + # keeps the raw name and only loses the ability to close the comment. + license_marker = Messages.comment_marker_text( + f"{first_alert.pkg_name}@{first_alert.pkg_version}" + ) + # Build license findings list license_findings = [] for alert in alerts: license_findings.append(alert.title) comment += f""" - + - + """ # Close table @@ -1408,8 +1452,11 @@ def create_sources(alert: Issue, style="md") -> tuple[str, str]: for source, manifest in alert.introduced_by: if style == "md": - add_str = f"
  • {manifest}
    • " - source_str = f"
  • {source}
    • " + # These land in rendered Markdown, where an unescaped path is read + # as markup. plain and raw are consumed by Slack, Jira and the + # console, which do not render HTML, so they stay verbatim. + add_str = f"
  • {Messages.html_text(manifest)}
    • " + source_str = f"
  • {Messages.html_text(source)}
    • " elif style == "plain": add_str = f"• {manifest}" source_str = f"• {source}" diff --git a/tests/unit/test_pr_comment_rendering.py b/tests/unit/test_pr_comment_rendering.py index 020b636e..717fa68f 100644 --- a/tests/unit/test_pr_comment_rendering.py +++ b/tests/unit/test_pr_comment_rendering.py @@ -9,6 +9,8 @@ from dataclasses import dataclass +import pytest + from socketsecurity.core.classes import Comment, Diff, Issue from socketsecurity.core.messages import Messages from socketsecurity.core.scm_comments import Comments @@ -358,3 +360,146 @@ def test_strips_the_action_filter(self): def test_returns_empty_when_absent(self): assert Comments.extract_report_url("no link here") == "" + + +# --- Escaping repo-derived values --------------------------------------------- +# +# Manifest paths and sources are file paths inside the customer's repository, so +# anyone who can open a pull request controls them: a directory named +# `![x](https://host/p.png)` holding a manifest puts that markup into a comment +# posted by a trusted integration. GitHub and GitLab sanitize comment HTML, so the +# exposure is external resource loading, phishing links and content spoofing +# rather than script execution. + + +@dataclass +class _RepoConfig(_FakeConfig): + """A config that reaches the branch which embeds the path verbatim. + + Without repo/branch, get_manifest_file_url returns "" or a percent-encoded + Socket link, and the path never lands in the comment -- so a test using the + bare config asserts nothing. + """ + repo: str = "acme/widgets" + branch: str = "main" + + +HOSTILE_PATHS = { + "image": "![x](https://evil.example/p.png)/package.json", + "link": "[click me](https://evil.example)/package.json", + "raw_tag": "/package.json", + "backtick": "`code`/package.json", + "pipe": "a|b/package.json", + "quote": 'a" onmouseover="x/package.json', + "comment_close": "x-->y/package.json", +} + + +def _rendered_with_path(path: str) -> str: + return Messages.security_comment_template( + _make_diff([_make_alert(manifests=path)]), _RepoConfig() + ) + + +def test_the_hostile_path_actually_reaches_the_comment(): + """Guards the fixture itself: if the path stops being rendered, the escaping + tests below would pass while asserting nothing.""" + body = _rendered_with_path("sentinel-path/package.json") + + assert "sentinel-path" in body + + +@pytest.mark.parametrize("name,path", sorted(HOSTILE_PATHS.items())) +def test_hostile_manifest_path_cannot_introduce_markup(name, path): + """In the rendered comment the path only ever lands inside an href, where + Markdown is inert. The property that matters there is that the value cannot + open a tag or close the attribute -- see create_sources for the context where + Markdown itself is live.""" + body = _rendered_with_path(path) + + rendered = [ln for ln in body.split("\n") if "Manifest File" in ln][0] + value = rendered.split('href="', 1)[1].split('"', 1)[0] + + for char in ("<", ">", '"'): + assert char not in value, f"{char!r} survived into the href: {value!r}" + assert_html_block_intact(body) + + +def test_quote_in_a_path_cannot_escape_the_href(): + body = _rendered_with_path('a" onmouseover="x/package.json') + + assert """ in body + assert 'href="https://github.com/acme/widgets/blob/main/a" ' not in body + + +def test_hostile_package_name_cannot_close_the_alert_marker(): + body = Messages.security_comment_template( + _make_diff([_make_alert(pkg_name="evil-->x")]), _FakeConfig() + ) + + # Exactly the terminator the CLI wrote, and no stray one inside the value. + for line in body.split("\n"): + if "socket-alert-" in line: + assert line.count("-->") == 1, line + + +def test_alert_text_from_the_api_is_escaped(): + body = Messages.security_comment_template( + _make_diff([_make_alert(description="")]), _FakeConfig() + ) + + assert "
    {action} - {alert.severity} + {Messages.html_attr(alert.severity)}
    - {alert.pkg_name}@{alert.pkg_version} - {Messages.inline_html_text(alert.title)} -

    Note: {Messages.inline_html_text(alert.description)}

    + {pkg_label} - {Messages.html_text(alert.title)} +

    Note: {Messages.html_text(alert.description)}

    {patched_version_html} -

    Source: Manifest File

    +

    Source: Manifest File

    ℹ️ Read more on: - This package | - This alert | + This package | + This alert | What is known malware?

    -

    Suggestion: {Messages.inline_html_text(alert.suggestion)}

    +

    Suggestion: {Messages.html_text(alert.suggestion)}

    {ignore_html}
    {action} {license_icon}
    - {first_alert.pkg_name}@{first_alert.pkg_version} has a License Policy Violation. + {license_label} has a License Policy Violation.

    License findings:

      """ for finding in license_findings: - comment += f"
    • {Messages.inline_html_text(finding)}
      • \n" + comment += f"
    • {Messages.html_text(finding)}
      • \n" # Generate proper manifest URL for license violations @@ -1032,13 +1076,13 @@ def security_comment_template(diff: Diff, config=None) -> str: license_ignore_html = ( f"

      Mark the package as acceptable risk: To ignore this alert only in this pull request, reply with the comment " - f"@SocketSecurity ignore {first_alert.pkg_type}/{first_alert.pkg_name}@{first_alert.pkg_version}. " + f"@SocketSecurity ignore {Messages.html_text(first_alert.pkg_type)}/{license_label}. " f"You can also ignore all packages with @SocketSecurity ignore-all. " f"To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

      " ) if show_ignore else "" comment += f"""
    -

    From: Manifest File

    -

    ℹ️ Read more on: This package | What is a license policy violation?

    +

    From: Manifest File

    +

    ℹ️ Read more on: This package | What is a license policy violation?

    Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

    Suggestion: Find a package that does not violate your license policy or adjust your policy to allow this package's license.

    @@ -1047,7 +1091,7 @@ def security_comment_template(diff: Diff, config=None) -> str: