From d3b0c222f1d37311f20f5ad0aa6da3275cd8a6d6 Mon Sep 17 00:00:00 2001 From: jdalton Date: Tue, 13 Oct 2026 12:25:40 -0400 Subject: [PATCH] Use server-provided scan report links --- socketsecurity/core/__init__.py | 65 ++++++--- socketsecurity/core/alert_selection.py | 2 + socketsecurity/core/classes.py | 10 +- socketsecurity/core/diff_result.py | 28 +++- socketsecurity/core/messages.py | 22 ++- socketsecurity/core/report_urls.py | 23 +++ socketsecurity/plugins/jira.py | 35 +++-- socketsecurity/plugins/slack.py | 6 +- tests/core/test_diff_identity_transport.py | 161 ++++++++++++++++++++- tests/unit/test_diff_identity.py | 9 ++ tests/unit/test_manifest_discovery.py | 25 ---- tests/unit/test_report_urls.py | 129 +++++++++++++++++ tests/unit/test_scan_scope.py | 63 ++++++++ 13 files changed, 505 insertions(+), 73 deletions(-) create mode 100644 socketsecurity/core/report_urls.py create mode 100644 tests/unit/test_report_urls.py diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index d8a11ee8..1a0df548 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -31,8 +31,11 @@ DiffScanResult, PackageComparisonResult, create_diff_scan_result, + matching_full_scan_url, + normalize_report_url, ) from socketsecurity.core.exceptions import APIResourceNotFound +from socketsecurity.core.report_urls import add_query_params from .resource_utils import check_file_count_against_ulimit from .socket_config import SocketConfig @@ -1315,8 +1318,9 @@ def create_full_scan_with_report_url( raise # Construct report URL - base_socket = "https://socket.dev/dashboard/org" - diff.report_url = f"{base_socket}/{self.config.org_slug}/sbom/{new_full_scan.id}" + diff.old_scan_url = None + diff.new_scan_url = normalize_report_url(new_full_scan.html_report_url) + diff.report_url = diff.new_scan_url diff.diff_url = diff.report_url diff.set_scan_ids(new_full_scan.id) diff.is_full_scan = True @@ -2050,6 +2054,9 @@ def _get_diff_scan_result( # them, so omit_unchanged and the bounded cached-polling contract cannot be # bypassed by an eager response. artifacts_dict = None + completed_diff_url = None + completed_old_scan_url = None + completed_new_scan_url = None # cached=true is the polling contract (202 while computing, 200 when # ready). The API ignores omit_license_details when cached=true - cached @@ -2098,6 +2105,13 @@ def _get_diff_scan_result( f"Error fetching diff scan {diff_scan_id}: unexpected response: {str(response)[:500]}" ) artifacts_dict = scan["artifacts"] + completed_diff_url = normalize_report_url(scan.get("html_url")) + completed_old_scan_url = matching_full_scan_url( + scan.get("before_full_scan"), head_full_scan_id + ) + completed_new_scan_url = matching_full_scan_url( + scan.get("after_full_scan"), new_full_scan_id + ) log.info( "Diff scan comparison ready in " f"{time.monotonic() - poll_start:.2f}s: id={diff_scan_id}, " @@ -2115,7 +2129,13 @@ def _get_diff_scan_result( last_interval = interval interval = min(interval * DIFF_SCAN_POLL_BACKOFF_MULTIPLIER, DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS) - return create_diff_scan_result(artifacts_dict, diff_scan_id) + return create_diff_scan_result( + artifacts_dict, + diff_scan_id, + diff_url=completed_diff_url, + old_scan_url=completed_old_scan_url, + new_scan_url=completed_new_scan_url, + ) def get_diff_scan_artifacts( self, @@ -2231,18 +2251,20 @@ def _get_package_comparison( if comparison is None: try: - diff_artifacts = ( - self.sdk.fullscans.stream_diff( + stream_response = self.sdk.fullscans.stream_diff( self.config.org_slug, head_full_scan_id, new_full_scan_id, use_types=True, include_license_details=str(include_license_details).lower() - ).data.artifacts - ) + ) + diff_artifacts = stream_response.data.artifacts comparison = DiffScanResult( artifacts=diff_artifacts, diff_scan_id=None, + diff_url=normalize_report_url( + getattr(stream_response.data, "diff_report_url", None) + ), ) except APIFailure as e: log.error(f"API Error: {e}") @@ -2327,6 +2349,9 @@ def _get_package_comparison( removed_packages=removed_packages, packages=packages, diff_scan_id=comparison.diff_scan_id, + diff_url=comparison.diff_url, + old_scan_url=comparison.old_scan_url, + new_scan_url=comparison.new_scan_url, ) def get_added_and_removed_packages( @@ -2409,6 +2434,7 @@ def create_new_diff( head_full_scan_id = self.resolve_base_full_scan_id(params) # If no head scan exists, create an empty baseline scan + baseline_report_url = None if head_full_scan_id is None: log.info("No previous scan found - creating empty baseline scan") new_params = copy.deepcopy(params.__dict__) @@ -2424,6 +2450,7 @@ def create_new_diff( try: head_full_scan = self.create_full_scan(empty_files, tmp_params, base_paths=base_paths) head_full_scan_id = head_full_scan.id + baseline_report_url = normalize_report_url(head_full_scan.html_report_url) log.info( "Baseline selected: source=empty " f"scan_id={json.dumps(head_full_scan_id)}" @@ -2513,22 +2540,26 @@ def create_new_diff( diff = self.create_diff_report(added_packages, removed_packages, unchanged_packages) diff.packages = packages - base_socket = "https://socket.dev/dashboard/org" diff.set_scan_ids( new_full_scan.id, old_scan_id=head_full_scan_id, diff_scan_id=comparison.diff_scan_id, ) - report_url = f"{base_socket}/{self.config.org_slug}/sbom/{diff.id}" - if not params.include_license_details: - report_url += "?include_license_details=false" - diff.report_url = report_url - - if head_full_scan_id is not None: - diff.diff_url = f"{base_socket}/{self.config.org_slug}/diff/{head_full_scan_id}/{diff.id}" - else: - diff.diff_url = diff.report_url + old_scan_url = comparison.old_scan_url or baseline_report_url + new_scan_url = comparison.new_scan_url or normalize_report_url( + new_full_scan.html_report_url + ) + diff.old_scan_url = old_scan_url + diff.new_scan_url = new_scan_url + diff.report_url = ( + new_scan_url + if params.include_license_details + else add_query_params( + new_scan_url, {"include_license_details": "false"} + ) + ) + diff.diff_url = comparison.diff_url # PR/MR runs previously logged only the scan IDs, so a CI log had no link to the # result. Logged here rather than at each call site so every diff flow gets it. diff --git a/socketsecurity/core/alert_selection.py b/socketsecurity/core/alert_selection.py index bc303705..14f1595b 100644 --- a/socketsecurity/core/alert_selection.py +++ b/socketsecurity/core/alert_selection.py @@ -41,6 +41,8 @@ def clone_diff_with_selected_alerts(diff: Diff, selected_alerts: List[Issue]) -> selected_diff.new_scan_id = getattr(diff, "new_scan_id", None) selected_diff.old_scan_id = getattr(diff, "old_scan_id", None) selected_diff.diff_scan_id = getattr(diff, "diff_scan_id", None) + selected_diff.old_scan_url = getattr(diff, "old_scan_url", None) + selected_diff.new_scan_url = getattr(diff, "new_scan_url", None) return selected_diff diff --git a/socketsecurity/core/classes.py b/socketsecurity/core/classes.py index 4504e1af..3eabd41c 100644 --- a/socketsecurity/core/classes.py +++ b/socketsecurity/core/classes.py @@ -569,8 +569,10 @@ class Diff: removed_alerts: list[Issue] id: str sbom: str - report_url: str - diff_url: str + report_url: Optional[str] + diff_url: Optional[str] + old_scan_url: Optional[str] + new_scan_url: Optional[str] new_scan_id: Optional[str] old_scan_id: Optional[str] diff_scan_id: Optional[str] @@ -607,6 +609,10 @@ def __init__(self, **kwargs): self.old_scan_id = None if not hasattr(self, "diff_scan_id"): self.diff_scan_id = None + if not hasattr(self, "old_scan_url"): + self.old_scan_url = None + if not hasattr(self, "new_scan_url"): + self.new_scan_url = None def set_scan_ids( self, diff --git a/socketsecurity/core/diff_result.py b/socketsecurity/core/diff_result.py index 16b2fb7d..112134ca 100644 --- a/socketsecurity/core/diff_result.py +++ b/socketsecurity/core/diff_result.py @@ -10,6 +10,9 @@ class DiffScanResult: artifacts: DiffArtifacts diff_scan_id: Optional[str] + diff_url: Optional[str] = None + old_scan_url: Optional[str] = None + new_scan_url: Optional[str] = None @dataclass @@ -18,10 +21,30 @@ class PackageComparisonResult: removed_packages: Dict[str, Package] packages: Dict[str, Package] diff_scan_id: Optional[str] + diff_url: Optional[str] = None + old_scan_url: Optional[str] = None + new_scan_url: Optional[str] = None + + +def normalize_report_url(value: object) -> Optional[str]: + if isinstance(value, str) and value: + return value + return None + + +def matching_full_scan_url(value: object, scan_id: str) -> Optional[str]: + if not isinstance(value, dict) or value.get("id") != scan_id: + return None + return normalize_report_url(value.get("html_url")) def create_diff_scan_result( - artifacts: Dict[str, object], diff_scan_id: Optional[str] + artifacts: Dict[str, object], + diff_scan_id: Optional[str], + *, + diff_url: Optional[str] = None, + old_scan_url: Optional[str] = None, + new_scan_url: Optional[str] = None, ) -> DiffScanResult: return DiffScanResult( artifacts=DiffArtifacts.from_dict({ @@ -29,4 +52,7 @@ def create_diff_scan_result( for key in ("added", "removed", "unchanged", "replaced", "updated") }), diff_scan_id=diff_scan_id, + diff_url=diff_url, + old_scan_url=old_scan_url, + new_scan_url=new_scan_url, ) diff --git a/socketsecurity/core/messages.py b/socketsecurity/core/messages.py index 4017a9f8..26c2460d 100644 --- a/socketsecurity/core/messages.py +++ b/socketsecurity/core/messages.py @@ -11,6 +11,7 @@ from prettytable import PrettyTable from socketsecurity.core.classes import Diff, Issue, Purl +from socketsecurity.core.report_urls import add_query_params log = logging.getLogger("socketcli") @@ -120,10 +121,10 @@ def get_manifest_file_url(diff: Diff, manifest_path: str, config=None) -> str: # Fallback to Socket file view for API or unknown repository types if hasattr(diff, 'report_url') and diff.report_url: - # Strip leading slash and URL encode for Socket dashboard - socket_path = clean_path.lstrip('/') - encoded_path = socket_path.replace('/', '%2F') - return f"{diff.report_url}?tab=files&file={encoded_path}" + return add_query_params( + diff.report_url, + {"tab": "files", "file": clean_path.lstrip('/')}, + ) or "" return "" @@ -1105,12 +1106,15 @@ def security_comment_template(diff: Diff, config=None) -> str: """ # Close table + report_link = ( + f"\n\n[View full report]({add_query_params(view_report_url, {'action': 'error,warn'})})" + if view_report_url + else "" + ) comment += f""" - - -[View full report]({view_report_url}?action=error%2Cwarn) +{report_link} """ return Messages.normalize_comment_html(comment) @@ -1281,7 +1285,9 @@ def short_dependency_overview_comment(diff: Diff) -> MdUtils: md.new_line("Review the following changes in direct dependencies. Learn more about [socket.dev](https://socket.dev)") md.new_line() md.new_line("The amount of dependency changes were to long for this comment. Please check out the full report") - md.new_line(f"To view more information about this report checkout the [Full Report]({diff.diff_url})") + view_report_url = Messages.get_view_report_url(diff) + if view_report_url: + md.new_line(f"To view more information about this report checkout the [Full Report]({view_report_url})") return md @staticmethod diff --git a/socketsecurity/core/report_urls.py b/socketsecurity/core/report_urls.py new file mode 100644 index 00000000..f7880e77 --- /dev/null +++ b/socketsecurity/core/report_urls.py @@ -0,0 +1,23 @@ +from typing import Mapping, Optional +from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit + + +def add_query_params( + url: Optional[str], params: Mapping[str, str] +) -> Optional[str]: + if not url: + return None + try: + parts = urlsplit(url) + except ValueError: + return url + replaced_keys = set(params) + query = [ + (key, value) + for key, value in parse_qsl(parts.query, keep_blank_values=True) + if key not in replaced_keys + ] + query.extend(params.items()) + return urlunsplit( + (parts.scheme, parts.netloc, parts.path, urlencode(query), parts.fragment) + ) diff --git a/socketsecurity/plugins/jira.py b/socketsecurity/plugins/jira.py index 9de187f9..bc4d80b2 100644 --- a/socketsecurity/plugins/jira.py +++ b/socketsecurity/plugins/jira.py @@ -1,10 +1,12 @@ import base64 +from typing import Optional import requests from socketsecurity.config import CliConfig from socketsecurity.core import log from socketsecurity.core.classes import Diff +from socketsecurity.core.messages import Messages from .base import Plugin @@ -24,8 +26,6 @@ def send(self, diff: Diff, config: CliConfig): parts = ["Security Issues found in Socket Security results"] pr = getattr(config, "pr_number", "") sha = getattr(config, "commit_sha", "")[:8] if getattr(config, "commit_sha", "") else "" - scan_link = getattr(diff, "diff_url", "") - if pr and pr != "0": parts.append(f"for PR {pr}") if sha: @@ -36,18 +36,7 @@ def send(self, diff: Diff, config: CliConfig): "type": "doc", "version": 1, "content": [ - { - "type": "paragraph", - "content": [ - {"type": "text", "text": "Security issues were found in this scan:"}, - {"type": "text", "text": "\n"}, - { - "type": "text", - "text": "View Socket Security scan results", - "marks": [{"type": "link", "attrs": {"href": scan_link}}] - } - ] - }, + self.create_adf_paragraph(diff), self.create_adf_table_from_diff(diff) ] } @@ -81,6 +70,24 @@ def send(self, diff: Diff, config: CliConfig): else: log.info(f"Jira ticket created: {response.json().get('key')}") + @staticmethod + def create_adf_paragraph(diff: Diff) -> dict[str, object]: + return { + "type": "paragraph", + "content": [ + {"type": "text", "text": "Security issues were found in this scan:"}, + {"type": "text", "text": "\n"}, + JiraPlugin._scan_link_adf(Messages.get_view_report_url(diff)), + ], + } + + @staticmethod + def _scan_link_adf(scan_link: Optional[str]) -> dict[str, object]: + text = {"type": "text", "text": "View Socket Security scan results"} + if scan_link: + text["marks"] = [{"type": "link", "attrs": {"href": scan_link}}] + return text + @staticmethod def flatten_adf_to_text(adf): def extract_text(node): diff --git a/socketsecurity/plugins/slack.py b/socketsecurity/plugins/slack.py index fae3b2c7..85ff8c7a 100644 --- a/socketsecurity/plugins/slack.py +++ b/socketsecurity/plugins/slack.py @@ -551,7 +551,7 @@ def _create_reachability_slack_blocks_from_structured(self, notification: dict, """ pr = getattr(config, "pr_number", None) sha = getattr(config, "commit_sha", None) - diff_url = getattr(diff, "diff_url", "") if diff else "" + diff_url = Messages.get_view_report_url(diff) if diff else "" title_part = "" if pr: @@ -690,8 +690,8 @@ def _normalize_url_config(self, url_input): def create_slack_blocks_from_diff(diff: Diff, config: CliConfig): pr = getattr(config, "pr_number", None) sha = getattr(config, "commit_sha", None) - scan_link = getattr(diff, "diff_url", "") - scan = f"<{scan_link}|scan>" + scan_link = Messages.get_view_report_url(diff) + scan = f"<{scan_link}|scan>" if scan_link else "scan" title_part = "" if pr: title_part += f" for PR {pr}" diff --git a/tests/core/test_diff_identity_transport.py b/tests/core/test_diff_identity_transport.py index 39158975..27a34ea6 100644 --- a/tests/core/test_diff_identity_transport.py +++ b/tests/core/test_diff_identity_transport.py @@ -92,6 +92,7 @@ def test_primary_diff_scan_uses_server_resource_id_and_real_cached_get( result = core._get_diff_scan_result("baseline-scan-id", "new-scan-id") assert result.diff_scan_id == "server-diff-id" + assert result.diff_url is None assert result.artifacts.added == [] assert [request.path for request in socket_api.requests] == [ "organizations", @@ -120,12 +121,31 @@ def duplicate(_request: SocketRequest) -> SocketResponse: socket_api.route( "GET", "orgs/fixture-org/diff-scans/listed-diff-id", - SocketResponse(200, completion_response()), + SocketResponse( + 200, + { + "diff_scan": { + "html_url": "https://reports.example.invalid/duplicate-diff?via=get#done", + "before_full_scan": { + "id": "baseline-scan-id", + "html_url": "https://reports.example.invalid/duplicate-before", + }, + "after_full_scan": { + "id": "new-scan-id", + "html_url": "https://reports.example.invalid/duplicate-after", + }, + "artifacts": empty_artifacts(), + } + }, + ), ) result = core._get_diff_scan_result("baseline-scan-id", "new-scan-id") assert result.diff_scan_id == "listed-diff-id" + assert result.diff_url == "https://reports.example.invalid/duplicate-diff?via=get#done" + assert result.old_scan_url == "https://reports.example.invalid/duplicate-before" + assert result.new_scan_url == "https://reports.example.invalid/duplicate-after" assert [request.path for request in socket_api.requests] == [ "organizations", "orgs/fixture-org/diff-scans/from-ids", @@ -140,6 +160,67 @@ def duplicate(_request: SocketRequest) -> SocketResponse: assert socket_api.requests[3].query == {"cached": ["true"]} +def test_primary_result_uses_server_urls_and_checks_nested_scan_ids( + core: Core, + socket_api: SocketApiFixture, +) -> None: + socket_api.route( + "POST", + "orgs/fixture-org/diff-scans/from-ids", + SocketResponse(201, diff_response("server-diff-id")), + ) + socket_api.route( + "GET", + "orgs/fixture-org/diff-scans/server-diff-id", + SocketResponse( + 200, + { + "diff_scan": { + "html_url": "https://reports.example.invalid/diff?pair=server#view", + "before_full_scan": { + "id": "requested-before", + "html_url": "https://reports.example.invalid/before?q=1", + }, + "after_full_scan": { + "id": "wrong-after", + "html_url": "https://reports.example.invalid/wrong", + }, + "artifacts": empty_artifacts(), + } + }, + ), + ) + + result = core._get_diff_scan_result("requested-before", "requested-after") + + assert result.diff_url == "https://reports.example.invalid/diff?pair=server#view" + assert result.old_scan_url == "https://reports.example.invalid/before?q=1" + assert result.new_scan_url is None + + +def test_streaming_comparison_carries_sdk_diff_report_url( + core: Core, + socket_api: SocketApiFixture, +) -> None: + socket_api.route( + "POST", + "orgs/fixture-org/diff-scans/from-ids", + SocketResponse(403, {"error": {"message": "forbidden"}}), + ) + socket_api.route( + "GET", + "orgs/fixture-org/full-scans/diff", + SocketResponse(200, stream_diff_response()), + ) + + result = core._get_package_comparison("baseline-scan-id", "new-scan-id") + + assert result.diff_scan_id is None + assert result.diff_url == "https://socket.dev/fixture" + assert result.old_scan_url is None + assert result.new_scan_url is None + + @pytest.mark.parametrize("failure_status", [403, 200]) def test_streaming_fallback_does_not_return_attempted_diff_id( core: Core, @@ -212,11 +293,15 @@ def test_compatibility_wrappers_preserve_legacy_return_shapes( @pytest.mark.parametrize("comparison_succeeds", [True, False]) +@pytest.mark.parametrize("include_license_details", [True, False]) +@pytest.mark.parametrize("null_urls", [True, False]) def test_create_new_diff_retains_real_empty_baseline_id( core: Core, socket_api: SocketApiFixture, tmp_path, comparison_succeeds: bool, + include_license_details: bool, + null_urls: bool, ) -> None: socket_api.route( "GET", @@ -257,7 +342,38 @@ def create_full_scan(_request: SocketRequest) -> SocketResponse: socket_api.route("POST", "orgs/fixture-org/full-scans", create_full_scan) if comparison_succeeds: - install_primary_routes(socket_api, "new-comparison-id") + socket_api.route( + "POST", + "orgs/fixture-org/diff-scans/from-ids", + SocketResponse(201, diff_response("new-comparison-id")), + ) + socket_api.route( + "GET", + "orgs/fixture-org/diff-scans/new-comparison-id", + SocketResponse( + 200, + { + "diff_scan": { + "html_url": None + if null_urls + else "https://reports.example.invalid/diff?canonical=1#comparison", + "before_full_scan": { + "id": "temporary-baseline-id", + "html_url": None + if null_urls + else "https://reports.example.invalid/before?source=get", + }, + "after_full_scan": { + "id": "new-full-scan-id", + "html_url": None + if null_urls + else "https://reports.example.invalid/after?source=get#scan", + }, + "artifacts": empty_artifacts(), + } + }, + ), + ) else: socket_api.route( "POST", @@ -277,7 +393,7 @@ def create_full_scan(_request: SocketRequest) -> SocketResponse: branch="feature", commit_hash="new-commit", ) - params.include_license_details = True + params.include_license_details = include_license_details diff = core.create_new_diff( [str(tmp_path)], @@ -290,6 +406,40 @@ def create_full_scan(_request: SocketRequest) -> SocketResponse: assert diff.new_scan_id == "new-full-scan-id" assert diff.old_scan_id == "temporary-baseline-id" assert diff.diff_scan_id == ("new-comparison-id" if comparison_succeeds else None) + expected_new_scan_url = ( + "https://socket.dev/scan/new-full-scan-id" + if null_urls or not comparison_succeeds + else "https://reports.example.invalid/after?source=get#scan" + ) + expected_report_url = ( + expected_new_scan_url + if include_license_details + else "" + ) + if not include_license_details: + report_base, separator, fragment = expected_new_scan_url.partition("#") + query_separator = "&" if "?" in report_base else "?" + expected_report_url = ( + f"{report_base}{query_separator}include_license_details=false" + f"{separator}{fragment}" if separator else + f"{report_base}{query_separator}include_license_details=false" + ) + assert diff.new_scan_url == expected_new_scan_url + assert diff.report_url == expected_report_url + if comparison_succeeds and not null_urls: + assert diff.diff_url == "https://reports.example.invalid/diff?canonical=1#comparison" + assert diff.old_scan_url == "https://reports.example.invalid/before?source=get" + elif comparison_succeeds: + assert diff.diff_url is None + assert diff.old_scan_url == "https://socket.dev/scan/temporary-baseline-id" + else: + assert diff.diff_url == "https://socket.dev/fixture" + assert diff.old_scan_url == "https://socket.dev/scan/temporary-baseline-id" + assert diff.new_scan_url == "https://socket.dev/scan/new-full-scan-id" + assert sum( + request.path == "orgs/fixture-org/full-scans/diff" + for request in socket_api.requests + ) == int(not comparison_succeeds) full_scan_requests = [ request for request in socket_api.requests @@ -386,6 +536,9 @@ def test_full_scan_identity_is_preserved_with_findings_disabled_or_enabled( assert result.diff_scan_id is None assert result.is_full_scan is True assert result.alerts_fetched is enable_json + assert result.new_scan_url == "https://socket.dev/scan/full-scan-id" + assert result.old_scan_url is None + assert result.report_url == result.diff_url == result.new_scan_url if enable_json: assert result.packages["artifact-fixture-id"].purl == "npm/sample-package@1.0.0" findings_requests = [ @@ -412,4 +565,6 @@ def test_no_change_paths_keep_sentinels_without_transport_calls( assert result.new_scan_id is None assert result.old_scan_id is None assert result.diff_scan_id is None + assert result.old_scan_url is None + assert result.new_scan_url is None assert len(socket_api.requests) == request_count_after_setup diff --git a/tests/unit/test_diff_identity.py b/tests/unit/test_diff_identity.py index 7dd60cf2..905bcd78 100644 --- a/tests/unit/test_diff_identity.py +++ b/tests/unit/test_diff_identity.py @@ -18,6 +18,8 @@ def test_diff_scan_ids_default_and_assignment(): assert diff.new_scan_id is None assert diff.old_scan_id is None assert diff.diff_scan_id is None + assert diff.old_scan_url is None + assert diff.new_scan_url is None diff.set_scan_ids("new-scan", old_scan_id="old-scan", diff_scan_id="diff-resource") @@ -86,6 +88,7 @@ def artifact(bucket: str) -> dict[str, str]: assert isinstance(result, DiffScanResult) assert result.diff_scan_id == "server-diff-id" + assert result.diff_url is None assert isinstance(result.artifacts, DiffArtifacts) for bucket in ("added", "removed", "unchanged", "replaced", "updated"): assert [item.id for item in getattr(result.artifacts, bucket)] == [ @@ -136,6 +139,7 @@ def test_package_comparison_result_retains_maps_and_identity(): assert result.removed_packages is removed assert result.packages is packages assert result.diff_scan_id is None + assert result.diff_url is None def test_alert_clone_preserves_scan_identity_and_no_run_absence(): @@ -163,6 +167,11 @@ def test_alert_clone_preserves_scan_identity_and_no_run_absence(): assert clone.new_scan_id == "new-scan" assert clone.old_scan_id == "old-scan" assert clone.diff_scan_id == "diff-resource" + diff.old_scan_url = "https://reports.invalid/old" + diff.new_scan_url = "https://reports.invalid/new" + clone = clone_diff_with_selected_alerts(diff, [issue]) + assert clone.old_scan_url == diff.old_scan_url + assert clone.new_scan_url == diff.new_scan_url assert diff.new_alerts == [issue] assert fallback_clone.new_scan_id == "fallback-new-scan" assert fallback_clone.old_scan_id == "fallback-old-scan" diff --git a/tests/unit/test_manifest_discovery.py b/tests/unit/test_manifest_discovery.py index 2bdc2bbd..feff7b14 100644 --- a/tests/unit/test_manifest_discovery.py +++ b/tests/unit/test_manifest_discovery.py @@ -282,31 +282,6 @@ def test_results_are_sorted_and_deduplicated_across_overlapping_patterns(tmp_pat assert len(found) == 2 -def test_explicit_discovery_results_prevent_a_second_walk(tmp_path): - manifest = tmp_path / "package.json" - manifest.write_text("{}", encoding="utf-8") - core = _make_core() - core.config.org_slug = "example" - core.cli_config = None - core.find_files = MagicMock(side_effect=AssertionError("unexpected second walk")) - core.create_full_scan = MagicMock(return_value=SimpleNamespace(id="scan-123")) - params = MagicMock() - - diff = core.create_full_scan_with_report_url( - [str(tmp_path)], - params, - explicit_files=[manifest.as_posix()], - ) - - core.find_files.assert_not_called() - core.create_full_scan.assert_called_once_with( - [manifest.as_posix()], - params, - base_paths=None, - ) - assert diff.id == "scan-123" - - def test_core_initialization_logs_organization_timing(caplog): sdk = MagicMock() sdk.org.get.return_value = { diff --git a/tests/unit/test_report_urls.py b/tests/unit/test_report_urls.py new file mode 100644 index 00000000..72d7d178 --- /dev/null +++ b/tests/unit/test_report_urls.py @@ -0,0 +1,129 @@ +from socketsecurity.config import CliConfig +from socketsecurity.core.classes import Diff, Issue +from socketsecurity.core.diff_result import matching_full_scan_url, normalize_report_url +from socketsecurity.core.messages import Messages +from socketsecurity.core.report_urls import add_query_params +from socketsecurity.plugins.jira import JiraPlugin +from socketsecurity.plugins.slack import SlackPlugin + + +def test_url_normalization_and_nested_scan_identity(): + assert normalize_report_url(" https://report.invalid/a?q=1#part ") == ( + " https://report.invalid/a?q=1#part " + ) + assert normalize_report_url(None) is None + assert normalize_report_url("") is None + assert normalize_report_url(42) is None + assert matching_full_scan_url( + {"id": "expected", "html_url": "https://report.invalid/scan"}, + "expected", + ) == "https://report.invalid/scan" + assert matching_full_scan_url( + {"id": "other", "html_url": "https://report.invalid/scan"}, + "expected", + ) is None + assert matching_full_scan_url( + {"id": "expected", "html_url": None}, "expected" + ) is None + assert matching_full_scan_url([], "expected") is None + + +def test_query_addition_preserves_unrelated_pairs_and_fragment(): + assert add_query_params( + "https://reports.invalid/path?keep=a%2Fb&keep=two&action=old#section", + {"action": "error,warn", "file": "folder/name file.txt"}, + ) == ( + "https://reports.invalid/path?keep=a%2Fb&keep=two&action=error%2Cwarn&" + "file=folder%2Fname+file.txt#section" + ) + assert add_query_params("https://reports.invalid/path#fragment", {"tab": "files"}) == ( + "https://reports.invalid/path?tab=files#fragment" + ) + assert add_query_params(None, {"tab": "files"}) is None + assert add_query_params("", {"tab": "files"}) is None + assert add_query_params("https://[malformed", {"tab": "files"}) == ( + "https://[malformed" + ) + + +def test_manifest_and_report_links_handle_query_and_missing_urls(): + diff = Diff(report_url="https://reports.invalid/scan?keep=yes#details") + manifest = Messages.get_manifest_file_url(diff, "folder/a b.txt") + assert manifest == ( + "https://reports.invalid/scan?keep=yes&tab=files&file=folder%2Fa+b.txt#details" + ) + assert Messages.get_manifest_file_url(Diff(), "folder/a.txt") == "" + + alert_diff = Diff( + diff_url="https://reports.invalid/diff?keep=yes#details", + report_url="https://reports.invalid/report", + new_alerts=[], + ) + assert Messages.get_view_report_url(alert_diff) == ( + "https://reports.invalid/diff?keep=yes#details" + ) + alert_diff.diff_url = None + assert Messages.get_view_report_url(alert_diff) == "https://reports.invalid/report" + alert_diff.report_url = None + assert Messages.get_view_report_url(alert_diff) == "" + + +def test_slack_and_jira_report_consumers_fall_back_or_omit_links(): + linked_diff = Diff( + report_url="https://reports.invalid/report", + diff_url=None, + ) + blocks = SlackPlugin.create_slack_blocks_from_diff( + linked_diff, CliConfig(api_token="fixture", repo="fixture/repo") + ) + assert "" in blocks[0]["text"]["text"] + paragraph = JiraPlugin.create_adf_paragraph(linked_diff) + assert paragraph["content"][2]["marks"][0]["attrs"]["href"] == ( + "https://reports.invalid/report" + ) + + missing_diff = Diff(report_url=None, diff_url=None) + blocks_without_link = SlackPlugin.create_slack_blocks_from_diff( + missing_diff, CliConfig(api_token="fixture", repo="fixture/repo") + ) + assert "<|scan>" not in blocks_without_link[0]["text"]["text"] + assert "this *scan*" in blocks_without_link[0]["text"]["text"] + paragraph_without_link = JiraPlugin.create_adf_paragraph(missing_diff) + assert paragraph_without_link["content"][2] == { + "type": "text", + "text": "View Socket Security scan results", + } + + +def test_alert_comment_uses_query_safe_report_link_and_omits_absence(): + issue = Issue( + pkg_name="sample", + pkg_version="1.0.0", + pkg_type="npm", + type="malware", + severity="high", + title="Finding", + description="Description", + suggestion="Upgrade", + purl="pkg:npm/sample@1.0.0", + url="https://alerts.invalid/finding", + manifests="package.json", + introduced_by=[], + error=True, + ) + diff = Diff( + new_alerts=[issue], + report_url="https://reports.invalid/scan?keep=yes#details", + diff_url=None, + ) + + comment = Messages.security_comment_template(diff) + + assert ( + "[View full report](https://reports.invalid/scan?keep=yes&action=error%2Cwarn#details)" + in comment + ) + diff.report_url = None + comment_without_link = Messages.security_comment_template(diff) + assert "View full report" not in comment_without_link + assert "(None" not in comment_without_link diff --git a/tests/unit/test_scan_scope.py b/tests/unit/test_scan_scope.py index 6ee6ca7d..237eede8 100644 --- a/tests/unit/test_scan_scope.py +++ b/tests/unit/test_scan_scope.py @@ -1,6 +1,8 @@ from __future__ import annotations from collections.abc import Iterator +from email import policy +from email.parser import BytesParser import pytest from socketdev import socketdev @@ -104,6 +106,67 @@ def test_scan_configuration_omits_absolute_and_manifest_paths(caplog) -> None: assert "requirements.txt" not in message +def test_explicit_manifests_are_uploaded_without_rediscovery( + caplog, + socket_api: SocketApiFixture, + tmp_path, +) -> None: + core = create_core(socket_api) + socket_api.route( + "POST", + "orgs/fixture-org/full-scans", + full_scan_response("scan-123"), + ) + selected = tmp_path / "package.json" + selected_content = '{"name":"explicit-selection-fixture"}\n' + selected.write_text(selected_content, encoding="utf-8") + unselected = tmp_path / "requirements.txt" + unselected_content = "excluded-selection-fixture==1.0.0\n" + unselected.write_text(unselected_content, encoding="utf-8") + params = FullScanParams( + repo="fixture-repo", + org_slug="fixture-org", + branch="feature", + ) + + with caplog.at_level("DEBUG", logger="socketdev"): + result = core.create_full_scan_with_report_url( + [str(tmp_path)], + params, + explicit_files=[str(selected)], + base_paths=[str(tmp_path)], + ) + + assert [(request.method, request.path) for request in socket_api.requests] == [ + ("GET", "organizations"), + ("POST", "orgs/fixture-org/full-scans"), + ] + upload_request = socket_api.requests[-1] + content_type = upload_request.headers["Content-Type"] + message = BytesParser(policy=policy.default).parsebytes( + f"Content-Type: {content_type}\r\nMIME-Version: 1.0\r\n\r\n".encode() + + upload_request.body + ) + uploaded_files = [part for part in message.iter_parts() if part.get_filename()] + assert len(uploaded_files) == 1 + assert uploaded_files[0].get_filename() == selected.name + uploaded_content = uploaded_files[0].get_payload(decode=True).decode("utf-8") + assert uploaded_content == selected_content + assert unselected_content not in upload_request.body.decode("latin-1") + assert any( + "manifests=1 manifest_source=provided" in record.message + for record in caplog.records + ) + assert not any( + "Starting manifest discovery" in record.message + or "Manifest discovery completed" in record.message + for record in caplog.records + ) + assert result.id == result.new_scan_id == "scan-123" + assert result.report_url == "https://socket.dev/scan/scan-123" + assert result.is_full_scan is True + + def test_multiple_scan_paths_are_uploaded_as_one_combined_full_scan( caplog, socket_api: SocketApiFixture,