diff --git a/.github/skills/project-planning/gitlab/pyproject.toml b/.github/skills/project-planning/gitlab/pyproject.toml index aa2fbc4990..00c31f64d3 100644 --- a/.github/skills/project-planning/gitlab/pyproject.toml +++ b/.github/skills/project-planning/gitlab/pyproject.toml @@ -20,6 +20,11 @@ fuzz = [ testpaths = ["tests"] pythonpath = ["scripts"] python_files = ["test_*.py", "fuzz_harness.py"] +addopts = "--cov --cov-report=term-missing --cov-fail-under=80" + +[tool.coverage.run] +branch = true +source = ["scripts"] [tool.ruff] line-length = 88 diff --git a/.github/skills/project-planning/gitlab/scripts/gitlab.py b/.github/skills/project-planning/gitlab/scripts/gitlab.py index d9679956dd..b62163e187 100644 --- a/.github/skills/project-planning/gitlab/scripts/gitlab.py +++ b/.github/skills/project-planning/gitlab/scripts/gitlab.py @@ -35,7 +35,7 @@ import urllib.request from dataclasses import dataclass, field from datetime import datetime, timezone -from typing import Any, Callable, NoReturn, cast +from typing import Any, Callable, cast sys.dont_write_bytecode = True @@ -196,12 +196,16 @@ class AuthContext: class GitLabError(Exception): """Base CLI failure carrying an exit code and a redacted string form. - Library-level helpers raise this instead of calling :func:`die`. A helper - that promises a return value must end every path in an explicit ``return`` - or ``raise``; relying on ``die`` never returning makes the contract - unverifiable by static analysis and turns a future change to ``die`` into a - silent ``None`` return. :func:`die` stays for the argument-parsing and - command-dispatch layer, where no value is promised. + This is the only failure mechanism in the module. Every error path raises + this class or a subclass; nothing calls ``sys.exit`` or raises + ``SystemExit`` outside the ``__main__`` guard. A single mechanism keeps the + contract verifiable by static analysis, guarantees that a helper promising + a return value cannot fall through to a silent ``None``, and gives + :func:`main` one place to emit and translate a failure. + + ``main`` catches this class, emits the redacted message once through + :func:`_emit`, and returns :attr:`exit_code` as the process status. Raising + sites therefore do not emit; the message travels on the exception. """ def __init__(self, message: str = "", exit_code: int = EXIT_FAILURE) -> None: @@ -307,21 +311,6 @@ def _response_request_id(response: Any) -> str: return "" -def die(message: str, exit_code: int = EXIT_FAILURE) -> NoReturn: - """Emit a redacted error and raise SystemExit. - - Args: - message: Error text. Routed through ``_emit`` so it is redacted and - mirrored to the module logger before reaching stderr. - exit_code: Process exit code. - - Returns: - Never returns. The annotation is kept simple for CLI usage. - """ - _emit(f"error: {message}") - raise SystemExit(exit_code) - - def _redact(text: str) -> str: """Remove common secret-looking values from any text bound for output.""" if not text: @@ -424,13 +413,13 @@ def _sanitize_remote_url(remote_url: str) -> str: def _validate_project_path(path: str) -> None: """Reject project paths that contain traversal or separator escapes.""" if not path: - die("invalid project path", EXIT_USAGE) + raise GitLabError("invalid project path", EXIT_USAGE) if any(char in path for char in {"%", "\\"}): - die("invalid project path", EXIT_USAGE) + raise GitLabError("invalid project path", EXIT_USAGE) for segment in path.split("/"): if segment in {"", ".", ".."}: - die("invalid project path", EXIT_USAGE) + raise GitLabError("invalid project path", EXIT_USAGE) def _summarize_error_body(raw_error: str) -> str: @@ -492,7 +481,9 @@ def _audit_attempt(actor: str, method: str, resource: str) -> None: try: _audit_write(_audit_event(actor, method, resource, "attempt")) except OSError as exc: - die(f"audit log write failed; refusing to proceed: {exc}", EXIT_FAILURE) + raise GitLabError( + f"audit log write failed; refusing to proceed: {exc}", EXIT_FAILURE + ) from exc def _audit_outcome( @@ -535,8 +526,10 @@ def _oauth_audit_attempt(operation: str) -> None: """Write an OAuth attempt before egress, failing closed when configured.""" try: _audit_write(_oauth_audit_event(operation, "attempt")) - except OSError: - die("audit log write failed; refusing OAuth request", EXIT_FAILURE) + except OSError as exc: + raise GitLabError( + "audit log write failed; refusing OAuth request", EXIT_FAILURE + ) from exc def _oauth_audit_outcome( @@ -567,16 +560,16 @@ def require_base_environment() -> None: gitlab_url = os.environ.get("GITLAB_URL", "") if not gitlab_url: - die("GITLAB_URL is not set", EXIT_USAGE) + raise GitLabError("GITLAB_URL is not set", EXIT_USAGE) try: gitlab_url = _normalize_base_url(gitlab_url) except ValueError as error: - die(str(error), EXIT_USAGE) + raise GitLabError(str(error), EXIT_USAGE) from error parsed_url = urllib.parse.urlsplit(gitlab_url) if parsed_url.scheme == "http": allow_insecure = os.environ.get("GITLAB_ALLOW_INSECURE", "").strip() == "1" if not _is_loopback(parsed_url.hostname) or not allow_insecure: - die( + raise GitLabError( "GITLAB_URL must use https:// for non-loopback hosts; " "plaintext http is allowed only for loopback hosts when " "GITLAB_ALLOW_INSECURE=1", @@ -598,18 +591,20 @@ def require_environment() -> None: if not mode: mode = "oauth" if mode not in {"oauth", "legacy-token"}: - die("GITLAB_AUTH_MODE must be oauth or legacy-token", EXIT_USAGE) + raise GitLabError("GITLAB_AUTH_MODE must be oauth or legacy-token", EXIT_USAGE) configured_token = os.environ.get("GITLAB_TOKEN", "") if mode == "legacy-token": if not configured_token: - die("GITLAB_TOKEN is not set for legacy-token mode", EXIT_USAGE) + raise GitLabError( + "GITLAB_TOKEN is not set for legacy-token mode", EXIT_USAGE + ) auth_context = AuthContext(mode=mode, issuer=gitlab_url, token=configured_token) else: if configured_token: - die("GITLAB_TOKEN must not be set in oauth mode", EXIT_USAGE) + raise GitLabError("GITLAB_TOKEN must not be set in oauth mode", EXIT_USAGE) client_id = os.environ.get("GITLAB_OAUTH_CLIENT_ID", "").strip() if not client_id: - die( + raise GitLabError( "GITLAB_OAUTH_CLIENT_ID is not set. Configure OAuth and run " "auth login, or explicitly set GITLAB_AUTH_MODE=legacy-token " "with GITLAB_TOKEN", @@ -621,14 +616,16 @@ def require_environment() -> None: store = credentials.load_store(store_path) profile = credentials.get_profile(store, profile_name) except credentials.CredentialError as exc: - die(str(exc), EXIT_USAGE) + raise GitLabError(str(exc), EXIT_USAGE) from exc if profile["issuer"] != gitlab_url or profile["client_id"] != client_id: - die( + raise GitLabError( "GitLab OAuth profile does not match this instance and client ID", EXIT_USAGE, ) if not profile["usable"]: - die("GitLab OAuth profile is unusable; run auth login", EXIT_USAGE) + raise GitLabError( + "GitLab OAuth profile is unusable; run auth login", EXIT_USAGE + ) auth_context = AuthContext( mode=mode, issuer=gitlab_url, @@ -706,7 +703,7 @@ def _oauth_profile(context: AuthContext) -> credentials.Profile: def _auth_headers() -> dict[str, str]: """Return the header for the resolved authentication mode.""" if auth_context is None: - die("GitLab authentication is not configured", EXIT_FAILURE) + raise GitLabError("GitLab authentication is not configured", EXIT_FAILURE) if auth_context.mode == "legacy-token": return {"PRIVATE-TOKEN": str(auth_context.token)} profile = _oauth_profile(auth_context) @@ -716,7 +713,7 @@ def _auth_headers() -> dict[str, str]: def _required_oauth_client_id(context: AuthContext) -> str: """Return the trusted OAuth client ID or fail on an incomplete context.""" if not context.client_id: - die("GitLab OAuth context is missing a client ID", EXIT_FAILURE) + raise GitLabError("GitLab OAuth context is missing a client ID", EXIT_FAILURE) return context.client_id @@ -818,10 +815,14 @@ def project() -> str: text=True, timeout=REQUEST_TIMEOUT, ).strip() - except subprocess.TimeoutExpired: - die("timed out resolving git remote for project", EXIT_FAILURE) - except (subprocess.CalledProcessError, FileNotFoundError): - die("GITLAB_PROJECT not set and no git remote found", EXIT_USAGE) + except subprocess.TimeoutExpired as exc: + raise GitLabError( + "timed out resolving git remote for project", EXIT_FAILURE + ) from exc + except (subprocess.CalledProcessError, FileNotFoundError) as exc: + raise GitLabError( + "GITLAB_PROJECT not set and no git remote found", EXIT_USAGE + ) from exc sanitized_remote_url = _sanitize_remote_url(remote_url) if remote_url.startswith("git@"): @@ -830,11 +831,13 @@ def project() -> str: parsed_remote = urllib.parse.urlsplit(remote_url) path = parsed_remote.path.lstrip("/") else: - die(f"cannot parse git remote URL: {sanitized_remote_url}", EXIT_USAGE) + raise GitLabError( + f"cannot parse git remote URL: {sanitized_remote_url}", EXIT_USAGE + ) path = strip_git_suffix(path) if not path: - die( + raise GitLabError( f"cannot extract project path from remote: {sanitized_remote_url}", EXIT_USAGE, ) @@ -845,10 +848,10 @@ def project() -> str: def validate_numeric_id(value: str) -> None: """Validate that a CLI argument is a numeric identifier.""" if not re.fullmatch(r"\d+", value): - die(f"expected numeric ID, got: {value}", EXIT_USAGE) + raise GitLabError(f"expected numeric ID, got: {value}", EXIT_USAGE) numeric_value = int(value) if numeric_value <= 0 or numeric_value > MAX_NUMERIC_ID: - die( + raise GitLabError( f"expected numeric ID between 1 and {MAX_NUMERIC_ID}, got: {value}", EXIT_USAGE, ) @@ -861,10 +864,12 @@ def validate_positive_int( ) -> None: """Validate that a CLI argument is a positive integer string.""" if not re.fullmatch(r"\d+", value): - die(f"{label} must be a positive integer, got: {value}", EXIT_USAGE) + raise GitLabError( + f"{label} must be a positive integer, got: {value}", EXIT_USAGE + ) numeric_value = int(value) if numeric_value <= 0 or numeric_value > upper_bound: - die( + raise GitLabError( f"{label} must be a positive integer between 1 and " f"{upper_bound}, got: {value}", EXIT_USAGE, @@ -874,13 +879,13 @@ def validate_positive_int( def validate_state(value: str) -> None: """Validate that a merge request state is allowed.""" if value not in VALID_MR_STATES: - die(f"invalid merge request state: {value}", EXIT_USAGE) + raise GitLabError(f"invalid merge request state: {value}", EXIT_USAGE) def validate_ref(value: str) -> None: """Validate that a pipeline ref matches the supported pattern.""" if not REF_PATTERN.fullmatch(value): - die(f"invalid ref: {value}", EXIT_USAGE) + raise GitLabError(f"invalid ref: {value}", EXIT_USAGE) def _read_capped(response: Any, limit: int, *, fail_on_limit: bool = True) -> bytes: @@ -889,7 +894,7 @@ def _read_capped(response: Any, limit: int, *, fail_on_limit: bool = True) -> by if chunk is None: return b"" if len(chunk) > limit and fail_on_limit: - die("response body exceeds size limit", EXIT_FAILURE) + raise GitLabError("response body exceeds size limit", EXIT_FAILURE) return chunk[:limit] @@ -921,16 +926,34 @@ def _request_bytes( content_type = "" if hasattr(response, "headers"): content_type = str(response.headers.get("Content-Type", "") or "") - result = _read_capped( - response, - MAX_BODY_BYTES, - fail_on_limit=require_json, - ) + try: + result = _read_capped( + response, + MAX_BODY_BYTES, + fail_on_limit=require_json, + ) + except GitLabError as error: + raise GitLabAPIError( + method=method, + resource=_scrub_url(url), + message=str(error), + request_id=_response_request_id(response), + ) from error if require_json and result.strip(): if not content_type: - die("unexpected Content-Type: ", EXIT_FAILURE) + raise GitLabAPIError( + method=method, + resource=_scrub_url(url), + message="unexpected Content-Type: ", + request_id=_response_request_id(response), + ) if not content_type.lower().startswith("application/json"): - die(f"unexpected Content-Type: {content_type}", EXIT_FAILURE) + raise GitLabAPIError( + method=method, + resource=_scrub_url(url), + message=f"unexpected Content-Type: {content_type}", + request_id=_response_request_id(response), + ) _audit_outcome(audit_actor, method, url, "success") return result except urllib.error.HTTPError as error: @@ -1035,7 +1058,9 @@ def parse_fields(arguments: list[str]) -> list[str]: current = arguments[index] if current == "--fields": if index + 1 >= len(arguments): - die("usage: --fields requires a comma-separated value list", EXIT_USAGE) + raise GitLabError( + "usage: --fields requires a comma-separated value list", EXIT_USAGE + ) selected_fields = arguments[index + 1].split(",") index += 2 continue @@ -1110,7 +1135,7 @@ def cmd_mr_list(args: list[str]) -> None: def cmd_mr_get(args: list[str]) -> None: """Get one merge request.""" if not args: - die("usage: gitlab mr-get ", EXIT_USAGE) + raise GitLabError("usage: gitlab mr-get ", EXIT_USAGE) merge_request_iid = args[0] validate_numeric_id(merge_request_iid) data = request( @@ -1126,11 +1151,11 @@ def cmd_mr_create(args: list[str]) -> None: """Create a merge request from JSON input.""" raw_payload = args[0] if args else sys.stdin.read(MAX_BODY_BYTES + 1) if not args and len(raw_payload) > MAX_BODY_BYTES: - die("request body exceeds size limit", EXIT_FAILURE) + raise GitLabError("request body exceeds size limit", EXIT_FAILURE) raw_payload = raw_payload.strip() usage = "usage: gitlab mr-create or pipe JSON to stdin" if not raw_payload: - die(usage, EXIT_USAGE) + raise GitLabError(usage, EXIT_USAGE) request( "POST", f"{api_url}/projects/{project()}/merge_requests", @@ -1141,16 +1166,16 @@ def cmd_mr_create(args: list[str]) -> None: def cmd_mr_update(args: list[str]) -> None: """Update a merge request from JSON input.""" if not args: - die("usage: gitlab mr-update ", EXIT_USAGE) + raise GitLabError("usage: gitlab mr-update ", EXIT_USAGE) merge_request_iid = args[0] validate_numeric_id(merge_request_iid) raw_payload = args[1] if len(args) > 1 else sys.stdin.read(MAX_BODY_BYTES + 1) if len(args) <= 1 and len(raw_payload) > MAX_BODY_BYTES: - die("request body exceeds size limit", EXIT_FAILURE) + raise GitLabError("request body exceeds size limit", EXIT_FAILURE) raw_payload = raw_payload.strip() usage = "usage: gitlab mr-update or pipe JSON to stdin" if not raw_payload: - die(usage, EXIT_USAGE) + raise GitLabError(usage, EXIT_USAGE) request( "PUT", f"{api_url}/projects/{project()}/merge_requests/{merge_request_iid}", @@ -1161,15 +1186,15 @@ def cmd_mr_update(args: list[str]) -> None: def cmd_mr_comment(args: list[str]) -> None: """Create a merge request note.""" if not args: - die("usage: gitlab mr-comment ", EXIT_USAGE) + raise GitLabError("usage: gitlab mr-comment ", EXIT_USAGE) merge_request_iid = args[0] validate_numeric_id(merge_request_iid) body = args[1] if len(args) > 1 else sys.stdin.read(MAX_BODY_BYTES + 1) if len(args) <= 1 and len(body) > MAX_BODY_BYTES: - die("request body exceeds size limit", EXIT_FAILURE) + raise GitLabError("request body exceeds size limit", EXIT_FAILURE) body = body.strip() if not body: - die( + raise GitLabError( "usage: gitlab mr-comment or pipe body to stdin", EXIT_USAGE, ) @@ -1183,7 +1208,7 @@ def cmd_mr_comment(args: list[str]) -> None: def cmd_mr_notes(args: list[str]) -> None: """List merge request notes.""" if not args: - die("usage: gitlab mr-notes [max]", EXIT_USAGE) + raise GitLabError("usage: gitlab mr-notes [max]", EXIT_USAGE) merge_request_iid = args[0] validate_numeric_id(merge_request_iid) max_results = args[1] if len(args) > 1 else "100" @@ -1206,7 +1231,7 @@ def cmd_mr_notes(args: list[str]) -> None: def cmd_pipeline_get(args: list[str]) -> None: """Get one pipeline.""" if not args: - die("usage: gitlab pipeline-get ", EXIT_USAGE) + raise GitLabError("usage: gitlab pipeline-get ", EXIT_USAGE) pipeline_id = args[0] validate_numeric_id(pipeline_id) data = request( @@ -1221,7 +1246,7 @@ def cmd_pipeline_get(args: list[str]) -> None: def cmd_pipeline_run(args: list[str]) -> None: """Trigger a pipeline for a branch or tag.""" if not args: - die("usage: gitlab pipeline-run ", EXIT_USAGE) + raise GitLabError("usage: gitlab pipeline-run ", EXIT_USAGE) validate_ref(args[0]) request("POST", f"{api_url}/projects/{project()}/pipelines", {"ref": args[0]}) @@ -1229,7 +1254,7 @@ def cmd_pipeline_run(args: list[str]) -> None: def cmd_pipeline_jobs(args: list[str]) -> None: """List pipeline jobs.""" if not args: - die("usage: gitlab pipeline-jobs ", EXIT_USAGE) + raise GitLabError("usage: gitlab pipeline-jobs ", EXIT_USAGE) pipeline_id = args[0] validate_numeric_id(pipeline_id) data = request( @@ -1244,7 +1269,7 @@ def cmd_pipeline_jobs(args: list[str]) -> None: def cmd_job_log(args: list[str]) -> None: """Print raw job trace output.""" if not args: - die("usage: gitlab job-log ", EXIT_USAGE) + raise GitLabError("usage: gitlab job-log ", EXIT_USAGE) job_id = args[0] validate_numeric_id(job_id) url = f"{api_url}/projects/{project()}/jobs/{job_id}/trace" @@ -1279,19 +1304,21 @@ def _auth_configuration() -> tuple[str, pathlib.Path, str]: """Resolve OAuth settings after rejecting mixed or legacy configuration.""" configured_mode = os.environ.get("GITLAB_AUTH_MODE", "oauth").strip() or "oauth" if configured_mode != "oauth": - die("auth commands require GITLAB_AUTH_MODE=oauth", EXIT_USAGE) + raise GitLabError("auth commands require GITLAB_AUTH_MODE=oauth", EXIT_USAGE) if os.environ.get("GITLAB_TOKEN", ""): - die("GITLAB_TOKEN must not be set for OAuth auth commands", EXIT_USAGE) + raise GitLabError( + "GITLAB_TOKEN must not be set for OAuth auth commands", EXIT_USAGE + ) global audit_actor require_base_environment() client_id = os.environ.get("GITLAB_OAUTH_CLIENT_ID", "").strip() if not client_id: - die("GITLAB_OAUTH_CLIENT_ID is not set", EXIT_USAGE) + raise GitLabError("GITLAB_OAUTH_CLIENT_ID is not set", EXIT_USAGE) try: profile_name = credentials.resolve_profile_name(None, os.environ) store_path = credentials.resolve_store_path(os.environ) except credentials.CredentialError as exc: - die(str(exc), EXIT_USAGE) + raise GitLabError(str(exc), EXIT_USAGE) from exc audit_actor = os.environ.get("GITLAB_AUDIT_ACTOR", "").strip() or "oauth" return profile_name, store_path, client_id @@ -1308,13 +1335,13 @@ def _save_auth_profile( credentials.set_profile(store, profile_name, profile) credentials.save_store(store_path, store) except credentials.CredentialError as exc: - die(str(exc), EXIT_FAILURE) + raise GitLabError(str(exc), EXIT_FAILURE) from exc def cmd_auth_login(args: list[str]) -> None: """Authenticate interactively through public-client PKCE.""" if args: - die("usage: gitlab auth login", EXIT_USAGE) + raise GitLabError("usage: gitlab auth login", EXIT_USAGE) profile_name, store_path, client_id = _auth_configuration() try: profile = oauth.authorization_code_login( @@ -1329,7 +1356,7 @@ def cmd_auth_login(args: list[str]) -> None: audit_outcome=_oauth_audit_outcome, ) except oauth.OAuthError as exc: - die(str(exc), EXIT_FAILURE) + raise GitLabError(_redact(str(exc)), EXIT_FAILURE) from None _save_auth_profile(profile_name, store_path, profile) _emit_stdout(f"authenticated GitLab OAuth profile {profile_name}") @@ -1337,7 +1364,7 @@ def cmd_auth_login(args: list[str]) -> None: def cmd_auth_device_login(args: list[str]) -> None: """Authenticate through human-assisted Device Authorization Grant.""" if args: - die("usage: gitlab auth device-login", EXIT_USAGE) + raise GitLabError("usage: gitlab auth device-login", EXIT_USAGE) profile_name, store_path, client_id = _auth_configuration() def emit(uri: str, code: str) -> None: @@ -1354,7 +1381,7 @@ def emit(uri: str, code: str) -> None: audit_outcome=_oauth_audit_outcome, ) except oauth.OAuthError as exc: - die(str(exc), EXIT_FAILURE) + raise GitLabError(_redact(str(exc)), EXIT_FAILURE) from None _save_auth_profile(profile_name, store_path, profile) _emit_stdout(f"authenticated GitLab OAuth profile {profile_name}") @@ -1362,13 +1389,13 @@ def emit(uri: str, code: str) -> None: def cmd_auth_status(args: list[str]) -> None: """Print secret-free OAuth profile status.""" if args: - die("usage: gitlab auth status", EXIT_USAGE) + raise GitLabError("usage: gitlab auth status", EXIT_USAGE) profile_name, store_path, client_id = _auth_configuration() try: store = credentials.load_store(store_path) profile = credentials.get_profile(store, profile_name) except credentials.CredentialError as exc: - die(str(exc), EXIT_FAILURE) + raise GitLabError(str(exc), EXIT_FAILURE) from exc _emit_stdout( json.dumps( { @@ -1387,7 +1414,7 @@ def cmd_auth_status(args: list[str]) -> None: def cmd_auth_logout(args: list[str]) -> None: """Delete one local OAuth profile without claiming server revocation.""" if args: - die("usage: gitlab auth logout", EXIT_USAGE) + raise GitLabError("usage: gitlab auth logout", EXIT_USAGE) profile_name, store_path, _client_id = _auth_configuration() try: with credentials.store_lock(store_path): @@ -1395,7 +1422,7 @@ def cmd_auth_logout(args: list[str]) -> None: removed = credentials.delete_profile(store, profile_name) credentials.save_store(store_path, store) except credentials.CredentialError as exc: - die(str(exc), EXIT_FAILURE) + raise GitLabError(str(exc), EXIT_FAILURE) from exc _emit_stdout( f"local profile {profile_name} {'removed' if removed else 'was absent'}; " "server authorization was not revoked" @@ -1417,10 +1444,14 @@ def main() -> int: if arguments and arguments[0] == "auth": if selected_fields: - die("--fields is not valid with auth commands", EXIT_USAGE) + raise GitLabError( + "--fields is not valid with auth commands", EXIT_USAGE + ) handler = AUTH_COMMANDS.get(arguments[1]) if len(arguments) >= 2 else None if handler is None: - die("usage: gitlab auth {login|device-login|status|logout}", EXIT_USAGE) + raise GitLabError( + "usage: gitlab auth {login|device-login|status|logout}", EXIT_USAGE + ) global _AUDIT_OP _AUDIT_OP = f"auth-{arguments[1]}" handler(arguments[2:]) @@ -1429,7 +1460,7 @@ def main() -> int: require_environment() if not arguments or arguments[0] not in COMMANDS: - die( + raise GitLabError( "usage: gitlab {mr-list|mr-get|mr-create|mr-update|mr-comment|" "auth|mr-notes|pipeline-get|pipeline-run|pipeline-jobs|job-log} " "[args...]", diff --git a/.github/skills/project-planning/gitlab/tests/fuzz_harness.py b/.github/skills/project-planning/gitlab/tests/fuzz_harness.py index 9a8b98a97b..602391df33 100644 --- a/.github/skills/project-planning/gitlab/tests/fuzz_harness.py +++ b/.github/skills/project-planning/gitlab/tests/fuzz_harness.py @@ -26,10 +26,10 @@ FUZZING = True -# A CLI failure surfaces as SystemExit from the dispatch-layer die() helper -# or as GitLabError from a library-level helper that promises a return -# value. Both are expected refusals, not fuzz findings. -_EXPECTED_CLI_ERRORS = (SystemExit, gitlab.GitLabError) +# A CLI failure surfaces as GitLabError, the module's single failure +# mechanism. Nothing raises SystemExit outside the __main__ guard, so an +# expected refusal is always this one type, not a fuzz finding. +_EXPECTED_CLI_ERRORS = (gitlab.GitLabError,) def fuzz_strip_git_suffix(data: bytes) -> None: @@ -200,7 +200,7 @@ def test_validate_numeric_id_accepts_digits(self, value: str) -> None: @pytest.mark.parametrize("value", ["", "abc", "12a", "-1"]) def test_validate_numeric_id_rejects_invalid_values(self, value: str) -> None: - with pytest.raises(SystemExit): + with pytest.raises(gitlab.GitLabError): gitlab.validate_numeric_id(value) def test_extract_field_handles_nested_values(self) -> None: diff --git a/.github/skills/project-planning/gitlab/tests/test_gitlab_audit.py b/.github/skills/project-planning/gitlab/tests/test_gitlab_audit.py index d3b6b99ede..7d8be1461e 100644 --- a/.github/skills/project-planning/gitlab/tests/test_gitlab_audit.py +++ b/.github/skills/project-planning/gitlab/tests/test_gitlab_audit.py @@ -123,7 +123,7 @@ def test_audit_fail_closed_blocks_request( _enable_audit(monkeypatch, log) opener = mocker.patch("gitlab._OPENER.open") - with pytest.raises(SystemExit): + with pytest.raises(gitlab.GitLabError): gitlab.request("GET", f"{TEST_API_URL}/projects/1/merge_requests/2") opener.assert_not_called() @@ -214,7 +214,7 @@ def test_oauth_attempt_failure_blocks_egress( monkeypatch.setenv("GITLAB_AUDIT_LOG", str(tmp_path / "missing" / "audit.jsonl")) opener = mocker.MagicMock() - with pytest.raises(SystemExit): + with pytest.raises(gitlab.GitLabError): gitlab.oauth.post_form( "https://gitlab.example.com", "/oauth/token", diff --git a/.github/skills/project-planning/gitlab/tests/test_gitlab_commands.py b/.github/skills/project-planning/gitlab/tests/test_gitlab_commands.py index 91f9bbd628..efadf7274e 100644 --- a/.github/skills/project-planning/gitlab/tests/test_gitlab_commands.py +++ b/.github/skills/project-planning/gitlab/tests/test_gitlab_commands.py @@ -78,13 +78,12 @@ def _assert_usage_error( command: CommandFn, args: list[str], expected_message: str, - capsys: pytest.CaptureFixture[str], ) -> None: - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: command(args) - assert exc_info.value.code == gitlab.EXIT_USAGE - assert expected_message in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_USAGE + assert expected_message in str(exc_info.value) @pytest.mark.parametrize( @@ -103,9 +102,8 @@ def test_commands_require_minimum_arguments( command: CommandFn, args: list[str], expected_message: str, - capsys: pytest.CaptureFixture[str], ) -> None: - _assert_usage_error(command, args, expected_message, capsys) + _assert_usage_error(command, args, expected_message) @pytest.mark.parametrize( @@ -289,11 +287,10 @@ def test_write_commands_require_stdin_or_inline_content( command: CommandFn, args: list[str], usage_message: str, - capsys: pytest.CaptureFixture[str], ) -> None: stdin_factory("") - _assert_usage_error(command, args, usage_message, capsys) + _assert_usage_error(command, args, usage_message) def test_mr_notes_uses_default_max_results( @@ -349,9 +346,9 @@ def test_redacts_and_truncates_job_log_output( assert "abc123" not in output assert "... [truncated]" in output - def test_requires_job_id(self, capsys: pytest.CaptureFixture[str]) -> None: - with pytest.raises(SystemExit) as exc_info: + def test_requires_job_id(self) -> None: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.cmd_job_log([]) - assert exc_info.value.code == gitlab.EXIT_USAGE - assert USAGE_JOB_LOG in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_USAGE + assert USAGE_JOB_LOG in str(exc_info.value) diff --git a/.github/skills/project-planning/gitlab/tests/test_gitlab_coverage.py b/.github/skills/project-planning/gitlab/tests/test_gitlab_coverage.py index 04907dc63e..3aaf19212b 100644 --- a/.github/skills/project-planning/gitlab/tests/test_gitlab_coverage.py +++ b/.github/skills/project-planning/gitlab/tests/test_gitlab_coverage.py @@ -30,24 +30,24 @@ def test_is_loopback_rejects_empty_host() -> None: def test_validate_project_path_rejects_traversal() -> None: - with pytest.raises(SystemExit): + with pytest.raises(gitlab.GitLabError): gitlab._validate_project_path("../escape") def test_validate_project_path_rejects_empty() -> None: - with pytest.raises(SystemExit): + with pytest.raises(gitlab.GitLabError): gitlab._validate_project_path("") def test_validate_numeric_id_rejects_non_numeric() -> None: - with pytest.raises(SystemExit): + with pytest.raises(gitlab.GitLabError): gitlab.validate_numeric_id("abc") def test_validate_numeric_id_rejects_out_of_range() -> None: - with pytest.raises(SystemExit): + with pytest.raises(gitlab.GitLabError): gitlab.validate_numeric_id("0") - with pytest.raises(SystemExit): + with pytest.raises(gitlab.GitLabError): gitlab.validate_numeric_id(str(gitlab.MAX_NUMERIC_ID + 1)) @@ -70,7 +70,7 @@ class _Response: def read(self, _amount: int) -> bytes: return b"x" * 32 - with pytest.raises(SystemExit): + with pytest.raises(gitlab.GitLabError): gitlab._read_capped(_Response(), 16, fail_on_limit=True) @@ -96,7 +96,7 @@ def read(self, _amount: int | None = None) -> bytes: mocker.patch("gitlab._OPENER.open", return_value=_Response()) - with pytest.raises(SystemExit): + with pytest.raises(gitlab.GitLabError): gitlab._request_bytes("GET", f"{TEST_API_URL}/projects/1", require_json=True) @@ -131,7 +131,7 @@ def test_mr_update_rejects_oversized_stdin( monkeypatch.setenv("GITLAB_PROJECT", "group/project") stdin_factory("x" * (gitlab.MAX_BODY_BYTES + 1)) # type: ignore[operator] - with pytest.raises(SystemExit): + with pytest.raises(gitlab.GitLabError): gitlab.cmd_mr_update(["9"]) @@ -143,5 +143,5 @@ def test_mr_comment_rejects_oversized_stdin( monkeypatch.setenv("GITLAB_PROJECT", "group/project") stdin_factory("x" * (gitlab.MAX_BODY_BYTES + 1)) # type: ignore[operator] - with pytest.raises(SystemExit): + with pytest.raises(gitlab.GitLabError): gitlab.cmd_mr_comment(["9"]) diff --git a/.github/skills/project-planning/gitlab/tests/test_gitlab_helpers.py b/.github/skills/project-planning/gitlab/tests/test_gitlab_helpers.py index f09d7de7aa..ddeb7f309f 100644 --- a/.github/skills/project-planning/gitlab/tests/test_gitlab_helpers.py +++ b/.github/skills/project-planning/gitlab/tests/test_gitlab_helpers.py @@ -83,15 +83,150 @@ def _network_invocations(source: str, module: str) -> list[tuple[str, str, str]] return visitor.invocations -class TestDie: - """Tests for die.""" +def _is_main_guard(node: ast.AST) -> bool: + """Report whether a node is the ``if __name__ == "__main__":`` guard.""" + if not isinstance(node, ast.If) or not isinstance(node.test, ast.Compare): + return False + left = node.test.left + operators = node.test.ops + comparators = node.test.comparators + return ( + isinstance(left, ast.Name) + and left.id == "__name__" + and len(operators) == 1 + and isinstance(operators[0], ast.Eq) + and len(comparators) == 1 + and isinstance(comparators[0], ast.Constant) + and comparators[0].value == "__main__" + ) + + +class _ExitMechanismVisitor(ast.NodeVisitor): + """Collect process-exit violations using lexical import bindings.""" + + _IMPLICIT_BINDINGS = { + "SystemExit": "builtins.SystemExit", + "sys": "sys", + } + + def __init__(self, guarded: set[int]) -> None: + self.guarded = guarded + self.scope_stack: list[tuple[dict[str, str], dict[str, str]]] = [] + self.violations: list[str] = [] + + def visit(self, node: ast.AST) -> object: + if id(node) in self.guarded: + return None + return super().visit(node) + + def visit_Module(self, node: ast.Module) -> None: + self._visit_scope(node.body, self._IMPLICIT_BINDINGS, is_class=False) + + def visit_FunctionDef(self, node: ast.FunctionDef) -> None: + if node.name == "die": + self.violations.append("def die") + self._visit_function(node) + + def visit_AsyncFunctionDef(self, node: ast.AsyncFunctionDef) -> None: + if node.name == "die": + self.violations.append("def die") + self._visit_function(node) + + def visit_ClassDef(self, node: ast.ClassDef) -> None: + for decorator in node.decorator_list: + self.visit(decorator) + for base in node.bases: + self.visit(base) + for keyword in node.keywords: + self.visit(keyword) + self._visit_scope(node.body, self._child_bindings(), is_class=True) + + def visit_Raise(self, node: ast.Raise) -> None: + raised = node.exc.func if isinstance(node.exc, ast.Call) else node.exc + if self._resolve_target(raised) == "builtins.SystemExit": + self.violations.append("raise SystemExit") + self.generic_visit(node) + + def visit_Call(self, node: ast.Call) -> None: + if self._resolve_target(node.func) == "sys.exit": + self.violations.append("sys.exit") + self.generic_visit(node) - def test_prints_error_and_exits(self, capsys: pytest.CaptureFixture[str]) -> None: - with pytest.raises(SystemExit) as exc_info: - gitlab.die("boom", gitlab.EXIT_USAGE) + def _visit_function(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> None: + for decorator in node.decorator_list: + self.visit(decorator) + self.visit(node.args) + if node.returns is not None: + self.visit(node.returns) + self._visit_scope(node.body, self._child_bindings(), is_class=False) + + def _visit_scope( + self, + body: list[ast.stmt], + inherited: dict[str, str], + *, + is_class: bool, + ) -> None: + bindings = inherited.copy() + for statement in body: + self._collect_import_bindings(statement, bindings) + child_bindings = inherited if is_class else bindings + self.scope_stack.append((bindings, child_bindings)) + for statement in body: + self.visit(statement) + self.scope_stack.pop() + + def _collect_import_bindings(self, node: ast.AST, bindings: dict[str, str]) -> None: + if id(node) in self.guarded or isinstance( + node, + (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef, ast.Lambda), + ): + return + if isinstance(node, ast.Import): + for alias in node.names: + bound_name = alias.asname or alias.name.partition(".")[0] + bindings[bound_name] = alias.name if alias.asname else bound_name + return + if isinstance(node, ast.ImportFrom) and node.module is not None: + for alias in node.names: + if alias.name != "*": + bindings[alias.asname or alias.name] = f"{node.module}.{alias.name}" + return + for child in ast.iter_child_nodes(node): + self._collect_import_bindings(child, bindings) + + def _child_bindings(self) -> dict[str, str]: + return self.scope_stack[-1][1] + + def _resolve_target(self, node: ast.AST | None) -> str | None: + if isinstance(node, ast.Name): + return self.scope_stack[-1][0].get(node.id) + if isinstance(node, ast.Attribute): + parent = self._resolve_target(node.value) + if parent is not None: + return f"{parent}.{node.attr}" + return None + + +def _exit_mechanism_violations(source: str) -> list[str]: + """Return violations of the single-failure-mechanism contract. + + ``GitLabError`` is the module's only failure mechanism. Process exit belongs + to the ``__main__`` guard alone, so a ``raise SystemExit`` or ``sys.exit`` + anywhere else, or any ``die`` definition, reintroduces the second mechanism + this contract exists to prevent. + """ + tree = ast.parse(source) + guarded: set[int] = set() + for node in ast.walk(tree): + if _is_main_guard(node): + for statement in node.body: + for child in ast.walk(statement): + guarded.add(id(child)) - assert exc_info.value.code == gitlab.EXIT_USAGE - assert capsys.readouterr().err.strip() == "error: boom" + visitor = _ExitMechanismVisitor(guarded) + visitor.visit(tree) + return visitor.violations class TestRedact: @@ -288,10 +423,91 @@ def test_source_contract_detects_non_owner_invocations( self, source: str, expected: str ) -> None: invocations = _network_invocations(source, "unexpected.py") - assert invocations assert invocations[0][1] == expected + def test_gitlab_error_is_the_only_failure_mechanism(self) -> None: + assert _exit_mechanism_violations(SOURCE) == [] + assert not hasattr(gitlab, "die") + + @pytest.mark.parametrize( + ("source", "expected"), + [ + ("def bypass():\n raise SystemExit(2)\n", "raise SystemExit"), + ("def bypass():\n sys.exit(2)\n", "sys.exit"), + ("def die(message):\n return message\n", "def die"), + ( + "import builtins\n\ndef bypass():\n raise builtins.SystemExit(2)\n", + "raise SystemExit", + ), + ( + "def owner():\n import builtins as bi\n" + " raise bi.SystemExit(2)\n\n" + "def sibling():\n raise bi.SystemExit(2)\n", + "raise SystemExit", + ), + ( + "class Owner:\n" + " from builtins import SystemExit as Stop\n" + " raise Stop(2)\n\n" + " def method(self):\n raise Stop(2)\n", + "raise SystemExit", + ), + ( + "def outer():\n import sys as system\n\n" + " def inner():\n system.exit(2)\n", + "sys.exit", + ), + ( + "from sys import exit\n\ndef bypass():\n exit(2)\n", + "sys.exit", + ), + ( + "def bypass():\n from sys import exit as stop\n stop(2)\n", + "sys.exit", + ), + ], + ) + def test_source_contract_detects_second_failure_mechanism( + self, source: str, expected: str + ) -> None: + assert _exit_mechanism_violations(source) == [expected] + + def test_source_contract_allows_guarded_process_exit(self) -> None: + guarded = 'if __name__ == "__main__":\n sys.exit(main())\n' + + assert _exit_mechanism_violations(guarded) == [] + + @pytest.mark.parametrize( + "source", + [ + ( + "class Owner:\n from sys import exit\n\n" + " def method(self):\n" + " def exit(code):\n return code\n\n" + " exit(2)\n" + ), + ( + "def owner():\n import sys as target\n\n" + "def sibling(target):\n target.exit(2)\n" + ), + ], + ) + def test_source_contract_allows_unrelated_exit_forms(self, source: str) -> None: + assert _exit_mechanism_violations(source) == [] + + @pytest.mark.parametrize( + "source", + [ + 'if __name__ != "__main__":\n sys.exit(2)\n', + ('if __name__ == "__main__":\n main()\nelse:\n sys.exit(2)\n'), + ], + ) + def test_source_contract_rejects_exit_outside_canonical_guard_body( + self, source: str + ) -> None: + assert _exit_mechanism_violations(source) == ["sys.exit"] + def test_job_log_output_is_redacted( self, configured_gitlab: object, @@ -334,16 +550,12 @@ def test_accepts_numeric_strings(self, value: str) -> None: gitlab.validate_numeric_id(value) @pytest.mark.parametrize("value", ["", "abc", "12a", "-1", "1.2", " 5 "]) - def test_rejects_non_numeric_values( - self, - value: str, - capsys: pytest.CaptureFixture[str], - ) -> None: - with pytest.raises(SystemExit) as exc_info: + def test_rejects_non_numeric_values(self, value: str) -> None: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.validate_numeric_id(value) - assert exc_info.value.code == gitlab.EXIT_USAGE - assert f"expected numeric ID, got: {value}" in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_USAGE + assert f"expected numeric ID, got: {value}" in str(exc_info.value) class TestValidatePositiveInt: @@ -354,18 +566,13 @@ def test_accepts_digit_strings(self, value: str) -> None: gitlab.validate_positive_int(value, "max_results") @pytest.mark.parametrize("value", ["", "ten", "5x", "-2", "3.14"]) - def test_rejects_invalid_values( - self, - value: str, - capsys: pytest.CaptureFixture[str], - ) -> None: - with pytest.raises(SystemExit) as exc_info: + def test_rejects_invalid_values(self, value: str) -> None: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.validate_positive_int(value, "max_results") - assert exc_info.value.code == gitlab.EXIT_USAGE - assert ( - f"max_results must be a positive integer, got: {value}" - in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_USAGE + assert f"max_results must be a positive integer, got: {value}" in str( + exc_info.value ) @@ -394,16 +601,13 @@ def test_fields_can_appear_before_command_arguments(self) -> None: assert cleaned == ["mr-get", "7"] assert gitlab.selected_fields == ["iid", "title"] - def test_requires_value_after_fields( - self, capsys: pytest.CaptureFixture[str] - ) -> None: - with pytest.raises(SystemExit) as exc_info: + def test_requires_value_after_fields(self) -> None: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.parse_fields(["mr-list", "--fields"]) - assert exc_info.value.code == gitlab.EXIT_USAGE - assert ( - "usage: --fields requires a comma-separated value list" - in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_USAGE + assert "usage: --fields requires a comma-separated value list" in str( + exc_info.value ) diff --git a/.github/skills/project-planning/gitlab/tests/test_gitlab_main.py b/.github/skills/project-planning/gitlab/tests/test_gitlab_main.py index 72f14168e9..e0b8293865 100644 --- a/.github/skills/project-planning/gitlab/tests/test_gitlab_main.py +++ b/.github/skills/project-planning/gitlab/tests/test_gitlab_main.py @@ -6,6 +6,8 @@ import json import pathlib +import traceback +from collections.abc import Callable import gitlab import pytest @@ -95,11 +97,14 @@ def test_rejects_fields_for_auth_commands( "sys.argv", ["gitlab", "--fields", "profile", "auth", "status"] ) - with pytest.raises(SystemExit) as exc_info: - gitlab.main() + assert gitlab.main() == gitlab.EXIT_USAGE - assert exc_info.value.code == gitlab.EXIT_USAGE - assert "--fields is not valid with auth commands" in capsys.readouterr().err + # main is the sole emission boundary: it must produce exactly one + # redacted "error: ..." line when GITLAB_DEBUG is unset. + assert ( + capsys.readouterr().err + == "error: --fields is not valid with auth commands\n" + ) @pytest.mark.parametrize( "argv", @@ -118,10 +123,7 @@ def test_rejects_invalid_auth_command_before_api_environment( ) monkeypatch.setattr("sys.argv", argv) - with pytest.raises(SystemExit) as exc_info: - gitlab.main() - - assert exc_info.value.code == gitlab.EXIT_USAGE + assert gitlab.main() == gitlab.EXIT_USAGE assert "gitlab auth {login|device-login|status|logout}" in ( capsys.readouterr().err ) @@ -156,10 +158,7 @@ def test_main_rejects_missing_or_unknown_command( monkeypatch.setattr(gitlab, "require_environment", lambda: None) monkeypatch.setattr("sys.argv", argv) - with pytest.raises(SystemExit) as exc_info: - gitlab.main() - - assert exc_info.value.code == gitlab.EXIT_USAGE + assert gitlab.main() == gitlab.EXIT_USAGE assert USAGE_MAIN in capsys.readouterr().err def test_main_passes_empty_arguments_when_only_fields_are_present( @@ -170,10 +169,7 @@ def test_main_passes_empty_arguments_when_only_fields_are_present( monkeypatch.setattr(gitlab, "require_environment", lambda: None) monkeypatch.setattr("sys.argv", ARGV_FIELDS_ONLY) - with pytest.raises(SystemExit) as exc_info: - gitlab.main() - - assert exc_info.value.code == gitlab.EXIT_USAGE + assert gitlab.main() == gitlab.EXIT_USAGE assert gitlab.selected_fields == FIELDS_MR assert USAGE_MAIN in capsys.readouterr().err @@ -220,10 +216,62 @@ def test_main_redacts_unexpected_exception( assert captured.err.strip() == "error: unexpected GitLab CLI failure" assert "hidden" not in captured.err + def test_main_handles_typed_error_at_single_redacted_boundary( + self, + monkeypatch: pytest.MonkeyPatch, + mocker: MockerFixture, + capsys: pytest.CaptureFixture[str], + ) -> None: + error = gitlab.GitLabError("private_token=hidden", gitlab.EXIT_USAGE) + debug_traceback = mocker.patch.object(gitlab, "_emit_debug_traceback") + monkeypatch.setattr(gitlab, "require_environment", lambda: None) + monkeypatch.setitem( + gitlab.COMMANDS, + "mr-list", + lambda _args: (_ for _ in ()).throw(error), + ) + monkeypatch.setattr("sys.argv", ARGV_MAIN_LIST) + + result = gitlab.main() + + assert result == gitlab.EXIT_USAGE + assert capsys.readouterr().err == "error: private_token=[REDACTED]\n" + debug_traceback.assert_called_once_with(error) + class TestAuthCommands: """Tests for stateful OAuth command behavior.""" + @pytest.mark.parametrize( + ("provider", "command"), + [ + ("gitlab.oauth.authorization_code_login", gitlab.cmd_auth_login), + ("gitlab.oauth.device_login", gitlab.cmd_auth_device_login), + ], + ) + def test_login_wrapper_traceback_excludes_provider_secret( + self, + monkeypatch: pytest.MonkeyPatch, + mocker: MockerFixture, + tmp_path: pathlib.Path, + provider: str, + command: Callable[[list[str]], None], + ) -> None: + store_path = tmp_path / "gitlab" / "gitlab-token.json" + _configure_oauth(monkeypatch, store_path) + mocker.patch( + provider, + side_effect=gitlab.oauth.OAuthError("access_token=provider-secret"), + ) + + with pytest.raises(gitlab.GitLabError) as exc_info: + command([]) + + formatted = "".join(traceback.format_exception(exc_info.value)) + assert "provider-secret" not in formatted + assert "access_token=[REDACTED]" in formatted + assert exc_info.value.exit_code == gitlab.EXIT_FAILURE + def test_login_surfaces_url_and_persists_profile( self, monkeypatch: pytest.MonkeyPatch, @@ -308,7 +356,6 @@ def test_rejects_mixed_legacy_credentials( self, monkeypatch: pytest.MonkeyPatch, tmp_path: pathlib.Path, - capsys: pytest.CaptureFixture[str], ) -> None: store_path = tmp_path / "gitlab" / "gitlab-token.json" monkeypatch.setenv("GITLAB_AUTH_MODE", "oauth") @@ -316,8 +363,8 @@ def test_rejects_mixed_legacy_credentials( monkeypatch.setenv("GITLAB_OAUTH_CLIENT_ID", "client") monkeypatch.setenv("GITLAB_TOKEN_STORE", str(store_path)) - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.cmd_auth_status([]) - assert exc_info.value.code == gitlab.EXIT_USAGE - assert "must not be set" in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_USAGE + assert "must not be set" in str(exc_info.value) diff --git a/.github/skills/project-planning/gitlab/tests/test_gitlab_transport.py b/.github/skills/project-planning/gitlab/tests/test_gitlab_transport.py index a578d4e638..8386f6d024 100644 --- a/.github/skills/project-planning/gitlab/tests/test_gitlab_transport.py +++ b/.github/skills/project-planning/gitlab/tests/test_gitlab_transport.py @@ -22,6 +22,7 @@ ) REQUEST_ENDPOINT = f"{TEST_API_URL}/test" +REQUEST_ENDPOINT_WITH_CONTEXT = f"{REQUEST_ENDPOINT}?private_token=hidden#fragment" REQUEST_JSON = {"iid": 7, "title": "MR"} REQUEST_BODY = '{"iid": 7, "title": "MR"}' NON_JSON_BODY = "plain text output" @@ -91,16 +92,15 @@ def test_rejects_non_origin_base_urls( self, monkeypatch: pytest.MonkeyPatch, base_url: str, - capsys: pytest.CaptureFixture[str], ) -> None: monkeypatch.setenv("GITLAB_URL", base_url) monkeypatch.setenv("GITLAB_TOKEN", TEST_GITLAB_TOKEN) - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.require_environment() - assert exc_info.value.code == gitlab.EXIT_USAGE - assert "origin-only" in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_USAGE + assert "origin-only" in str(exc_info.value) def test_accepts_clean_origin_base_url( self, monkeypatch: pytest.MonkeyPatch @@ -118,10 +118,10 @@ def test_oauth_mode_rejects_legacy_token( monkeypatch.setenv("GITLAB_AUTH_MODE", "oauth") monkeypatch.setenv("GITLAB_OAUTH_CLIENT_ID", "client") - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.require_environment() - assert exc_info.value.code == gitlab.EXIT_USAGE + assert exc_info.value.exit_code == gitlab.EXIT_USAGE def test_legacy_mode_is_explicit(self) -> None: gitlab.require_environment() @@ -132,15 +132,14 @@ def test_legacy_mode_is_explicit(self) -> None: def test_unset_mode_does_not_infer_legacy_from_token( self, monkeypatch: pytest.MonkeyPatch, - capsys: pytest.CaptureFixture[str], ) -> None: monkeypatch.delenv("GITLAB_AUTH_MODE") - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.require_environment() - assert exc_info.value.code == gitlab.EXIT_USAGE - assert "must not be set in oauth mode" in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_USAGE + assert "must not be set in oauth mode" in str(exc_info.value) def test_oauth_mode_loads_bound_profile( self, @@ -190,15 +189,14 @@ def test_rejects_invalid_environment( env_name: str, env_value: str, expected_message: str, - capsys: pytest.CaptureFixture[str], ) -> None: monkeypatch.setenv(env_name, env_value) - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.require_environment() - assert exc_info.value.code == gitlab.EXIT_USAGE - assert expected_message in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_USAGE + assert expected_message in str(exc_info.value) class TestProject: @@ -229,39 +227,35 @@ def test_parses_supported_remote_urls( assert gitlab.project() == expected def test_requires_remote_when_project_not_configured( - self, mocker: MockerFixture, capsys: pytest.CaptureFixture[str] + self, mocker: MockerFixture ) -> None: mocker.patch("subprocess.check_output", side_effect=FileNotFoundError) - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.project() - assert exc_info.value.code == gitlab.EXIT_USAGE - assert PROJECT_NOT_FOUND in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_USAGE + assert PROJECT_NOT_FOUND in str(exc_info.value) - def test_rejects_unparseable_remote( - self, mocker: MockerFixture, capsys: pytest.CaptureFixture[str] - ) -> None: + def test_rejects_unparseable_remote(self, mocker: MockerFixture) -> None: mocker.patch( "subprocess.check_output", return_value="ssh://gitlab.example.com/group/project.git\n", ) - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.project() - assert exc_info.value.code == gitlab.EXIT_USAGE - assert PARSE_REMOTE_ERROR in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_USAGE + assert PARSE_REMOTE_ERROR in str(exc_info.value) - def test_rejects_empty_path_after_host( - self, mocker: MockerFixture, capsys: pytest.CaptureFixture[str] - ) -> None: + def test_rejects_empty_path_after_host(self, mocker: MockerFixture) -> None: mocker.patch( "subprocess.check_output", return_value="https://gitlab.example.com/.git\n" ) - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.project() - assert exc_info.value.code == gitlab.EXIT_USAGE - assert EMPTY_REMOTE_PATH_ERROR in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_USAGE + assert EMPTY_REMOTE_PATH_ERROR in str(exc_info.value) @pytest.mark.parametrize( "remote_url", @@ -275,15 +269,14 @@ def test_rejects_invalid_project_paths( self, mocker: MockerFixture, remote_url: str, - capsys: pytest.CaptureFixture[str], ) -> None: mocker.patch("subprocess.check_output", return_value=remote_url) - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.project() - assert exc_info.value.code == gitlab.EXIT_USAGE - assert "invalid project path" in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_USAGE + assert "invalid project path" in str(exc_info.value) def test_accepts_owner_project_path_from_remote( self, mocker: MockerFixture @@ -673,9 +666,7 @@ def test_falls_back_to_redacted_raw_error_body( class TestGitLabTransportHardening: """Regression tests for hardened transport behavior.""" - def test_uses_timeout_for_git_remote_lookup( - self, mocker: MockerFixture, capsys: pytest.CaptureFixture[str] - ) -> None: + def test_uses_timeout_for_git_remote_lookup(self, mocker: MockerFixture) -> None: captured: dict[str, object] = {} def fake_check_output(*args: object, **kwargs: object) -> str: @@ -687,12 +678,12 @@ def fake_check_output(*args: object, **kwargs: object) -> str: mocker.patch("subprocess.check_output", side_effect=fake_check_output) - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.project() - assert exc_info.value.code == gitlab.EXIT_FAILURE + assert exc_info.value.exit_code == gitlab.EXIT_FAILURE assert captured["kwargs"]["timeout"] == gitlab.REQUEST_TIMEOUT - assert "timed out resolving git remote for project" in capsys.readouterr().err + assert "timed out resolving git remote for project" in str(exc_info.value) def test_prints_redacted_and_capped_non_json_output( self, @@ -725,23 +716,54 @@ def test_rejects_missing_or_non_json_content_types( expected_fragment: str, configured_gitlab: ConfiguredGitLab, response_factory: ResponseFactory, - capsys: pytest.CaptureFixture[str], mocker: MockerFixture, ) -> None: response = response_factory(REQUEST_BODY) response.headers = {"Content-Type": content_type} if content_type else {} mocker.patch("gitlab._OPENER.open", return_value=response) - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabAPIError) as exc_info: gitlab._request_bytes( "GET", - REQUEST_ENDPOINT, + REQUEST_ENDPOINT_WITH_CONTEXT, headers={"PRIVATE-TOKEN": TEST_GITLAB_TOKEN}, require_json=True, ) - assert exc_info.value.code == gitlab.EXIT_FAILURE - assert expected_fragment in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_FAILURE + assert exc_info.value.method == "GET" + assert exc_info.value.resource == REQUEST_ENDPOINT + rendered = str(exc_info.value) + assert expected_fragment in rendered + assert "hidden" not in rendered + assert "fragment" not in rendered + + def test_rejects_oversized_json_response_as_api_error( + self, + configured_gitlab: ConfiguredGitLab, + response_factory: ResponseFactory, + mocker: MockerFixture, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.setattr(gitlab, "MAX_BODY_BYTES", 16) + response = _json_response(response_factory, "x" * 17) + mocker.patch("gitlab._OPENER.open", return_value=response) + + with pytest.raises(gitlab.GitLabAPIError) as exc_info: + gitlab._request_bytes( + "POST", + REQUEST_ENDPOINT_WITH_CONTEXT, + headers={"PRIVATE-TOKEN": TEST_GITLAB_TOKEN}, + require_json=True, + ) + + assert exc_info.value.exit_code == gitlab.EXIT_FAILURE + assert exc_info.value.method == "POST" + assert exc_info.value.resource == REQUEST_ENDPOINT + rendered = str(exc_info.value) + assert "response body exceeds size limit" in rendered + assert "hidden" not in rendered + assert "fragment" not in rendered def test_allows_application_json_content_type( self, @@ -804,10 +826,10 @@ def test_requires_https_for_non_localhost( ) -> None: monkeypatch.setenv("GITLAB_URL", "http://example.com") - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.require_environment() - assert exc_info.value.code == gitlab.EXIT_USAGE + assert exc_info.value.exit_code == gitlab.EXIT_USAGE def test_rejects_non_localhost_http_even_when_allow_env_set( self, monkeypatch: pytest.MonkeyPatch @@ -816,10 +838,10 @@ def test_rejects_non_localhost_http_even_when_allow_env_set( monkeypatch.setenv("GITLAB_TOKEN", TEST_GITLAB_TOKEN) monkeypatch.setenv("GITLAB_ALLOW_INSECURE", "1") - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.require_environment() - assert exc_info.value.code == gitlab.EXIT_USAGE + assert exc_info.value.exit_code == gitlab.EXIT_USAGE def test_rejects_loopback_http_without_allow_env( self, monkeypatch: pytest.MonkeyPatch @@ -828,10 +850,10 @@ def test_rejects_loopback_http_without_allow_env( monkeypatch.setenv("GITLAB_TOKEN", TEST_GITLAB_TOKEN) monkeypatch.delenv("GITLAB_ALLOW_INSECURE", raising=False) - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.require_environment() - assert exc_info.value.code == gitlab.EXIT_USAGE + assert exc_info.value.exit_code == gitlab.EXIT_USAGE def test_accepts_loopback_http_with_allow_env( self, monkeypatch: pytest.MonkeyPatch @@ -846,27 +868,26 @@ def test_accepts_loopback_http_with_allow_env( assert gitlab.api_url == "http://127.0.0.1:8080/api/v4" def test_rejects_invalid_mr_state(self) -> None: - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.cmd_mr_list(["invalid-state"]) - assert exc_info.value.code == gitlab.EXIT_USAGE + assert exc_info.value.exit_code == gitlab.EXIT_USAGE def test_rejects_invalid_ref(self) -> None: - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.cmd_pipeline_run(["invalid ref"]) - assert exc_info.value.code == gitlab.EXIT_USAGE + assert exc_info.value.exit_code == gitlab.EXIT_USAGE def test_rejects_zero_for_positive_integer(self) -> None: - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.validate_positive_int("0", "max_results") - assert exc_info.value.code == gitlab.EXIT_USAGE + assert exc_info.value.exit_code == gitlab.EXIT_USAGE def test_rejects_oversized_stdin_payload_before_parsing( self, monkeypatch: pytest.MonkeyPatch, - capsys: pytest.CaptureFixture[str], mocker: MockerFixture, ) -> None: monkeypatch.setattr( @@ -876,11 +897,11 @@ def test_rejects_oversized_stdin_payload_before_parsing( ) mocker.patch("gitlab.load_json_payload", side_effect=AssertionError) - with pytest.raises(SystemExit) as exc_info: + with pytest.raises(gitlab.GitLabError) as exc_info: gitlab.cmd_mr_create([]) - assert exc_info.value.code == gitlab.EXIT_FAILURE - assert "request body exceeds size limit" in capsys.readouterr().err + assert exc_info.value.exit_code == gitlab.EXIT_FAILURE + assert "request body exceeds size limit" in str(exc_info.value) def test_redacts_sensitive_error_bodies( self, diff --git a/docs/docusaurus/package-lock.json b/docs/docusaurus/package-lock.json index b88115a07a..dc7d8d8f35 100644 --- a/docs/docusaurus/package-lock.json +++ b/docs/docusaurus/package-lock.json @@ -10646,39 +10646,6 @@ "postcss": "^8.1.0" } }, - "node_modules/autoprefixer/node_modules/browserslist": { - "version": "4.28.7", - "resolved": "https://registry.npmjs.org/browserslist/-/browserslist-4.28.7.tgz", - "integrity": "sha512-JxV13hNrFxqjOc8alRbq9dK1MM79NEXYpma2B2J4wAtpWS5zIEIKqWPGCl7N4o7Uc7B7itylh7SuDujATRyyTw==", - "funding": [ - { - "type": "opencollective", - "url": "https://opencollective.com/browserslist" - }, - { - "type": "tidelift", - "url": "https://tidelift.com/funding/github/npm/browserslist" - }, - { - "type": "github", - "url": "https://github.com/sponsors/ai" - } - ], - "license": "MIT", - "dependencies": { - "baseline-browser-mapping": "^2.10.44", - "caniuse-lite": "^1.0.30001806", - "electron-to-chromium": "^1.5.393", - "node-releases": "^2.0.51", - "update-browserslist-db": "^1.2.3" - }, - "bin": { - "browserslist": "cli.js" - }, - "engines": { - "node": "^6 || ^7 || ^8 || ^9 || ^10 || ^11 || ^12 || >=13.7" - } - }, "node_modules/available-typed-arrays": { "version": "1.0.7", "resolved": "https://registry.npmjs.org/available-typed-arrays/-/available-typed-arrays-1.0.7.tgz", @@ -10949,9 +10916,9 @@ } }, "node_modules/baseline-browser-mapping": { - "version": "2.11.5", - "resolved": "https://registry.npmjs.org/baseline-browser-mapping/-/baseline-browser-mapping-2.11.5.tgz", - "integrity": "sha512-xJo6a6YZnwZfnyGmQKWMbVOcii7XRibjOskRh+WJ9UHQoX16xrQrcIgAMQOzfvs8XiLMx6ih/fsLPF73iY2D1A==", + "version": "2.11.20", + "resolved": "https://registry.npmjs.org/baseline-browser-mapping/-/baseline-browser-mapping-2.11.20.tgz", + "integrity": "sha512-H0ulySigv6icDJ1F7SjtdCD6PrhTpdYCmP0CactWy1+ekh0AFd0o1Wn5T8b+hnTmdBx19u9yhL6wvCylXMY7zw==", "license": "Apache-2.0", "bin": { "baseline-browser-mapping": "dist/cli.cjs" @@ -11097,9 +11064,9 @@ } }, "node_modules/browserslist": { - "version": "4.28.2", - "resolved": "https://registry.npmjs.org/browserslist/-/browserslist-4.28.2.tgz", - "integrity": "sha512-48xSriZYYg+8qXna9kwqjIVzuQxi+KYWp2+5nCYnYKPTr0LvD89Jqk2Or5ogxz0NUMfIjhh2lIUX/LyX9B4oIg==", + "version": "4.28.8", + "resolved": "https://registry.npmjs.org/browserslist/-/browserslist-4.28.8.tgz", + "integrity": "sha512-V2NpofLblG64mfOtSgDhOJESZEGogzDMBv/q+W6oc4LXWP/q75eOXoOaaOu1EOadB9U4Bwx/e0yzbvwKH8zalA==", "funding": [ { "type": "opencollective", @@ -11116,11 +11083,11 @@ ], "license": "MIT", "dependencies": { - "baseline-browser-mapping": "^2.10.12", - "caniuse-lite": "^1.0.30001782", - "electron-to-chromium": "^1.5.328", - "node-releases": "^2.0.36", - "update-browserslist-db": "^1.2.3" + "baseline-browser-mapping": "^2.11.12", + "caniuse-lite": "^1.0.30001809", + "electron-to-chromium": "^1.5.402", + "node-releases": "^2.0.53", + "update-browserslist-db": "^1.3.0" }, "bin": { "browserslist": "cli.js" @@ -11322,9 +11289,9 @@ } }, "node_modules/caniuse-lite": { - "version": "1.0.30001806", - "resolved": "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001806.tgz", - "integrity": "sha512-72Cuvd95zbSYPKq6Fhg8eDJRlzgWDf7/mtoZv6Qe/DYNCEBdNxoA3+rZAU2ZhGCpZlns3EssFavaZomckT5Uuw==", + "version": "1.0.30001810", + "resolved": "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001810.tgz", + "integrity": "sha512-TITQPUkaz+aVk5GL6NhOdwk1aEaNTSDPsGFWrTuhKGtjTF70jL/Oht2W4c6rXUe5fu7Ie19VIahAXHIIiWWNeg==", "funding": [ { "type": "opencollective", @@ -13598,9 +13565,9 @@ "license": "MIT" }, "node_modules/electron-to-chromium": { - "version": "1.5.397", - "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.5.397.tgz", - "integrity": "sha512-khGTy9U9x02KEtsKM8vx5A62BsRmcOsIgDpWr1ImE32Ax8GxHGPHZf+Eu9H8zOOyHJnB0jTbseyTHbq2XCT8yw==", + "version": "1.5.420", + "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.5.420.tgz", + "integrity": "sha512-2yD6XreGusOfNV+dUcvipJEXc3n/n7fgr7996aszTG+YY5E4mqM4tOq/3uhP129cazL9YHbVWSpc79ePotWtPA==", "license": "ISC" }, "node_modules/emittery": { @@ -14958,9 +14925,9 @@ "license": "MIT" }, "node_modules/fast-uri": { - "version": "3.1.5", - "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.5.tgz", - "integrity": "sha512-gHwA1O9LDIcKunMKhObS/HimwtehO1nPUECKAu5TpKgaO19fcWEl4bliWe1jWxVFvIXztJjjQ4L8XQ1EU9f7Jw==", + "version": "3.1.6", + "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.6.tgz", + "integrity": "sha512-7Ical1vFEMr0onbVzEDIreM22I4khW+fzyQPwvAFWBp1iwdshSZRsL4jjRvPG9JP1uiqMHRto+YU6R2/CzDz5Q==", "funding": [ { "type": "github", @@ -23150,9 +23117,9 @@ "license": "MIT" }, "node_modules/node-releases": { - "version": "2.0.51", - "resolved": "https://registry.npmjs.org/node-releases/-/node-releases-2.0.51.tgz", - "integrity": "sha512-wRNIrw4DmVLKQlbgOMdkMx27Wrpzes2hh5Jtbi2bjPd+4wJstWIqP5A+lscnqbm0xxmT5Bpg8Lec5ItEBwx6BQ==", + "version": "2.0.54", + "resolved": "https://registry.npmjs.org/node-releases/-/node-releases-2.0.54.tgz", + "integrity": "sha512-YHs7BmmcsdAI5Ozuf8JZo6PT0mv2GIWC9vMfvUC3dp65M8hn7Ux8CPL+2oBI7juNuj9d0ndhTcznq2ODBps9cQ==", "license": "MIT", "engines": { "node": ">=18" @@ -25806,12 +25773,13 @@ } }, "node_modules/qs": { - "version": "6.15.2", - "resolved": "https://registry.npmjs.org/qs/-/qs-6.15.2.tgz", - "integrity": "sha512-Rzq0KEyX/w/tEybncDgdkZrJgVUsUMk3xjh3t5bv3S1HTAtg+uOYt72+ZfwiQwKdysThkTBdL/rTi6HDmX9Ddw==", + "version": "6.16.0", + "resolved": "https://registry.npmjs.org/qs/-/qs-6.16.0.tgz", + "integrity": "sha512-h6fhOIaRrID2CbEY2fqs+7t+UXZo+MLAnU5gRIq85uFtdiUPCdsApMlHhXogKVM4HM2DVbIjGNTTYH2OcmP1vA==", "license": "BSD-3-Clause", "dependencies": { - "side-channel": "^1.1.0" + "es-define-property": "^1.0.1", + "side-channel": "^1.1.1" }, "engines": { "node": ">=0.6" @@ -27397,14 +27365,14 @@ } }, "node_modules/side-channel": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/side-channel/-/side-channel-1.1.0.tgz", - "integrity": "sha512-ZX99e6tRweoUXqR+VBrslhda51Nh5MTQwou5tnUDgbtyM0dBgmhEDtWGP/xbKn6hqfPRHujUNwz5fy/wbbhnpw==", + "version": "1.1.1", + "resolved": "https://registry.npmjs.org/side-channel/-/side-channel-1.1.1.tgz", + "integrity": "sha512-6x6dK6zJdpTzF4sQeNYxwtvBzf6Eg4GtlesS94HOvTudUeyK2WXAaIfmDgsyslYrRBeFIlsi54AYsFGUuhmvrQ==", "license": "MIT", "dependencies": { "es-errors": "^1.3.0", - "object-inspect": "^1.13.3", - "side-channel-list": "^1.0.0", + "object-inspect": "^1.13.4", + "side-channel-list": "^1.0.1", "side-channel-map": "^1.0.1", "side-channel-weakmap": "^1.0.2" }, @@ -29183,9 +29151,9 @@ } }, "node_modules/update-browserslist-db": { - "version": "1.2.3", - "resolved": "https://registry.npmjs.org/update-browserslist-db/-/update-browserslist-db-1.2.3.tgz", - "integrity": "sha512-Js0m9cx+qOgDxo0eMiFGEueWztz+d4+M3rGlmKPT+T4IS/jP4ylw3Nwpu6cpTTP8R1MAC1kF4VbdLt3ARf209w==", + "version": "1.3.2", + "resolved": "https://registry.npmjs.org/update-browserslist-db/-/update-browserslist-db-1.3.2.tgz", + "integrity": "sha512-UQ+MSxlhRm1bzjhU+DcuXfjFO1FzNtqhK5+9Yvlp90ItDLk5vT932A0rFu619nf7RVS+Y/VeaUW1jaRDqZ8VJw==", "funding": [ { "type": "opencollective", diff --git a/docs/docusaurus/package.json b/docs/docusaurus/package.json index 1dc09e6868..d0d1e7e997 100644 --- a/docs/docusaurus/package.json +++ b/docs/docusaurus/package.json @@ -83,8 +83,9 @@ "brace-expansion@^1": "1.1.18", "brace-expansion@^2": "2.1.4", "brace-expansion@^5": "5.0.9", + "browserslist": "4.28.8", "express": "5.2.1", - "fast-uri": "3.1.5", + "fast-uri": "3.1.6", "http-proxy-middleware": "2.0.10", "image-size": "npm:image-size-next@2.1.1", "js-yaml@^3": "3.15.1", @@ -95,6 +96,7 @@ "nanoid": "3.3.18", "picomatch": "4.0.4", "postcss": "8.5.25", + "qs": "6.16.0", "serialize-javascript": "7.0.5", "shell-quote": "1.9.0", "svgo@^3": "3.3.4", diff --git a/package-lock.json b/package-lock.json index 267b4be709..2b06913e91 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5038,9 +5038,9 @@ "license": "MIT" }, "node_modules/fast-uri": { - "version": "3.1.5", - "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.5.tgz", - "integrity": "sha512-gHwA1O9LDIcKunMKhObS/HimwtehO1nPUECKAu5TpKgaO19fcWEl4bliWe1jWxVFvIXztJjjQ4L8XQ1EU9f7Jw==", + "version": "3.1.6", + "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.6.tgz", + "integrity": "sha512-7Ical1vFEMr0onbVzEDIreM22I4khW+fzyQPwvAFWBp1iwdshSZRsL4jjRvPG9JP1uiqMHRto+YU6R2/CzDz5Q==", "dev": true, "funding": [ { @@ -11734,13 +11734,14 @@ } }, "node_modules/qs": { - "version": "6.15.2", - "resolved": "https://registry.npmjs.org/qs/-/qs-6.15.2.tgz", - "integrity": "sha512-Rzq0KEyX/w/tEybncDgdkZrJgVUsUMk3xjh3t5bv3S1HTAtg+uOYt72+ZfwiQwKdysThkTBdL/rTi6HDmX9Ddw==", + "version": "6.16.0", + "resolved": "https://registry.npmjs.org/qs/-/qs-6.16.0.tgz", + "integrity": "sha512-h6fhOIaRrID2CbEY2fqs+7t+UXZo+MLAnU5gRIq85uFtdiUPCdsApMlHhXogKVM4HM2DVbIjGNTTYH2OcmP1vA==", "dev": true, "license": "BSD-3-Clause", "dependencies": { - "side-channel": "^1.1.0" + "es-define-property": "^1.0.1", + "side-channel": "^1.1.1" }, "engines": { "node": ">=0.6" @@ -13028,15 +13029,15 @@ } }, "node_modules/side-channel": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/side-channel/-/side-channel-1.1.0.tgz", - "integrity": "sha512-ZX99e6tRweoUXqR+VBrslhda51Nh5MTQwou5tnUDgbtyM0dBgmhEDtWGP/xbKn6hqfPRHujUNwz5fy/wbbhnpw==", + "version": "1.1.1", + "resolved": "https://registry.npmjs.org/side-channel/-/side-channel-1.1.1.tgz", + "integrity": "sha512-6x6dK6zJdpTzF4sQeNYxwtvBzf6Eg4GtlesS94HOvTudUeyK2WXAaIfmDgsyslYrRBeFIlsi54AYsFGUuhmvrQ==", "dev": true, "license": "MIT", "dependencies": { "es-errors": "^1.3.0", - "object-inspect": "^1.13.3", - "side-channel-list": "^1.0.0", + "object-inspect": "^1.13.4", + "side-channel-list": "^1.0.1", "side-channel-map": "^1.0.1", "side-channel-weakmap": "^1.0.2" }, @@ -13048,14 +13049,14 @@ } }, "node_modules/side-channel-list": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/side-channel-list/-/side-channel-list-1.0.0.tgz", - "integrity": "sha512-FCLHtRD/gnpCiCHEiJLOwdmFP+wzCmDEkc9y7NsYxeF4u7Btsn1ZuwgwJGxImImHicJArLP4R0yX4c2KCrMrTA==", + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/side-channel-list/-/side-channel-list-1.0.1.tgz", + "integrity": "sha512-mjn/0bi/oUURjc5Xl7IaWi/OJJJumuoJFQJfDDyO46+hBWsfaVM65TBHq2eoZBhzl9EchxOijpkbRC8SVBQU0w==", "dev": true, "license": "MIT", "dependencies": { "es-errors": "^1.3.0", - "object-inspect": "^1.13.3" + "object-inspect": "^1.13.4" }, "engines": { "node": ">= 0.4" diff --git a/package.json b/package.json index baeac80a57..bb692e487a 100644 --- a/package.json +++ b/package.json @@ -133,7 +133,7 @@ "basic-ftp": "6.0.1", "brace-expansion@^2": "2.1.4", "brace-expansion@^5": "5.0.9", - "fast-uri": "3.1.5", + "fast-uri": "3.1.6", "form-data": "4.0.6", "ip-address": "10.4.0", "js-yaml": "4.3.1", @@ -141,6 +141,7 @@ "markdown-it": "14.2.0", "picomatch@^2": "2.3.2", "picomatch@^4": "4.0.4", + "qs": "6.16.0", "smol-toml": "1.6.1", "tmp": "0.2.7", "undici": "7.29.0", diff --git a/scripts/extension/marketplace-publisher/package-lock.json b/scripts/extension/marketplace-publisher/package-lock.json index a5a41d43c7..de0dd49b2d 100644 --- a/scripts/extension/marketplace-publisher/package-lock.json +++ b/scripts/extension/marketplace-publisher/package-lock.json @@ -1443,9 +1443,9 @@ } }, "node_modules/fast-uri": { - "version": "3.1.5", - "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.5.tgz", - "integrity": "sha512-gHwA1O9LDIcKunMKhObS/HimwtehO1nPUECKAu5TpKgaO19fcWEl4bliWe1jWxVFvIXztJjjQ4L8XQ1EU9f7Jw==", + "version": "3.1.6", + "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.6.tgz", + "integrity": "sha512-7Ical1vFEMr0onbVzEDIreM22I4khW+fzyQPwvAFWBp1iwdshSZRsL4jjRvPG9JP1uiqMHRto+YU6R2/CzDz5Q==", "funding": [ { "type": "github", @@ -2640,9 +2640,9 @@ } }, "node_modules/qs": { - "version": "6.15.3", - "resolved": "https://registry.npmjs.org/qs/-/qs-6.15.3.tgz", - "integrity": "sha512-O9gl3zCl5h5blw1KGUzQKhA5oUXSl8rwUIM5o0S3nCXMliSvy5Dzx7/DJcI+SwgICv+IneSZwhBh1oSyEHA71A==", + "version": "6.16.0", + "resolved": "https://registry.npmjs.org/qs/-/qs-6.16.0.tgz", + "integrity": "sha512-h6fhOIaRrID2CbEY2fqs+7t+UXZo+MLAnU5gRIq85uFtdiUPCdsApMlHhXogKVM4HM2DVbIjGNTTYH2OcmP1vA==", "license": "BSD-3-Clause", "dependencies": { "es-define-property": "^1.0.1", diff --git a/scripts/extension/marketplace-publisher/package.json b/scripts/extension/marketplace-publisher/package.json index 8bf267d42a..e24761f2a0 100644 --- a/scripts/extension/marketplace-publisher/package.json +++ b/scripts/extension/marketplace-publisher/package.json @@ -6,5 +6,9 @@ "license": "MIT", "dependencies": { "@vscode/vsce": "3.9.2" + }, + "overrides": { + "fast-uri": "3.1.6", + "qs": "6.16.0" } } diff --git a/scripts/security/Install-PSModules.ps1 b/scripts/security/Install-PSModules.ps1 index 8dd97fa5d0..50a37a19e8 100644 --- a/scripts/security/Install-PSModules.ps1 +++ b/scripts/security/Install-PSModules.ps1 @@ -10,7 +10,9 @@ Reads the pinned module manifest and installs each module at the declared version. Modules already present at the correct version are skipped unless -Force is specified. Transient PSGallery failures are retried with - exponential backoff. + exponential backoff. If PowerShellGet's default registration returns + without making PSGallery visible, installation uses a temporary repository + at the canonical endpoint and removes it afterward. Colocation rationale: this script lives in scripts/security/ because it consumes ps-module-versions.json (the pinned-version manifest that the @@ -76,6 +78,7 @@ param( ) $ErrorActionPreference = 'Stop' +$PSGallerySourceUri = 'https://www.powershellgallery.com/api/v2' #region Functions @@ -144,6 +147,57 @@ function Test-ModulePresent { return [bool]$installed } +function Register-DefaultPSGallery { + <# + .SYNOPSIS + Registers PowerShellGet's default PSGallery repository. + .OUTPUTS + [bool] True when PSGallery is discoverable after registration. + #> + [CmdletBinding()] + [OutputType([bool])] + param() + + $null = Register-PSRepository -Default -ErrorAction Stop + return [bool](Get-PSRepository -Name 'PSGallery' -ErrorAction SilentlyContinue) +} + +function Initialize-Repository { + <# + .SYNOPSIS + Initializes the requested repository before installation. + .OUTPUTS + [string] Repository name to use for installation. + #> + [CmdletBinding()] + [OutputType([string])] + param( + [Parameter(Mandatory = $true)] + [ValidateNotNullOrEmpty()] + [string]$Name + ) + + $repo = Get-PSRepository -Name $Name -ErrorAction SilentlyContinue + if ($repo) { + return $Name + } + + if ($Name -ne 'PSGallery') { + return $Name + } + + if (Register-DefaultPSGallery) { + Write-Host "๐Ÿ“ฆ Registered repository $Name" -ForegroundColor DarkCyan + return $Name + } + + $temporaryName = "HVEPSGallery-$PID-$([guid]::NewGuid().ToString('N'))" + $null = Register-PSRepository -Name $temporaryName -SourceLocation $PSGallerySourceUri ` + -InstallationPolicy Untrusted -ErrorAction Stop + Write-Host "๐Ÿ“ฆ Registered temporary repository $temporaryName" -ForegroundColor DarkCyan + return $temporaryName +} + function Install-SingleModule { <# .SYNOPSIS @@ -179,29 +233,62 @@ function Install-SingleModule { $isCI = $env:GITHUB_ACTIONS -eq 'true' - for ($attempt = 1; $attempt -le $MaxAttempts; $attempt++) { - try { - Install-Module -Name $Name -RequiredVersion $Version -Force -Scope $Scope -Repository $Repository -ErrorAction Stop - Write-Host "โœ… Installed $Name $Version (attempt $attempt)" -ForegroundColor Green - return - } - catch { - if ($attempt -eq $MaxAttempts) { - $msg = "Failed to install $Name $Version after $MaxAttempts attempts: $($_.Exception.Message)" + $resolvedRepository = $Repository + $installError = $null + + try { + $resolvedRepository = Initialize-Repository -Name $Repository + for ($attempt = 1; $attempt -le $MaxAttempts; $attempt++) { + try { + # -Force suppresses the prompt for the temporary Untrusted source; + # RequiredVersion and the canonical source remain fixed. + Install-Module -Name $Name -RequiredVersion $Version -Force -Scope $Scope -Repository $resolvedRepository -ErrorAction Stop + Write-Host "โœ… Installed $Name $Version (attempt $attempt)" -ForegroundColor Green + return + } + catch { + if ($attempt -eq $MaxAttempts) { + $msg = "Failed to install $Name $Version after $MaxAttempts attempts: $($_.Exception.Message)" + if ($isCI) { + Write-Host "::error::$msg" + } + throw $msg + } + $delay = $BaseDelaySeconds * [math]::Pow(2, $attempt - 1) + $warnMsg = "Attempt $attempt/$MaxAttempts failed for ${Name}: $($_.Exception.Message). Retrying in ${delay}s..." if ($isCI) { - Write-Host "::error::$msg" + Write-Host "::warning::$warnMsg" } - throw $msg + Write-Host "โš ๏ธ $warnMsg" -ForegroundColor Yellow + Start-Sleep -Seconds $delay } - $delay = $BaseDelaySeconds * [math]::Pow(2, $attempt - 1) - $warnMsg = "Attempt $attempt/$MaxAttempts failed for ${Name}: $($_.Exception.Message). Retrying in ${delay}s..." - if ($isCI) { - Write-Host "::warning::$warnMsg" + } + } + catch { + $installError = $_ + } + finally { + if ($resolvedRepository -ne $Repository) { + try { + $null = Unregister-PSRepository -Name $resolvedRepository -ErrorAction Stop + Write-Host "๐Ÿงน Removed temporary repository $resolvedRepository" -ForegroundColor DarkCyan + } + catch { + $cleanupMessage = "Failed to remove temporary repository ${resolvedRepository}: $($_.Exception.Message)" + if ($null -eq $installError) { + throw $cleanupMessage + } + if ($isCI) { + Write-Host "::warning::$cleanupMessage" + } + Write-Warning $cleanupMessage } - Write-Host "โš ๏ธ $warnMsg" -ForegroundColor Yellow - Start-Sleep -Seconds $delay } } + + if ($null -ne $installError) { + throw $installError + } } function Invoke-PSModuleInstall { diff --git a/scripts/security/README.md b/scripts/security/README.md index eb53d69d02..7671ae59a8 100644 --- a/scripts/security/README.md +++ b/scripts/security/README.md @@ -409,13 +409,13 @@ it to `scripts/lib/`. #### Contract -| Aspect | Detail | -|--------------|--------------------------------------------------------------------------------------------------------------------------------------------| -| Error mode | `$ErrorActionPreference = 'Stop'`; throws on exhausted retries | -| Exit code | 0 on success, 1 on any module install failure | -| Logging | Timestamped `Write-Host` (green success, yellow retry, red failure); emits `::warning::` annotations when `$env:GITHUB_ACTIONS -eq 'true'` | -| Idempotent | Skips modules already present at the required version (`Get-Module -ListAvailable`) unless `-Force` is specified | -| Side effects | `Import-Module` each installed module into the session when `-Import` is specified | +| Aspect | Detail | +|--------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| Error mode | `$ErrorActionPreference = 'Stop'`; throws on exhausted retries | +| Exit code | 0 on success, 1 on any module install failure | +| Logging | Timestamped `Write-Host` (green success, yellow retry, red failure); emits `::warning::` annotations when `$env:GITHUB_ACTIONS -eq 'true'` | +| Idempotent | Skips modules already present at the required version (`Get-Module -ListAvailable`) unless `-Force` is specified; verifies default PSGallery registration before use | +| Side effects | Imports requested modules; creates and removes a temporary canonical PSGallery alias when default registration is unavailable; cleanup also runs after an installation failure | #### Parameters diff --git a/scripts/tests/security/Install-PSModules.Tests.ps1 b/scripts/tests/security/Install-PSModules.Tests.ps1 index dee8b16324..974f74b528 100644 --- a/scripts/tests/security/Install-PSModules.Tests.ps1 +++ b/scripts/tests/security/Install-PSModules.Tests.ps1 @@ -170,10 +170,92 @@ Describe 'Test-ModulePresent' -Tag 'Unit' { } } +Describe 'Register-DefaultPSGallery' -Tag 'Unit' { + It 'Uses only the PowerShellGet default parameter set' { + $command = (Get-Command Register-DefaultPSGallery).ScriptBlock.Ast.Find({ + param($Ast) + $Ast -is [System.Management.Automation.Language.CommandAst] -and + $Ast.GetCommandName() -eq 'Register-PSRepository' + }, $true) + + $command.Extent.Text | Should -BeExactly 'Register-PSRepository -Default -ErrorAction Stop' + } + +} + +Describe 'Initialize-Repository' -Tag 'Unit' { + Context 'when the repository is already registered' { + BeforeAll { + Mock Get-PSRepository { + [PSCustomObject]@{ Name = 'PSGallery' } + } + Mock Register-PSRepository {} + } + + It 'Does not register the repository again' { + $result = Initialize-Repository -Name 'PSGallery' + + $result | Should -BeExactly 'PSGallery' + Should -Invoke Register-PSRepository -Times 0 -Exactly + } + } + + Context 'when PSGallery is missing' { + BeforeAll { + Mock Get-PSRepository { $null } + Mock Register-DefaultPSGallery { $true } + } + + It 'Registers PSGallery with the default parameter set' { + $result = Initialize-Repository -Name 'PSGallery' + + $result | Should -BeExactly 'PSGallery' + Should -Invoke Register-DefaultPSGallery -Times 1 -Exactly + } + } + + Context 'when default PSGallery registration remains unavailable' { + BeforeAll { + Mock Get-PSRepository { $null } + Mock Register-DefaultPSGallery { $false } + Mock Register-PSRepository {} + } + + It 'Returns a temporary repository at the canonical endpoint' { + $result = Initialize-Repository -Name 'PSGallery' + + $result | Should -BeLike "HVEPSGallery-$PID-*" + Should -Invoke Register-PSRepository -Times 1 -Exactly -ParameterFilter { + $Name -like "HVEPSGallery-$PID-*" -and + $SourceLocation -eq 'https://www.powershellgallery.com/api/v2' -and + $InstallationPolicy -eq 'Untrusted' + } + } + } + + Context 'when a non-PSGallery repository is missing' { + BeforeAll { + Mock Get-PSRepository { $null } + Mock Register-PSRepository {} + } + + It 'Does not register an alternate repository automatically' { + $result = Initialize-Repository -Name 'CustomRepo' + + $result | Should -BeExactly 'CustomRepo' + Should -Invoke Register-PSRepository -Times 0 -Exactly + } + } +} + Describe 'Install-SingleModule' -Tag 'Unit' { Context 'when Install-Module succeeds on first attempt' { BeforeAll { Mock Install-Module {} + Mock Get-PSRepository { + [PSCustomObject]@{ Name = 'PSGallery' } + } + Mock Register-PSRepository {} } It 'Calls Install-Module exactly once' { @@ -191,6 +273,77 @@ Describe 'Install-SingleModule' -Tag 'Unit' { } } + Context 'when default registration requires a temporary repository' { + BeforeAll { + Mock Initialize-Repository { "HVEPSGallery-$PID-test" } + Mock Install-Module {} + Mock Unregister-PSRepository {} + } + + It 'Installs from the temporary repository and removes it' { + Install-SingleModule -Name 'TestMod' -Version '1.0.0' -Scope 'CurrentUser' ` + -Repository 'PSGallery' -MaxAttempts 3 -BaseDelaySeconds 10 + + Should -Invoke Install-Module -Times 1 -Exactly -ParameterFilter { + $Repository -eq "HVEPSGallery-$PID-test" + } + Should -Invoke Unregister-PSRepository -Times 1 -Exactly -ParameterFilter { + $Name -eq "HVEPSGallery-$PID-test" + } + } + } + + Context 'when installation from a temporary repository fails' { + BeforeAll { + Mock Initialize-Repository { "HVEPSGallery-$PID-test" } + Mock Install-Module { throw 'installation failed' } + Mock Unregister-PSRepository {} + } + + It 'Removes the temporary repository before propagating the failure' { + { Install-SingleModule -Name 'TestMod' -Version '1.0.0' -Scope 'CurrentUser' ` + -Repository 'PSGallery' -MaxAttempts 1 -BaseDelaySeconds 1 } | + Should -Throw '*Failed to install TestMod*' + + Should -Invoke Unregister-PSRepository -Times 1 -Exactly -ParameterFilter { + $Name -eq "HVEPSGallery-$PID-test" + } + } + } + + Context 'when temporary repository cleanup also fails' { + BeforeAll { + Mock Initialize-Repository { "HVEPSGallery-$PID-test" } + Mock Install-Module { throw 'installation failed' } + Mock Unregister-PSRepository { throw 'cleanup failed' } + Mock Write-Warning {} + } + + It 'Preserves the installation failure' { + { Install-SingleModule -Name 'TestMod' -Version '1.0.0' -Scope 'CurrentUser' ` + -Repository 'PSGallery' -MaxAttempts 1 -BaseDelaySeconds 1 } | + Should -Throw '*Failed to install TestMod*' + + Should -Invoke Write-Warning -Times 1 -Exactly -ParameterFilter { + $Message -like 'Failed to remove temporary repository*' + } + } + } + + Context 'when cleanup is the only failure' { + BeforeAll { + Mock Initialize-Repository { "HVEPSGallery-$PID-test" } + Mock Install-Module {} + Mock Unregister-PSRepository { throw 'cleanup failed' } + } + + It 'Fails instead of reporting successful cleanup' { + { Install-SingleModule -Name 'TestMod' -Version '1.0.0' -Scope 'CurrentUser' ` + -Repository 'PSGallery' -MaxAttempts 1 -BaseDelaySeconds 1 } | + Should -Throw '*Failed to remove temporary repository*' + } + } + Context 'when Install-Module fails twice then succeeds' { BeforeAll { $script:CallCount = 0 @@ -200,6 +353,10 @@ Describe 'Install-SingleModule' -Tag 'Unit' { throw "PSGallery transient failure" } } + Mock Get-PSRepository { + [PSCustomObject]@{ Name = 'PSGallery' } + } + Mock Register-PSRepository {} } BeforeEach { $script:CallCount = 0 @@ -230,6 +387,10 @@ Describe 'Install-SingleModule' -Tag 'Unit' { Context 'when Install-Module fails on all attempts' { BeforeAll { Mock Install-Module { throw "PSGallery is down" } + Mock Get-PSRepository { + [PSCustomObject]@{ Name = 'PSGallery' } + } + Mock Register-PSRepository {} } It 'Throws after exhausting retries' { @@ -251,6 +412,10 @@ Describe 'Install-SingleModule' -Tag 'Unit' { Context 'when running in GitHub Actions' { BeforeAll { Mock Install-Module { throw "network error" } + Mock Get-PSRepository { + [PSCustomObject]@{ Name = 'PSGallery' } + } + Mock Register-PSRepository {} } BeforeEach { $script:OrigGA = $env:GITHUB_ACTIONS @@ -295,6 +460,10 @@ Describe 'Invoke-PSModuleInstall end-to-end' -Tag 'Unit' { 'FakeModuleB' { [PSCustomObject]@{ Version = [version]'2.5.0' } } } } + Mock Get-PSRepository { + [PSCustomObject]@{ Name = 'PSGallery' } + } + Mock Register-PSRepository {} Mock Install-Module {} Mock Import-Module {} } @@ -311,6 +480,10 @@ Describe 'Invoke-PSModuleInstall end-to-end' -Tag 'Unit' { Mock Get-Module { [PSCustomObject]@{ Version = [version]'1.0.0' } } + Mock Get-PSRepository { + [PSCustomObject]@{ Name = 'PSGallery' } + } + Mock Register-PSRepository {} Mock Install-Module {} Mock Import-Module {} } @@ -331,6 +504,10 @@ Describe 'Invoke-PSModuleInstall end-to-end' -Tag 'Unit' { 'FakeModuleB' { [PSCustomObject]@{ Version = [version]'2.5.0' } } } } + Mock Get-PSRepository { + [PSCustomObject]@{ Name = 'PSGallery' } + } + Mock Register-PSRepository {} Mock Install-Module {} Mock Import-Module {} } @@ -351,6 +528,10 @@ Describe 'Invoke-PSModuleInstall end-to-end' -Tag 'Unit' { 'FakeModuleB' { [PSCustomObject]@{ Version = [version]'2.5.0' } } } } + Mock Get-PSRepository { + [PSCustomObject]@{ Name = 'PSGallery' } + } + Mock Register-PSRepository {} Mock Install-Module {} Mock Import-Module {} }