diff --git a/CHANGELOG.md b/CHANGELOG.md index c87837da..c35b9442 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,28 @@ # Changelog +## 2.6.5 + +### 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.6.4 ### Changed: bump pinned @coana-tech/cli to 15.10.13 diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 66193f38..546297ca 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. + #### Merge-base baselines in Buildkite (dynamic pipelines) Notes for using `--base-commit-sha` (see the @@ -139,6 +186,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 @@ -149,6 +208,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: @@ -165,3 +262,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 49be8dfa..9d5d6ab4 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/pyproject.toml b/pyproject.toml index 313a8fae..48ae5907 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,7 +6,7 @@ build-backend = "hatchling.build" [project] name = "socketsecurity" -version = "2.6.4" +version = "2.6.5" requires-python = ">= 3.11" license = {"file" = "LICENSE"} dependencies = [ diff --git a/socketsecurity/__init__.py b/socketsecurity/__init__.py index 9d9963d7..5875d7cb 100644 --- a/socketsecurity/__init__.py +++ b/socketsecurity/__init__.py @@ -1,3 +1,3 @@ __author__ = 'socket.dev' -__version__ = '2.6.4' +__version__ = '2.6.5' USER_AGENT = f'SocketPythonCLI/{__version__}' diff --git a/socketsecurity/config.py b/socketsecurity/config.py index 26542447..6ef8597c 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 7be24858..8f794684 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -1329,7 +1329,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. @@ -1352,6 +1353,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 @@ -1361,6 +1364,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 {} @@ -1444,7 +1449,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. @@ -1477,6 +1483,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 @@ -1488,7 +1496,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 @@ -1592,7 +1601,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. @@ -1604,6 +1614,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: @@ -1728,7 +1740,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/messages.py b/socketsecurity/core/messages.py index 319e454b..6b502bb9 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: """ @@ -857,6 +866,11 @@ 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 = ( + f"

Patched version: {escape(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 @@ -877,6 +891,7 @@ def security_comment_template(diff: Diff, config=None) -> str:
{alert.pkg_name}@{alert.pkg_version} - {alert.title}

Note: {alert.description}

+ {patched_version_html}

Source: Manifest File

ℹ️ Read more on: This package | @@ -1247,6 +1262,7 @@ def create_console_security_alert_table(diff: Diff) -> PrettyTable: [ "Alert", "Package", + "Patched Version", "url", "Introduced by", "Manifest File", @@ -1267,6 +1283,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/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 0d8bcccb..4b68e208 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 @@ -107,6 +108,11 @@ def get_api_request_timeout(config: CliConfig) -> int: return config.timeout if config.timeout is not None else DEFAULT_API_TIMEOUT +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}" return socketdev( @@ -568,10 +574,26 @@ def main_code(): core.config.excluded_ecosystems = config.excluded_ecosystems 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: @@ -684,7 +706,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=sbom_files_to_submit) + 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=sbom_files_to_submit, + 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 @@ -807,14 +838,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=sbom_files_to_submit) + 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=sbom_files_to_submit, + 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=sbom_files_to_submit) + 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=sbom_files_to_submit, + external_href=pr_context.url, + ) output_handler.handle_output(diff) elif (config.enable_diff or force_diff_mode) and force_api_mode: @@ -868,7 +917,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=sbom_files_to_submit + explicit_files=sbom_files_to_submit, + 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 be369429..d0e019cc 100644 --- a/tests/core/test_diff_scan_polling.py +++ b/tests/core/test_diff_scan_polling.py @@ -104,6 +104,15 @@ def test_duplicate_conflict_uses_cached_polling(core, diff_scan_get_response): assert len(artifacts.added) > 0 +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), the comparison falls back to the legacy streaming diff transparently.""" diff --git a/tests/unit/test_cli_config.py b/tests/unit/test_cli_config.py index 39447c2c..f70cda2b 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_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 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 39f59f5b..8efa6521 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 - # --------------------------------------------------------------------------- # 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/uv.lock b/uv.lock index 8fd80913..156b9936 100644 --- a/uv.lock +++ b/uv.lock @@ -1282,7 +1282,7 @@ wheels = [ [[package]] name = "socketsecurity" -version = "2.6.4" +version = "2.6.5" source = { editable = "." } dependencies = [ { name = "beautifulsoup4" }, 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.