This is an automated email from the ASF dual-hosted git repository. sbp pushed a commit to branch sbp in repository https://gitbox.apache.org/repos/asf/tooling-trusted-releases.git
commit d434f57471e16e0c090ce32abe2c208023c14b3e Author: Sean B. Palmer <[email protected]> AuthorDate: Thu Feb 19 20:28:03 2026 +0000 Set stricter permissions on all directories in revisions --- atr/admin/__init__.py | 5 ++++- atr/api/__init__.py | 5 ++++- atr/post/draft.py | 17 +++++------------ atr/server.py | 24 ++++++++++++++++++++++- atr/storage/writers/announce.py | 6 +++++- atr/storage/writers/release.py | 42 +++++++++++++++++++++++++---------------- atr/storage/writers/revision.py | 12 ++++++++++++ atr/util.py | 8 ++++++++ pyproject.toml | 1 + 9 files changed, 88 insertions(+), 32 deletions(-) diff --git a/atr/admin/__init__.py b/atr/admin/__init__.py index 3fd943d5..8cb1ae8f 100644 --- a/atr/admin/__init__.py +++ b/atr/admin/__init__.py @@ -993,7 +993,10 @@ async def _delete_releases(session: web.Committer, releases_to_delete: list[str] raise RuntimeError(f"Release {release_name} has no committee") async with storage.write(session) as write: wafa = write.as_foundation_admin(release.committee.name) - await wafa.release.delete(release.project.name, release.version) + error = await wafa.release.delete(release.project.name, release.version) + # Ensure that deletion errors are reported to the user + if error is not None: + raise RuntimeError(error) success_count += 1 except base.ASFQuartException as e: log.error(f"Error deleting release {release_name}: {e}") diff --git a/atr/api/__init__.py b/atr/api/__init__.py index f157411b..ce998057 100644 --- a/atr/api/__init__.py +++ b/atr/api/__init__.py @@ -918,7 +918,10 @@ async def release_delete(data: models.api.ReleaseDeleteArgs) -> DictResponse: async with storage.write(asf_uid) as write: wafa = write.as_foundation_admin(data.project) - await wafa.release.delete(data.project, data.version) + error = await wafa.release.delete(data.project, data.version) + # Ensure that deletion errors are reported to the user + if error is not None: + raise RuntimeError(error) return models.api.ReleaseDeleteResults( endpoint="/release/delete", deleted=True, diff --git a/atr/post/draft.py b/atr/post/draft.py index b3a1e0cc..d68826ca 100644 --- a/atr/post/draft.py +++ b/atr/post/draft.py @@ -20,7 +20,6 @@ from __future__ import annotations from typing import TYPE_CHECKING import aiofiles.os -import aioshutil import asfquart.base as base import quart @@ -49,19 +48,13 @@ async def delete(session: web.Committer, project_name: str, version_name: str) - # Delete the metadata from the database async with storage.write(session) as write: wacp = await write.as_project_committee_participant(project_name) - await wacp.release.delete( + error = await wacp.release.delete( project_name, version_name, phase=sql.ReleasePhase.RELEASE_CANDIDATE_DRAFT, include_downloads=False ) - - # Delete the files on disk, including all revisions - # We can't use util.release_directory_base here because we don't have the release object - draft_dir = util.get_unfinished_dir() / project_name / version_name - if await aiofiles.os.path.exists(draft_dir): - # Believe this to be another bug in mypy Protocol handling - # TODO: Confirm that this is a bug, and report upstream - # Changing it to str(...) doesn't work either - # Yet it works in preview.py - await aioshutil.rmtree(draft_dir) + # Ensure that deletion errors are reported to the user + if error is not None: + await quart.flash(f"Error deleting candidate draft: {error}", "error") + return await session.redirect(get.compose.selected, project_name=project_name, version_name=version_name) return await session.redirect(get.root.index, success="Candidate draft deleted successfully") diff --git a/atr/server.py b/atr/server.py index f32269a2..06ddabaf 100644 --- a/atr/server.py +++ b/atr/server.py @@ -177,9 +177,15 @@ def _app_dirs_setup(state_dir_str: str, hot_reload: bool) -> None: util.get_tmp_dir(), util.get_unfinished_dir(), ] + unfinished_dir = util.get_unfinished_dir() for directory in directories_to_ensure: directory.mkdir(parents=True, exist_ok=True) - util.chmod_directories(directory, permissions=0o755) + if directory != unfinished_dir: + util.chmod_directories(directory, permissions=0o755) + else: + # Revision directories and descendants must be 555 + # Therefore we handle those separately + _enforce_unfinished_permissions(unfinished_dir) def _app_setup_api_docs(app: base.QuartApp) -> None: @@ -596,6 +602,22 @@ def _create_app(app_config: type[config.AppConfig]) -> base.QuartApp: return app +def _enforce_unfinished_permissions(unfinished_dir: pathlib.Path) -> None: + # Set ancestor directories of revisions to 755 + for dirpath, _dirnames, _filenames in os.walk(unfinished_dir, topdown=True): + path = pathlib.Path(dirpath) + depth = len(path.relative_to(unfinished_dir).parts) + if depth < 3: + os.chmod(path, 0o755) + + # Set revision directories and their descendants to 555 + for dirpath, _dirnames, _filenames in os.walk(unfinished_dir, topdown=False): + path = pathlib.Path(dirpath) + depth = len(path.relative_to(unfinished_dir).parts) + if depth >= 3: + os.chmod(path, 0o555) + + async def _initialise_pubsub(conf: type[config.AppConfig], app: base.QuartApp): pubsub_url = conf.PUBSUB_URL pubsub_user = conf.PUBSUB_USER diff --git a/atr/storage/writers/announce.py b/atr/storage/writers/announce.py index cddba047..5db5a945 100644 --- a/atr/storage/writers/announce.py +++ b/atr/storage/writers/announce.py @@ -204,7 +204,11 @@ class CommitteeMember(CommitteeParticipant): ) if unfinished_revisions_path: # This removes all of the prior revisions - await aioshutil.rmtree(str(unfinished_revisions_path)) + # Each prior revision directory is immutable + await util.delete_immutable_directory( + unfinished_revisions_path, + reason="user {self.__asf_uid} is releasing {project_name} {version_name} {preview_revision_number}", + ) except Exception as e: raise storage.AccessError(f"Error moving files: {e!s}") diff --git a/atr/storage/writers/release.py b/atr/storage/writers/release.py index 7bce2997..a937b207 100644 --- a/atr/storage/writers/release.py +++ b/atr/storage/writers/release.py @@ -25,7 +25,6 @@ import hashlib from typing import TYPE_CHECKING, Final import aiofiles.os -import aioshutil import sqlalchemy import sqlalchemy.engine as engine import sqlmodel @@ -104,7 +103,10 @@ class CommitteeParticipant(FoundationCommitter): release = await self.__data.release( project_name=project_name, version=version, phase=phase, _committee=True ).demand(storage.AccessError(f"Release '{project_name} {version}' not found.")) - release_dir = util.release_directory_base(release) + release_dirs = [ + util.release_directory_base(release), + util.get_attestable_dir() / project_name / version, + ] # Delete from the database using bulk SQL DELETE for efficiency log.info(f"Deleting database records for release: {project_name} {version}") @@ -144,18 +146,21 @@ class CommitteeParticipant(FoundationCommitter): await self.__data.execute(counter_delete_stmt) log.info(f"Deleted revision counter for test release: {release_name}") - await self.__data.commit() - + # Filesystem deletions are more likely to have permission errors than database deletions + # Therefore we do filesystem deletions first if include_downloads: await self.__delete_release_data_downloads(release) - warning = await self.__delete_release_data_filesystem(release_dir, project_name, version) + error = await self.__delete_release_data_filesystem(release_dirs, project_name, version) + + await self.__data.commit() + self.__write_as.append_to_audit_log( asf_uid=self.__asf_uid, project_name=project_name, version=version, - warning=warning, + error=error, ) - return warning + return error async def delete_empty_directory( self, project_name: str, version_name: str, dir_to_delete_rel: pathlib.Path @@ -545,21 +550,26 @@ class CommitteeParticipant(FoundationCommitter): continue async def __delete_release_data_filesystem( - self, release_dir: pathlib.Path, project_name: str, version: str + self, release_dirs: Sequence[pathlib.Path], project_name: str, version: str ) -> str | None: - # Delete from the filesystem - try: + delete_errors: list[str] = [] + for release_dir in release_dirs: if await aiofiles.os.path.isdir(release_dir): - log.info(f"Deleting filesystem directory: {release_dir}") - await aioshutil.rmtree(release_dir) - log.info(f"Successfully deleted directory: {release_dir}") + try: + log.info(f"Deleting filesystem directory: {release_dir}") + await util.delete_immutable_directory( + release_dir, reason=f"user {self.__asf_uid} is deleting release {project_name} {version}" + ) + log.info(f"Successfully deleted directory: {release_dir}") + except Exception as e: + log.exception(f"Error deleting filesystem directory {release_dir}:") + delete_errors.append(f"{release_dir}: {e!s}") else: log.warning(f"Filesystem directory not found, skipping deletion: {release_dir}") - except Exception as e: - log.exception(f"Error deleting filesystem directory {release_dir}:") + if delete_errors: return ( f"Database records for '{project_name} {version}' deleted," - f" but failed to delete filesystem directory: {e!s}" + f" but failed to delete filesystem directories: {', '.join(delete_errors)}" ) return None diff --git a/atr/storage/writers/revision.py b/atr/storage/writers/revision.py index 4e7f6713..20be7bcd 100644 --- a/atr/storage/writers/revision.py +++ b/atr/storage/writers/revision.py @@ -250,12 +250,24 @@ class CommitteeParticipant(FoundationCommitter): # Ensure that the parent directory exists await aiofiles.os.makedirs(new_revision_dir.parent, exist_ok=True) + # Raise an error if the destination directory already exists + # This can happen for example if there was a previous failed cleanup + if await aiofiles.os.path.exists(new_revision_dir): + raise types.FailedError(f"Revision directory {new_revision_dir} already exists") + # Rename the temporary interim directory to the new revision number await aiofiles.os.rename(temp_dir, new_revision_dir) except Exception: await aioshutil.rmtree(temp_dir) raise + # Change permissions of all directories in the new revision directory to 555 + # This prevents accidental modifications to any directory in the new revision + # This must be done after the rename, otherwise the rename will fail + # The ".." entry in a directory is modified when it is moved between parents + # (Additionally, on macOS a 555 directory cannot be renamed within the same parent) + await asyncio.to_thread(util.chmod_directories, new_revision_dir, 0o555) + policy = release.release_policy or release.project.release_policy await attestable.write_files_data( diff --git a/atr/util.py b/atr/util.py index 60a78df9..3730c62b 100644 --- a/atr/util.py +++ b/atr/util.py @@ -385,6 +385,14 @@ def create_secure_ssl_context() -> ssl.SSLContext: return ctx +async def delete_immutable_directory(path: pathlib.Path, reason: str) -> None: + if not reason: + raise ValueError("Reason is required to delete an immutable directory") + log.info(f"Deleting immutable directory {path} because {reason}") + await asyncio.to_thread(chmod_directories, path, 0o755) + await aioshutil.rmtree(path) + + def email_from_uid(uid: str) -> str | None: if m := re.search(r"<([^>]+)>", uid): return m.group(1).lower() diff --git a/pyproject.toml b/pyproject.toml index e9baf395..4e5e1e07 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -130,6 +130,7 @@ extend-exclude = [ [tool.ruff.lint] ignore = [ "S101", # assert + "S103", # file permissions "S104", # interfaces "S105", # hardcoded password "S106", # password keyword --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
