diff --git a/CHANGELOG.md b/CHANGELOG.md index 2fa5ad6..3909587 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # Changelog +## 2.9.8 + +### Fixed: oversized commit messages no longer fail the scan + +- The 200-character cap on the commit message now applies to the value read from the + repository, not only to `--commit-message`. A truncated message ends in `...` and the + truncation is reported at INFO. +- A full scan refused for its size (HTTP 413, 414 or 431) now distinguishes possible + upload-size and request-metadata causes and reports what to shorten. + ## 2.9.7 ### Changed: bump pinned @coana-tech/cli to 15.10.51 diff --git a/pyproject.toml b/pyproject.toml index 3b73053..db09130 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,7 +6,7 @@ build-backend = "hatchling.build" [project] name = "socketsecurity" -version = "2.9.7" +version = "2.9.8" requires-python = ">= 3.11" license = {"file" = "LICENSE"} dependencies = [ diff --git a/socketsecurity/__init__.py b/socketsecurity/__init__.py index de72965..5730d9b 100644 --- a/socketsecurity/__init__.py +++ b/socketsecurity/__init__.py @@ -1,3 +1,3 @@ __author__ = 'socket.dev' -__version__ = '2.9.7' +__version__ = '2.9.8' USER_AGENT = f'SocketPythonCLI/{__version__}' diff --git a/socketsecurity/config.py b/socketsecurity/config.py index bb2ad00..d497f8e 100644 --- a/socketsecurity/config.py +++ b/socketsecurity/config.py @@ -19,6 +19,32 @@ def get_plugin_config_from_env(prefix: str) -> dict: return {} +# commit_message rides in the query string of POST /v0/orgs/{org}/full-scans, so an +# oversized message overflows the edge proxy's request line limit before the API ever +# sees it. The API itself has no length validation on the field; the rejection comes +# from the proxy, which reports 413, 414 or 431 depending on which limit it checks. 200 +# chars is a conservative ceiling given URL encoding can 2-3x the raw character count. +MAX_COMMIT_MESSAGE_LENGTH = 200 + + +COMMIT_MESSAGE_TRUNCATION_MARKER = "..." + + +def truncate_commit_message(commit_message: Optional[str]) -> Optional[str]: + """Cap commit_message to a length the full-scan request line can carry.""" + if commit_message and len(commit_message) > MAX_COMMIT_MESSAGE_LENGTH: + # INFO, not DEBUG: the scan keeps the truncated value, so for a CI job that does + # not pass --enable-debug this line is the only explanation of why the message in + # the dashboard is clipped. + logging.info( + f"commit_message truncated from {len(commit_message)} to " + f"{MAX_COMMIT_MESSAGE_LENGTH} characters to stay within API request size limits" + ) + keep = MAX_COMMIT_MESSAGE_LENGTH - len(COMMIT_MESSAGE_TRUNCATION_MARKER) + return commit_message[:keep] + COMMIT_MESSAGE_TRUNCATION_MARKER + return commit_message + + def load_cli_config_file(config_path: str) -> dict: """ Load CLI defaults from a JSON or TOML file. @@ -201,7 +227,13 @@ class CliConfig: legal: bool = False legal_format: str = "socket" config_file: Optional[str] = None - + + def __post_init__(self): + # Capped on construction so that every source of commit_message -- the + # --commit-message flag, a config file, the git backfill in socketcli -- lands + # under the limit. + self.commit_message = truncate_commit_message(self.commit_message) + @classmethod def from_args(cls, args_list: Optional[List[str]] = None) -> 'CliConfig': parser = create_argument_parser() @@ -257,19 +289,6 @@ def from_args(cls, args_list: Optional[List[str]] = None) -> 'CliConfig': if commit_message and commit_message.startswith('"') and commit_message.endswith('"'): commit_message = commit_message[1:-1] - # Truncate to avoid 413s from oversized URL query parameters. - # The API has no application-layer length validation on commit_message; - # the 413 originates from an infrastructure-layer URL length limit - # (nginx/Cloudflare). 200 chars chosen as a conservative ceiling given - # URL encoding can 2-3x raw character count. - MAX_COMMIT_MESSAGE_LENGTH = 200 - if commit_message and len(commit_message) > MAX_COMMIT_MESSAGE_LENGTH: - logging.debug( - f"commit_message truncated from {len(commit_message)} to " - f"{MAX_COMMIT_MESSAGE_LENGTH} characters to avoid API request size limits" - ) - commit_message = commit_message[:MAX_COMMIT_MESSAGE_LENGTH] - config_args = { 'api_token': api_token, 'repo': args.repo, diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index fe49bce..811bf65 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -113,6 +113,12 @@ FULL_SCAN_UPLOAD_MAX_ATTEMPTS = len(FULL_SCAN_UPLOAD_BACKOFF_SCHEDULE_SECONDS) FULL_SCAN_UPLOAD_BACKOFF_JITTER_SECONDS = 2.0 +# Statuses that mean the request is too large to process. Scan metadata travels in the +# query string of the full-scan POST, while manifests travel in its multipart body. A +# 413 can refer to either part; 414 points to the URL, and some proxies report 431 when +# the encoded request target exceeds their header limit. None are transient. +REQUEST_TOO_LARGE_STATUS_CODES = (413, 414, 431) + # Diff-scan polling policy. The legacy scan comparison (fullscans.stream_diff) holds a # single HTTP connection open, fully idle, while the backend computes the diff; network # middleboxes with TCP idle timeouts (notably Azure NAT gateways, which default to @@ -1118,6 +1124,23 @@ def create_full_scan(self, files: List[str], params: FullScanParams, base_paths: res = self.sdk.fullscans.post(upload_files, params, use_types=True, use_lazy_loading=True, max_open_files=50, base_paths=base_paths) break except APIFailure as error: + if error.status_code in REQUEST_TOO_LARGE_STATUS_CODES: + if error.status_code == 413: + guidance = ( + "The response does not distinguish between an oversized multipart " + "upload and oversized scan metadata in the request URL. Reduce the " + "uploaded scan inputs, or pass a shorter --commit-message." + ) + else: + guidance = ( + "Scan metadata is sent in the request URL. Pass a shorter " + "--commit-message or shorten other scan metadata." + ) + raise APIFailure( + f"Full scan request rejected as too large (HTTP {error.status_code}). " + f"{guidance}\n{error}", + status_code=error.status_code, + ) from error if backoff_seconds is None or not error.is_transient_error(): raise wait_seconds = backoff_seconds + random.uniform( diff --git a/socketsecurity/socketcli.py b/socketsecurity/socketcli.py index f585d99..2be6850 100644 --- a/socketsecurity/socketcli.py +++ b/socketsecurity/socketcli.py @@ -12,7 +12,7 @@ from socketdev import socketdev from socketdev.fullscans import FullScanParams -from socketsecurity.config import CliConfig +from socketsecurity.config import CliConfig, truncate_commit_message from socketsecurity.core import Core from socketsecurity.core.classes import Diff from socketsecurity.core.cli_client import CliClient @@ -210,6 +210,35 @@ def create_scm_scan( return diff, False +def apply_git_context(config: CliConfig) -> Tuple[bool, Optional[Git]]: + """ + Fill in any repo details the caller did not pass from the checkout at target_path. + + Returns whether target_path is a git repository, along with the Git handle when it is. + """ + try: + git_repo = Git(config.target_path) + except InvalidGitRepositoryError: + log.debug("Not a git repository, setting ignore_commit_files=True") + config.ignore_commit_files = True + return False, None + except NoSuchPathError: + raise Exception(f"Unable to find path {config.target_path}") + + if not config.repo: + config.repo = git_repo.repo_name + if not config.commit_sha: + config.commit_sha = git_repo.commit_str + if not config.branch: + config.branch = git_repo.branch + if not config.committers: + config.committers = [git_repo.get_formatted_committer()] + if not config.commit_message: + # A repository's commit message is unbounded and ships in the query string. + config.commit_message = truncate_commit_message(git_repo.commit_message) + return True, git_repo + + def build_socket_sdk(config: CliConfig) -> socketdev: cli_user_agent_string = f"SocketPythonCLI/{config.version}" return socketdev( @@ -402,27 +431,7 @@ def main_code(): discovered_scan_files = None # Git setup - is_repo = False - git_repo: Git - try: - git_repo = Git(config.target_path) - is_repo = True - if not config.repo: - config.repo = git_repo.repo_name - if not config.commit_sha: - config.commit_sha = git_repo.commit_str - if not config.branch: - config.branch = git_repo.branch - if not config.committers: - config.committers = [git_repo.get_formatted_committer()] - if not config.commit_message: - config.commit_message = git_repo.commit_message - except InvalidGitRepositoryError: - is_repo = False - log.debug("Not a git repository, setting ignore_commit_files=True") - config.ignore_commit_files = True - except NoSuchPathError: - raise Exception(f"Unable to find path {config.target_path}") + is_repo, git_repo = apply_git_context(config) # Track whether repo/branch fell back to the default sentinels so reachability can skip # forwarding them as coana cache-bucket keys (computed before any workspace suffixing). diff --git a/tests/unit/test_cli_config.py b/tests/unit/test_cli_config.py index f70cda2..636ee9d 100644 --- a/tests/unit/test_cli_config.py +++ b/tests/unit/test_cli_config.py @@ -31,14 +31,15 @@ def test_truncated_above_limit(self): config = CliConfig.from_args( ["--api-token", "test", "--commit-message", "a" * 250] ) - assert config.commit_message == "a" * 200 + assert config.commit_message == "a" * 197 + "..." + assert len(config.commit_message) == 200 def test_quote_strip_runs_before_truncation(self): quoted = '"' + ("b" * 250) + '"' config = CliConfig.from_args( ["--api-token", "test", "--commit-message", quoted] ) - assert config.commit_message == "b" * 200 + assert config.commit_message == "b" * 197 + "..." class TestCliConfig: diff --git a/tests/unit/test_commit_message_truncation.py b/tests/unit/test_commit_message_truncation.py new file mode 100644 index 0000000..46d7bde --- /dev/null +++ b/tests/unit/test_commit_message_truncation.py @@ -0,0 +1,113 @@ +import subprocess + +import pytest + +from socketsecurity.config import ( + COMMIT_MESSAGE_TRUNCATION_MARKER, + MAX_COMMIT_MESSAGE_LENGTH, + CliConfig, + truncate_commit_message, +) +from socketsecurity.socketcli import apply_git_context + + +def _git(path, *args): + return subprocess.run( + ["git", *args], + cwd=path, + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + +@pytest.fixture +def repo_with_large_commit_message(tmp_path): + """A checkout whose HEAD commit message is far larger than the cap (~14 KB).""" + path = tmp_path / "repo" + path.mkdir() + _git(path, "init", "-b", "main") + _git(path, "config", "user.name", "Socket Test") + _git(path, "config", "user.email", "socket@example.com") + (path / "package.json").write_text("{}\n", encoding="utf-8") + _git(path, "add", "package.json") + _git(path, "commit", "-m", "Release notes\n\n" + ("- bumped a dependency\n" * 700)) + return path + + +class TestTruncateCommitMessage: + def test_none_passes_through(self): + assert truncate_commit_message(None) is None + + def test_empty_passes_through(self): + assert truncate_commit_message("") == "" + + def test_under_limit_is_unchanged(self): + msg = "a normal short commit message" + assert truncate_commit_message(msg) == msg + + def test_at_limit_is_unchanged(self): + msg = "a" * MAX_COMMIT_MESSAGE_LENGTH + assert truncate_commit_message(msg) == msg + + def test_over_limit_is_capped(self): + capped = truncate_commit_message("a" * 14_000) + assert len(capped) == MAX_COMMIT_MESSAGE_LENGTH + assert capped.endswith(COMMIT_MESSAGE_TRUNCATION_MARKER) + + def test_marker_fits_inside_the_limit(self): + # The marker replaces the tail rather than extending past it, so the capped value + # never grows the request line beyond what the proxy accepts. + assert truncate_commit_message("a" * 201) == ( + "a" * (MAX_COMMIT_MESSAGE_LENGTH - len(COMMIT_MESSAGE_TRUNCATION_MARKER)) + + COMMIT_MESSAGE_TRUNCATION_MARKER + ) + + +class TestCliConfigInvariant: + def test_direct_construction_is_capped(self): + config = CliConfig(api_token="test", repo="widgets", commit_message="a" * 14_000) + assert len(config.commit_message) == MAX_COMMIT_MESSAGE_LENGTH + + def test_config_file_value_is_capped(self, tmp_path): + config_file = tmp_path / "socketcli.json" + config_file.write_text('{"commit_message": "%s"}' % ("a" * 14_000), encoding="utf-8") + config = CliConfig.from_args(["--api-token", "test", "--config", str(config_file)]) + assert len(config.commit_message) == MAX_COMMIT_MESSAGE_LENGTH + + +class TestGitBackfill: + def test_message_read_from_git_is_capped(self, repo_with_large_commit_message): + config = CliConfig(api_token="test", repo=None, target_path=str(repo_with_large_commit_message)) + assert config.commit_message is None + + is_repo, git_repo = apply_git_context(config) + + assert is_repo is True + # The repository really does carry an oversized message; the cap is what keeps it + # out of the full-scan query string. + assert len(git_repo.commit_message) > 14_000 + assert len(config.commit_message) == MAX_COMMIT_MESSAGE_LENGTH + assert config.commit_message.startswith("Release notes") + assert config.commit_message.endswith(COMMIT_MESSAGE_TRUNCATION_MARKER) + + def test_explicit_message_is_not_overwritten_by_git(self, repo_with_large_commit_message): + config = CliConfig( + api_token="test", + repo=None, + target_path=str(repo_with_large_commit_message), + commit_message="explicit message", + ) + + apply_git_context(config) + + assert config.commit_message == "explicit message" + + def test_non_repo_path_reports_no_repo(self, tmp_path): + config = CliConfig(api_token="test", repo=None, target_path=str(tmp_path)) + + is_repo, git_repo = apply_git_context(config) + + assert is_repo is False + assert git_repo is None + assert config.ignore_commit_files is True diff --git a/tests/unit/test_full_scan_retry.py b/tests/unit/test_full_scan_retry.py index b31bb11..2485a7a 100644 --- a/tests/unit/test_full_scan_retry.py +++ b/tests/unit/test_full_scan_retry.py @@ -283,3 +283,46 @@ def test_retry_decision_delegates_to_sdk_classification( core_with_mock_sdk.create_full_scan([str(manifest)], MagicMock()) assert core_with_mock_sdk.sdk.fullscans.post.call_count == expected_calls + + +@pytest.mark.parametrize("status_code", [414, 431]) +def test_oversized_request_target_is_not_retried_and_names_the_cause( + core_with_mock_sdk, tmp_path, no_sleep, status_code +): + """ + URI and header size failures are deterministic for the same request, and the SDK's + message does not say which metadata to shorten. + """ + manifest = tmp_path / "package.json" + manifest.write_text("{}") + core_with_mock_sdk.sdk.fullscans.post.side_effect = _catch_all_failure(status_code) + + with pytest.raises(APIFailure) as exc_info: + core_with_mock_sdk.create_full_scan([str(manifest)], MagicMock()) + + assert core_with_mock_sdk.sdk.fullscans.post.call_count == 1 + no_sleep.assert_not_called() + message = str(exc_info.value) + assert f"rejected as too large (HTTP {status_code})" in message + assert "--commit-message" in message + # The SDK's original text is kept so the proxy's own response stays available. + assert f"original_status_code:{status_code}" in message + assert exc_info.value.status_code == status_code + + +def test_413_reports_upload_and_metadata_causes(core_with_mock_sdk, tmp_path, no_sleep): + manifest = tmp_path / "package.json" + manifest.write_text("{}") + core_with_mock_sdk.sdk.fullscans.post.side_effect = _catch_all_failure(413) + + with pytest.raises(APIFailure) as exc_info: + core_with_mock_sdk.create_full_scan([str(manifest)], MagicMock()) + + assert core_with_mock_sdk.sdk.fullscans.post.call_count == 1 + no_sleep.assert_not_called() + message = str(exc_info.value) + assert "oversized multipart upload" in message + assert "oversized scan metadata" in message + assert "--commit-message" in message + assert "original_status_code:413" in message + assert exc_info.value.status_code == 413 diff --git a/uv.lock b/uv.lock index 96f0318..030af62 100644 --- a/uv.lock +++ b/uv.lock @@ -1293,7 +1293,7 @@ wheels = [ [[package]] name = "socketsecurity" -version = "2.9.7" +version = "2.9.8" source = { editable = "." } dependencies = [ { name = "beautifulsoup4" },