This is an automated email from the ASF dual-hosted git repository. arm pushed a commit to branch safe_path in repository https://gitbox.apache.org/repos/asf/tooling-trusted-releases.git
commit 1206ad31298c18ffb227924112b7b33f04c1ea0f Author: Alastair McFarlane <[email protected]> AuthorDate: Fri Mar 20 13:13:57 2026 +0000 #915 - Add safe path type --- atr/models/api.py | 4 ++-- atr/models/safe.py | 53 +++++++++++++++++++++++++++++++++++++++++ atr/post/announce.py | 2 +- atr/post/upload.py | 12 ++++++---- atr/shared/announce.py | 24 +++---------------- atr/shared/upload.py | 5 ++-- atr/storage/writers/announce.py | 8 ++++--- atr/storage/writers/release.py | 12 +++++----- atr/tasks/svn.py | 6 +++-- 9 files changed, 84 insertions(+), 42 deletions(-) diff --git a/atr/models/api.py b/atr/models/api.py index feb489bf..3c651b8f 100644 --- a/atr/models/api.py +++ b/atr/models/api.py @@ -360,7 +360,7 @@ class PublisherReleaseAnnounceArgs(schema.Strict): revision: safe.RevisionNumber = schema.example("00005") email_to: str = schema.example("[email protected]") body: str = schema.example("The Apache Example team is pleased to announce the release of Example 1.0.0...") - path_suffix: str = schema.example("example/1.0.0") + path_suffix: safe.OptionalRelPath = schema.example("example/1.0.0") class PublisherReleaseAnnounceResults(schema.Strict): @@ -399,7 +399,7 @@ class ReleaseAnnounceArgs(schema.Strict): revision: safe.RevisionNumber = schema.example("00005") email_to: str = schema.example("[email protected]") body: str = schema.example("The Apache Example team is pleased to announce the release of Example 1.0.0...") - path_suffix: str = schema.example("example/1.0.0") + path_suffix: safe.OptionalRelPath = schema.example("example/1.0.0") class ReleaseAnnounceResults(schema.Strict): diff --git a/atr/models/safe.py b/atr/models/safe.py index a69750dd..bd6d392b 100644 --- a/atr/models/safe.py +++ b/atr/models/safe.py @@ -17,6 +17,7 @@ from __future__ import annotations +import pathlib import string import unicodedata from typing import Annotated, Any, Final @@ -25,6 +26,7 @@ import pydantic _ALPHANUM: Final = frozenset(string.ascii_letters + string.digits + "-") _NUMERIC: Final = frozenset(string.digits) +_PATH_CHARS: Final = frozenset(string.ascii_letters + string.digits + "-._+~/()") | frozenset("/") _VERSION_CHARS: Final = _ALPHANUM | frozenset(".+") @@ -114,6 +116,42 @@ class ReleaseKey(Alphanumeric): return _VERSION_CHARS +class RelPath(SafeType): + """A relative file path that has been validated for safety.""" + + @classmethod + def _valid_chars(cls) -> frozenset[str]: + return _PATH_CHARS + + def _additional_validations(self, value: str) -> None: + posix_path = pathlib.PurePosixPath(value) + windows_path = pathlib.PureWindowsPath(value) + if posix_path.is_absolute() or windows_path.is_absolute(): + raise ValueError("Absolute paths are not allowed") + if "//" in value: + raise ValueError("Path cannot contain empty segments") + for segment in pathlib.Path(value).parts: + if segment in (".", ".."): + raise ValueError("Path cannot contain directory traversal") + if segment in (".git", ".svn"): + raise ValueError("Path cannot contain SCM directories") + if segment.startswith(".") and not segment.startswith(".atr") and segment != ".gitkeep": + raise ValueError("Path cannot contain dotfiles") + + def as_path(self) -> pathlib.Path: + """Return the validated path as a pathlib.Path.""" + return pathlib.Path(self._value) + + def append(self, path: str) -> RelPath: + return RelPath(f"{self!s}/{path}") + + def prepend(self, path: str) -> RelPath: + return RelPath(f"{path}/{self!s}") + + def removeprefix(self, prefix): + return RelPath(self._value.removeprefix(prefix)) + + class RevisionNumber(Numeric): """A revision number that has been validated for safety.""" @@ -138,6 +176,21 @@ def _empty_to_none(v: object) -> object: return v +def _strip_slashes_or_none(v: object) -> object: + """Strip leading/trailing slashes from a path string; return None if only slashes.""" + if isinstance(v, str): + stripped = v.strip("/") + if not stripped: + return None + return stripped + return v + + +type OptionalRelPath = Annotated[ + RelPath | None, + pydantic.BeforeValidator(_strip_slashes_or_none), +] + type OptionalRevisionNumber = Annotated[ RevisionNumber | None, pydantic.BeforeValidator(_empty_to_none), diff --git a/atr/post/announce.py b/atr/post/announce.py index d7f0b2be..4c625d91 100644 --- a/atr/post/announce.py +++ b/atr/post/announce.py @@ -64,7 +64,7 @@ async def selected( preview_revision_number = release.safe_latest_revision_number # Validate that the revision number matches - if announce_form.revision_number != str(preview_revision_number): + if announce_form.revision_number != preview_revision_number: return await session.redirect( get.announce.selected, error=f"The release has been updated since you loaded the form. " diff --git a/atr/post/upload.py b/atr/post/upload.py index 1da1d2b6..1b63e13e 100644 --- a/atr/post/upload.py +++ b/atr/post/upload.py @@ -232,10 +232,12 @@ async def _add_files( ) -def _construct_svn_url(committee_key: str, area: shared.upload.SvnArea, path: str, *, is_podling: bool) -> str: +def _construct_svn_url( + committee_key: str, area: shared.upload.SvnArea, path: safe.RelPath, *, is_podling: bool +) -> safe.RelPath: if is_podling: - return f"{area.value}/incubator/{committee_key}/{path}" - return f"{area.value}/{committee_key}/{path}" + return path.prepend(f"{area.value}/incubator/{committee_key}") + return path.prepend(f"{area.value}/{committee_key}") def _json_error(message: str, status: int) -> web.WerkzeugResponse: @@ -256,9 +258,9 @@ async def _svn_import( ) -> web.WerkzeugResponse: # audit_guidance any file uploads are from known and managed repositories so file size is not an issue try: - target_subdirectory = str(svn_form.target_subdirectory) if svn_form.target_subdirectory else None + target_subdirectory = svn_form.target_subdirectory if svn_form.target_subdirectory else None svn_area = shared.upload.SvnArea.DEV - svn_path = svn_form.svn_path or "" + svn_path = svn_form.svn_path async with db.session() as data: release = await session.release(project_key, version_key, data=data) diff --git a/atr/shared/announce.py b/atr/shared/announce.py index d565804b..e156cdbe 100644 --- a/atr/shared/announce.py +++ b/atr/shared/announce.py @@ -17,15 +17,14 @@ from typing import Literal -import pydantic - import atr.form as form +import atr.models.safe as safe class AnnounceForm(form.Form): """Form for announcing a release preview.""" - revision_number: str = form.label("Revision number", widget=form.Widget.HIDDEN) + revision_number: safe.RevisionNumber = form.label("Revision number", widget=form.Widget.HIDDEN) mailing_list: str = form.label( "Send vote email to", widget=form.Widget.CUSTOM, @@ -33,25 +32,8 @@ class AnnounceForm(form.Form): subject: str = form.label("Subject", widget=form.Widget.CUSTOM) subject_template_hash: str = form.label("Subject template hash", widget=form.Widget.HIDDEN) body: str = form.label("Body", widget=form.Widget.CUSTOM) - download_path_suffix: str = form.label("Download path suffix", widget=form.Widget.CUSTOM) + download_path_suffix: safe.OptionalRelPath = form.label("Download path suffix", widget=form.Widget.CUSTOM) confirm_announce: Literal["CONFIRM"] = form.label( "Confirm", "Type CONFIRM (in capitals) to enable the submit button.", ) - - @pydantic.field_validator("download_path_suffix") - @classmethod - def validate_and_normalize_download_path_suffix(cls, suffix: str) -> str: - if (".." in suffix) or ("//" in suffix): - raise ValueError("Download path suffix must not contain .. or //") - if suffix.startswith("./"): - suffix = suffix[1:] - elif suffix == ".": - suffix = "/" - if not suffix.startswith("/"): - suffix = "/" + suffix - if not suffix.endswith("/"): - suffix = suffix + "/" - if "/." in suffix: - raise ValueError("Download path suffix must not contain /.") - return suffix diff --git a/atr/shared/upload.py b/atr/shared/upload.py index dc24892a..4528c7ff 100644 --- a/atr/shared/upload.py +++ b/atr/shared/upload.py @@ -19,6 +19,7 @@ import enum from typing import Annotated, Literal import atr.form as form +from atr.models import safe type ADD_FILES = Literal["add_files"] type SVN_IMPORT = Literal["svn_import"] @@ -41,7 +42,7 @@ class SvnImportForm(form.Form): # "Select whether to import from dev or release.", # widget=form.Widget.RADIO, # ) - svn_path: form.URLPath = form.label( + svn_path: safe.RelPath = form.label( "SVN path", "Path within the committee's svn:dist directory, e.g. 'java-library/4_0_4' or '3.1.5rc1'.", ) @@ -50,7 +51,7 @@ class SvnImportForm(form.Form): "Specify an SVN revision number or leave as HEAD for the latest.", default="HEAD", ) - target_subdirectory: form.Filename = form.label( + target_subdirectory: safe.OptionalRelPath = form.label( "Target subdirectory", "Optional: Subdirectory to place imported files, defaulting to the root.", ) diff --git a/atr/storage/writers/announce.py b/atr/storage/writers/announce.py index d4cd9909..c1390220 100644 --- a/atr/storage/writers/announce.py +++ b/atr/storage/writers/announce.py @@ -109,7 +109,7 @@ class CommitteeMember(CommitteeParticipant): preview_revision_number: safe.RevisionNumber, recipient: str, body: str, - download_path_suffix: str, + download_path_suffix: safe.RelPath | None, asf_uid: str, fullname: str, subject_template_hash: str | None = None, @@ -254,14 +254,16 @@ class CommitteeMember(CommitteeParticipant): self, committee: sql.Committee, unfinished_path: pathlib.Path, - download_path_suffix: str, + download_path_suffix: safe.RelPath | None, dry_run: bool = False, preserve: bool = False, ) -> None: """Hard link the release files to the downloads directory.""" # TODO: Rename *_dir functions to _path functions downloads_base_path = paths.get_downloads_dir() - downloads_path = downloads_base_path / committee.key / download_path_suffix.removeprefix("/") + downloads_path = downloads_base_path / committee.key + if download_path_suffix is not None: + downloads_path = downloads_path / download_path_suffix.as_path() # The "exist_ok" parameter means to overwrite files if True # We only overwrite if we're not preserving, so we supply "not preserve" # TODO: Add a test for this diff --git a/atr/storage/writers/release.py b/atr/storage/writers/release.py index 0f5da9ba..ea7e12f9 100644 --- a/atr/storage/writers/release.py +++ b/atr/storage/writers/release.py @@ -274,14 +274,14 @@ class CommitteeParticipant(FoundationCommitter): self, project_key: safe.ProjectKey, version_key: safe.VersionKey, - svn_url: str, - revision: str, - target_subdirectory: str | None, + svn_url: safe.RelPath, + svn_revision: str, + target_subdirectory: safe.RelPath | None, ) -> sql.Task: task_args = { - "svn_url": svn_url, - "revision": revision, - "target_subdirectory": target_subdirectory, + "svn_url": str(svn_url), + "revision": svn_revision, + "target_subdirectory": str(target_subdirectory), "project_key": str(project_key), "version_key": str(version_key), "asf_uid": self.__asf_uid, diff --git a/atr/tasks/svn.py b/atr/tasks/svn.py index e7a99f62..56c4ae47 100644 --- a/atr/tasks/svn.py +++ b/atr/tasks/svn.py @@ -75,6 +75,8 @@ async def _import_files_core(args: SvnImport) -> str: project = safe.ProjectKey(args.project_key) version = safe.VersionKey(args.version_key) + svn_path = safe.RelPath(args.svn_url) + revision = safe.RevisionNumber(args.revision) log.info(f"Starting SVN import for {args.project_key}-{args.version_key}") # We have to use a temporary directory otherwise SVN thinks it's a pegged revision @@ -107,9 +109,9 @@ async def _import_files_core(args: SvnImport) -> str: "--trust-server-cert-failures", "unknown-ca,cn-mismatch", "-r", - args.revision, + revision, "--", - f"{_SVN_BASE_URL}/{args.svn_url}", + f"{_SVN_BASE_URL}/{svn_path!s}", str(temp_export_path), ] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
