diff --git a/.apm/instructions/architecture.instructions.md b/.apm/instructions/architecture.instructions.md index cb0c6caa17..fa2d52b339 100644 --- a/.apm/instructions/architecture.instructions.md +++ b/.apm/instructions/architecture.instructions.md @@ -78,6 +78,7 @@ semicolon-delimited, and specific to the file(s) that own the fact. | Agent Plugin producer portable-surface admission | bundle/agent_plugin_exporter.py (_require_portable_agent_plugin) | `src/apm_cli/bundle/agent_plugin_exporter.py` | | APMPackage interpreted-manifest construction | models/apm_package.py (APMPackage.from_mapping) | `src/apm_cli/models/apm_package.py` | | Agent Plugin compatibility package projection | agent_plugins/projection.py (project_agent_plugin_package) | `src/apm_cli/agent_plugins/projection.py`; `src/apm_cli/models/validation.py` | +| Strict percent-encoded URL-path decoding and safety | utils/path_security.py (decode_url_path_segments) | `src/apm_cli/utils/path_security.py` | Host + credential resolution includes public github.com anonymous-first ordering. diff --git a/.github/instructions/architecture.instructions.md b/.github/instructions/architecture.instructions.md index cb0c6caa17..fa2d52b339 100644 --- a/.github/instructions/architecture.instructions.md +++ b/.github/instructions/architecture.instructions.md @@ -78,6 +78,7 @@ semicolon-delimited, and specific to the file(s) that own the fact. | Agent Plugin producer portable-surface admission | bundle/agent_plugin_exporter.py (_require_portable_agent_plugin) | `src/apm_cli/bundle/agent_plugin_exporter.py` | | APMPackage interpreted-manifest construction | models/apm_package.py (APMPackage.from_mapping) | `src/apm_cli/models/apm_package.py` | | Agent Plugin compatibility package projection | agent_plugins/projection.py (project_agent_plugin_package) | `src/apm_cli/agent_plugins/projection.py`; `src/apm_cli/models/validation.py` | +| Strict percent-encoded URL-path decoding and safety | utils/path_security.py (decode_url_path_segments) | `src/apm_cli/utils/path_security.py` | Host + credential resolution includes public github.com anonymous-first ordering. diff --git a/docs/src/content/docs/producer/publish-to-a-marketplace.md b/docs/src/content/docs/producer/publish-to-a-marketplace.md index 2a9d7a4f7d..e3dd90fe86 100644 --- a/docs/src/content/docs/producer/publish-to-a-marketplace.md +++ b/docs/src/content/docs/producer/publish-to-a-marketplace.md @@ -168,6 +168,10 @@ use `ADO_APM_PAT`, and use the portless root-hosted use the Azure CLI bearer. See [authentication](../../getting-started/authentication/#azure-devops). +Percent-encode ADO path spaces, for example `My%20Projects`. The generated +URL keeps that encoded spelling, while APM uses the decoded organization, +project, and repository identity for resolution and authentication. + Before: ```yaml diff --git a/docs/src/content/docs/reference/manifest-schema.md b/docs/src/content/docs/reference/manifest-schema.md index 5e00a112fc..16be25018a 100644 --- a/docs/src/content/docs/reference/manifest-schema.md +++ b/docs/src/content/docs/reference/manifest-schema.md @@ -949,7 +949,7 @@ When `sourceBase` is set, relative package sources compose onto that base. For e A relative `source` may use arbitrary path depth. A value whose leading segments form a host-prefixed shape (`//`) or a full `https://` URL is always treated as a per-entry override and ignores `sourceBase`. A value that looks like it is trying to name a host (a dotted, FQDN-like first segment) but does **not** form a valid override shape is rejected at parse time rather than silently composed onto the base -- this avoids a confused-deputy footgun. To target a different host, use an explicit host-prefixed override or a full `https://` URL instead of a relative source. -`sourceBase` must start with `https://`, use a FQDN host, include at least one path segment, and omit userinfo, ports, query strings, fragments, and a trailing `.git`. Each path segment uses letters, digits, `.`, `_`, or `-`; empty, `.` and `..` segments are refused. +`sourceBase` must start with `https://`, use a FQDN host, include at least one path segment, and omit userinfo, ports, query strings, fragments, and a trailing `.git`. Path segments may percent-encode UTF-8 bytes, such as `My%20Projects`. Empty, `.`, `..`, malformed escapes, encoded separators, and recursive encodings are refused. APM keeps the encoded URL in generated output while using decoded ADO coordinates for repository and authentication lookup. Non-default hosts -- GitHub Enterprise, self-hosted GitLab, and Azure DevOps -- authenticate via the standard APM token chain -- see the diff --git a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md index 46e891e5a8..404dcfd899 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -597,6 +597,9 @@ preserved through to the consumer. APM appends each repository name without a `.git` suffix. Authentication uses `ADO_APM_PAT` when set, or an Azure CLI bearer credential when the PAT is unset and `az` is signed in: +Percent-encode spaces in ADO paths, such as `My%20Projects`. The generated +source URL preserves the encoding while APM resolves its decoded ADO identity. + ```yaml marketplace: sourceBase: https://dev.azure.com/contoso/platform/_git diff --git a/scripts/lint-architecture-boundaries.sh b/scripts/lint-architecture-boundaries.sh index 6119399266..df0393ad76 100755 --- a/scripts/lint-architecture-boundaries.sh +++ b/scripts/lint-architecture-boundaries.sh @@ -834,6 +834,48 @@ if ! printf '%s\n' "$packed_source_body" \ violations=$((violations + 1)) fi +marketplace_check_coordinates_body=$(awk ' + /^def _entry_coordinates\(/ {flag=1} + flag && /^def / && !/^def _entry_coordinates\(/ {exit} + flag {print} +' src/apm_cli/commands/marketplace/check.py) +marketplace_check_parallel_parser_hits=$(printf '%s\n' "$marketplace_check_coordinates_body" \ + | grep -En 'split_source_base\(|decode_url_path_segments\(|urlparse\(' \ + | grep -v 'architecture-authority-exempt:' || true) +if ! printf '%s\n' "$marketplace_check_coordinates_body" \ + | grep -Fq 'DependencyReference.parse(entry.source_url)' \ + || ! printf '%s\n' "$marketplace_check_coordinates_body" \ + | grep -Fq 'DependencyReference.parse(source_url)' \ + || [ -n "$marketplace_check_parallel_parser_hits" ]; then + echo "[x] Marketplace check source coordinates must use DependencyReference parsing" + [ -n "$marketplace_check_parallel_parser_hits" ] && echo "$marketplace_check_parallel_parser_hits" + violations=$((violations + 1)) +fi + +echo "[*] AC10a: strict URL-path decoding authority" +url_path_owner="src/apm_cli/utils/path_security.py" +url_path_parallel_decoders=$( + grep -REn --include='*.py' 'unquote(_to_bytes)?\(' \ + src/apm_cli/marketplace/yml_schema.py \ + src/apm_cli/models/dependency/reference.py \ + src/apm_cli/commands/marketplace/__init__.py \ + | grep -v 'architecture-authority-exempt:' || true +) +if ! grep -Fq 'def parse_url_path_segments(' "$url_path_owner" \ + || ! grep -Fq 'decode_url_path_segments(parsed.path, context=context)' \ + src/apm_cli/marketplace/yml_schema.py \ + || ! grep -Fq 'decode_url_path_segments(parsed.path, context="sourceBase")' \ + src/apm_cli/marketplace/yml_schema.py \ + || ! grep -Fq 'parse_url_path_segments(' \ + src/apm_cli/models/dependency/reference.py \ + || ! grep -Fq 'decode_url_path_segments(parsed.path, context="marketplace URL path")' \ + src/apm_cli/commands/marketplace/__init__.py \ + || [ -n "$url_path_parallel_decoders" ]; then + echo "[x] Strict percent-encoded URL paths must use path_security parsing" + [ -n "$url_path_parallel_decoders" ] && echo "$url_path_parallel_decoders" + violations=$((violations + 1)) +fi + echo "[*] AC10b: local marketplace audit resolution authority" if ! grep -Fq 'resolve_local_plugin_path(' src/apm_cli/marketplace/audit.py \ || grep -Fq '_resolve_local_relative_source' src/apm_cli/marketplace/audit.py \ diff --git a/src/apm_cli/commands/marketplace/__init__.py b/src/apm_cli/commands/marketplace/__init__.py index bdcab4cfef..29ecf1217e 100644 --- a/src/apm_cli/commands/marketplace/__init__.py +++ b/src/apm_cli/commands/marketplace/__init__.py @@ -40,7 +40,11 @@ from ...marketplace.ref_resolver import RefResolver, RemoteRef from ...marketplace.semver import SemVer, parse_semver, satisfies_range from ...marketplace.yml_schema import load_marketplace_yml -from ...utils.path_security import PathTraversalError, validate_path_segments +from ...utils.path_security import ( + PathTraversalError, + decode_url_path_segments, + validate_path_segments, +) from .._helpers import _get_console, _is_interactive if TYPE_CHECKING: @@ -312,12 +316,7 @@ def _parse_marketplace_source(source: str, host_flag: str | None) -> tuple[str, embedded_host = (parsed.hostname or "").strip().lower() if not embedded_host: raise ValueError(f"HTTPS URL is missing a host: '{raw}'") - # Validate path segments for traversal markers. - from urllib.parse import unquote as _unquote - - path_segments = [s for s in _unquote(parsed.path or "").split("/") if s] - for seg in path_segments: - validate_path_segments(seg, context="marketplace URL path", reject_empty=True) + path_segments = decode_url_path_segments(parsed.path, context="marketplace URL path") if not path_segments: raise ValueError(f"HTTPS URL is missing a repo path: '{raw}'") host_info = AuthResolver.classify_host(embedded_host) @@ -339,10 +338,8 @@ def _parse_marketplace_source(source: str, host_flag: str | None) -> tuple[str, return raw, kind, embedded_host # --- Shorthand (OWNER/REPO or HOST/OWNER/.../REPO) -------------------- - from urllib.parse import unquote as _unquote - - raw_decoded = _unquote(raw) - segments = [seg for seg in raw_decoded.split("/") if seg] + raw_segments = raw.split("/") + segments = list(decode_url_path_segments(raw, context="marketplace source path")) if len(segments) < 2: raise ValueError( f"Invalid format: '{raw}'. " @@ -359,15 +356,15 @@ def _parse_marketplace_source(source: str, host_flag: str | None) -> tuple[str, ) embedded_host = segments[0].lower() segments = segments[1:] + raw_segments = raw_segments[1:] repo_name = segments[-1] owner_segments = segments[:-1] if not owner_segments or not repo_name: raise ValueError(f"Invalid format: '{raw}'. Expected 'OWNER/REPO'.") - owner_path = "/".join(owner_segments) - validate_path_segments(owner_path, context="marketplace owner path", reject_empty=True) - validate_path_segments(repo_name, context="marketplace repo name", reject_empty=True) + raw_owner_path = "/".join(raw_segments[: len(owner_segments)]) + raw_repo_name = raw_segments[len(owner_segments)] if embedded_host and host_flag and host_flag.strip().lower() != embedded_host: import shlex as _shlex @@ -383,7 +380,7 @@ def _parse_marketplace_source(source: str, host_flag: str | None) -> tuple[str, resolved_host = (host_flag or "").strip().lower() or embedded_host or default_host() host_info = AuthResolver.classify_host(resolved_host) kind = _host_kind_to_fetcher_kind(host_info.kind) - url = f"https://{resolved_host}/{owner_path}/{repo_name}" + url = f"https://{resolved_host}/{raw_owner_path}/{raw_repo_name}" return url, kind, resolved_host diff --git a/src/apm_cli/commands/marketplace/check.py b/src/apm_cli/commands/marketplace/check.py index c3c92edfcc..ba84c525e7 100644 --- a/src/apm_cli/commands/marketplace/check.py +++ b/src/apm_cli/commands/marketplace/check.py @@ -13,7 +13,7 @@ from ...marketplace.errors import GitLsRemoteError, OfflineMissError from ...marketplace.ref_resolver import RefResolver from ...marketplace.semver import satisfies_range -from ...marketplace.yml_schema import PackageEntry, split_source_base +from ...marketplace.yml_schema import PackageEntry from ...utils.github_host import is_azure_devops_hostname from . import ( _CheckResult, @@ -30,27 +30,40 @@ def _entry_coordinates( entry: PackageEntry, source_base: str | None -) -> tuple[str | None, str, str | None]: - """Return ``(host, owner_repo, org)`` for *entry*, mirroring the build-time +) -> tuple[str | None, str, str | None, str | None]: + """Return ``(host, owner_repo, org, display_url)`` for *entry*, mirroring the build-time routing in ``MarketplaceBuilder._remote_source_coordinates`` so that ``check`` and ``pack`` resolve every entry against the same host AND the same per-org auth hint. - - A per-entry host (``host.tld/owner/repo`` or full URL) is an override - with no org hint, matching the builder. + - Full HTTPS entries retain their encoded display URL while + ``DependencyReference`` supplies decoded repository and ADO coordinates. + - A host-prefixed shorthand (``host.tld/owner/repo``) is an override with + no org hint, matching the builder. - Otherwise, when ``marketplace.sourceBase`` is set, a host-less source - composes onto the base and the base's leading path segment becomes the - per-org auth hint (so ``GITHUB_APM_PAT_{ORG}`` resolves identically to - ``pack``). + composes onto the base. ``DependencyReference`` supplies its decoded + identity and ADO org hint, while a non-ADO sourceBase keeps its decoded + leading path segment as the per-org hint. - Otherwise the source stays a default-host ``owner/repo`` with no hint. """ + from ...models.dependency.reference import DependencyReference + if entry.host: - return entry.host, entry.source, None + if entry.source_url is not None: + dependency = DependencyReference.parse(entry.source_url) + return ( + dependency.host, + dependency.repo_url, + dependency.ado_organization, + entry.source_url, + ) + return entry.host, entry.source, None, None if source_base: - base_host, base_path = split_source_base(source_base) - org = base_path.split("/", 1)[0] if base_path else None - return base_host, f"{base_path}/{entry.source}", org - return None, entry.source, None + source_url = f"{source_base}/{entry.source}" + dependency = DependencyReference.parse(source_url) + org = dependency.ado_organization or dependency.repo_url.split("/", 1)[0] + return dependency.host, dependency.repo_url, org, source_url + return None, entry.source, None, None @marketplace.command(help="Validate marketplace entries are resolvable") @@ -134,8 +147,10 @@ def _resolver_for(host: str | None, org: str | None) -> RefResolver: continue try: # Resolve each entry against its effective host + composed path. - host, owner_repo, org = _entry_coordinates(entry, source_base) - if host and is_azure_devops_hostname(host): + host, owner_repo, org, display_url = _entry_coordinates(entry, source_base) + if display_url is not None: + remote_label = display_url + elif host and is_azure_devops_hostname(host): remote_label = f"https://{host}/{owner_repo}" else: remote_label = f"https://{host}/{owner_repo}.git" if host else owner_repo diff --git a/src/apm_cli/marketplace/builder.py b/src/apm_cli/marketplace/builder.py index 816ce8e846..b14fdab556 100644 --- a/src/apm_cli/marketplace/builder.py +++ b/src/apm_cli/marketplace/builder.py @@ -31,7 +31,7 @@ if TYPE_CHECKING: from ..core.auth import AuthContext, HostInfo -from ..utils.github_host import default_host +from ..utils.github_host import default_host, is_azure_devops_hostname from ..utils.path_security import ensure_path_within from ..utils.yaml_io import load_yaml_str from ._io import atomic_write @@ -585,11 +585,31 @@ def _remote_source_coordinates( ) -> tuple[str | None, str, str | None, str | None]: """Return ``(host, repo_path, source_url, org_hint)`` for a remote entry.""" if entry.host: + if entry.source_url is not None: + from ..models.dependency.reference import DependencyReference + + dependency = DependencyReference.parse(entry.source_url) + return ( + dependency.host, + dependency.repo_url, + entry.source_url, + dependency.ado_organization, + ) return entry.host, entry.source, None, None source_base_parts = self._get_source_base_parts() if source_base_parts is not None: repo_path = f"{source_base_parts.path_prefix}/{entry.source}" source_url = f"{source_base_parts.source_base}/{entry.source}" + if is_azure_devops_hostname(source_base_parts.host): + from ..models.dependency.reference import DependencyReference + + dependency = DependencyReference.parse(source_url) + return ( + dependency.host, + dependency.repo_url, + source_url, + dependency.ado_organization, + ) logger.debug( "Composed marketplace source %r onto sourceBase %r as %r", entry.source, diff --git a/src/apm_cli/marketplace/yml_schema.py b/src/apm_cli/marketplace/yml_schema.py index 51405a2b91..63d748e1eb 100644 --- a/src/apm_cli/marketplace/yml_schema.py +++ b/src/apm_cli/marketplace/yml_schema.py @@ -39,7 +39,11 @@ import yaml -from ..utils.path_security import PathTraversalError, validate_path_segments +from ..utils.path_security import ( + PathTraversalError, + decode_url_path_segments, + validate_path_segments, +) from .errors import MarketplaceYmlError from .output_profiles import MARKETPLACE_OUTPUTS, known_output_names @@ -95,8 +99,11 @@ # validate_path_segments(), which rejects empty, '.', and '..' path segments. _SEGMENT_PAT = r"[A-Za-z0-9._-]+" _OWNER_REPO_PAT = rf"{_SEGMENT_PAT}/{_SEGMENT_PAT}" -_HTTPS_REPOSITORY_PAT = rf"{_SEGMENT_PAT}(?:/{_SEGMENT_PAT})+" +_URL_SEGMENT_PAT = r"(?:[A-Za-z0-9._-]|%[0-9A-Fa-f]{2})+" +_HTTPS_REPOSITORY_PAT = rf"{_URL_SEGMENT_PAT}(?:/{_URL_SEGMENT_PAT})+" _RELATIVE_SOURCE_PAT = rf"{_SEGMENT_PAT}(?:/{_SEGMENT_PAT})*" +_SOURCE_BASE_SEGMENT_PAT = _URL_SEGMENT_PAT +_SOURCE_BASE_PATH_PAT = rf"{_SOURCE_BASE_SEGMENT_PAT}(?:/{_SOURCE_BASE_SEGMENT_PAT})*" SOURCE_RE = re.compile( r"^(?:" @@ -107,7 +114,7 @@ r")$" ) LOCAL_SOURCE_RE = re.compile(r"^\./") -SOURCE_BASE_RE = re.compile(rf"^https://{_HOST_PAT}/{_RELATIVE_SOURCE_PAT}$") +SOURCE_BASE_RE = re.compile(rf"^https://{_HOST_PAT}/{_SOURCE_BASE_PATH_PAT}$") _RELATIVE_SOURCE_RE = re.compile(rf"^{_RELATIVE_SOURCE_PAT}$") # Matches ``host.tld/owner/repo`` (3 segments, first is FQDN-ish). _HOST_PREFIXED_SOURCE_RE = re.compile(rf"^({_HOST_PAT})/({_OWNER_REPO_PAT})$") @@ -359,6 +366,8 @@ class PackageEntry: # ``host.tld/owner/repo``. ``None`` means use the default host # (``GITHUB_HOST`` env or ``github.com``). host: str | None = None + # Original full HTTPS source retained for encoded output presentation. + source_url: str | None = None @dataclass(frozen=True) @@ -496,6 +505,10 @@ def validate_source_value( raise _source_error(context, source, source_base=source_base) is_local = bool(LOCAL_SOURCE_RE.match(source)) try: + if source.startswith("https://"): + parsed = _urlparse.urlparse(source) + decode_url_path_segments(parsed.path, context=context) + return # Local paths legitimately start with ``.`` (current dir) and # may have trailing-slash forms like ``./``. Allow ``.`` here. validate_path_segments(source, context=context, allow_current_dir=is_local) @@ -523,8 +536,8 @@ def parse_source_base(raw: Any) -> str | None: if not raw_source_base.startswith("https://"): raise MarketplaceYmlError("'sourceBase' must start with https://") - parsed = _urlparse.urlparse(raw_source_base) - source_base = raw_source_base.rstrip("/") + source_base = raw_source_base.removesuffix("/") + parsed = _urlparse.urlparse(source_base) if parsed.username or parsed.password or "@" in parsed.netloc: raise MarketplaceYmlError("'sourceBase' must not include userinfo") if ":" in parsed.netloc: @@ -538,19 +551,16 @@ def parse_source_base(raw: Any) -> str | None: if source_base.endswith(".git"): raise MarketplaceYmlError("'sourceBase' must not end with .git") - path = parsed.path.lstrip("/") - if path.endswith("/"): - path = path[:-1] - if not path: - raise MarketplaceYmlError("'sourceBase' must include at least one path segment") try: - validate_path_segments(path, context="sourceBase", reject_empty=True) + decode_url_path_segments(parsed.path, context="sourceBase") except PathTraversalError as exc: raise MarketplaceYmlError(str(exc)) from exc if not SOURCE_BASE_RE.match(source_base): raise MarketplaceYmlError( - "'sourceBase' path segments may only contain letters, digits, dot, underscore, or hyphen" + "'sourceBase' path segments may only contain letters, digits, dot, underscore, hyphen, " + "or percent-encoded bytes" ) + return source_base @@ -817,6 +827,7 @@ def _parse_package_entry( is_local = bool(LOCAL_SOURCE_RE.match(source)) # Detect host-prefixed source (e.g. ``host.tld/owner/repo``) and split # the host off so downstream consumers continue to see ``owner/repo``. + source_url = source if source.startswith("https://") else None host: str | None = None if not is_local: host, source = split_host_from_source(source) @@ -973,6 +984,7 @@ def _parse_package_entry( category=category, is_local=is_local, host=host, + source_url=source_url, ) diff --git a/src/apm_cli/models/dependency/identity.py b/src/apm_cli/models/dependency/identity.py index cdb36d6128..ff1bb1c3b6 100644 --- a/src/apm_cli/models/dependency/identity.py +++ b/src/apm_cli/models/dependency/identity.py @@ -27,6 +27,7 @@ # list-form ``argv`` so there is no shell-expansion vector. _ADO_PATH_SEGMENT_RE = r"^[a-zA-Z0-9._\- ]+$" _NON_ADO_PATH_SEGMENT_RE = r"^[a-zA-Z0-9._~-]+$" +_PERCENT_ENCODED_NON_ADO_PATH_SEGMENT_RE = r"^(?:[a-zA-Z0-9._~-]|%[0-9A-Fa-f]{2})+$" _RANGE_PREFIX_RE = re.compile(r"^(>=|<=|>|<|\^|~|=)") _DEFAULT_SCHEME_PORTS: dict[str, int] = {"https": 443, "http": 80, "ssh": 22} diff --git a/src/apm_cli/models/dependency/reference.py b/src/apm_cli/models/dependency/reference.py index f863297bbd..265c86bdc3 100644 --- a/src/apm_cli/models/dependency/reference.py +++ b/src/apm_cli/models/dependency/reference.py @@ -22,12 +22,14 @@ ) from ...utils.path_security import ( PathTraversalError, + parse_url_path_segments, validate_path_segments, ) from ..validation import InvalidVirtualPackageExtensionError from .identity import ( _DEFAULT_SCHEME_PORTS, _NON_ADO_PATH_SEGMENT_RE, + _PERCENT_ENCODED_NON_ADO_PATH_SEGMENT_RE, InvalidSemverRangeError, _is_valid_registry_semver_range, _looks_like_invalid_semver_range, @@ -1492,17 +1494,26 @@ def _validate_url_repo_path(cls, parsed_url) -> tuple[str, str | None]: if not is_supported_git_host(hostname): raise ValueError(unsupported_host_error(hostname or parsed_url.netloc)) - path = parsed_url.path.strip("/") - if not path: - raise ValueError("Repository path cannot be empty") - - if path.endswith(".git"): - path = path[:-4] - - path_parts = [urllib.parse.unquote(p) for p in path.split("/")] + try: + raw_path_parts, decoded_path_parts = parse_url_path_segments( + parsed_url.path, + context="repository URL path", + ) + except PathTraversalError as exc: + raise ValueError(str(exc)) from exc + path_parts = list(decoded_path_parts) + presentation_path_parts = list(raw_path_parts) + if path_parts[-1].endswith(".git"): + path_parts[-1] = path_parts[-1][:-4] + if presentation_path_parts[-1].endswith(".git"): + presentation_path_parts[-1] = presentation_path_parts[-1][:-4] + path = "/".join(path_parts) if "_git" in path_parts: git_idx = path_parts.index("_git") path_parts = path_parts[:git_idx] + path_parts[git_idx + 1 :] + presentation_path_parts = ( + presentation_path_parts[:git_idx] + presentation_path_parts[git_idx + 1 :] + ) is_ado_host = is_azure_devops_hostname(hostname) @@ -1570,6 +1581,7 @@ def _validate_url_repo_path(cls, parsed_url) -> tuple[str, str | None]: # :meth:`_extract_artifactory_prefix`. if is_artifactory_path(path_parts): path_parts = path_parts[2:] + presentation_path_parts = presentation_path_parts[2:] for pp in path_parts: if any(pp.endswith(ext) for ext in cls.VIRTUAL_FILE_EXTENSIONS): raise ValueError( @@ -1577,17 +1589,22 @@ def _validate_url_repo_path(cls, parsed_url) -> tuple[str, str | None]: f"Use the dict format with 'path:' for virtual packages in HTTPS URLs" ) - allowed_pattern = _path_segment_pattern(is_ado_host) + validation_parts = path_parts if is_ado_host else presentation_path_parts + allowed_pattern = ( + _path_segment_pattern(is_ado_host) + if is_ado_host + else _PERCENT_ENCODED_NON_ADO_PATH_SEGMENT_RE + ) validate_path_segments( - "/".join(path_parts), + "/".join(validation_parts), context="repository URL path", reject_empty=True, ) - for part in path_parts: + for part in validation_parts: if not re.match(allowed_pattern, part): raise ValueError(f"Invalid repository path component: {part}") - return "/".join(path_parts), url_virtual_path + return "/".join(path_parts if is_ado_host else presentation_path_parts), url_virtual_path @classmethod def _parse_standard_url( @@ -1661,7 +1678,7 @@ def _parse_standard_url( return host, port, repo_url, reference, alias, effective_is_virtual, effective_virtual_path @classmethod - def _validate_final_repo_fields(cls, host, repo_url): + def _validate_final_repo_fields(cls, host, repo_url, *, allow_percent_encoded: bool = False): """Validate a repository path and return its ADO coordinates when applicable.""" is_ado_final = host and is_azure_devops_hostname(host) if is_ado_final: @@ -1676,7 +1693,12 @@ def _validate_final_repo_fields(cls, host, repo_url): segments = repo_url.split("/") if len(segments) < 2: raise ValueError(f"Invalid repository format: {repo_url}. Expected 'user/repo'") - if not all(re.match(_NON_ADO_PATH_SEGMENT_RE, s) for s in segments): + allowed_pattern = ( + _PERCENT_ENCODED_NON_ADO_PATH_SEGMENT_RE + if allow_percent_encoded + else _NON_ADO_PATH_SEGMENT_RE + ) + if not all(re.match(allowed_pattern, s) for s in segments): raise ValueError(f"Invalid repository format: {repo_url}. Contains invalid characters") validate_path_segments(repo_url, context="repository path") for seg in segments: @@ -1745,8 +1767,6 @@ def parse(cls, dependency_str: str) -> "DependencyReference": if not dependency_str.strip(): raise ValueError("Empty dependency string") - dependency_str = urllib.parse.unquote(dependency_str) - if any(ord(c) < 32 for c in dependency_str): raise ValueError("Dependency string contains invalid control characters") @@ -1814,7 +1834,13 @@ def parse(cls, dependency_str: str) -> "DependencyReference": # Phase 3: full validation (all hosts) + ADO field extraction. # canonical_ado_coordinates is for consumers with validated input only. - ado_organization, ado_project, ado_repo = cls._validate_final_repo_fields(host, repo_url) + ado_organization, ado_project, ado_repo = cls._validate_final_repo_fields( + host, + repo_url, + allow_percent_encoded=dependency_str.strip() + .lower() + .startswith(("https://", "http://")), + ) if alias and not re.match(r"^[a-zA-Z0-9._-]+$", alias): raise ValueError( diff --git a/src/apm_cli/utils/path_security.py b/src/apm_cli/utils/path_security.py index 1b4ebb2024..fae3e61586 100644 --- a/src/apm_cli/utils/path_security.py +++ b/src/apm_cli/utils/path_security.py @@ -29,6 +29,97 @@ class PathTraversalError(ValueError): """Raised when a computed path escapes its expected base directory.""" +def decode_url_path_segments( + raw_path: str, + *, + context: str = "URL path", +) -> tuple[str, ...]: + """Strictly decode safe URL path segments without changing URL structure. + + Callers must parse a URL before passing its ``path`` component here. The + returned values are decoded identity material; callers retain ``raw_path`` + when they need to render or transport the original encoded URL. + + A literal ``/`` separates segments. Percent escapes are validated before + decoding, decoded as strict UTF-8, and may not introduce separators, + traversal names, empty values, or another percent escape. Rejecting a + residual percent escape prevents multi-encoded traversal and separator + payloads from being accepted after a bounded number of decode passes. + """ + _, decoded_segments = parse_url_path_segments(raw_path, context=context) + return decoded_segments + + +def parse_url_path_segments( + raw_path: str, + *, + context: str = "URL path", +) -> tuple[tuple[str, ...], tuple[str, ...]]: + """Return validated raw and decoded URL path segments. + + The raw segments retain their encoded presentation for URL transport. The + decoded segments are suitable only for provider identities that require + decoded values, such as Azure DevOps organization and project names. + """ + if not isinstance(raw_path, str): + raise PathTraversalError(f"Invalid {context}: URL path must be a string") + + path = raw_path[1:] if raw_path.startswith("/") else raw_path + if not path: + raise PathTraversalError(f"Invalid {context}: path segments must not be empty") + + raw_segments = tuple(path.split("/")) + decoded_segments: list[str] = [] + for raw_segment in raw_segments: + if not raw_segment: + raise PathTraversalError(f"Invalid {context}: path segments must not be empty") + if "\\" in raw_segment: + raise PathTraversalError( + f"Invalid {context}: path segments must not contain path separators" + ) + index = 0 + while index < len(raw_segment): + character = raw_segment[index] + if ord(character) < 0x21 or ord(character) > 0x7E: + raise PathTraversalError( + f"Invalid {context}: path segments must use percent-encoded UTF-8 bytes" + ) + if character == "%": + if ( + index + 2 >= len(raw_segment) + or raw_segment[index + 1] not in "0123456789abcdefABCDEF" + or raw_segment[index + 2] not in "0123456789abcdefABCDEF" + ): + raise PathTraversalError(f"Invalid {context}: malformed percent-encoding") + index += 3 + else: + index += 1 + try: + decoded = _up.unquote_to_bytes(raw_segment).decode("utf-8") + except UnicodeDecodeError as exc: + raise PathTraversalError( + f"Invalid {context}: percent-encoding must be valid UTF-8" + ) from exc + if not decoded: + raise PathTraversalError(f"Invalid {context}: path segments must not be empty") + if any(ord(character) < 0x20 or ord(character) == 0x7F for character in decoded): + raise PathTraversalError( + f"Invalid {context}: percent-encoding must not decode to control characters" + ) + if "%" in decoded: + raise PathTraversalError(f"Invalid {context}: residual percent-encoding is not allowed") + if "/" in decoded or "\\" in decoded: + raise PathTraversalError( + f"Invalid {context}: percent-encoding must not decode to a path separator" + ) + if decoded in {".", ".."}: + raise PathTraversalError( + f"Invalid {context}: segment '{raw_segment}' is a traversal sequence" + ) + decoded_segments.append(decoded) + return raw_segments, tuple(decoded_segments) + + def validate_path_segments( path_str: str, *, diff --git a/tests/integration/test_architecture_authorities.py b/tests/integration/test_architecture_authorities.py index 8557fdf2db..1a71e47dd7 100644 --- a/tests/integration/test_architecture_authorities.py +++ b/tests/integration/test_architecture_authorities.py @@ -1434,6 +1434,115 @@ def test_packed_marketplace_source_parsing_has_single_owner() -> None: assert "Packed marketplace sources must use DependencyReference.parse_from_dict" in guard +def test_marketplace_check_source_coordinates_use_single_parser() -> None: + """Marketplace check source coordinates must use DependencyReference.""" + root = Path(__file__).parents[2] + check = (root / "src/apm_cli/commands/marketplace/check.py").read_text(encoding="utf-8") + guard = (root / "scripts/lint-architecture-boundaries.sh").read_text(encoding="utf-8") + + helper = check.split("def _entry_coordinates(", maxsplit=1)[1].split("\ndef ", maxsplit=1)[0] + assert "DependencyReference.parse(entry.source_url)" in helper + assert "DependencyReference.parse(source_url)" in helper + assert "Marketplace check source coordinates must use DependencyReference parsing" in guard + + +def test_marketplace_check_coordinate_guard_rejects_parallel_parser(tmp_path: Path) -> None: + """AC10 rejects sourceBase parsing that bypasses DependencyReference.""" + root = Path(__file__).parents[2] + sandbox = tmp_path / "repo" + shutil.copytree( + root, + sandbox, + ignore=shutil.ignore_patterns( + ".git", + ".venv", + ".pytest_cache", + "__pycache__", + "build", + "dist", + "node_modules", + ), + ) + check_path = sandbox / "src/apm_cli/commands/marketplace/check.py" + check_path.write_text( + check_path.read_text(encoding="utf-8").replace( + "dependency = DependencyReference.parse(source_url)", + "dependency = DependencyReference(repo_url=entry.source)", + 1, + ), + encoding="utf-8", + ) + + result = subprocess.run( + ("bash", "scripts/lint-architecture-boundaries.sh"), + cwd=sandbox, + capture_output=True, + text=True, + check=False, + timeout=300, + ) + + assert result.returncode == 1 + assert "Marketplace check source coordinates must use DependencyReference parsing" in ( + result.stdout + ) + + +def test_strict_url_path_decoding_has_single_owner() -> None: + """AC10a keeps URL structure parsing separate from percent decoding.""" + root = Path(__file__).parents[2] + owner = (root / "src/apm_cli/utils/path_security.py").read_text(encoding="utf-8") + schema = (root / "src/apm_cli/marketplace/yml_schema.py").read_text(encoding="utf-8") + reference = (root / "src/apm_cli/models/dependency/reference.py").read_text(encoding="utf-8") + guard = (root / "scripts/lint-architecture-boundaries.sh").read_text(encoding="utf-8") + + assert "def parse_url_path_segments(" in owner + assert "decode_url_path_segments(parsed.path, context=context)" in schema + assert "parse_url_path_segments(" in reference + assert "dependency_str = urllib.parse.unquote(dependency_str)" not in reference + assert "Strict percent-encoded URL paths must use path_security parsing" in guard + + +def test_strict_url_path_decoder_guard_rejects_parallel_decoder(tmp_path: Path) -> None: + """AC10a catches a mutation that reintroduces a local URL decoder.""" + root = Path(__file__).parents[2] + sandbox = tmp_path / "repo" + shutil.copytree( + root, + sandbox, + ignore=shutil.ignore_patterns( + ".git", + ".venv", + ".pytest_cache", + "__pycache__", + "build", + "dist", + "node_modules", + ), + ) + schema_path = sandbox / "src/apm_cli/marketplace/yml_schema.py" + schema_path.write_text( + schema_path.read_text(encoding="utf-8").replace( + "decode_url_path_segments(parsed.path, context=context)", + "_urlparse.unquote(parsed.path)", + 1, + ), + encoding="utf-8", + ) + + result = subprocess.run( + ("bash", "scripts/lint-architecture-boundaries.sh"), + cwd=sandbox, + capture_output=True, + text=True, + check=False, + timeout=300, + ) + + assert result.returncode == 1 + assert "Strict percent-encoded URL paths must use path_security parsing" in (result.stdout) + + def test_packed_marketplace_source_owner_guard_rejects_parallel_parser( tmp_path: Path, ) -> None: diff --git a/tests/integration/test_marketplace_nested_https_source_lifecycle.py b/tests/integration/test_marketplace_nested_https_source_lifecycle.py index a77b9cdf34..1a348e163b 100644 --- a/tests/integration/test_marketplace_nested_https_source_lifecycle.py +++ b/tests/integration/test_marketplace_nested_https_source_lifecycle.py @@ -8,6 +8,7 @@ import pytest +from apm_cli.utils.yaml_io import load_yaml from tests.utils.apm_lifecycle_runner import ApmLifecycleRunner from tests.utils.artifact_snapshot import ArtifactSnapshot, assert_unchanged from tests.utils.isolated_apm_environment import IsolatedApmEnvironment @@ -23,11 +24,21 @@ _NESTED_SOURCE = "https://git.example.invalid/group/subgroup/marketplace-package.git" _UNSAFE_NESTED_SOURCE = "https://git.example.invalid/group/%2e%2e/marketplace-package.git" +_ENCODED_ADO_BASE = "https://dev.azure.com/contoso/My%20Projects/_git" +_ENCODED_ADO_SOURCE = f"{_ENCODED_ADO_BASE}/agent-skills" +_SHA = "a" * 40 -def _write_marketplace_config(project: Path, source: str) -> None: +def _write_marketplace_config( + project: Path, + source: str, + *, + source_base: str | None = None, + ref: str = "v1.0.0", +) -> None: """Write a minimal marketplace manifest with one remote package.""" project.mkdir() + source_base_line = f" sourceBase: {source_base}\n" if source_base else "" (project / "apm.yml").write_text( f"""\ name: nested-marketplace @@ -36,10 +47,10 @@ def _write_marketplace_config(project: Path, source: str) -> None: marketplace: owner: name: Test Owner - packages: +{source_base_line} packages: - name: nested-package source: {source} - ref: v1.0.0 + ref: {ref} """, encoding="utf-8", ) @@ -103,3 +114,42 @@ def test_marketplace_check_offline_rejects_unsafe_nested_https_source_before_ref assert "marketplace config error" in diagnostics assert "No cached refs (offline)" not in diagnostics assert_unchanged(before, ArtifactSnapshot.capture(project)) + + +@pytest.mark.parametrize( + ("source_base", "source", "expected_url"), + [ + (_ENCODED_ADO_BASE, "agent-skills", _ENCODED_ADO_SOURCE), + (None, _ENCODED_ADO_SOURCE, _ENCODED_ADO_SOURCE), + ], +) +def test_pack_offline_dry_run_preserves_encoded_ado_source_without_writes( + tmp_path: Path, + apm_binary_path: Path, + source_base: str | None, + source: str, + expected_url: str, +) -> None: + """Both source forms pack offline without changing project bytes.""" + isolated = IsolatedApmEnvironment.create(tmp_path / "isolated", base_env=dict(os.environ)) + project = isolated.work_root / "encoded-ado-marketplace" + _write_marketplace_config(project, source, source_base=source_base, ref=_SHA) + before = ArtifactSnapshot.capture(project) + runner = ApmLifecycleRunner((str(apm_binary_path),)) + + (result,) = runner.run_sequence( + (("pack", "--offline", "--dry-run", "--verbose"),), + expected_returncodes=(0,), + scenario_id="marketplace-encoded-ado-pack", + cwd=project, + env=isolated.subprocess_env(overrides={"COLUMNS": "240"}), + ) + + assert result.returncode == 0 + manifest = load_yaml(project / "apm.yml") + marketplace = manifest["marketplace"] + if source_base is not None: + assert urlparse(marketplace["sourceBase"]).path == urlparse(_ENCODED_ADO_BASE).path + else: + assert urlparse(marketplace["packages"][0]["source"]).path == urlparse(expected_url).path + assert_unchanged(before, ArtifactSnapshot.capture(project)) diff --git a/tests/unit/commands/test_marketplace_check.py b/tests/unit/commands/test_marketplace_check.py index b9e5b29e0d..0a7a8e00d4 100644 --- a/tests/unit/commands/test_marketplace_check.py +++ b/tests/unit/commands/test_marketplace_check.py @@ -5,7 +5,8 @@ import textwrap from pathlib import Path # noqa: F401 from types import SimpleNamespace -from unittest.mock import ANY, MagicMock, patch +from unittest.mock import ANY, MagicMock, call, patch +from urllib.parse import urlparse import pytest from click.testing import CliRunner @@ -550,6 +551,127 @@ def test_heads_prefix_not_confused_with_tags(self, MockResolver, runner, tmp_pat class TestCheckPerHostResolution: + @patch( + "apm_cli.commands.marketplace.check.resolve_auth_for_host", + return_value=SimpleNamespace( + token="ado-pat", + auth_scheme="bearer", + host_info=SimpleNamespace(kind="azure_devops"), + ), + ) + @patch("apm_cli.commands.marketplace.check.RefResolver") + def test_encoded_ado_source_base_and_https_source_share_decoded_identity( + self, MockResolver, mock_token, runner, tmp_path, monkeypatch + ): + """Use the same decoded ADO identity and org hint for both source forms.""" + monkeypatch.chdir(tmp_path) + encoded_source = "https://dev.azure.com/cont%6Fso/My%20Projects/_git/agent-skills" + (tmp_path / "marketplace.yml").write_text( + textwrap.dedent(f"""\ + name: encoded-ado + description: Encoded Azure DevOps sources + version: 1.0.0 + owner: + name: Contoso + sourceBase: https://dev.azure.com/cont%6Fso/My%20Projects/_git + packages: + - name: from-base + source: agent-skills + version: "^1.0.0" + - name: direct + source: {encoded_source} + version: "^1.0.0" + """), + encoding="utf-8", + ) + mock_inst = MockResolver.return_value + mock_inst.list_remote_refs.return_value = _REFS_GOOD + mock_inst.close = MagicMock() + + result = runner.invoke(marketplace, ["check", "--verbose"]) + + assert result.exit_code == 0 + displayed_paths = [ + urlparse(token.strip("(),.;'\"")).path + for token in result.output.split() + if token.startswith("https://") + ] + assert displayed_paths == [ + "/cont%6Fso/My%20Projects/_git/agent-skills", + "/cont%6Fso/My%20Projects/_git/agent-skills", + ] + mock_token.assert_called_once_with( + "dev.azure.com", offline=False, org="contoso", auth_resolver=ANY + ) + mock_inst.list_remote_refs.assert_has_calls( + [ + call("contoso/My Projects/agent-skills"), + call("contoso/My Projects/agent-skills"), + ] + ) + + @patch( + "apm_cli.commands.marketplace.check.resolve_auth_for_host", + return_value=SimpleNamespace( + token="glpat-xyz", + auth_scheme="basic", + host_info=SimpleNamespace(kind="gitlab"), + ), + ) + @patch("apm_cli.commands.marketplace.check.RefResolver") + def test_encoded_gitlab_source_base_and_https_source_keep_transport_path( + self, MockResolver, mock_token, runner, tmp_path, monkeypatch + ): + """Keep encoded non-ADO paths intact through check and git resolution.""" + monkeypatch.chdir(tmp_path) + encoded_source = "https://gitlab.example.com/team/My%20Group/direct-tool" + (tmp_path / "marketplace.yml").write_text( + textwrap.dedent(f"""\ + name: encoded-gitlab + description: Encoded GitLab sources + version: 1.0.0 + owner: + name: Team + sourceBase: https://gitlab.example.com/team/My%20Group + packages: + - name: from-base + source: agent-skills + version: "^1.0.0" + - name: direct + source: {encoded_source} + version: "^1.0.0" + """), + encoding="utf-8", + ) + mock_inst = MockResolver.return_value + mock_inst.list_remote_refs.return_value = _REFS_GOOD + mock_inst.close = MagicMock() + + result = runner.invoke(marketplace, ["check", "--verbose"]) + + assert result.exit_code == 0 + displayed_paths = [ + urlparse(token.strip("(),.;'\"")).path + for token in result.output.split() + if token.startswith("https://") + ] + assert displayed_paths == [ + "/team/My%20Group/agent-skills", + "/team/My%20Group/direct-tool", + ] + mock_token.assert_has_calls( + [ + call("gitlab.example.com", offline=False, org="team", auth_resolver=ANY), + call("gitlab.example.com", offline=False, org=None, auth_resolver=ANY), + ] + ) + mock_inst.list_remote_refs.assert_has_calls( + [ + call("team/My%20Group/agent-skills"), + call("team/My%20Group/direct-tool"), + ] + ) + @patch( "apm_cli.commands.marketplace.check.resolve_auth_for_host", return_value=SimpleNamespace( diff --git a/tests/unit/marketplace/test_marketplace_source_base.py b/tests/unit/marketplace/test_marketplace_source_base.py index 07ebbeef70..e7b43c602c 100644 --- a/tests/unit/marketplace/test_marketplace_source_base.py +++ b/tests/unit/marketplace/test_marketplace_source_base.py @@ -75,12 +75,40 @@ def _load_config(tmp_path: Path, source_base: str | None, packages: str) -> Mark class TestSourceBaseSchema: + @pytest.mark.parametrize( + ("source_base", "normalized"), + [ + ( + "https://gitlab.example.com/platform/marketplaces/", + "https://gitlab.example.com/platform/marketplaces", + ), + ( + "https://dev.azure.com/contoso/platform/_git/", + "https://dev.azure.com/contoso/platform/_git", + ), + ], + ) + def test_normalizes_trailing_slash_before_validating_source_base( + self, tmp_path: Path, source_base: str, normalized: str + ) -> None: + config = _load_config( + tmp_path, + source_base, + f""" + - name: tool + source: tool + ref: {_SHA} + """, + ) + + assert config.source_base == normalized + def test_accepts_single_and_nested_relative_sources_when_source_base_is_set( self, tmp_path: Path ) -> None: config = _load_config( tmp_path, - "https://gitlab.example.com/platform/marketplaces/", + "https://gitlab.example.com/platform/marketplaces", f""" - name: single source: single-tool @@ -97,6 +125,20 @@ def test_accepts_single_and_nested_relative_sources_when_source_base_is_set( assert config.packages[1].source == "team/tools/nested-tool" assert config.packages[1].host is None + def test_accepts_percent_encoded_space_in_ado_source_base(self, tmp_path: Path) -> None: + config = _load_config( + tmp_path, + "https://dev.azure.com/contoso/My%20Projects/_git", + f""" + - name: ado-tool + source: agent-skills + ref: {_SHA} + """, + ) + + assert config.source_base == "https://dev.azure.com/contoso/My%20Projects/_git" + assert config.packages[0].source == "agent-skills" + def test_absent_source_base_keeps_owner_repo_source_unchanged(self, tmp_path: Path) -> None: config = _load_config( tmp_path, @@ -125,6 +167,12 @@ def test_absent_source_base_keeps_owner_repo_source_unchanged(self, tmp_path: Pa ("https://gitlab.example.com/group//repo", "empty"), ("https://gitlab.example.com/group//", "empty"), ("https://gitlab.example.com/group/../repo", "traversal"), + ("https://gitlab.example.com/group/%", "malformed"), + ("https://gitlab.example.com/group/%FF", "UTF-8"), + ("https://gitlab.example.com/group/%2Frepo", "path separator"), + ("https://gitlab.example.com/group/%252E%252E", "residual"), + ("https://dev.azure.com/contoso/%2e%2e/_git", "traversal"), + ("https://dev.azure.com/contoso/My%2FProjects/_git", "path separator"), ], ) def test_rejects_source_base_security_guard_violations( @@ -153,6 +201,18 @@ def test_rejects_single_segment_source_without_source_base(self, tmp_path: Path) """, ) + def test_rejects_literal_space_relative_source_with_source_base(self, tmp_path: Path) -> None: + with pytest.raises(MarketplaceYmlError, match="must be one of"): + _load_config( + tmp_path, + "https://gitlab.example.com/platform/marketplaces", + f""" + - name: tool + source: team/My Group + ref: {_SHA} + """, + ) + @pytest.mark.parametrize( "source", [ @@ -317,7 +377,7 @@ def test_ado_shaped_source_base_composes_relative_repo(self, tmp_path: Path) -> builder = MarketplaceBuilder.from_config(config, tmp_path, BuildOptions(offline=True)) resolved = builder._resolve_entry(config.packages[0]) - assert resolved.source_repo == "contoso/platform/_git/agent-skills" + assert resolved.source_repo == "contoso/platform/agent-skills" assert resolved.host == "dev.azure.com" parsed = urlparse(resolved.source_url or "") assert parsed.scheme == "https" @@ -374,6 +434,118 @@ def test_source_base_resolution_uses_base_org_for_auth_context(self, tmp_path: P assert resolved.source_repo == "contoso/marketplaces/tool" assert auth.calls == [("github.com", "contoso")] + def test_encoded_ado_sources_preserve_urls_and_share_decoded_identity( + self, tmp_path: Path + ) -> None: + (tmp_path / "base").mkdir() + (tmp_path / "direct").mkdir() + source_base = _load_config( + tmp_path / "base", + "https://dev.azure.com/contoso/My%20Projects/_git", + f""" + - name: from-base + source: agent-skills + ref: {_SHA} + """, + ) + direct_source = _load_config( + tmp_path / "direct", + None, + f""" + - name: direct + source: https://dev.azure.com/contoso/My%20Projects/_git/agent-skills + ref: {_SHA} + """, + ) + + base_builder = MarketplaceBuilder.from_config( + source_base, tmp_path / "base", BuildOptions(offline=True) + ) + direct_builder = MarketplaceBuilder.from_config( + direct_source, tmp_path / "direct", BuildOptions(offline=True) + ) + base_resolved = base_builder._resolve_entry(source_base.packages[0]) + direct_resolved = direct_builder._resolve_entry(direct_source.packages[0]) + + assert base_resolved.source_repo == "contoso/My Projects/agent-skills" + assert direct_resolved.source_repo == base_resolved.source_repo + assert urlparse(base_resolved.source_url or "").path == ( + "/contoso/My%20Projects/_git/agent-skills" + ) + assert urlparse(direct_resolved.source_url or "").path == ( + "/contoso/My%20Projects/_git/agent-skills" + ) + for builder, resolved in ( + (base_builder, base_resolved), + (direct_builder, direct_resolved), + ): + source = builder.compose_marketplace_json([resolved])["plugins"][0]["source"] + assert urlparse(source["url"]).path == "/contoso/My%20Projects/_git/agent-skills" + + def test_encoded_generic_source_base_preserves_transport_path(self, tmp_path: Path) -> None: + config = _load_config( + tmp_path, + "https://gitlab.example.com/team/My%20Group", + f""" + - name: encoded-gitlab + source: agent-skills + ref: {_SHA} + """, + ) + builder = MarketplaceBuilder.from_config(config, tmp_path, BuildOptions(offline=True)) + + resolved = builder._resolve_entry(config.packages[0]) + + assert resolved.source_repo == "team/My%20Group/agent-skills" + assert urlparse(resolved.source_url or "").path == "/team/My%20Group/agent-skills" + source = builder.compose_marketplace_json([resolved])["plugins"][0]["source"] + assert urlparse(source["url"]).path == "/team/My%20Group/agent-skills" + + def test_encoded_ado_organization_is_used_for_auth_lookup(self, tmp_path: Path) -> None: + config = _load_config( + tmp_path, + "https://dev.azure.com/cont%6Fso/platform/_git", + f""" + - name: ado-tool + source: agent-skills + ref: {_SHA} + """, + ) + auth = _RecordingAuthResolver() + builder = MarketplaceBuilder.from_config( + config, + tmp_path, + BuildOptions(offline=False), + auth_resolver=auth, + ) + + builder._resolve_entry(config.packages[0]) + + assert auth.calls == [("dev.azure.com", "contoso")] + + @pytest.mark.parametrize( + "source", + [ + "https://dev.azure.com/contoso/%/_git/repo", + "https://dev.azure.com/contoso/%FF/_git/repo", + "https://dev.azure.com/contoso/%2F/_git/repo", + "https://dev.azure.com/contoso/%252E%252E/_git/repo", + ], + ) + def test_unsafe_full_https_source_is_rejected_before_resolution( + self, tmp_path: Path, source: str + ) -> None: + with pytest.raises(MarketplaceYmlError): + _load_config( + tmp_path, + None, + f""" + - name: unsafe + source: {source} + ref: {_SHA} + """, + ) + class TestSourceBaseEditor: def test_add_plugin_entry_accepts_relative_source_when_source_base_is_set( diff --git a/tests/unit/marketplace/test_parser.py b/tests/unit/marketplace/test_parser.py index dd0c957835..2467c2f116 100644 --- a/tests/unit/marketplace/test_parser.py +++ b/tests/unit/marketplace/test_parser.py @@ -118,3 +118,29 @@ def test_https_ado_url_classified_as_git() -> None: # Use urlsplit().hostname for exact host match (CodeQL: avoid substring sanitization). assert urlsplit(url).hostname == "dev.azure.com" assert host == "dev.azure.com" + + +def test_https_ado_url_preserves_encoded_path_presentation() -> None: + url, kind, host = _parse_marketplace_source( + "https://dev.azure.com/contoso/My%20Projects/_git/agent-forge", + host_flag=None, + ) + + parsed = urlsplit(url) + assert kind == "git" + assert host == "dev.azure.com" + assert parsed.path == "/contoso/My%20Projects/_git/agent-forge" + + +@pytest.mark.parametrize( + "raw", + [ + "https://dev.azure.com/contoso/%/_git/repo", + "https://dev.azure.com/contoso/%FF/_git/repo", + "https://dev.azure.com/contoso/%2F/_git/repo", + "https://dev.azure.com/contoso/%252E%252E/_git/repo", + ], +) +def test_https_ado_url_rejects_unsafe_encoded_path(raw: str) -> None: + with pytest.raises(ValueError): + _parse_marketplace_source(raw, host_flag=None) diff --git a/tests/unit/test_path_security.py b/tests/unit/test_path_security.py index 11d692d913..c507254d96 100644 --- a/tests/unit/test_path_security.py +++ b/tests/unit/test_path_security.py @@ -8,13 +8,16 @@ import shutil # noqa: F401 from pathlib import Path +from urllib.parse import urlparse import pytest from apm_cli.models.dependency import DependencyReference from apm_cli.utils.path_security import ( PathTraversalError, + decode_url_path_segments, ensure_path_within, + parse_url_path_segments, safe_rmtree, validate_path_segments, ) @@ -226,6 +229,57 @@ def test_empty_string_with_reject_empty(self): validate_path_segments("", reject_empty=True) +# --------------------------------------------------------------------------- +# Strict URL-path decoding +# --------------------------------------------------------------------------- + + +class TestDecodeUrlPathSegments: + """Lock strict URL decoding to one canonical safety boundary.""" + + @pytest.mark.parametrize( + ("raw_path", "expected"), + [ + ( + "contoso/My%20Projects/_git/agent-skills", + ("contoso", "My Projects", "_git", "agent-skills"), + ), + ( + "/contoso/My%20Projects/_git/agent-skills", + ("contoso", "My Projects", "_git", "agent-skills"), + ), + ], + ) + def test_decodes_safe_encoded_segments(self, raw_path: str, expected: tuple[str, ...]) -> None: + assert decode_url_path_segments(raw_path) == expected + + def test_retains_encoded_segments_for_url_transport(self) -> None: + raw, decoded = parse_url_path_segments("team/My%20Group") + + assert raw == ("team", "My%20Group") + assert decoded == ("team", "My Group") + + @pytest.mark.parametrize( + "raw_path", + [ + "contoso/%/repo", + "contoso/%2/repo", + "contoso/%GG/repo", + "contoso/%FF/repo", + "contoso/%2F/repo", + r"contoso/%5C/repo", + "contoso/%00/repo", + "contoso//repo", + "contoso/%252E%252E/repo", + "contoso/%252F/repo", + "contoso/%2E%2E/repo", + ], + ) + def test_rejects_unsafe_or_recursive_encoded_segments(self, raw_path: str) -> None: + with pytest.raises(PathTraversalError): + decode_url_path_segments(raw_path) + + # --------------------------------------------------------------------------- # DependencyReference parse-time traversal rejection # --------------------------------------------------------------------------- @@ -285,6 +339,42 @@ def test_parse_accepts_normal_virtual_package(self): dep = DependencyReference.parse("owner/repo/prompts/my-file.prompt.md") assert dep.is_virtual is True + def test_full_https_ado_decodes_identity_after_structural_parse(self) -> None: + dep = DependencyReference.parse( + "https://dev.azure.com/contoso/My%20Projects/_git/agent-skills" + ) + + assert dep.repo_url == "contoso/My Projects/agent-skills" + assert dep.ado_organization == "contoso" + assert dep.ado_project == "My Projects" + assert dep.ado_repo == "agent-skills" + parsed = urlparse(dep.to_github_url()) + assert parsed.path == "/contoso/My%20Projects/_git/agent-skills" + + def test_full_https_non_ado_keeps_encoded_transport_path(self) -> None: + dep = DependencyReference.parse("https://gitlab.example.com/team/My%20Group") + + assert dep.repo_url == "team/My%20Group" + parsed = urlparse(dep.to_github_url()) + assert parsed.path == "/team/My%20Group" + + @pytest.mark.parametrize( + "url", + [ + "https://dev.azure.com/contoso/%/_git/repo", + "https://dev.azure.com/contoso/%FF/_git/repo", + "https://dev.azure.com/contoso/%2F/_git/repo", + "https://dev.azure.com/contoso/%252E%252E/_git/repo", + "https://gitlab.example.com/team/%", + "https://gitlab.example.com/team/%FF", + "https://gitlab.example.com/team/%2F", + "https://gitlab.example.com/team/%252E%252E", + ], + ) + def test_full_https_unsafe_encoded_path_fails_during_parse(self, url: str) -> None: + with pytest.raises(ValueError): + DependencyReference.parse(url) + # --- SSH URL traversal rejection --- def test_ssh_parse_rejects_dotdot_in_repo(self):