diff --git a/CHANGELOG.md b/CHANGELOG.md index b6eebb26..469a4eee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,121 @@ # Changelog +## 2.9.0 + +### Added: patched versions in human-readable security output + +- The native console alert table now includes a `Patched Version` column, + populated from `props.firstPatchedVersionIdentifier` when the API provides it. +- GitHub pull request and GitLab merge request security comments now show the + patched version in each applicable alert's details. + +### Fixed: CLI scans retain pull request context in the Socket Dashboard + +- Pull request numbers are detected from standard GitHub Actions, GitLab CI, + and Azure Pipelines environments when `--pr-number` is not supplied. An + explicitly supplied value, including `0`, remains authoritative. +- The Buildkite workflow and CI/CD guide now forward `BUILDKITE_PULL_REQUEST` + explicitly and document provider selection for Dashboard PR association. With + `--integration github` or `--integration gitlab`, the repository slug and host + for the link are read from `BUILDKITE_REPO`, covering self-hosted installations. +- `--scm github` and `--scm gitlab` now imply the matching scan integration + unless `--integration` is explicitly supplied. +- Diff scans include the detected pull request or merge request URL as their + external link, allowing Dashboard reports to retain their CI change context. + 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. +- A `--pr-number` value that is not a positive integer is now normalized to `0` + before the GitHub adapter reads it, so Buildkite's `false` on a branch build no + longer makes that build look like a pull request event. + +### Changed: GitHub and GitLab branch pipelines create full scans + +- With `--scm github` or `--scm gitlab`, only pull request and merge request + events create diff scans. Every other pipeline, including default-branch + pushes, creates a full scan. The detected event type is authoritative: + `--enable-diff` and `--ignore-commit-files` no longer opt an SCM branch run + into comparison mode. +- Those runs no longer set a blocking exit code. A full scan has no baseline, so + it cannot distinguish newly introduced alerts from pre-existing ones; the CLI + now behaves as if `--disable-blocking` was supplied, matching how it already + treats a run with no supported manifest files. Pull request and merge request + pipelines are unaffected and still block. +- `--generate-license` and `--legal-format fossa` fetch the package list on this + path, so attribution files generated from a branch pipeline are complete rather + than empty. +- Console-only full scans link to the Socket report and state that findings were + not fetched for console output instead of presenting an empty local alert list + as "No issues found." +- License enrichment keeps the package namespace in PURL requests and response + matching, so scoped npm packages and namespaced Maven packages receive their + license details. + +### Changed: `@SocketSecurity ignore` requires write access + +- An ignore command suppresses a security alert, but the CLI honored one from any + commenter, including a drive-by comment from someone with no access to the + repository. Commands are now accepted only from an author with write access. +- On GitHub this is read from the effective repository permission and cached per + commenter for the run. Write, maintain, or admin access is required; relationship + labels such as `MEMBER` and `COLLABORATOR` are not treated as permissions. +- GitLab notes carry no equivalent field, so project membership is read once per + run (only when an ignore command is present) and Developer or above is required. + If that lookup cannot be answered — a `CI_JOB_TOKEN` generally cannot read the + members API — the command is still honored and a warning names the author, so + enabling this does not silently break pipelines that relied on ignore commands. + Use a `GITLAB_TOKEN` with API read access to get enforcement. +- A rejected command is logged and is also absent from the ignore telemetry, which + records what was acted on. No acknowledgement reaction is added to a comment that + was not honored. +- `--ignore-authorization` selects the policy: `enforce` (default) requires write + access and honors the command with a warning where the provider cannot report it, + `strict` rejects it in that case instead, and `off` performs no check. + +### Fixed: GitLab authentication fallback never ran + +- When a GitLab token's type cannot be inferred from its shape, the CLI guesses + between Bearer and PRIVATE-TOKEN and retries once under the other scheme on a + 401. That retry never happened: the retry caught `requests.exceptions.HTTPError`, + but the HTTP client translates every request error into `APIFailure` first, so a + misclassified token failed the run instead of falling back. +- API failures raised by the CLI's HTTP client now carry their HTTP status code. + Without it a 401 was indistinguishable from any other failure, and + `is_transient_error` could not classify one either. +- The CLI's `APIFailure` now subclasses the SDK exception of the same name. They + were independent types, so an `except APIFailure` importing the SDK's — which is + what every handler in `socketsecurity.core` does — did not catch a failure raised + by the HTTP client. + +### Fixed: pull request and merge request comment accuracy + +- Per-alert ignore instructions now use ecosystem-qualified package names and + accept scoped packages while remaining compatible with older bare-name replies. + A leading npm scope is no longer mistaken for an ecosystem, so + `ignore @types/node@*` no longer also ignores the package named `node`. +- Dependency overviews preserve added, updated, removed, and replaced package + classifications instead of presenting updates as new dependencies. Added and + updated rows keep their diff badge; removed and replaced, which have no + published badge, use a text label. +- Shared security comment copy no longer describes GitLab merge request output + as Socket for GitHub. +- Updating a security comment in the legacy table format no longer raises on a + malformed row. Each row was unpacked through four consecutive splits with no + bounds checks, so a cell carrying an extra `|`, a package cell that is not a + markdown link, or a name with no version ended the run before it reported + status — and a scoped package name in Socket's own table was enough to trigger + it. Rows are now parsed defensively, and a row that cannot be read keeps its + alert reported. Ignore commands for a scoped package are accepted there in both + the ecosystem-qualified and bare forms. +- Server URLs read from `GITHUB_SERVER_URL` and `CI_SERVER_URL` are validated as + http(s) URLs before being composed into a diff scan's external link, matching + the check already applied to the other repository URLs read from CI. +- Repository-derived values are escaped before they are rendered into a pull + request or merge request comment. Manifest paths and sources are file paths from + the scanned repository, and alert text comes from the API; neither is markup the + CLI authored, so both are now escaped at the point they are interpolated. The + alert markers can no longer be terminated early by a package name. Slack, Jira + and console output are unchanged, since none of them render HTML. + ## 2.8.1 ### Changed: bump pinned @coana-tech/cli to 15.10.40 diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 968799b5..3c819b13 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -2,6 +2,10 @@ Use this guide for pipeline-focused CLI usage across platforms. +The shell commands in the recommended patterns are CI-provider neutral. Buildkite +pipeline equivalents and provider-specific considerations are called out alongside +the relevant guidance below. + ## Recommended patterns ### Dashboard-style reachable SARIF @@ -27,6 +31,27 @@ socketcli \ --strict-blocking ``` +### Buildkite: retain SARIF as a build artifact + +Either recommended pattern can run directly in a Buildkite command step. When the +scan writes SARIF, add +[`artifact_paths`](https://buildkite.com/docs/pipelines/configure/artifacts#upload-artifacts-with-a-command-step) +so developers can download the report from the build after the command finishes: + +```yaml +steps: + - label: ":socket: Socket reachable diff" + command: | + socketcli \ + --reach \ + --sarif-file results.sarif \ + --sarif-scope diff \ + --sarif-reachability reachable \ + --strict-blocking + artifact_paths: + - "results.sarif" +``` + ## Config file usage in CI Use `--config .socketcli.toml` or `--config .socketcli.json` to keep pipeline commands small. @@ -60,6 +85,9 @@ Equivalent JSON: } ``` +The Buildkite examples below use the same checked-in `.socketcli.toml` file; no +Buildkite-specific config-file format is required. + ## Platform examples ### GitHub Actions @@ -306,14 +334,33 @@ initial timeout signal or 137 if `SIGKILL` is involved. ### Buildkite +This example assumes a GitHub-hosted repository. Change +`SOCKET_SCM_INTEGRATION` to `gitlab` for a GitLab-hosted repository, or `api` +when provider association is not wanted. The doubled dollar signs defer +Buildkite variable expansion until the command runs on an agent. + ```yaml +env: + SOCKET_SCM_INTEGRATION: "github" + steps: - label: "Socket scan" - command: "socketcli --config .socketcli.toml --target-path ." - env: - SOCKET_SECURITY_API_TOKEN: "${SOCKET_SECURITY_API_TOKEN}" + command: | + socketcli \ + --config .socketcli.toml \ + --target-path . \ + --integration "$${SOCKET_SCM_INTEGRATION:-api}" \ + --pr-number "$${BUILDKITE_PULL_REQUEST:-0}" + secrets: + - SOCKET_SECURITY_API_TOKEN ``` +The `secrets` block expects a +[Buildkite secret](https://buildkite.com/docs/pipelines/security/secrets/buildkite-secrets) +named `SOCKET_SECURITY_API_TOKEN`. If your organization uses an external secrets +plugin or an agent hook instead, remove that block and inject the same environment +variable through your existing mechanism. Do not store the token in pipeline YAML. + The CLI reads Buildkite's native `BUILDKITE_COMMIT`, `BUILDKITE_BRANCH`, `BUILDKITE_PULL_REQUEST`, and `BUILDKITE_PULL_REQUEST_BASE_BRANCH` variables. For pull-request builds, ensure the checkout contains the base branch and the @@ -321,11 +368,12 @@ checked-out head commit. The CLI uses those local refs first and performs a targeted fetch only when a required ref or its comparison history is missing; it does not fetch every remote ref and tag during startup. -When `--scm github` is used from Buildkite, the CLI also derives GitHub comment -context from `BUILDKITE_REPO`, `BUILDKITE_BUILD_CHECKOUT_PATH`, and the variables -above. Set `GH_API_TOKEN` to a GitHub token with the required repository access. -GitHub Enterprise users should also set `GITHUB_API_URL`; GitHub.com defaults to -`https://api.github.com`. +When `--scm github` is used from Buildkite, the CLI also posts GitHub PR comments. +It identifies the repository from `BUILDKITE_REPO` and takes the rest of the build +context from `BUILDKITE_BUILD_CHECKOUT_PATH` and the variables above — see +[Buildkite PR context](#buildkite-pr-context). Set `GH_API_TOKEN` to a GitHub token +with the required repository access. GitHub Enterprise users should also set +`GITHUB_API_URL`; GitHub.com defaults to `https://api.github.com`. #### Merge-base baselines in Buildkite (dynamic pipelines) @@ -385,6 +433,18 @@ socket_scan: SOCKET_SECURITY_API_TOKEN: $SOCKET_SECURITY_API_TOKEN ``` +### Azure Pipelines + +```yaml +- script: | + socketcli \ + --integration azure \ + --enable-diff \ + --target-path "$(Build.SourcesDirectory)" + env: + SOCKET_SECURITY_API_TOKEN: $(SOCKET_SECURITY_API_TOKEN) +``` + ### Bitbucket Pipelines ```yaml @@ -395,6 +455,69 @@ pipelines: - socketcli --config .socketcli.toml --target-path . ``` +## Scan type by pipeline + +With `--scm github` or `--scm gitlab`, the detected event decides the scan type: + +| Event | Scan | Blocks the build | +|:------|:-----|:-----------------| +| Pull request / merge request | Diff scan against the repository's baseline | Yes, on newly introduced alerts | +| Any other pipeline, including default-branch pushes | Full scan | No | + +A full scan has no baseline, so it cannot tell a newly introduced alert from one +that was already there. Rather than block on a number that would mean something +different depending on which output format was enabled, those runs behave as if +`--disable-blocking` was supplied and report through the Dashboard instead. This +matches how the CLI already treats a run with no supported manifest files. + +The event type is authoritative once `--scm` is set: `--enable-diff` and +`--ignore-commit-files` do not turn a branch pipeline into a comparison. To diff +a branch build, drop `--scm` and use `--enable-diff` with `--integration`, which +runs the comparison without the PR comment adapter. + +`--generate-license` and `--legal-format fossa` work on both paths; a full scan +fetches the package list for them. + +## Pull request and Dashboard association + +The CLI sends the resolved pull request number with each full scan and attaches +the pull request URL to diff scans so the Socket Dashboard can associate the +report with its originating change. If `--pr-number` is supplied, it wins; +passing `--pr-number 0` explicitly disables automatic association. Any value that +is not a positive integer, including Buildkite's `false`, means no pull request. + +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. 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 +remain limited to the existing GitHub and GitLab SCM adapters; Azure receives +console output and Dashboard association but does not post a PR comment. + ## Workflow templates Prebuilt examples in this repo: @@ -411,3 +534,11 @@ Prebuilt examples in this repo: - `--sarif-grouping alert` currently applies to `--sarif-scope full`. - Diff-based SARIF can validly be empty when there are no matching net-new alerts. - Keep API tokens in secret stores (`SOCKET_SECURITY_API_TOKEN`), not in config files. +- In Buildkite pipeline YAML, follow its + [runtime interpolation](https://buildkite.com/docs/pipelines/configure/environment-variables#runtime-variable-interpolation) + guidance and use `$$` for variables that must expand when the command runs rather + than when the pipeline is uploaded. +- Security findings with `props.firstPatchedVersionIdentifier` show that value in + the console table, including native Buildkite job logs, and in GitHub/GitLab + security comments when that SCM adapter is configured. Findings without a known + patched release leave the console cell blank and omit the comment field. diff --git a/docs/cli-reference.md b/docs/cli-reference.md index f64de267..566eba55 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -238,7 +238,7 @@ If you don't want to provide the Socket API Token every time then you can use th | `--repo` | False | *auto* | Repository name in owner/repo format (auto-detected from git remote) | | `--workspace` | False | | The Socket workspace to associate the scan with (e.g. `my-org` in `my-org/my-repo`). See note below. | | `--repo-is-public` | False | False | If set, flags a new repository creation as public. Defaults to false. | -| `--integration` | False | api | Integration type (api, github, gitlab, azure, bitbucket) | +| `--integration` | False | api | Integration type (api, github, gitlab, azure, bitbucket). When omitted, `--scm github` or `--scm gitlab` implies the matching integration. | | `--owner` | False | | Name of the integration owner, defaults to the socket organization slug | | `--branch` | False | *auto* | Branch name (auto-detected from git) | | `--committers` | False | *auto* | Committer(s) to filter by (auto-detected from git commit) | @@ -252,7 +252,7 @@ If you don't want to provide the Socket API Token every time then you can use th #### Pull Request and Commit | Parameter | Required | Default | Description | |:-----------------|:---------|:--------|:-----------------------------------------------| -| `--pr-number` | False | "0" | Pull request number | +| `--pr-number` | False | *auto* | Pull request number. Auto-detected in GitHub Actions, GitLab CI, and Azure Pipelines; explicitly passing `0` disables detection. | | `--commit-message` | False | *auto* | Commit message (auto-detected from git) | | `--commit-sha` | False | *auto* | Commit SHA (auto-detected from git) | | `--base-scan-id` | False | | Full scan ID to diff against, overriding the repository's head scan as the baseline. Mutually exclusive with `--base-commit-sha` | @@ -431,7 +431,8 @@ The launcher can be tuned via the `SOCKET_CLI_COANA_LAUNCHER` environment variab |:-------------------------|:---------|:--------|:----------------------------------------------------------------------| | `--ignore-commit-files` | False | False | Ignore commit files | | `--disable-blocking` | False | False | Non-blocking CI mode: the CLI always exits **0**, even when blocking alerts are present (including with `--strict-blocking`). Also exits 0 on uncaught runtime errors and Socket API failures, so the job is treated as successful while findings and errors are still logged. Takes precedence over `--strict-blocking`. | -| `--disable-ignore` | False | False | Disable support for `@SocketSecurity ignore` commands in PR comments. When set, alerts cannot be suppressed via comments and ignore instructions are hidden from comment output. | +| `--disable-ignore` | False | False | Disable support for `@SocketSecurity ignore` commands in PR comments. When set, alerts cannot be suppressed via comments and ignore instructions are hidden from comment output. See [Who can ignore an alert](#who-can-ignore-an-alert). | +| `--ignore-authorization` | False | enforce | Who may suppress alerts with `@SocketSecurity ignore`. `enforce` requires write access and honors the command with a warning when the provider cannot report it; `strict` rejects it in that case; `off` honors any commenter. See [Who can ignore an alert](#who-can-ignore-an-alert). | | `--strict-blocking` | False | False | Fail on ANY security policy violations (blocking severity), not just new ones. Only works in diff mode. See [Strict Blocking Mode](#strict-blocking-mode) for details. | | `--enable-diff` | False | False | Enable diff mode even when using `--integration api` (forces diff mode without SCM integration) | | `--scm` | False | api | Source control management type | @@ -690,6 +691,37 @@ The CLI uses intelligent default branch detection with the following priority: Both `--default-branch` and `--pending-head` parameters are automatically synchronized to ensure consistent behavior. +## Who can ignore an alert + +`@SocketSecurity ignore /@` and +`@SocketSecurity ignore-all` suppress security findings, so the CLI honors them +only from a commenter with write access to the repository. A command from anyone +else is skipped, logged with the author's name, and the alerts it named stay +reported. `--disable-ignore` turns the feature off entirely. + +| Provider | How access is determined | If it cannot be determined | +|:---------|:-------------------------|:---------------------------| +| GitHub | Effective repository permission, read once per commenter per run. Write, maintain, or admin access is honored. | The command is honored and a warning is logged. | +| GitLab | Project membership, read once per run when an ignore command is present. Developer (30) or above is honored. | The command is honored and a warning is logged. | + +The GitHub check needs a token that can read repository metadata. GitLab notes +carry no permission field, so that check needs a `GITLAB_TOKEN` that can read +`GET /projects/:id/members/all`. A `CI_JOB_TOKEN` generally cannot. + +`--ignore-authorization` decides what happens when access cannot be determined: + +| Value | Verified write access | Access cannot be determined | +|:------|:----------------------|:----------------------------| +| `enforce` (default) | Honored | Honored, with a warning naming the author | +| `strict` | Honored | Rejected | +| `off` | Honored | Honored, no check performed | + +`enforce` closes the hole wherever the provider can answer, without breaking a +pipeline whose token cannot read membership. `strict` closes it everywhere, at the +cost of failing those pipelines. `off` restores the prior behavior and should be +paired with `--disable-ignore` unless you specifically need comment-driven ignores +from unverified authors. + ## GitLab Token Configuration GitLab token/auth behavior and CI examples are documented in [`ci-cd.md`](ci-cd.md). diff --git a/pyproject.toml b/pyproject.toml index 33d2d4df..334ff1b4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,7 +6,7 @@ build-backend = "hatchling.build" [project] name = "socketsecurity" -version = "2.8.1" +version = "2.9.0" requires-python = ">= 3.11" license = {"file" = "LICENSE"} dependencies = [ diff --git a/socketsecurity/__init__.py b/socketsecurity/__init__.py index 6cf31cd7..ce3e70ab 100644 --- a/socketsecurity/__init__.py +++ b/socketsecurity/__init__.py @@ -1,3 +1,3 @@ __author__ = 'socket.dev' -__version__ = '2.8.1' +__version__ = '2.9.0' USER_AGENT = f'SocketPythonCLI/{__version__}' diff --git a/socketsecurity/config.py b/socketsecurity/config.py index 35904976..cfea4189 100644 --- a/socketsecurity/config.py +++ b/socketsecurity/config.py @@ -115,6 +115,7 @@ class CliConfig: branch: str = "" committers: Optional[List[str]] = None pr_number: str = "0" + pr_number_explicit: bool = False commit_message: Optional[str] = None default_branch: bool = False target_path: str = "./" @@ -143,6 +144,7 @@ class CliConfig: ignore_commit_files: bool = False disable_blocking: bool = False disable_ignore: bool = False + ignore_authorization: str = "enforce" # Tri-state log-upload preference: True = --upload-logs, False = --no-upload-logs, # None = neither (server-side override decides). upload_logs: Optional[bool] = None @@ -219,6 +221,17 @@ def from_args(cls, args_list: Optional[List[str]] = None) -> 'CliConfig': parser.set_defaults(**normalized_defaults) args = parser.parse_args(args_list) + integration_explicit = hasattr(args, "integration") + pr_number_explicit = hasattr(args, "pr_number") + + integration_type = getattr(args, "integration", "api") + pr_number = getattr(args, "pr_number", "0") + if ( + not integration_explicit and + integration_type == "api" and + args.scm in ("github", "gitlab") + ): + integration_type = args.scm if args.reach_exclude_paths: logging.warning( @@ -262,7 +275,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,9 +306,10 @@ def from_args(cls, args_list: Optional[List[str]] = None) -> 'CliConfig': 'ignore_commit_files': args.ignore_commit_files, 'disable_blocking': args.disable_blocking, 'disable_ignore': args.disable_ignore, + 'ignore_authorization': args.ignore_authorization, 'upload_logs': args.upload_logs, 'strict_blocking': args.strict_blocking, - 'integration_type': args.integration, + 'integration_type': integration_type, 'pending_head': args.pending_head, 'timeout': args.timeout, 'exit_code_on_api_error': args.exit_code_on_api_error, @@ -519,8 +534,12 @@ def create_argument_parser() -> argparse.ArgumentParser: "--integration", choices=INTEGRATION_TYPES, metavar="", - help="Integration type of api, github, gitlab, azure, or bitbucket. Defaults to api", - default="api" + help=( + "Integration type of api, github, gitlab, azure, or bitbucket. " + "Defaults to api; --scm github/gitlab implies the matching integration " + "when this option is omitted" + ), + default=argparse.SUPPRESS ) integration_group.add_argument( "--owner", @@ -535,13 +554,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", @@ -703,6 +726,19 @@ def create_argument_parser() -> argparse.ArgumentParser: action="store_true", help="If true, the new scan will be set as the branch's head scan" ) + config_group.add_argument( + "--ignore-authorization", + dest="ignore_authorization", + choices=["enforce", "strict", "off"], + default="enforce", + help=( + "Who may suppress alerts with @SocketSecurity ignore comments. " + "'enforce' (default) requires write access, and honors the command with " + "a warning when the provider cannot report the commenter's access. " + "'strict' rejects the command in that case instead. " + "'off' honors a command from any commenter." + ) + ) config_group.add_argument( "--pending_head", dest="pending_head", diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index b1b1d65b..ca8db896 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -1279,6 +1279,7 @@ def create_full_scan_with_report_url( diff.report_url = f"{base_socket}/{self.config.org_slug}/sbom/{new_full_scan.id}" diff.diff_url = diff.report_url diff.id = new_full_scan.id + diff.is_full_scan = True needs_alerts = ( self.cli_config is not None @@ -1288,30 +1289,43 @@ def create_full_scan_with_report_url( or self.cli_config.enable_sarif ) ) + # --generate-license (and --legal-format fossa, which it gates) enumerates + # diff.packages rather than the alert list, so a full scan has to carry the + # package map even when no alert-bearing output format is enabled. Without + # this, an SCM branch pipeline writes an attribution file with zero packages. + # Keep in sync with _requires_unchanged_artifacts, which lists the same + # consumers for the comparison path. + needs_license_artifacts = ( + self.cli_config is not None and self.cli_config.generate_license + ) - if needs_alerts: - log.info("Output format requires alerts, fetching SBOM data for full scan") + if needs_alerts or needs_license_artifacts: + log.info("Output format requires SBOM data, fetching it for the full scan") sbom_start = time.time() sbom_artifacts_dict = self.get_sbom_data(new_full_scan.id) sbom_artifacts = self.get_sbom_data_list(sbom_artifacts_dict) packages = self._create_packages_dict_without_license_text(sbom_artifacts) + if needs_license_artifacts: + packages = self._add_license_details(packages) diff.packages = packages - all_alerts_collection: Dict[str, List[Issue]] = {} - for package_id, package in packages.items(): - self.add_package_alerts_to_collection( - package=package, - alerts_collection=all_alerts_collection, - packages=packages - ) + if needs_alerts: + all_alerts_collection: Dict[str, List[Issue]] = {} + for package_id, package in packages.items(): + self.add_package_alerts_to_collection( + package=package, + alerts_collection=all_alerts_collection, + packages=packages + ) - consolidated: Set[str] = set() - for alert_key, alerts in all_alerts_collection.items(): - for alert in alerts: - alert_str = f"{alert.purl},{alert.type}" - if (alert.error or alert.warn) and alert_str not in consolidated: - diff.new_alerts.append(alert) - consolidated.add(alert_str) + consolidated: Set[str] = set() + for alert_key, alerts in all_alerts_collection.items(): + for alert in alerts: + alert_str = f"{alert.purl},{alert.type}" + if (alert.error or alert.warn) and alert_str not in consolidated: + diff.new_alerts.append(alert) + consolidated.add(alert_str) + diff.alerts_fetched = True sbom_end = time.time() log.info( @@ -1323,6 +1337,29 @@ def create_full_scan_with_report_url( return diff + def _add_license_details(self, packages: dict[str, Package]) -> dict[str, Package]: + """Populate licenseAttrib/licenseDetails on a full scan's package map. + + get_license_text_via_purl keys off ``ecosystem/name@version`` because that is + what the PURL endpoint echoes back, while a full scan's package map is keyed + by artifact id. Build a purl-keyed view over the same Package objects so the + enrichment lands on the map the caller keeps. + """ + batch_size = self.cli_config.max_purl_batch_size if self.cli_config else 5000 + packages_by_purl = {} + for package in packages.values(): + qualified_name = package.name + if package.namespace: + qualified_name = f"{package.namespace.strip('/')}/{qualified_name}" + packages_by_purl[ + f"{package.type}/{qualified_name}@{package.version}" + ] = package + self.get_license_text_via_purl( + packages_by_purl, + batch_size=batch_size, + ) + return packages + def get_full_scan(self, full_scan_id: str) -> FullScan: """ Get a FullScan object for an existing full scan including sbom_artifacts and packages. @@ -1627,6 +1664,9 @@ def get_license_text_via_purl(self, packages: dict[str, Package], batch_size: in for result in results: ecosystem = result["type"] name = result["name"] + namespace = (result.get("namespace") or "").strip("/") + if namespace and not name.startswith(f"{namespace}/"): + name = f"{namespace}/{name}" package_version = result["version"] licenseDetails = result.get("licenseDetails") licenseAttrib = result.get("licenseAttrib") @@ -1640,7 +1680,8 @@ def get_license_text_via_purl(self, packages: dict[str, Package], batch_size: in def get_diff_scan_artifacts( self, head_full_scan_id: str, - new_full_scan_id: str + new_full_scan_id: str, + external_href: Optional[str] = None ) -> DiffArtifacts: """Compare two full scans via the diff-scans endpoints, polling for the result. @@ -1663,6 +1704,8 @@ def get_diff_scan_artifacts( Args: head_full_scan_id: The before/base full scan ID new_full_scan_id: The after/head full scan ID + external_href: Optional pull request or merge request URL to associate + with the diff scan in the Socket Dashboard Returns: DiffArtifacts with the added/removed/unchanged/replaced/updated lists @@ -1672,6 +1715,13 @@ 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 + # external_href is only honored while a diff scan is being created, + # so a re-run over an already-compared scan pair needs + # on_duplicate=update to apply the link to the existing resource. It + # answers 200 with the same {"diff_scan": ...} envelope as a create. + create_params["on_duplicate"] = "update" try: result = self.sdk.diffscans.create_from_ids(self.config.org_slug, create_params) diff_scan = result.get("diff_scan") or {} @@ -1680,11 +1730,13 @@ 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 when there is no pull request context to attach, and on + # deployments that answer 409 regardless. Do NOT switch this to + # on_duplicate=redirect: the SDK follows that 302 automatically with + # a GET that lacks cached=true, which can leave the connection idle + # while an existing diff scan is still computing. Resolve the + # duplicate explicitly so every result fetch continues through the + # bounded cached polling path below. existing = self.sdk.diffscans.list( self.config.org_slug, params={ @@ -1820,7 +1872,8 @@ def get_added_and_removed_packages( self, head_full_scan_id: str, new_full_scan_id: str, - include_license_details: bool = False + include_license_details: bool = False, + external_href: Optional[str] = None ) -> Tuple[Dict[str, Package], Dict[str, Package], Dict[str, Package]]: """ Get packages that were added and removed between scans. @@ -1853,6 +1906,8 @@ def get_added_and_removed_packages( is retained as an explicit override seam, not wired to the ``--exclude-license-details`` user flag (which still governs the human-facing dashboard report URL). + external_href: Optional pull request or merge request URL to associate + with the primary diff-scan resource Returns: Tuple of (added_packages, removed_packages) dictionaries @@ -1864,7 +1919,8 @@ def get_added_and_removed_packages( try: diff_artifacts = self.get_diff_scan_artifacts( head_full_scan_id, - new_full_scan_id + new_full_scan_id, + external_href=external_href, ) except Exception as error: # SDK error messages can span many lines (path + response headers); the @@ -1980,7 +2036,8 @@ def create_new_diff( save_files_list_path: Optional[str] = None, save_manifest_tar_path: Optional[str] = None, base_paths: Optional[List[str]] = None, - explicit_files: Optional[List[str]] = None + explicit_files: Optional[List[str]] = None, + external_href: Optional[str] = None ) -> Diff: """Create a new diff using the Socket SDK. @@ -1992,6 +2049,8 @@ def create_new_diff( save_manifest_tar_path: Optional path to save manifest files tar.gz archive base_paths: List of base paths for the scan (optional) explicit_files: Optional list of explicit files to use instead of discovering files + external_href: Optional pull request or merge request URL to associate + with the diff scan """ log.debug(f"starting create_new_diff with no_change: {no_change}") if no_change: @@ -2126,7 +2185,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 @@ -2190,16 +2250,22 @@ def create_diff_report( alerts_in_removed_packages: Dict[str, List[Issue]] = {} alerts_in_unchanged_packages: Dict[str, List[Issue]] = {} - seen_new_packages = set() - seen_removed_packages = set() + seen_packages = { + "added": set(), + "updated": set(), + "removed": set(), + "replaced": set(), + } for package_id, package in added_packages.items(): purl = self.create_purl(package_id, added_packages) base_purl = f"{purl.ecosystem}/{purl.name}@{purl.version}" - if (not direct_only or package.direct) and base_purl not in seen_new_packages: - diff.new_packages.append(purl) - seen_new_packages.add(base_purl) + change_type = "updated" if package.diffType == "updated" else "added" + target = diff.updated_packages if change_type == "updated" else diff.new_packages + if (not direct_only or package.direct) and base_purl not in seen_packages[change_type]: + target.append(purl) + seen_packages[change_type].add(base_purl) self.add_package_alerts_to_collection( package=package, @@ -2211,9 +2277,11 @@ def create_diff_report( purl = self.create_purl(package_id, removed_packages) base_purl = f"{purl.ecosystem}/{purl.name}@{purl.version}" - if (not direct_only or package.direct) and base_purl not in seen_removed_packages: - diff.removed_packages.append(purl) - seen_removed_packages.add(base_purl) + change_type = "replaced" if package.diffType == "replaced" else "removed" + target = diff.replaced_packages if change_type == "replaced" else diff.removed_packages + if (not direct_only or package.direct) and base_purl not in seen_packages[change_type]: + target.append(purl) + seen_packages[change_type].add(base_purl) self.add_package_alerts_to_collection( package=package, @@ -2338,23 +2406,24 @@ def get_source_data(package: Package, packages: dict) -> list: @staticmethod def add_purl_capabilities(diff: Diff) -> None: """ - Adds capability information to each package in the diff's new_packages list. + Adds capability information to the diff's added and updated packages. + + Both lists are walked because an updated package is still newly present at + its new version, so its capabilities are as relevant as an added one's. Args: diff: Diff object to update with capability information """ - new_packages = [] - for purl in diff.new_packages: - if purl.id in diff.new_capabilities: - new_purl = Purl( - **{**purl.__dict__, - "capabilities": diff.new_capabilities[purl.id]} - ) - new_packages.append(new_purl) - else: - new_packages.append(purl) - - diff.new_packages = new_packages + for attribute in ("new_packages", "updated_packages"): + packages = [] + for purl in getattr(diff, attribute): + if purl.id in diff.new_capabilities: + purl = Purl( + **{**purl.__dict__, + "capabilities": diff.new_capabilities[purl.id]} + ) + packages.append(purl) + setattr(diff, attribute, packages) def add_package_alerts_to_collection(self, package: Package, alerts_collection: dict, packages: dict) -> dict: """ diff --git a/socketsecurity/core/alert_selection.py b/socketsecurity/core/alert_selection.py index ae5b4772..132be294 100644 --- a/socketsecurity/core/alert_selection.py +++ b/socketsecurity/core/alert_selection.py @@ -31,7 +31,9 @@ def clone_diff_with_selected_alerts(diff: Diff, selected_alerts: List[Issue]) -> removed_alerts=[], diff_url=getattr(diff, "diff_url", ""), new_packages=getattr(diff, "new_packages", []), + updated_packages=getattr(diff, "updated_packages", []), removed_packages=getattr(diff, "removed_packages", []), + replaced_packages=getattr(diff, "replaced_packages", []), packages=getattr(diff, "packages", {}), ) selected_diff.id = getattr(diff, "id", "") diff --git a/socketsecurity/core/classes.py b/socketsecurity/core/classes.py index db145221..f978701d 100644 --- a/socketsecurity/core/classes.py +++ b/socketsecurity/core/classes.py @@ -507,7 +507,9 @@ class Diff: """ new_packages: list[Purl] + updated_packages: list[Purl] removed_packages: list[Purl] + replaced_packages: list[Purl] packages: dict[str, Package] new_capabilities: Dict[str, List[str]] new_alerts: list[Issue] @@ -518,6 +520,8 @@ class Diff: report_url: str diff_url: str new_scan_id: str + is_full_scan: bool + alerts_fetched: bool def __init__(self, **kwargs): if kwargs: @@ -525,8 +529,12 @@ def __init__(self, **kwargs): setattr(self, key, value) if not hasattr(self, "new_packages"): self.new_packages = [] + if not hasattr(self, "updated_packages"): + self.updated_packages = [] if not hasattr(self, "removed_packages"): self.removed_packages = [] + if not hasattr(self, "replaced_packages"): + self.replaced_packages = [] if not hasattr(self, "new_alerts"): self.new_alerts = [] if not hasattr(self, "unchanged_alerts"): @@ -535,6 +543,10 @@ def __init__(self, **kwargs): self.removed_alerts = [] if not hasattr(self, "new_capabilities"): self.new_capabilities = {} + if not hasattr(self, "is_full_scan"): + self.is_full_scan = False + if not hasattr(self, "alerts_fetched"): + self.alerts_fetched = False def __str__(self): return json.dumps(self.__dict__) @@ -548,8 +560,10 @@ def to_dict(self) -> dict: """ return { "new_packages": [p.to_dict() for p in self.new_packages], + "updated_packages": [p.to_dict() for p in self.updated_packages], "new_capabilities": self.new_capabilities, "removed_packages": [p.to_dict() for p in self.removed_packages], + "replaced_packages": [p.to_dict() for p in self.replaced_packages], "new_alerts": [alert.__dict__ for alert in self.new_alerts], "unchanged_alerts": [alert.__dict__ for alert in self.unchanged_alerts] if hasattr(self, "unchanged_alerts") else [], "removed_alerts": [alert.__dict__ for alert in self.removed_alerts] if hasattr(self, "removed_alerts") else [], diff --git a/socketsecurity/core/cli_client.py b/socketsecurity/core/cli_client.py index 2e941e7a..405a7443 100644 --- a/socketsecurity/core/cli_client.py +++ b/socketsecurity/core/cli_client.py @@ -56,7 +56,12 @@ def request( except requests.exceptions.RequestException as e: logger.error(f"API request failed: {str(e)}") - raise APIFailure(f"Request failed: {str(e)}") + # Carry the status forward. Callers that need to react to a specific + # code -- the GitLab auth fallback to the other token scheme, and + # APIFailure.is_transient_error -- have no other way to recover it + # once the requests exception has been translated. + status_code = e.response.status_code if e.response is not None else None + raise APIFailure(f"Request failed: {str(e)}", status_code=status_code) from e def post_telemetry_events(self, org_slug: str, events: List[Dict]) -> None: """Post telemetry events one at a time to the v0 telemetry API. Fire-and-forget — logs errors but never raises.""" diff --git a/socketsecurity/core/exceptions.py b/socketsecurity/core/exceptions.py index 03e69b87..84466805 100644 --- a/socketsecurity/core/exceptions.py +++ b/socketsecurity/core/exceptions.py @@ -1,3 +1,5 @@ +from socketdev.exceptions import APIFailure as SdkAPIFailure + __all__ = [ "APIFailure", "APIKeyMissing", @@ -18,8 +20,15 @@ class APIKeyMissing(Exception): pass -class APIFailure(Exception): - """Raised when there is an error using the API""" +class APIFailure(SdkAPIFailure): + """Raised when there is an error using the API. + + Subclasses the SDK's exception of the same name so a handler written against + either one catches both. They were independent Exception subclasses, so an + ``except APIFailure`` importing the SDK's -- which every handler in + socketsecurity.core does -- silently let a CliClient failure through, and the + status code the SDK class carries was unavailable to anything raised here. + """ pass diff --git a/socketsecurity/core/git_remote.py b/socketsecurity/core/git_remote.py new file mode 100644 index 00000000..eb5b9c02 --- /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 case. +_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/messages.py b/socketsecurity/core/messages.py index d968c14b..61a769da 100644 --- a/socketsecurity/core/messages.py +++ b/socketsecurity/core/messages.py @@ -4,6 +4,7 @@ import re import uuid from datetime import datetime, timezone +from html import escape from pathlib import Path from mdutils import MdUtils @@ -15,6 +16,13 @@ class Messages: + @staticmethod + def get_patched_version(alert: Issue) -> str: + """Return the first patched version exposed by an alert, if any.""" + props = getattr(alert, "props", {}) or {} + value = props.get("firstPatchedVersionIdentifier") + return str(value) if value not in (None, "") else "" + @staticmethod def map_severity_to_sarif(severity: str) -> str: """ @@ -829,6 +837,37 @@ def inline_html_text(value) -> str: return "" return " ".join(str(value).split()) + @staticmethod + def html_text(value) -> str: + """Flatten a value onto one line and escape it for an HTML text node. + + Manifest paths and sources come from the customer's repository, so any PR + author controls them: a directory named ``![x](https://host/p.png)`` or + carrying a raw tag would otherwise render as that markup inside a comment + posted by a trusted integration. Alert text comes from the API and is + escaped for the same reason, since neither is markup the CLI authored. + """ + return escape(Messages.inline_html_text(value)) + + @staticmethod + def html_attr(value) -> str: + """Escape a value for an HTML attribute, quotes included. + + Used for href and src, where an unescaped quote closes the attribute and + everything after it is read as more attributes. + """ + return escape(Messages.inline_html_text(value), quote=True) + + @staticmethod + def comment_marker_text(value) -> str: + """Neutralize an HTML comment terminator inside a marker value. + + The alert markers carry the package name so the comment can be rewritten + later, and the parser reads them back verbatim -- so this cannot escape the + value, only stop it ending the comment early. + """ + return str(value or "").replace("-->", "-->").replace(" @@ -949,39 +988,48 @@ 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"{Messages.html_text(patched_version)}

