From 4be88c974d2f8bba90d4510c85f65e9ae8420ec2 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 1/5] feat(output): show patched versions in security findings --- socketsecurity/core/messages.py | 18 ++++++++ tests/unit/test_messages.py | 81 +++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 tests/unit/test_messages.py diff --git a/socketsecurity/core/messages.py b/socketsecurity/core/messages.py index 673dde5..d4e8c9e 100644 --- a/socketsecurity/core/messages.py +++ b/socketsecurity/core/messages.py @@ -4,7 +4,9 @@ import re import uuid from datetime import datetime, timezone +from html import escape from pathlib import Path + from mdutils import MdUtils from prettytable import PrettyTable @@ -14,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: """ @@ -948,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 @@ -968,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 | @@ -1331,6 +1347,7 @@ def create_console_security_alert_table(diff: Diff) -> PrettyTable: [ "Alert", "Package", + "Patched Version", "url", "Introduced by", "Manifest File", @@ -1351,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 0000000..ce6b696 --- /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 ba96d5b3cc7e145367b27d5967000ffd8da58206 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 2/5] feat(ci): preserve pull request context in scan metadata --- docs/ci-cd.md | 111 +++++++++++- docs/cli-reference.md | 4 +- socketsecurity/config.py | 45 +++-- 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 | 55 ++++++ tests/unit/test_pull_request_context.py | 228 ++++++++++++++++++++++++ tests/unit/test_socketcli.py | 12 +- workflows/buildkite.yml | 21 ++- 11 files changed, 680 insertions(+), 37 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 061d18e..ec91aba 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 @@ -73,14 +101,33 @@ Equivalent JSON: ### 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 @@ -152,6 +199,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 @@ -162,6 +221,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: @@ -178,3 +275,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 6c94daf..d0f3998 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -175,7 +175,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) | @@ -189,7 +189,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 2654244..6ef8597 100644 --- a/socketsecurity/config.py +++ b/socketsecurity/config.py @@ -1,12 +1,14 @@ import argparse +import json import logging import os +import tomllib from dataclasses import asdict, dataclass, field from typing import List, Optional -from socketsecurity import __version__ + from socketdev import INTEGRATION_TYPES, IntegrationType -import json -import tomllib + +from socketsecurity import __version__ def get_plugin_config_from_env(prefix: str) -> dict: @@ -113,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 = "./" @@ -206,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: @@ -217,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( @@ -260,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), @@ -292,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, @@ -517,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", @@ -533,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 a5305be..ce41a1b 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -1596,7 +1596,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. @@ -1619,6 +1620,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 @@ -1628,6 +1631,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 {} @@ -1776,7 +1781,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. @@ -1809,6 +1815,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 @@ -1820,7 +1828,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 @@ -1933,7 +1942,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. @@ -1945,6 +1955,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: @@ -2069,7 +2081,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 0000000..cff0c18 --- /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 0c967cc..e0a4ab2 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 @@ -127,6 +128,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}" @@ -596,10 +601,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: @@ -718,7 +739,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 @@ -840,14 +870,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: @@ -904,7 +952,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 d8c0e39..df95f04 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 39447c2..f70cda2 100644 --- a/tests/unit/test_cli_config.py +++ b/tests/unit/test_cli_config.py @@ -1,4 +1,5 @@ import pytest + from socketsecurity.config import CliConfig @@ -67,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 0000000..5ad1290 --- /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 8cae52b..ee446d2 100644 --- a/tests/unit/test_socketcli.py +++ b/tests/unit/test_socketcli.py @@ -2,11 +2,10 @@ import pytest -from socketsecurity.core.classes import Diff, Package from socketsecurity import socketcli +from socketsecurity.core.classes import Diff, Package from socketsecurity.socketcli import build_license_artifact_payload, should_write_comment - # --------------------------------------------------------------------------- # Exit-code-on-api-error (flag-only, non-breaking for 2.3.x). # @@ -63,6 +62,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 a2f8e45..3657f28 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 0c1d5649ba4eae700300bb0a1d8c7ac39b3f1dd3 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 3/5] chore(release): bump version to 2.8.0 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 2115073..3eb379d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,28 @@ # Changelog +## 2.8.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.7.0 ### Fixed: unreadable reachability facts no longer report a blocking package diff --git a/pyproject.toml b/pyproject.toml index 293bbd1..0cafbaf 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,7 +6,7 @@ build-backend = "hatchling.build" [project] name = "socketsecurity" -version = "2.7.0" +version = "2.8.0" requires-python = ">= 3.11" license = {"file" = "LICENSE"} dependencies = [ diff --git a/socketsecurity/__init__.py b/socketsecurity/__init__.py index d72ecc6..a97b4d1 100644 --- a/socketsecurity/__init__.py +++ b/socketsecurity/__init__.py @@ -1,3 +1,3 @@ __author__ = 'socket.dev' -__version__ = '2.7.0' +__version__ = '2.8.0' USER_AGENT = f'SocketPythonCLI/{__version__}' diff --git a/uv.lock b/uv.lock index 90f2366..542a862 100644 --- a/uv.lock +++ b/uv.lock @@ -1282,7 +1282,7 @@ wheels = [ [[package]] name = "socketsecurity" -version = "2.7.0" +version = "2.8.0" source = { editable = "." } dependencies = [ { name = "beautifulsoup4" }, From 635bc7d91e5288ca38675780a3873e50c43307b3 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 4/5] 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 ec91aba..045c10a 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -135,11 +135,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) @@ -246,13 +247,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 0000000..9ca5dc5 --- /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 cff0c18..60ad396 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 7504a46..9ec1e4c 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 0000000..c589132 --- /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 3cd035500a1982ef571d7519847e3485dce5a520 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 5/5] 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 +++++++++++++++++++++++++++- tests/unit/test_socketcli.py | 5 ++- 4 files changed, 70 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3eb379d..9490dc4 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.7.0 diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index ce41a1b..5e648e5 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -1633,6 +1633,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 {} @@ -1641,11 +1649,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 df95f04..d30bf19 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): diff --git a/tests/unit/test_socketcli.py b/tests/unit/test_socketcli.py index ee446d2..14f8372 100644 --- a/tests/unit/test_socketcli.py +++ b/tests/unit/test_socketcli.py @@ -4,7 +4,10 @@ from socketsecurity import socketcli from socketsecurity.core.classes import Diff, Package -from socketsecurity.socketcli import build_license_artifact_payload, should_write_comment +from socketsecurity.socketcli import ( + build_license_artifact_payload, + should_write_comment, +) # --------------------------------------------------------------------------- # Exit-code-on-api-error (flag-only, non-breaking for 2.3.x).