" + if patched_version else "" + ) # Generate proper manifest URL manifest_url = Messages.get_manifest_file_url(diff, alert.manifests, config) + pkg_label = Messages.html_text(f"{alert.pkg_name}@{alert.pkg_version}") + pkg_marker = Messages.comment_marker_text(f"{alert.pkg_name}@{alert.pkg_version}") # Generate a table row for each alert ignore_html = ( f"

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

" ) if show_ignore else "" comment += f""" - + - + """ # Add license policy violation entries grouped by PURL @@ -992,24 +1040,31 @@ def security_comment_template(diff: Diff, config=None) -> str: # Use orange diamond for license policy violations license_icon = "🔶" + license_label = Messages.html_text( + f"{first_alert.pkg_name}@{first_alert.pkg_version}" + ) + license_marker = Messages.comment_marker_text( + f"{first_alert.pkg_name}@{first_alert.pkg_version}" + ) + # Build license findings list license_findings = [] for alert in alerts: license_findings.append(alert.title) comment += f""" - + - + """ # Close table @@ -1232,6 +1287,22 @@ def create_remove_line(diff: Diff, md: MdUtils) -> MdUtils: md.new_line(removed_line) return md + # Change types the shared badge host publishes an image for. Removed and + # replaced have no artwork, so they fall back to a bold text label rather than + # rendering a broken image; added and updated render the available badges. + DIFF_BADGES = { + "Added": "diff-added.svg", + "Updated": "diff-updated.svg", + } + + @staticmethod + def get_diff_badge(change: str, package_url: str) -> str: + """Return the Dependency Overview cell marking how a package changed.""" + badge = Messages.DIFF_BADGES.get(change) + if not badge: + return f"**{change}**" + return f"[![{change}](https://github-app-statics.socket.dev/{badge})]({package_url})" + @staticmethod def create_added_table(diff: Diff, md: MdUtils) -> MdUtils: """ @@ -1253,51 +1324,58 @@ def create_added_table(diff: Diff, md: MdUtils) -> MdUtils: num_of_overview_columns = len(overview_table) count = 0 - for added in diff.new_packages: - added: Purl # Ensure `added` has scores and relevant attributes. - - package_url = f"[{added.purl}]({added.url})" - diff_badge = f"[![+](https://github-app-statics.socket.dev/diff-added.svg)]({added.url})" - - # Scores dynamically converted to badge URLs and linked - def score_to_badge(score): - score_percent = int(score * 100) # Convert to integer percentage - return f"[![{score_percent}](https://github-app-statics.socket.dev/score-{score_percent}.svg)]({added.url})" - - def get_score_for_badge(score_name: str) -> float: - scores = getattr(added, "scores", None) - if isinstance(scores, dict): - raw_score = scores.get(score_name) - else: - raw_score = getattr(scores, score_name, None) if scores is not None else None - - if raw_score is None: - return 1.0 - - score = float(raw_score) - if score > 1: - score = score / 100 - return max(0.0, min(score, 1.0)) - - # Generate badges for each score type - supply_chain_risk_badge = score_to_badge(get_score_for_badge("supplyChain")) - vulnerability_badge = score_to_badge(get_score_for_badge("vulnerability")) - quality_badge = score_to_badge(get_score_for_badge("quality")) - maintenance_badge = score_to_badge(get_score_for_badge("maintenance")) - license_badge = score_to_badge(get_score_for_badge("license")) - - # Add the row for this package - row = [ - diff_badge, - package_url, - supply_chain_risk_badge, - vulnerability_badge, - quality_badge, - maintenance_badge, - license_badge - ] - overview_table.extend(row) - count += 1 # Count total packages + changes = ( + ("Added", diff.new_packages), + ("Updated", diff.updated_packages), + ("Removed", diff.removed_packages), + ("Replaced", diff.replaced_packages), + ) + for change, packages in changes: + for package in packages: + package: Purl + + package_url = f"[{package.purl}]({package.url})" + diff_badge = Messages.get_diff_badge(change, package.url) + + # Scores dynamically converted to badge URLs and linked + def score_to_badge(score): + score_percent = int(score * 100) # Convert to integer percentage + return f"[![{score_percent}](https://github-app-statics.socket.dev/score-{score_percent}.svg)]({package.url})" + + def get_score_for_badge(score_name: str) -> float: + scores = getattr(package, "scores", None) + if isinstance(scores, dict): + raw_score = scores.get(score_name) + else: + raw_score = getattr(scores, score_name, None) if scores is not None else None + + if raw_score is None: + return 1.0 + + score = float(raw_score) + if score > 1: + score = score / 100 + return max(0.0, min(score, 1.0)) + + # Generate badges for each score type + supply_chain_risk_badge = score_to_badge(get_score_for_badge("supplyChain")) + vulnerability_badge = score_to_badge(get_score_for_badge("vulnerability")) + quality_badge = score_to_badge(get_score_for_badge("quality")) + maintenance_badge = score_to_badge(get_score_for_badge("maintenance")) + license_badge = score_to_badge(get_score_for_badge("license")) + + # Add the row for this package + row = [ + diff_badge, + package_url, + supply_chain_risk_badge, + vulnerability_badge, + quality_badge, + maintenance_badge, + license_badge + ] + overview_table.extend(row) + count += 1 # Calculate total rows for table num_of_overview_rows = count + 1 # Include header row @@ -1332,6 +1410,7 @@ def create_console_security_alert_table(diff: Diff) -> PrettyTable: [ "Alert", "Package", + "Patched Version", "url", "Introduced by", "Manifest File", @@ -1352,6 +1431,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, @@ -1367,8 +1447,11 @@ def create_sources(alert: Issue, style="md") -> tuple[str, str]: for source, manifest in alert.introduced_by: if style == "md": - add_str = f"
  • {manifest}
    • " - source_str = f"
  • {source}
    • " + # These land in rendered Markdown, where an unescaped path is read + # as markup. plain and raw are consumed by Slack, Jira and the + # console, which do not render HTML, so they stay verbatim. + add_str = f"
  • {Messages.html_text(manifest)}
    • " + source_str = f"
  • {Messages.html_text(source)}
    • " elif style == "plain": add_str = f"• {manifest}" source_str = f"• {source}" diff --git a/socketsecurity/core/pull_request.py b/socketsecurity/core/pull_request.py new file mode 100644 index 00000000..2dcf77f1 --- /dev/null +++ b/socketsecurity/core/pull_request.py @@ -0,0 +1,144 @@ +import re +from dataclasses import dataclass +from typing import Mapping, Optional +from urllib.parse import urlparse + +from socketsecurity.core.git_remote import parse_git_remote + + +@dataclass(frozen=True) +class PullRequestContext: + number: int = 0 + url: Optional[str] = None + + +def parse_pull_request_number(value) -> int: + """Coerce a configured or CI-supplied pull request number to a positive int. + + Anything that is not a positive integer means "no pull request", including the + literal ``false`` that Buildkite puts in ``BUILDKITE_PULL_REQUEST`` on non-PR + builds. Callers that hand the value on to a comment adapter should store this + result rather than the raw string, which is truthy. + """ + try: + parsed = int(value) + except (TypeError, ValueError): + return 0 + return parsed if parsed > 0 else 0 + + +def _http_url(value: Optional[str]) -> Optional[str]: + """Return ``value`` if it is an http(s) URL with a host, else ``None``. + + Every URL fragment read out of the CI environment goes through here before it + is composed into a link, because the result is sent to the API as a diff scan's + ``external_href``. Standard runners set these variables themselves, so this is + defense in depth rather than a live hole. + """ + if not value: + return None + url = value.strip().rstrip("/") + parsed = urlparse(url) + return url if parsed.scheme in ("http", "https") and parsed.netloc else None + + +def _repository_url(value: Optional[str]) -> Optional[str]: + if not value: + return None + url = value.strip().rstrip("/") + if url.endswith(".git"): + url = url[:-4] + return _http_url(url) + + +def _github_number(env: Mapping[str, str]) -> int: + number = parse_pull_request_number(env.get("PR_NUMBER")) + if number: + return number + match = re.match(r"^refs/pull/(\d+)/", env.get("GITHUB_REF", "")) + return parse_pull_request_number(match.group(1)) if match else 0 + + +def _github_url(number: int, repo: Optional[str], env: Mapping[str, str]) -> Optional[str]: + 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 + if not repository or "/" not in repository: + return None + server = ( + _http_url(env.get("GITHUB_SERVER_URL")) + or (_http_url(f"https://{remote_host}") if remote_host else None) + or "https://github.com" + ) + return f"{server}/{repository.strip('/')}/pull/{number}" + + +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_git_remote(env.get("BUILDKITE_REPO")) + project_path = env.get("CI_PROJECT_PATH") or remote_path or repo + server = ( + _http_url(env.get("CI_SERVER_URL")) + or (_http_url(f"https://{remote_host}") if remote_host else None) + ) + if server and project_path and "/" in project_path: + project_url = f"{server}/{project_path.strip('/')}" + return f"{project_url}/-/merge_requests/{number}" if project_url else None + + +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 = parse_pull_request_number(configured_number) + + if not configured_explicit and not number: + if provider == "github": + number = _github_number(environment) + elif provider == "gitlab": + number = parse_pull_request_number(environment.get("CI_MERGE_REQUEST_IID")) + elif provider == "azure": + number = ( + parse_pull_request_number(environment.get("SYSTEM_PULLREQUEST_PULLREQUESTNUMBER")) or + parse_pull_request_number(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/core/scm/github.py b/socketsecurity/core/scm/github.py index 7504a46c..8cf39aae 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] @@ -166,9 +154,21 @@ def from_env(cls, pr_number: Optional[str] = None) -> 'GithubConfig': class Github: - def __init__(self, client: CliClient, config: Optional[GithubConfig] = None): + WRITE_PERMISSIONS = frozenset({"write", "maintain", "admin"}) + + def __init__( + self, + client: CliClient, + config: Optional[GithubConfig] = None, + ignore_authorization: str = "enforce", + ): self.config = config or GithubConfig.from_env() self.client = client + self.ignore_authorization = ignore_authorization + # Permission is stable for the duration of one CLI run. Cache both + # positive and negative answers so several ignore comments by the same + # author do not each make an API request. + self._ignore_permission_cache: dict[str, Optional[bool]] = {} if not self.config.token: log.error("Unable to get Github API Token") @@ -236,7 +236,65 @@ def get_comments_for_pr(self) -> dict: else: log.error(raw_comments) - return Comments.check_for_socket_comments(comments) + gate = None if self.ignore_authorization == "off" else self.is_ignore_authorized + return Comments.check_for_socket_comments(comments, gate) + + def is_ignore_authorized(self, comment: Comment) -> bool: + """Whether a commenter may suppress alerts with @SocketSecurity ignore. + + ``author_association`` describes a social relationship to the repository, + not the author's role: an organization member or outside collaborator can + still have read-only access. Ask GitHub for the effective repository + permission instead, and cache the answer for subsequent comments. + """ + author = Comments.comment_author_name(comment) + if author == "an unknown user": + permission = None + elif author in self._ignore_permission_cache: + permission = self._ignore_permission_cache[author] + else: + path = ( + f"repos/{self.config.owner}/{self.config.repository}/" + f"collaborators/{author}/permission" + ) + try: + response = self.client.request( + path=path, + headers=self.config.headers, + base_url=self.config.api_url, + ) + result = response.json() + if not isinstance(result, dict) or not isinstance( + result.get("permission"), str + ): + log.warning("Unexpected GitHub repository permission response") + permission = None + else: + permission = ( + result["permission"].casefold() in self.WRITE_PERMISSIONS + ) + except Exception as error: + log.warning( + f"Could not read GitHub repository permission for {author}: {error}" + ) + permission = None + self._ignore_permission_cache[author] = permission + + if permission is not None: + return permission + if self.ignore_authorization == "strict": + log.warning( + f"Rejecting @SocketSecurity ignore from {author}: GitHub repository " + "permission could not be read and --ignore-authorization is strict." + ) + return False + log.warning( + f"Honoring @SocketSecurity ignore from {author} without verifying write " + "access: GitHub repository permission could not be read. Use a token " + "with repository metadata access, or --ignore-authorization strict to " + "reject instead." + ) + return True def add_socket_comments( self, diff --git a/socketsecurity/core/scm/gitlab.py b/socketsecurity/core/scm/gitlab.py index 2c3947de..b3740314 100644 --- a/socketsecurity/core/scm/gitlab.py +++ b/socketsecurity/core/scm/gitlab.py @@ -5,6 +5,7 @@ from typing import Optional import requests +from socketdev.exceptions import APIFailure from socketsecurity import USER_AGENT from socketsecurity.core import log @@ -126,37 +127,54 @@ def _get_auth_headers(token: str) -> dict: } class Gitlab: - def __init__(self, client: CliClient, config: Optional[GitlabConfig] = None): + # GitLab access levels: 30 Developer, 40 Maintainer, 50 Owner. Reporter (20) + # and Guest (10) cannot push, so they cannot suppress an alert either. + MIN_IGNORE_ACCESS_LEVEL = 30 + # Bounded so a project with a very large membership cannot stall a scan. Past + # the cap the answer is "undetermined", handled the same as a failed lookup. + MEMBER_PAGE_SIZE = 100 + MEMBER_PAGE_LIMIT = 10 + + def __init__( + self, + client: CliClient, + config: Optional[GitlabConfig] = None, + ignore_authorization: str = "enforce", + ): self.config = config or GitlabConfig.from_env() self.client = client + self.ignore_authorization = ignore_authorization + # None until the first ignore comment forces a lookup; stays None when the + # members API cannot be read, which is the "undetermined" state. + self._member_access: Optional[dict] = None + self._member_lookup_attempted = False def _request_with_fallback(self, **kwargs): - """ - Make a request with automatic fallback between Bearer and PRIVATE-TOKEN authentication. - This provides robustness when the initial token type detection is incorrect. + """Request with one retry under the other GitLab auth scheme on a 401. + + _get_auth_headers guesses between Bearer and PRIVATE-TOKEN from the shape of + the token, and the guess can be wrong for tokens that do not match a known + pattern. Rather than fail the run, try the other scheme once. + + Catches APIFailure, not requests.exceptions.HTTPError: CliClient translates + every requests error into APIFailure, which does not inherit from HTTPError, + so catching the latter here never fired and the fallback never ran. """ try: - # Try the initial request with the configured headers return self.client.request(**kwargs) - except requests.exceptions.HTTPError as e: - # Check if this is an authentication error (401) - if e.response and e.response.status_code == 401: - log.debug("Authentication failed with initial headers, trying fallback method") - - # Determine the fallback headers - original_headers = kwargs.get('headers', self.config.headers) - fallback_headers = self._get_fallback_headers(original_headers) - - if fallback_headers and fallback_headers != original_headers: - log.debug("Retrying request with fallback authentication method") - kwargs['headers'] = fallback_headers - return self.client.request(**kwargs) - - # Re-raise the original exception if it's not an auth error or fallback failed - raise - except Exception: - # Handle other types of exceptions that don't have response attribute - raise + except APIFailure as error: + if error.status_code != 401: + raise + + log.debug("Authentication failed with initial headers, trying fallback method") + original_headers = kwargs.get('headers', self.config.headers) + fallback_headers = self._get_fallback_headers(original_headers) + if not fallback_headers or fallback_headers == original_headers: + raise + + log.debug("Retrying request with fallback authentication method") + kwargs['headers'] = fallback_headers + return self.client.request(**kwargs) def _get_fallback_headers(self, original_headers: dict) -> dict: """ @@ -256,7 +274,88 @@ def get_comments_for_pr(self) -> dict: comment.body_list = comment.body.split("\n") else: log.error(raw_comments) - return Comments.check_for_socket_comments(comments) + gate = None if self.ignore_authorization == "off" else self.is_ignore_authorized + return Comments.check_for_socket_comments(comments, gate) + + def _load_member_access(self) -> Optional[dict]: + """Map project member user id -> access level, or None if unreadable. + + ``members/all`` is used rather than a per-user lookup because it answers + non-membership with a 200 and an absent id. CliClient collapses every HTTP + error into APIFailure without a status code, so a per-user 404 -- exactly + the outsider case this guards against -- would be indistinguishable from a + token that cannot read the endpoint, and would have to fail open. + """ + if self._member_lookup_attempted: + return self._member_access + self._member_lookup_attempted = True + if not self.config.mr_project_id: + return None + + access: dict = {} + for page in range(1, Gitlab.MEMBER_PAGE_LIMIT + 1): + path = ( + f"projects/{self.config.mr_project_id}/members/all" + f"?per_page={Gitlab.MEMBER_PAGE_SIZE}&page={page}" + ) + try: + response = self._request_with_fallback( + path=path, + headers=self.config.headers, + base_url=self.config.api_url + ) + members = response.json() + except Exception as error: + log.warning(f"Could not read GitLab project members: {error}") + return None + if not isinstance(members, list): + log.warning("Unexpected GitLab project members response") + return None + for member in members: + if isinstance(member, dict) and member.get("id") is not None: + access[member["id"]] = member.get("access_level") or 0 + if len(members) < Gitlab.MEMBER_PAGE_SIZE: + self._member_access = access + return access + + log.warning( + f"GitLab project has more than {Gitlab.MEMBER_PAGE_SIZE * Gitlab.MEMBER_PAGE_LIMIT} " + "members; cannot confirm ignore-command authorization" + ) + return None + + def is_ignore_authorized(self, comment: Comment) -> bool: + """Whether a commenter may suppress alerts with @SocketSecurity ignore. + + GitLab notes carry no permission field, so this costs one members lookup + per run (cached, and only when an ignore command is actually present). + + When membership can be read the answer is definitive. When it cannot -- a + CI_JOB_TOKEN generally cannot read the members API -- the command is + honored and a warning is logged, so turning this on does not silently break + pipelines that were already relying on ignore commands. Set a token with + API read access to get enforcement. + """ + access = self._load_member_access() + if access is None: + author = Comments.comment_author_name(comment) + if self.ignore_authorization == "strict": + log.warning( + f"Rejecting @SocketSecurity ignore from {author}: GitLab project " + "membership could not be read and --ignore-authorization is strict." + ) + return False + log.warning( + f"Honoring @SocketSecurity ignore from {author} without verifying " + "write access: GitLab project membership could not be read. Use a " + "token with API read access, or --ignore-authorization strict to " + "reject instead." + ) + return True + + author = getattr(comment, "author", None) or {} + user_id = author.get("id") + return access.get(user_id, 0) >= Gitlab.MIN_IGNORE_ACCESS_LEVEL def add_socket_comments( self, diff --git a/socketsecurity/core/scm_comments.py b/socketsecurity/core/scm_comments.py index 7c479b72..3ef0e3a6 100644 --- a/socketsecurity/core/scm_comments.py +++ b/socketsecurity/core/scm_comments.py @@ -1,5 +1,6 @@ import json import re +from typing import Callable, Optional from requests import Response @@ -11,6 +12,12 @@ class Comments: VIEW_REPORT_PATTERN = re.compile(r"\[View full report\]\(([^)\s]+)\)") + @staticmethod + def comment_author_name(comment: Comment) -> str: + """Best-effort display name for a comment author, across providers.""" + user = getattr(comment, "user", None) or getattr(comment, "author", None) or {} + return user.get("login") or user.get("username") or "an unknown user" + @staticmethod def process_response(response: Response) -> dict: output = {} @@ -37,10 +44,10 @@ def remove_alerts(comments: dict, new_alerts: list) -> list: if ignore_all: break else: - full_name = f"{alert.pkg_type}/{alert.pkg_name}" - purl = (full_name, alert.pkg_version) - purl_star = (full_name, "*") - if purl in ignore_commands or purl_star in ignore_commands: + if any( + Comments.is_ignore(alert.pkg_name, alert.pkg_version, name, version, alert.pkg_type) + for name, version in ignore_commands + ): log.info(f"Alerts for {alert.pkg_name}@{alert.pkg_version} ignored") else: log.info(f"Adding alert {alert.type} for {alert.pkg_name}@{alert.pkg_version}") @@ -66,8 +73,10 @@ def get_ignore_options(comments: dict) -> [bool, list]: ignore_all = True else: command = command.lstrip("ignore").strip() - name, version = command.split("@") - data = (name, version) + name, separator, version = command.rpartition("@") + if not separator or not name or not version: + raise ValueError("Expected package@version") + data = (name.strip(), version.strip()) ignore_commands.append(data) except Exception as error: log.error(f"Unable to process ignore command for {comment}") @@ -75,11 +84,30 @@ def get_ignore_options(comments: dict) -> [bool, list]: return ignore_all, ignore_commands @staticmethod - def is_ignore(pkg_name: str, pkg_version: str, name: str, version: str) -> bool: - result = False - if pkg_name == name and (pkg_version == version or version == "*"): - result = True - return result + def is_ignore( + pkg_name: str, pkg_version: str, name: str, version: str, + pkg_type: str = "" + ) -> bool: + """Match an alert's package against one parsed ignore command. + + Generated commands are ecosystem-qualified (``npm/lodash@4.17.21``) but + replies typed by hand, and commands written by older CLI versions, use the + bare package name, so both have to match. + + Callers that parse the package out of a ``start-socket-alert`` marker have no + pkg_type to compare against and instead strip the ecosystem off the command. + An npm scope looks the same as an ecosystem prefix there, so only strip when + the leading segment cannot be one: without the guard, + ``ignore @types/node@*`` would also silently ignore alerts for a package + literally named ``node``. + """ + package_names = {pkg_name} + if pkg_type: + package_names.add(f"{pkg_type}/{pkg_name}") + target_names = {name} + if not pkg_type and "/" in name and not name.startswith("@"): + target_names.add(name.split("/", 1)[1]) + return bool(package_names & target_names) and (pkg_version == version or version == "*") @staticmethod def is_heading_line(line) -> bool: @@ -112,6 +140,33 @@ def process_security_comment(comment: Comment, comments) -> str: return new_body + @staticmethod + def parse_alert_table_row(line: str) -> Optional[tuple[str, str, str]]: + """Pull ``(ecosystem, package, version)`` out of a legacy alert table row. + + Returns None for any row that does not have the expected shape rather than + raising. The row comes back from the provider's API, so its contents are + outside this process's control. Malformed cells must not interrupt status + reporting. A row that cannot be read is a row whose alert stays reported. + """ + cells = line.strip().lstrip("|").rstrip("|").split("|") + if len(cells) != 5: + return None + package = cells[1] + if "](" not in package: + return None + details = package.split("](", 1)[0].lstrip("[") + if "/" not in details: + return None + ecosystem, remainder = details.split("/", 1) + if "@" not in remainder: + return None + # Split from the right: a scoped name carries its own "@". + pkg_name, pkg_version = remainder.rsplit("@", 1) + if not pkg_name or not pkg_version: + return None + return ecosystem, pkg_name, pkg_version + @staticmethod def process_original_security_comment( comment: Comment, @@ -127,19 +182,21 @@ def process_original_security_comment( start = True lines.append(line) elif start and "end-socket-alerts-table" not in line and not Comments.is_heading_line(line) and line != '': - title, package, introduced_by, manifest, ci = line.lstrip("|").rstrip("|").split("|") - details, _ = package.split("](") - ecosystem, details = details.split("/", 1) - ecosystem = ecosystem.lstrip("[") - pkg_name, pkg_version = details.split("@") - pkg_name = f"{ecosystem}/{pkg_name}" + parsed = Comments.parse_alert_table_row(line) # ignore_all has to be checked outside the loop: an ignore-all # comment produces no ignore_commands, so a loop-internal check # never runs and every row was kept. - ignore = ignore_all or any( - Comments.is_ignore(pkg_name, pkg_version, name, version) - for name, version in ignore_commands - ) + if parsed is None: + # An unparseable row cannot be evaluated against the ignore + # commands, so keep it: leaving an alert reported is the safe + # direction, and the comment body is not ours to discard. + ignore = ignore_all + else: + ecosystem, pkg_name, pkg_version = parsed + ignore = ignore_all or any( + Comments.is_ignore(pkg_name, pkg_version, name, version, ecosystem) + for name, version in ignore_commands + ) if not ignore: kept_alert = True lines.append(line) @@ -187,7 +244,7 @@ def process_updated_security_comment( # Extract package name and version from the comment try: start_marker = stripped[len("" in body assert "" in body + def test_copy_is_provider_neutral(self): + body = Messages.security_comment_template( + _make_diff([_make_alert()]), _FakeConfig(scm="gitlab") + ) + assert "Socket for GitHub" not in body + assert "Learn more about [Socket]" in body + class TestSecurityCommentTemplateWithNoAlerts: def test_no_alerts_omits_the_empty_table(self): @@ -232,6 +241,23 @@ def test_ignoring_every_alert_individually_collapses_too(self): assert "No dependency alerts to report" in new_body + def test_qualified_scoped_package_ignore_matches_comment_marker(self): + security = _security_comment_with([ + _make_alert( + pkg_name="@socketsecurity/example", + purl="pkg:npm/@socketsecurity/example@4.17.21", + ) + ]) + comments = { + "security": security, + "ignore": [_make_comment( + "SocketSecurity ignore npm/@socketsecurity/example@4.17.21", + comment_id=2, + )], + } + + assert "No dependency alerts to report" in Comments.process_security_comment(security, comments) + def test_no_ignore_commands_leaves_alerts_in_place(self): security = self._two_alert_comment() comments = {"security": security, "ignore": []} @@ -265,6 +291,17 @@ def test_collapsed_body_is_stable_when_reprocessed(self): [View full report](https://socket.dev/report/legacy?action=error%2Cwarn) """ +SCOPED_LEGACY_COMMENT = """ + + +|Alert|Package|Introduced by|Manifest File|CI| +|:---|:---|:---|:---|:---| +|Known Malware|[npm/@socketsecurity/example@1.0.0](https://socket.dev/z)|example|package.json|:no_entry_sign:| + + +[View full report](https://socket.dev/report/legacy?action=error%2Cwarn) +""" + class TestProcessOriginalSecurityComment: def test_partial_ignore_keeps_remaining_row(self): @@ -292,6 +329,27 @@ def test_ignore_all_collapses_to_the_no_alerts_body(self): assert "No dependency alerts to report" in new_body assert "[View full report](https://socket.dev/report/legacy)" in new_body + def test_scoped_package_row_does_not_raise(self): + """A scoped name carries its own "@", so the split must come from the right.""" + security = _make_comment(SCOPED_LEGACY_COMMENT) + comments = {"security": security, "ignore": []} + + new_body = Comments.process_security_comment(security, comments) + + assert "npm/@socketsecurity/example@1.0.0" in new_body + + def test_scoped_package_row_is_ignorable_both_ways(self): + for command in ( + "SocketSecurity ignore npm/@socketsecurity/example@1.0.0", + "SocketSecurity ignore @socketsecurity/example@1.0.0", + ): + security = _make_comment(SCOPED_LEGACY_COMMENT) + comments = {"security": security, "ignore": [_make_comment(command, comment_id=2)]} + + new_body = Comments.process_security_comment(security, comments) + + assert "No dependency alerts to report" in new_body, command + class TestExtractReportUrl: def test_strips_the_action_filter(self): @@ -302,3 +360,146 @@ def test_strips_the_action_filter(self): def test_returns_empty_when_absent(self): assert Comments.extract_report_url("no link here") == "" + + +# --- Escaping repo-derived values --------------------------------------------- +# +# Manifest paths and sources are file paths inside the customer's repository, so +# anyone who can open a pull request controls them: a directory named +# `![x](https://host/p.png)` holding a manifest puts that markup into a comment +# posted by a trusted integration. GitHub and GitLab sanitize comment HTML, so the +# exposure is external resource loading, phishing links and content spoofing +# rather than script execution. + + +@dataclass +class _RepoConfig(_FakeConfig): + """A config that reaches the branch which embeds the path verbatim. + + Without repo/branch, get_manifest_file_url returns "" or a percent-encoded + Socket link, and the path never lands in the comment -- so a test using the + bare config asserts nothing. + """ + repo: str = "acme/widgets" + branch: str = "main" + + +HOSTILE_PATHS = { + "image": "![x](https://evil.example/p.png)/package.json", + "link": "[click me](https://evil.example)/package.json", + "raw_tag": "/package.json", + "backtick": "`code`/package.json", + "pipe": "a|b/package.json", + "quote": 'a" onmouseover="x/package.json', + "comment_close": "x-->y/package.json", +} + + +def _rendered_with_path(path: str) -> str: + return Messages.security_comment_template( + _make_diff([_make_alert(manifests=path)]), _RepoConfig() + ) + + +def test_the_hostile_path_actually_reaches_the_comment(): + """Guards the fixture itself: if the path stops being rendered, the escaping + tests below would pass while asserting nothing.""" + body = _rendered_with_path("sentinel-path/package.json") + + assert "sentinel-path" in body + + +@pytest.mark.parametrize("name,path", sorted(HOSTILE_PATHS.items())) +def test_hostile_manifest_path_cannot_introduce_markup(name, path): + """In the rendered comment the path only ever lands inside an href, where + Markdown is inert. The property that matters there is that the value cannot + open a tag or close the attribute -- see create_sources for the context where + Markdown itself is live.""" + body = _rendered_with_path(path) + + rendered = [ln for ln in body.split("\n") if "Manifest File" in ln][0] + value = rendered.split('href="', 1)[1].split('"', 1)[0] + + for char in ("<", ">", '"'): + assert char not in value, f"{char!r} survived into the href: {value!r}" + assert_html_block_intact(body) + + +def test_quote_in_a_path_cannot_escape_the_href(): + body = _rendered_with_path('a" onmouseover="x/package.json') + + assert """ in body + assert 'href="https://github.com/acme/widgets/blob/main/a" ' not in body + + +def test_hostile_package_name_cannot_close_the_alert_marker(): + body = Messages.security_comment_template( + _make_diff([_make_alert(pkg_name="evil-->x")]), _FakeConfig() + ) + + # Exactly the terminator the CLI wrote, and no stray one inside the value. + for line in body.split("\n"): + if "socket-alert-" in line: + assert line.count("-->") == 1, line + + +def test_alert_text_from_the_api_is_escaped(): + body = Messages.security_comment_template( + _make_diff([_make_alert(description="")]), _FakeConfig() + ) + + assert "
    {action} - {alert.severity} + {Messages.html_attr(alert.severity)}
    - {alert.pkg_name}@{alert.pkg_version} - {Messages.inline_html_text(alert.title)} -

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

    -

    Source: Manifest File

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

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

    + {patched_version_html} +

    Source: Manifest File

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

    -

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

    +

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

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

    License findings:

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

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

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

    From: Manifest File

    -

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

    +

    From: Manifest File

    +

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

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

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

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