From 1c39d8f1c55e18b1e201db32d2e9a9c0676eacce Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 13 Jul 2026 15:13:27 +0200 Subject: [PATCH 1/3] feat: move workspace to platform cache directory (#12) The per-document workspace previously lived in a .docx// folder next to the source file, cluttering user directories and risking collisions between same-named documents. It now lives under the platform user cache, in docx-editor// where is sha256 of the document's absolute path: Linux $XDG_CACHE_HOME/docx-editor (fallback ~/.cache/docx-editor) macOS ~/Library/Caches/docx-editor Windows %LOCALAPPDATA%\docx-editor (fallback ~/AppData/Local) The base directory can be overridden, in precedence order, by the workspace_dir argument to Document.open()/Workspace(), the DOCX_EDITOR_WORKSPACE_DIR environment variable, or the platform default. A relative override resolves against the source document's directory, so workspace_dir=".docx" reproduces the old next-to-file layout for debugging. Path resolution is centralized in Workspace._resolve_workspace_path(), replacing the duplicated logic in __init__, exists(), and delete(). Base directory cleanup suppresses OSError, since the base is now shared across documents and processes. No auto-migration: workspaces created by older versions are not found, and leftover .docx/ folders can be deleted. --- docs/api.md | 24 ++++++- docs/quickstart.md | 18 ++++- docx_editor/document.py | 24 +++++-- docx_editor/workspace.py | 112 +++++++++++++++++++++++++------ skills/docx/SKILL.md | 5 +- tests/conftest.py | 28 +++++--- tests/test_document.py | 22 +++++- tests/test_workspace.py | 141 ++++++++++++++++++++++++++++++++++++--- 8 files changed, 323 insertions(+), 51 deletions(-) diff --git a/docs/api.md b/docs/api.md index f554ee4..b68f0ed 100644 --- a/docs/api.md +++ b/docs/api.md @@ -10,7 +10,7 @@ from docx_editor import Document ### Opening Documents -#### `Document.open(path, author=None, force_recreate=False)` +#### `Document.open(path, author=None, force_recreate=False, workspace_dir=None)` Open a Word document for editing. @@ -19,12 +19,13 @@ Open a Word document for editing. - `path` (str | Path): Path to the .docx file - `author` (str, optional): Author name for tracked changes. Defaults to system username. - `force_recreate` (bool): If True, delete any existing workspace (stale or in-sync) before opening — any unsaved edits in the workspace are discarded. Use this to recover from `WorkspaceSyncError`. Defaults to False. +- `workspace_dir` (str | Path, optional): Base directory for the workspace. Overrides the `DOCX_EDITOR_WORKSPACE_DIR` environment variable and the platform cache default (see [Workspace location](#workspace-location)). A relative path resolves against the document's directory, so `workspace_dir=".docx"` keeps the workspace next to the file. Defaults to None. **Returns:** Document instance ready for editing **Raises:** -- `WorkspaceSyncError`: If the source `.docx` was modified since the workspace was created. Pass `force_recreate=True` to discard the stale workspace and re-unpack from the current source. The workspace is never deleted silently. +- `WorkspaceSyncError`: If the source `.docx` was modified since the workspace was created. Pass `force_recreate=True` to discard the stale workspace and re-unpack from the current source. The workspace is never deleted silently. The error message includes the workspace path. **Example:** @@ -33,6 +34,25 @@ doc = Document.open("contract.docx") doc = Document.open("contract.docx", author="Legal Team") ``` +#### Workspace location + +When you open a document, its unpacked OOXML contents are stored in a workspace directory. By default this lives under the platform user cache, in a subfolder `docx-editor/` where `` is derived from the document's absolute path: + +| Platform | Default base directory | +| --- | --- | +| Linux | `$XDG_CACHE_HOME/docx-editor` (falls back to `~/.cache/docx-editor`) | +| macOS | `~/Library/Caches/docx-editor` | +| Windows | `%LOCALAPPDATA%\docx-editor` (falls back to `~\AppData\Local\docx-editor`) | + +To override the location: + +- Set the `DOCX_EDITOR_WORKSPACE_DIR` environment variable to a base directory, or +- Pass `workspace_dir=` to `Document.open()` (takes precedence over the environment variable). + +A **relative** override resolves against the document's directory, so `workspace_dir=".docx"` reproduces the old next-to-file layout (handy for debugging). An **absolute** override is used as-is. Cleanup semantics are unchanged: the workspace persists until `close()` is called, and `close(cleanup=False)` preserves it for inspection. + +> **Note:** The default location moved from the old `.docx//` folder next to the document to the platform cache. Workspaces created by older versions are no longer found; any unsaved edits still in them are ignored. Delete leftover `.docx/` folders, or pass `workspace_dir=".docx"` to keep using the old layout. + ### Properties #### `author` diff --git a/docs/quickstart.md b/docs/quickstart.md index fbbae7e..4e5e38d 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -23,12 +23,26 @@ with Document.open("contract.docx", author="Legal Team") as doc: doc.save() ``` -If the source `.docx` was modified outside this library since the workspace was created, `Document.open()` raises `WorkspaceSyncError` instead of silently discarding the workspace. Pass `force_recreate=True` to acknowledge the divergence and re-unpack from the current source: +If the source `.docx` was modified outside this library since the workspace was created, `Document.open()` raises `WorkspaceSyncError` instead of silently discarding the workspace. The error message includes the workspace path. Pass `force_recreate=True` to acknowledge the divergence and re-unpack from the current source: ```python doc = Document.open("contract.docx", force_recreate=True) ``` +## Workspace location + +Opening a document unpacks it into a workspace directory. By default this lives under the platform user cache — `~/.cache/docx-editor/` on Linux (or `$XDG_CACHE_HOME`), `~/Library/Caches/docx-editor/` on macOS, `%LOCALAPPDATA%\docx-editor\` on Windows — in a subfolder named after a hash of the document's absolute path. + +Override the base directory with the `DOCX_EDITOR_WORKSPACE_DIR` environment variable, or per-call with `workspace_dir=`: + +```python +# Keep the workspace next to the document (relative path → resolved against +# the document's directory); useful for inspecting the unpacked XML. +doc = Document.open("contract.docx", workspace_dir=".docx") +``` + +The default location moved from the old `.docx//` folder next to the document. Leftover `.docx/` folders from older versions are no longer used and can be deleted. + ## Paragraph References Tracked edit methods are scoped to a hash-anchored paragraph reference. Start by listing paragraphs: @@ -203,7 +217,7 @@ print(f"Rejected {count} revisions") doc.save() # Save to original path doc.save("contract_v2.docx") # Save to a new path doc.close() # Delete workspace -doc.close(cleanup=False) # Keep workspace for inspection +doc.close(cleanup=False) # Keep workspace (in the cache dir) for inspection ``` ## Complete Example diff --git a/docx_editor/document.py b/docx_editor/document.py index 86af3db..24b7b7c 100644 --- a/docx_editor/document.py +++ b/docx_editor/document.py @@ -83,11 +83,17 @@ def open( path: str | Path, author: str | None = None, force_recreate: bool = False, + workspace_dir: str | Path | None = None, ) -> "Document": """Open a Word document for editing. - Creates a workspace in .docx/{document_stem}/ alongside the document. - The workspace persists until close() is called. + Creates a workspace holding the document's unpacked contents. By + default the workspace lives under the platform user cache directory + (Linux: ``$XDG_CACHE_HOME`` or ``~/.cache``; macOS: + ``~/Library/Caches``; Windows: ``%LOCALAPPDATA%``), in a subfolder + named ``docx-editor/`` where ```` is derived from the + document's absolute path. The workspace persists until close() is + called. Args: path: Path to the .docx file @@ -95,14 +101,20 @@ def open( force_recreate: If True, delete any existing workspace (stale or in-sync) before opening. Any unsaved edits in the workspace are discarded. Use this to recover from WorkspaceSyncError. + workspace_dir: Base directory for the workspace. Overrides the + DOCX_EDITOR_WORKSPACE_DIR environment variable and the platform + cache default. A relative path resolves against the document's + directory, so ``workspace_dir=".docx"`` keeps the workspace next + to the file (handy for debugging). Returns: Document instance ready for editing Raises: WorkspaceSyncError: If the source document was modified since the - workspace was created. Pass force_recreate=True to discard the - stale workspace and re-unpack from the current source. + workspace was created. The message includes the workspace path. + Pass force_recreate=True to discard the stale workspace and + re-unpack from the current source. Example: doc = Document.open("contract.docx") @@ -111,9 +123,9 @@ def open( path = Path(path).resolve() if force_recreate: - Workspace.delete(path) + Workspace.delete(path, workspace_dir=workspace_dir) - workspace = Workspace(path, author=author, create=True) + workspace = Workspace(path, author=author, create=True, workspace_dir=workspace_dir) return cls(workspace) @property diff --git a/docx_editor/workspace.py b/docx_editor/workspace.py index a36f891..33db79a 100644 --- a/docx_editor/workspace.py +++ b/docx_editor/workspace.py @@ -1,10 +1,18 @@ """Workspace management for docx_editor. -Manages the .docx/ workspace folder that contains unpacked document contents. +Manages the workspace folder that holds a document's unpacked OOXML contents. +By default the workspace lives under the platform user cache directory (see +:func:`_default_cache_dir`); the location can be overridden with the +``DOCX_EDITOR_WORKSPACE_DIR`` environment variable or an explicit +``workspace_dir`` argument. """ +import contextlib import getpass +import hashlib import json +import os +import sys from datetime import datetime, timezone from pathlib import Path from typing import Any @@ -19,19 +27,40 @@ from .ooxml import pack_document, unpack_document +def _default_cache_dir() -> Path: + """Return the platform-appropriate user cache base for workspaces. + + - Windows: ``%LOCALAPPDATA%\\docx-editor`` (``~/AppData/Local`` fallback) + - macOS: ``~/Library/Caches/docx-editor`` + - Linux/other: ``$XDG_CACHE_HOME/docx-editor`` (``~/.cache`` fallback) + """ + if os.name == "nt": + base = os.environ.get("LOCALAPPDATA") + root = Path(base) if base else Path.home() / "AppData" / "Local" + return root / "docx-editor" + if sys.platform == "darwin": + return Path.home() / "Library" / "Caches" / "docx-editor" + xdg = os.environ.get("XDG_CACHE_HOME") + root = Path(xdg) if xdg else Path.home() / ".cache" + return root / "docx-editor" + + class Workspace: - """Manages the .docx/ workspace folder for a document. + """Manages the per-document workspace folder. - The workspace is stored in a hidden .docx/ folder in the same directory - as the source document, with a subfolder named after the document stem. + The workspace holds the unpacked OOXML contents of a single document. It is + stored in a subdirectory named ``sha256(source_path)[:16]`` under a base + directory resolved (in order) from an explicit ``workspace_dir`` argument, + the ``DOCX_EDITOR_WORKSPACE_DIR`` environment variable, or the platform user + cache directory. Pass ``workspace_dir=".docx"`` to keep the workspace next + to the source file for debugging. Attributes: source_path: Path to the original .docx file - workspace_path: Path to the workspace folder (.docx/{stem}/) + workspace_path: Path to this document's workspace folder meta: Dictionary containing workspace metadata """ - WORKSPACE_DIR = ".docx" # If you rename META_FILE, also update EXCLUDED_PATHS in ooxml/pack.py — # otherwise the renamed file will be packed into the .docx and Word will # flag it as "unreadable content" (issue #8). @@ -44,6 +73,7 @@ def __init__( source_path: str | Path, author: str | None = None, create: bool = True, + workspace_dir: str | Path | None = None, ): """Initialize workspace for a document. @@ -51,6 +81,10 @@ def __init__( source_path: Path to the .docx file author: Author name for tracked changes (defaults to system user) create: If True, create workspace if it doesn't exist + workspace_dir: Base directory for the workspace. Overrides the + DOCX_EDITOR_WORKSPACE_DIR environment variable and the platform + cache default. A relative path resolves against the source + document's directory (e.g. ".docx" keeps it next to the file). Raises: DocumentNotFoundError: If the source document doesn't exist @@ -66,8 +100,8 @@ def __init__( raise InvalidDocumentError(f"Not a .docx file: {source_path}") # Determine workspace path - workspace_dir = self.source_path.parent / self.WORKSPACE_DIR - self.workspace_path = workspace_dir / self.source_path.stem + self.workspace_dir = workspace_dir + self.workspace_path = self._resolve_workspace_path(self.source_path, workspace_dir) # Set author (default to system user) self._author = author or getpass.getuser() @@ -99,6 +133,36 @@ def __init__( raise WorkspaceError(f"Invalid workspace (no meta.json): {self.workspace_path}") self.meta = loaded_meta + @classmethod + def _resolve_workspace_path(cls, source_path: Path, workspace_dir: str | Path | None) -> Path: + """Resolve the workspace directory for a source document. + + Precedence for the base directory: + 1. the explicit ``workspace_dir`` argument + 2. the ``DOCX_EDITOR_WORKSPACE_DIR`` environment variable + 3. the platform user cache (see :func:`_default_cache_dir`) + + A relative base resolves against ``source_path.parent`` (so + ``workspace_dir=".docx"`` reproduces the old next-to-file layout for + debugging); an absolute base is used as-is. The per-document + subdirectory is always ``sha256(str(source_path))[:16]``. + + Args: + source_path: Resolved (absolute) path to the .docx file. + workspace_dir: Explicit base override, or None. + """ + if workspace_dir is not None: + base = Path(workspace_dir) + else: + env = os.environ.get("DOCX_EDITOR_WORKSPACE_DIR") + base = Path(env) if env else _default_cache_dir() + + if not base.is_absolute(): + base = source_path.parent / base + + subdir = hashlib.sha256(str(source_path).encode()).hexdigest()[:16] + return base / subdir + @property def author(self) -> str: """Get the author name for tracked changes.""" @@ -211,10 +275,13 @@ def close(self, cleanup: bool = True) -> None: shutil.rmtree(self.workspace_path) - # Remove .docx parent if empty - workspace_dir = self.workspace_path.parent - if workspace_dir.exists() and not any(workspace_dir.iterdir()): - workspace_dir.rmdir() + # Remove the base directory if it is now empty. The base is shared + # across documents/processes, so the check-then-rmdir can race — + # suppress the resulting OSError (a lingering empty cache dir is fine). + base_dir = self.workspace_path.parent + with contextlib.suppress(OSError): + if base_dir.exists() and not any(base_dir.iterdir()): + base_dir.rmdir() def get_xml_path(self, relative_path: str) -> Path: """Get the full path to an XML file in the workspace. @@ -243,26 +310,27 @@ def sync_check(self) -> bool: ) @classmethod - def exists(cls, source_path: str | Path) -> bool: + def exists(cls, source_path: str | Path, workspace_dir: str | Path | None = None) -> bool: """Check if a workspace exists for a document. Args: source_path: Path to the .docx file + workspace_dir: Base directory override (see __init__) Returns: True if workspace exists """ source_path = Path(source_path).resolve() - workspace_dir = source_path.parent / cls.WORKSPACE_DIR - workspace_path = workspace_dir / source_path.stem + workspace_path = cls._resolve_workspace_path(source_path, workspace_dir) return workspace_path.exists() @classmethod - def delete(cls, source_path: str | Path) -> bool: + def delete(cls, source_path: str | Path, workspace_dir: str | Path | None = None) -> bool: """Delete workspace for a document if it exists. Args: source_path: Path to the .docx file + workspace_dir: Base directory override (see __init__) Returns: True if workspace was deleted, False if it didn't exist @@ -270,16 +338,18 @@ def delete(cls, source_path: str | Path) -> bool: import shutil source_path = Path(source_path).resolve() - workspace_dir = source_path.parent / cls.WORKSPACE_DIR - workspace_path = workspace_dir / source_path.stem + workspace_path = cls._resolve_workspace_path(source_path, workspace_dir) if not workspace_path.exists(): return False shutil.rmtree(workspace_path) - # Remove .docx parent if empty - if workspace_dir.exists() and not any(workspace_dir.iterdir()): - workspace_dir.rmdir() + # Remove the base directory if it is now empty (see close() for the + # shared-base race that OSError suppression guards against). + base_dir = workspace_path.parent + with contextlib.suppress(OSError): + if base_dir.exists() and not any(base_dir.iterdir()): + base_dir.rmdir() return True diff --git a/skills/docx/SKILL.md b/skills/docx/SKILL.md index 3ef5e39..4096099 100644 --- a/skills/docx/SKILL.md +++ b/skills/docx/SKILL.md @@ -188,7 +188,8 @@ with Document.open("contract.docx", author=author) as doc: doc.save() # Overwrites original # or doc.save("reviewed.docx") # Save to new file # Workspace is cleaned up automatically on normal exit -# On exception, workspace is preserved for inspection +# On exception, the workspace is preserved for inspection in the user cache +# dir (~/.cache/docx-editor// on Linux; error messages print the exact path) ``` Without context manager: @@ -606,7 +607,7 @@ Benefits: If unsure, ask the user: "Should I use Opus (best), Sonnet (recommended) or Haiku (faster/cheaper) for this task?" -**Editing in parallel**: NOT safe for the same document. docx_editor uses a shared workspace - concurrent edits will overwrite each other. Edit documents sequentially, or use different files. +**Editing in parallel**: NOT safe for the same document. The workspace is keyed by the document's absolute path in the user cache dir, so two processes editing the same file share one workspace and will overwrite each other. Edit the same document sequentially. Different files never collide (each gets its own workspace), so editing distinct documents in parallel is fine. ### Limitations diff --git a/tests/conftest.py b/tests/conftest.py index 8a4897e..ae42b26 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -45,6 +45,20 @@ def temp_dir(): shutil.rmtree(temp, ignore_errors=True) +@pytest.fixture(autouse=True) +def isolated_workspace_base(monkeypatch): + """Isolate every test's workspace base from the real user cache. + + Points DOCX_EDITOR_WORKSPACE_DIR at a throwaway per-test directory so tests + never write to ~/.cache/docx-editor/ and implicitly exercise the env-var + resolution path. + """ + base = tempfile.mkdtemp(prefix="docx_editor_ws_") + monkeypatch.setenv("DOCX_EDITOR_WORKSPACE_DIR", base) + yield Path(base) + shutil.rmtree(base, ignore_errors=True) + + @pytest.fixture def temp_docx(simple_docx, temp_dir) -> Path: """Copy simple.docx to a temp location for testing.""" @@ -55,11 +69,9 @@ def temp_docx(simple_docx, temp_dir) -> Path: @pytest.fixture def clean_workspace(temp_docx): - """Ensure no workspace exists for the test document.""" - workspace_dir = temp_docx.parent / ".docx" - if workspace_dir.exists(): - shutil.rmtree(workspace_dir) - yield temp_docx - # Cleanup after test - if workspace_dir.exists(): - shutil.rmtree(workspace_dir) + """Alias for temp_docx, kept for backwards compatibility. + + Workspace isolation is handled by the autouse isolated_workspace_base + fixture, so no manual cleanup is needed here. + """ + return temp_docx diff --git a/tests/test_document.py b/tests/test_document.py index 0c3ae92..6b1b88a 100644 --- a/tests/test_document.py +++ b/tests/test_document.py @@ -39,6 +39,26 @@ def test_open_force_recreate(self, clean_workspace): doc2 = Document.open(clean_workspace, force_recreate=True) doc2.close() + def test_open_with_workspace_dir(self, clean_workspace, tmp_path): + """Test that workspace_dir is threaded through to the Workspace.""" + doc = Document.open(clean_workspace, workspace_dir=tmp_path) + + assert doc._workspace.workspace_path.parent == tmp_path + assert doc._workspace.workspace_path.exists() + assert Workspace.exists(clean_workspace, workspace_dir=tmp_path) + + doc.close() + + def test_open_force_recreate_with_workspace_dir(self, clean_workspace, tmp_path): + """Test force_recreate deletes the workspace under the given workspace_dir.""" + doc1 = Document.open(clean_workspace, workspace_dir=tmp_path) + doc1.close(cleanup=False) + assert Workspace.exists(clean_workspace, workspace_dir=tmp_path) + + doc2 = Document.open(clean_workspace, force_recreate=True, workspace_dir=tmp_path) + assert doc2._workspace.workspace_path.parent == tmp_path + doc2.close() + class TestDocumentSave: """Tests for saving documents.""" @@ -276,7 +296,7 @@ def test_force_recreate_with_persistent_sync_error(self, clean_workspace): # Make Workspace always raise WorkspaceSyncError with patch("docx_editor.document.Workspace") as mock_workspace_cls: mock_workspace_cls.side_effect = WorkspaceSyncError("Persistent error") - mock_workspace_cls.delete = lambda p: True + mock_workspace_cls.delete = lambda p, workspace_dir=None: True with pytest.raises(WorkspaceSyncError): Document.open(clean_workspace, force_recreate=True) diff --git a/tests/test_workspace.py b/tests/test_workspace.py index 0964a06..be3fdb4 100644 --- a/tests/test_workspace.py +++ b/tests/test_workspace.py @@ -1,6 +1,11 @@ """Tests for workspace management.""" +import hashlib import json +import os +import re +import sys +from pathlib import Path import pytest @@ -9,7 +14,7 @@ InvalidDocumentError, WorkspaceSyncError, ) -from docx_editor.workspace import Workspace +from docx_editor.workspace import Workspace, _default_cache_dir class TestWorkspaceCreation: @@ -190,8 +195,8 @@ def test_workspace_exists_no_meta_json(self, clean_workspace, temp_dir): from docx_editor.exceptions import WorkspaceExistsError # Create workspace directory without meta.json - workspace_dir = clean_workspace.parent / ".docx" / clean_workspace.stem - workspace_dir.mkdir(parents=True) + workspace_path = Workspace._resolve_workspace_path(clean_workspace.resolve(), None) + workspace_path.mkdir(parents=True) with pytest.raises(WorkspaceExistsError): Workspace(clean_workspace) @@ -199,7 +204,7 @@ def test_workspace_exists_no_meta_json(self, clean_workspace, temp_dir): # Cleanup import shutil - shutil.rmtree(clean_workspace.parent / ".docx") + shutil.rmtree(workspace_path) def test_workspace_create_false_not_found(self, clean_workspace): """Test error when workspace not found with create=False.""" @@ -213,8 +218,8 @@ def test_workspace_create_false_no_meta(self, clean_workspace): from docx_editor.exceptions import WorkspaceError # Create workspace directory without meta.json - workspace_dir = clean_workspace.parent / ".docx" / clean_workspace.stem - workspace_dir.mkdir(parents=True) + workspace_path = Workspace._resolve_workspace_path(clean_workspace.resolve(), None) + workspace_path.mkdir(parents=True) with pytest.raises(WorkspaceError, match="Invalid workspace"): Workspace(clean_workspace, create=False) @@ -222,7 +227,7 @@ def test_workspace_create_false_no_meta(self, clean_workspace): # Cleanup import shutil - shutil.rmtree(clean_workspace.parent / ".docx") + shutil.rmtree(workspace_path) def test_load_meta_corrupt_json(self, clean_workspace): """Test that corrupt meta.json returns None in _load_meta.""" @@ -285,8 +290,7 @@ def test_sync_check_source_modified(self, clean_workspace): workspace2 = Workspace.__new__(Workspace) workspace2.source_path = clean_workspace.resolve() workspace2._author = "test" - workspace_dir = clean_workspace.parent / ".docx" - workspace2.workspace_path = workspace_dir / clean_workspace.stem + workspace2.workspace_path = Workspace._resolve_workspace_path(clean_workspace.resolve(), None) workspace2.meta = workspace2._load_meta() assert workspace2.sync_check() is False @@ -367,3 +371,122 @@ def test_workspace_create_false_with_valid_meta(self, clean_workspace): assert workspace2.meta is not None workspace2.close() + + +class TestDefaultCacheDir: + """Tests for the _default_cache_dir platform helper.""" + + def test_linux_with_xdg_cache_home(self, monkeypatch): + monkeypatch.setattr(os, "name", "posix") + monkeypatch.setattr(sys, "platform", "linux") + monkeypatch.setenv("XDG_CACHE_HOME", "/custom/cache") + assert _default_cache_dir() == Path("/custom/cache") / "docx-editor" + + def test_linux_without_xdg_cache_home(self, monkeypatch): + monkeypatch.setattr(os, "name", "posix") + monkeypatch.setattr(sys, "platform", "linux") + monkeypatch.delenv("XDG_CACHE_HOME", raising=False) + assert _default_cache_dir() == Path.home() / ".cache" / "docx-editor" + + def test_macos(self, monkeypatch): + monkeypatch.setattr(os, "name", "posix") + monkeypatch.setattr(sys, "platform", "darwin") + assert _default_cache_dir() == Path.home() / "Library" / "Caches" / "docx-editor" + + # The Windows branch constructs pathlib.Path, which resolves to WindowsPath + # only when os.name == "nt". Patching os.name on a POSIX host makes Path + # raise UnsupportedOperation, so these run for real only on Windows. + @pytest.mark.skipif(os.name != "nt", reason="requires a Windows host") + def test_windows_with_localappdata(self, monkeypatch): + monkeypatch.setenv("LOCALAPPDATA", r"C:\Users\x\AppData\Local") + assert _default_cache_dir() == Path(r"C:\Users\x\AppData\Local") / "docx-editor" + + @pytest.mark.skipif(os.name != "nt", reason="requires a Windows host") + def test_windows_without_localappdata(self, monkeypatch): + monkeypatch.delenv("LOCALAPPDATA", raising=False) + assert _default_cache_dir() == Path.home() / "AppData" / "Local" / "docx-editor" + + +class TestWorkspaceLocation: + """Tests for workspace location resolution (cache dir, env var, override).""" + + @staticmethod + def _expected_subdir(source_path): + return hashlib.sha256(str(source_path.resolve()).encode()).hexdigest()[:16] + + def test_default_lands_in_platform_cache(self, temp_docx, monkeypatch, tmp_path): + """With no override, workspace lands under the platform cache base.""" + monkeypatch.delenv("DOCX_EDITOR_WORKSPACE_DIR", raising=False) + monkeypatch.setattr(os, "name", "posix") + monkeypatch.setattr(sys, "platform", "linux") + monkeypatch.setenv("XDG_CACHE_HOME", str(tmp_path)) + + workspace = Workspace(temp_docx) + expected = tmp_path / "docx-editor" / self._expected_subdir(temp_docx) + assert workspace.workspace_path == expected + assert workspace.workspace_path.exists() + workspace.close() + + def test_env_var_used_when_no_param(self, temp_docx, tmp_path, monkeypatch): + """DOCX_EDITOR_WORKSPACE_DIR is used as the base when no param is given.""" + monkeypatch.setenv("DOCX_EDITOR_WORKSPACE_DIR", str(tmp_path)) + workspace = Workspace(temp_docx) + assert workspace.workspace_path == tmp_path / self._expected_subdir(temp_docx) + workspace.close() + + def test_explicit_param_overrides_env_var(self, temp_docx, tmp_path, monkeypatch): + """The workspace_dir param wins over the env var.""" + env_base = tmp_path / "env_base" + param_base = tmp_path / "param_base" + monkeypatch.setenv("DOCX_EDITOR_WORKSPACE_DIR", str(env_base)) + + workspace = Workspace(temp_docx, workspace_dir=param_base) + assert workspace.workspace_path == param_base / self._expected_subdir(temp_docx) + # The env-var base must not have been touched. + assert not (env_base / self._expected_subdir(temp_docx)).exists() + workspace.close() + + def test_relative_workspace_dir_next_to_source(self, temp_docx): + """A relative workspace_dir resolves against the source directory.""" + workspace = Workspace(temp_docx, workspace_dir=".docx") + expected = temp_docx.parent / ".docx" / self._expected_subdir(temp_docx) + assert workspace.workspace_path == expected + assert workspace.workspace_path.exists() + workspace.close() + + def test_distinct_sources_get_distinct_dirs(self, simple_docx, temp_dir, tmp_path, monkeypatch): + """Two different source files map to different hash subdirs, same base.""" + import shutil + + monkeypatch.setenv("DOCX_EDITOR_WORKSPACE_DIR", str(tmp_path)) + doc_a = temp_dir / "a.docx" + doc_b = temp_dir / "b.docx" + shutil.copy(simple_docx, doc_a) + shutil.copy(simple_docx, doc_b) + + ws_a = Workspace(doc_a) + ws_b = Workspace(doc_b) + assert ws_a.workspace_path != ws_b.workspace_path + assert ws_a.workspace_path.parent == tmp_path + assert ws_b.workspace_path.parent == tmp_path + + ws_a.close() + ws_b.close() + + def test_sync_error_message_contains_workspace_path(self, temp_docx, tmp_path, monkeypatch): + """WorkspaceSyncError from the cache location prints the workspace path.""" + import time + + monkeypatch.setenv("DOCX_EDITOR_WORKSPACE_DIR", str(tmp_path)) + workspace = Workspace(temp_docx) + workspace_path = workspace.workspace_path + workspace.close(cleanup=False) + + # Modify the source so the mtime check fails on reopen. + time.sleep(0.1) + temp_docx.write_bytes(temp_docx.read_bytes() + b"\x00") + + with pytest.raises(WorkspaceSyncError, match=re.escape(str(workspace_path))): + Workspace(temp_docx) + + Workspace.delete(temp_docx) From 77ec2492b1d096b1e80be6f33627c3fc1d04e028 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 13 Jul 2026 15:59:30 +0200 Subject: [PATCH 2/3] fix: harden workspace cache path resolution and cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the cache migration, from a multi-reviewer pass. Crash: the workspace dir was keyed by sha256(str(path).encode()), but str(Path) carries undecodable filename bytes as surrogates, which strict UTF-8 rejects. Opening e.g. "café.docx" (latin-1 on disk) raised UnicodeEncodeError. Hash via os.fsencode(os.path.normcase(path)) instead; this is byte-identical for all decodable paths, so no workspace moves. normcase also folds case on Windows, where one file could otherwise map to two workspaces. Data loss: close()/delete() removed the base directory when it was left empty. That was safe when the base was the library's own .docx/ folder, but the base is now the shared cache root -- or whatever directory the caller passed via workspace_dir=/DOCX_EDITOR_WORKSPACE_DIR. The library was deleting a directory it did not create. It now removes only the document's own workspace folder. Two tests asserted the old behavior and are rewritten. Path resolution: overrides are now tilde-expanded (DOCX_EDITOR_WORKSPACE_DIR =~/ws created a literal "~" directory next to each document), an empty or whitespace override counts as unset and falls through to the next precedence level (workspace_dir="" silently rebased onto the document's own directory), and a relative XDG_CACHE_HOME/%LOCALAPPDATA% is ignored per the XDG spec rather than scattering caches beside every document. Permissions: the workspace holds the document's plaintext in a shared, predictably-named cache, so it and its base are created 0o700 instead of inheriting a 0o775 umask default. mkdir(parents=True, mode=) applies the mode to the leaf only, so the base is created explicitly. Provenance: a workspace whose meta.json names a different source document is now refused instead of silently adopted -- adopting it would let a later save() pack that other document's XML over this one's source file. Errors: filesystem failures at the new cache boundary (unwritable base, no resolvable home) surfaced as raw PermissionError/RuntimeError out of Document.open(). They are now WorkspaceError, naming the override to set. Also: add a public Document.workspace_path -- the docs told users to inspect the preserved workspace but gave them no way to locate it now that it is an opaque hash under the cache. Drop the write-only self.workspace_dir, hoist the function-local shutil import, and correct the "unsaved edits in the workspace" wording (edits reach the workspace only on save()). --- docs/api.md | 18 +++++- docx_editor/document.py | 22 +++++-- docx_editor/workspace.py | 129 +++++++++++++++++++++++++++------------ openspec/project.md | 2 +- tests/test_workspace.py | 125 +++++++++++++++++++++++++++++++++---- 5 files changed, 236 insertions(+), 60 deletions(-) diff --git a/docs/api.md b/docs/api.md index b68f0ed..c5befac 100644 --- a/docs/api.md +++ b/docs/api.md @@ -18,7 +18,7 @@ Open a Word document for editing. - `path` (str | Path): Path to the .docx file - `author` (str, optional): Author name for tracked changes. Defaults to system username. -- `force_recreate` (bool): If True, delete any existing workspace (stale or in-sync) before opening — any unsaved edits in the workspace are discarded. Use this to recover from `WorkspaceSyncError`. Defaults to False. +- `force_recreate` (bool): If True, delete any existing workspace (stale or in-sync) before opening — whatever XML it holds is discarded — and re-unpack from the current source. Use this to recover from `WorkspaceSyncError`. Defaults to False. - `workspace_dir` (str | Path, optional): Base directory for the workspace. Overrides the `DOCX_EDITOR_WORKSPACE_DIR` environment variable and the platform cache default (see [Workspace location](#workspace-location)). A relative path resolves against the document's directory, so `workspace_dir=".docx"` keeps the workspace next to the file. Defaults to None. **Returns:** Document instance ready for editing @@ -49,9 +49,13 @@ To override the location: - Set the `DOCX_EDITOR_WORKSPACE_DIR` environment variable to a base directory, or - Pass `workspace_dir=` to `Document.open()` (takes precedence over the environment variable). -A **relative** override resolves against the document's directory, so `workspace_dir=".docx"` reproduces the old next-to-file layout (handy for debugging). An **absolute** override is used as-is. Cleanup semantics are unchanged: the workspace persists until `close()` is called, and `close(cleanup=False)` preserves it for inspection. +Both overrides are tilde-expanded, and an empty value counts as unset. A **relative** override resolves against the document's directory, so `workspace_dir=".docx"` reproduces the old next-to-file layout (handy for debugging). An **absolute** override is used as-is. (A relative `XDG_CACHE_HOME` / `%LOCALAPPDATA%` is ignored, per the XDG spec.) -> **Note:** The default location moved from the old `.docx//` folder next to the document to the platform cache. Workspaces created by older versions are no longer found; any unsaved edits still in them are ignored. Delete leftover `.docx/` folders, or pass `workspace_dir=".docx"` to keep using the old layout. +Cleanup semantics are unchanged: the workspace persists until `close()` is called, and `close(cleanup=False)` preserves it for inspection. `close()` removes only the document's own workspace folder — the base directory is shared and is never deleted, so it is safe to point `workspace_dir` at a directory you also use for other things. + +The workspace directory is created with owner-only permissions (`0o700`), since it holds the document's plaintext in a shared cache location. Use `doc.workspace_path` to locate it. + +> **Note:** The default location moved from the old `.docx//` folder next to the document to the platform cache. Workspaces created by older versions are no longer found and are simply ignored. Delete leftover `.docx/` folders, or pass `workspace_dir=".docx"` to keep using the old layout. ### Properties @@ -71,6 +75,14 @@ Get the path to the source document. print(doc.source_path) # Path("/path/to/contract.docx") ``` +#### `workspace_path` + +Get the path to this document's workspace folder. Since the workspace lives in the user cache by default, this is how you locate the unpacked XML — for example after `close(cleanup=False)`, or when a workspace was preserved because an exception was raised. + +```python +print(doc.workspace_path) # Path("~/.cache/docx-editor/0bebafb463a87cfa") +``` + ### Track Changes Methods #### `paragraph_count()` diff --git a/docx_editor/document.py b/docx_editor/document.py index 24b7b7c..18b4657 100644 --- a/docx_editor/document.py +++ b/docx_editor/document.py @@ -99,13 +99,15 @@ def open( path: Path to the .docx file author: Author name for tracked changes (defaults to system username) force_recreate: If True, delete any existing workspace (stale or - in-sync) before opening. Any unsaved edits in the workspace - are discarded. Use this to recover from WorkspaceSyncError. + in-sync) before opening, discarding whatever XML it holds, and + re-unpack from the current source. Use this to recover from + WorkspaceSyncError. workspace_dir: Base directory for the workspace. Overrides the DOCX_EDITOR_WORKSPACE_DIR environment variable and the platform - cache default. A relative path resolves against the document's - directory, so ``workspace_dir=".docx"`` keeps the workspace next - to the file (handy for debugging). + cache default. Tilde-expanded; an empty value counts as unset. + A relative path resolves against the document's directory, so + ``workspace_dir=".docx"`` keeps the workspace next to the file + (handy for debugging). Returns: Document instance ready for editing @@ -138,6 +140,16 @@ def source_path(self) -> Path: """Get the path to the source document.""" return self._workspace.source_path + @property + def workspace_path(self) -> Path: + """Get the path to this document's workspace folder. + + The workspace lives under the user cache by default, so this is the + only way to locate the unpacked XML — e.g. after close(cleanup=False) + or when a workspace is preserved because an exception was raised. + """ + return self._workspace.workspace_path + # ==================== Track Changes API ==================== def find_text(self, text: str, occurrence: int = 0): diff --git a/docx_editor/workspace.py b/docx_editor/workspace.py index 33db79a..6f60afb 100644 --- a/docx_editor/workspace.py +++ b/docx_editor/workspace.py @@ -7,11 +7,11 @@ ``workspace_dir`` argument. """ -import contextlib import getpass import hashlib import json import os +import shutil import sys from datetime import datetime, timezone from pathlib import Path @@ -27,21 +27,48 @@ from .ooxml import pack_document, unpack_document +def _cache_root_from_env(name: str) -> Path | None: + """Return ``$name`` as a cache root, or None if it is unusable. + + Per the XDG base directory spec, a relative value "must be ignored and a + default equivalent [...] used instead" — the same rule is applied to + %LOCALAPPDATA%. Without it a relative value would later be joined to the + source document's directory, silently scattering caches next to documents. + """ + value = os.environ.get(name, "").strip() + if not value: + return None + root = Path(os.path.expanduser(value)) + return root if root.is_absolute() else None + + +def _home() -> Path: + """Return the user's home directory as a WorkspaceError-domain failure.""" + try: + return Path.home() + except RuntimeError as exc: # no HOME and no passwd entry (slim containers) + raise WorkspaceError( + "Cannot determine the home directory for the default workspace cache. " + "Set DOCX_EDITOR_WORKSPACE_DIR to an absolute path, or pass workspace_dir=." + ) from exc + + def _default_cache_dir() -> Path: """Return the platform-appropriate user cache base for workspaces. - Windows: ``%LOCALAPPDATA%\\docx-editor`` (``~/AppData/Local`` fallback) - macOS: ``~/Library/Caches/docx-editor`` - Linux/other: ``$XDG_CACHE_HOME/docx-editor`` (``~/.cache`` fallback) + + A relative ``%LOCALAPPDATA%``/``$XDG_CACHE_HOME`` is ignored (see + :func:`_cache_root_from_env`). """ if os.name == "nt": - base = os.environ.get("LOCALAPPDATA") - root = Path(base) if base else Path.home() / "AppData" / "Local" + root = _cache_root_from_env("LOCALAPPDATA") or _home() / "AppData" / "Local" return root / "docx-editor" if sys.platform == "darwin": - return Path.home() / "Library" / "Caches" / "docx-editor" - xdg = os.environ.get("XDG_CACHE_HOME") - root = Path(xdg) if xdg else Path.home() / ".cache" + return _home() / "Library" / "Caches" / "docx-editor" + root = _cache_root_from_env("XDG_CACHE_HOME") or _home() / ".cache" return root / "docx-editor" @@ -100,7 +127,6 @@ def __init__( raise InvalidDocumentError(f"Not a .docx file: {source_path}") # Determine workspace path - self.workspace_dir = workspace_dir self.workspace_path = self._resolve_workspace_path(self.source_path, workspace_dir) # Set author (default to system user) @@ -111,6 +137,7 @@ def __init__( # Check if it's stale or matches current document existing_meta = self._load_meta() if existing_meta: + self._check_provenance(existing_meta) source_mtime = self.source_path.stat().st_mtime if existing_meta.get("source_mtime") != source_mtime: raise WorkspaceSyncError( @@ -131,8 +158,25 @@ def __init__( loaded_meta = self._load_meta() if not loaded_meta: raise WorkspaceError(f"Invalid workspace (no meta.json): {self.workspace_path}") + self._check_provenance(loaded_meta) self.meta = loaded_meta + def _check_provenance(self, meta: dict[str, Any]) -> None: + """Refuse to adopt a workspace that was unpacked from a different document. + + The workspace directory is keyed by a hash of the source path, so a + mismatch here means either a hash collision or a directory planted by + something else. Adopting it would let a later save() pack that other + document's XML over this one's source file. + """ + recorded = meta.get("source_path") + if recorded != str(self.source_path): + raise WorkspaceError( + f"Workspace {self.workspace_path} belongs to a different document " + f"({recorded!r}, expected {str(self.source_path)!r}). " + f"Delete it or use force_recreate=True." + ) + @classmethod def _resolve_workspace_path(cls, source_path: Path, workspace_dir: str | Path | None) -> Path: """Resolve the workspace directory for a source document. @@ -142,26 +186,35 @@ def _resolve_workspace_path(cls, source_path: Path, workspace_dir: str | Path | 2. the ``DOCX_EDITOR_WORKSPACE_DIR`` environment variable 3. the platform user cache (see :func:`_default_cache_dir`) - A relative base resolves against ``source_path.parent`` (so - ``workspace_dir=".docx"`` reproduces the old next-to-file layout for - debugging); an absolute base is used as-is. The per-document - subdirectory is always ``sha256(str(source_path))[:16]``. + Both overrides are tilde-expanded, and an empty/whitespace-only value + counts as unset. A relative base resolves against ``source_path.parent`` + (so ``workspace_dir=".docx"`` reproduces the old next-to-file layout for + debugging); an absolute base is used as-is. + + The per-document subdirectory is ``sha256(normcase(source_path))[:16]``, + hashed via :func:`os.fsencode` so undecodable filename bytes (which + ``str(Path)`` carries as surrogates) do not raise UnicodeEncodeError. + ``normcase`` folds case on Windows, so one file cannot map to two + workspaces there; it is a no-op on POSIX. Args: source_path: Resolved (absolute) path to the .docx file. workspace_dir: Explicit base override, or None. """ - if workspace_dir is not None: - base = Path(workspace_dir) - else: - env = os.environ.get("DOCX_EDITOR_WORKSPACE_DIR") - base = Path(env) if env else _default_cache_dir() + base = None + for override in (workspace_dir, os.environ.get("DOCX_EDITOR_WORKSPACE_DIR")): + text = str(override).strip() if override is not None else "" + if text: # an empty/whitespace override falls through to the next level + base = Path(os.path.expanduser(text)) + break + if base is None: + base = _default_cache_dir() if not base.is_absolute(): base = source_path.parent / base - subdir = hashlib.sha256(str(source_path).encode()).hexdigest()[:16] - return base / subdir + key = os.fsencode(os.path.normcase(source_path)) + return base / hashlib.sha256(key).hexdigest()[:16] @property def author(self) -> str: @@ -190,8 +243,19 @@ def document_xml_path(self) -> Path: def _create_workspace(self) -> None: """Create the workspace by unpacking the document.""" - # Create workspace directory - self.workspace_path.mkdir(parents=True, exist_ok=True) + # Create workspace directory. The workspace holds the document's + # plaintext in a shared, predictably-named cache base, so restrict it to + # the owner. mkdir(parents=True, mode=...) applies the mode to the leaf + # only, so the base is created separately to keep it 0o700 too. + try: + self.workspace_path.parent.mkdir(parents=True, mode=0o700, exist_ok=True) + self.workspace_path.mkdir(mode=0o700, exist_ok=True) + except OSError as exc: + raise WorkspaceError( + f"Cannot create workspace at {self.workspace_path}: {exc}. " + f"Set DOCX_EDITOR_WORKSPACE_DIR to a writable absolute path, " + f"or pass workspace_dir=." + ) from exc # Unpack document rsid = unpack_document(self.source_path, self.workspace_path) @@ -270,19 +334,13 @@ def close(self, cleanup: bool = True) -> None: Args: cleanup: If True, delete the workspace folder """ + # Only this document's workspace is removed. The base directory is + # shared (and may be a caller-supplied one via workspace_dir=/ + # DOCX_EDITOR_WORKSPACE_DIR), so it is left alone even when empty — + # _create_workspace() recreates it on demand. if cleanup and self.workspace_path.exists(): - import shutil - shutil.rmtree(self.workspace_path) - # Remove the base directory if it is now empty. The base is shared - # across documents/processes, so the check-then-rmdir can race — - # suppress the resulting OSError (a lingering empty cache dir is fine). - base_dir = self.workspace_path.parent - with contextlib.suppress(OSError): - if base_dir.exists() and not any(base_dir.iterdir()): - base_dir.rmdir() - def get_xml_path(self, relative_path: str) -> Path: """Get the full path to an XML file in the workspace. @@ -335,21 +393,12 @@ def delete(cls, source_path: str | Path, workspace_dir: str | Path | None = None Returns: True if workspace was deleted, False if it didn't exist """ - import shutil - source_path = Path(source_path).resolve() workspace_path = cls._resolve_workspace_path(source_path, workspace_dir) if not workspace_path.exists(): return False + # As in close(), the shared base directory is left in place. shutil.rmtree(workspace_path) - - # Remove the base directory if it is now empty (see close() for the - # shared-base race that OSError suppression guards against). - base_dir = workspace_path.parent - with contextlib.suppress(OSError): - if base_dir.exists() and not any(base_dir.iterdir()): - base_dir.rmdir() - return True diff --git a/openspec/project.md b/openspec/project.md index afb410b..f778667 100644 --- a/openspec/project.md +++ b/openspec/project.md @@ -32,7 +32,7 @@ docx_editor/ ├── xml_editor.py # Core XML manipulation (XMLEditor, DocxXMLEditor) ├── track_changes.py # RevisionManager - track changes operations ├── comments.py # CommentManager - comment operations -├── workspace.py # Workspace management (.docx/{stem}/ folders) +├── workspace.py # Workspace management (user cache dir, / folders) ├── exceptions.py # Custom exception hierarchy └── ooxml/ # OOXML utilities (pack/unpack) ``` diff --git a/tests/test_workspace.py b/tests/test_workspace.py index be3fdb4..39fc7b1 100644 --- a/tests/test_workspace.py +++ b/tests/test_workspace.py @@ -4,6 +4,7 @@ import json import os import re +import stat import sys from pathlib import Path @@ -12,6 +13,7 @@ from docx_editor.exceptions import ( DocumentNotFoundError, InvalidDocumentError, + WorkspaceError, WorkspaceSyncError, ) from docx_editor.workspace import Workspace, _default_cache_dir @@ -298,26 +300,30 @@ def test_sync_check_source_modified(self, clean_workspace): # Cleanup Workspace.delete(clean_workspace) - def test_close_removes_empty_parent_dir(self, clean_workspace): - """Test that close removes empty .docx parent directory.""" + def test_close_keeps_shared_base_dir(self, clean_workspace): + """close() removes only this workspace, never the shared base directory. + + The base may be a caller-supplied directory (workspace_dir= / + DOCX_EDITOR_WORKSPACE_DIR), so the library must not delete it. + """ workspace = Workspace(clean_workspace) - parent_dir = workspace.workspace_path.parent + base_dir = workspace.workspace_path.parent workspace.close(cleanup=True) - # Parent .docx dir should be removed if empty - assert not parent_dir.exists() + assert not workspace.workspace_path.exists() + assert base_dir.exists() - def test_delete_removes_empty_parent_dir(self, clean_workspace): - """Test that delete removes empty .docx parent directory.""" + def test_delete_keeps_shared_base_dir(self, clean_workspace): + """delete() removes only this workspace, never the shared base directory.""" workspace = Workspace(clean_workspace) - parent_dir = workspace.workspace_path.parent + base_dir = workspace.workspace_path.parent workspace.close(cleanup=False) - Workspace.delete(clean_workspace) + assert Workspace.delete(clean_workspace) is True - # Parent .docx dir should be removed if empty - assert not parent_dir.exists() + assert not workspace.workspace_path.exists() + assert base_dir.exists() def test_load_meta_returns_existing(self, clean_workspace): """Test that existing valid meta.json is returned by _load_meta. @@ -490,3 +496,100 @@ def test_sync_error_message_contains_workspace_path(self, temp_docx, tmp_path, m Workspace(temp_docx) Workspace.delete(temp_docx) + + +class TestWorkspaceLocationHardening: + """Regression tests for workspace path resolution edge cases.""" + + def test_non_utf8_source_filename(self, simple_docx, temp_dir, tmp_path, monkeypatch): + """A source path with undecodable bytes must not raise UnicodeEncodeError. + + str(Path) carries such bytes as surrogates, which strict UTF-8 .encode() + rejects; the hash key goes through os.fsencode instead. + """ + import shutil + + monkeypatch.setenv("DOCX_EDITOR_WORKSPACE_DIR", str(tmp_path)) + # "café.docx" in latin-1 — undecodable as UTF-8, so it round-trips as a surrogate. + weird = temp_dir / os.fsdecode(b"caf\xe9.docx") + shutil.copy(simple_docx, weird) + + workspace = Workspace(weird) + assert workspace.workspace_path.exists() + assert workspace.workspace_path.parent == tmp_path + workspace.close() + + def test_env_var_is_tilde_expanded(self, temp_docx, tmp_path, monkeypatch): + """A ~ in the env var is expanded, not taken as a literal directory name.""" + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setenv("DOCX_EDITOR_WORKSPACE_DIR", "~/ws") + + workspace = Workspace(temp_docx) + assert workspace.workspace_path.parent == tmp_path / "ws" + assert "~" not in str(workspace.workspace_path) + workspace.close() + + def test_empty_workspace_dir_is_treated_as_unset(self, temp_docx, tmp_path, monkeypatch): + """workspace_dir="" must not rebase the workspace onto the document's own dir.""" + monkeypatch.setenv("DOCX_EDITOR_WORKSPACE_DIR", str(tmp_path)) + + workspace = Workspace(temp_docx, workspace_dir="") + assert workspace.workspace_path.parent == tmp_path + assert workspace.workspace_path.parent != temp_docx.parent + workspace.close() + + def test_relative_xdg_cache_home_is_ignored(self, monkeypatch, tmp_path): + """Per the XDG spec, a relative XDG_CACHE_HOME is ignored, not honored.""" + monkeypatch.setattr(os, "name", "posix") + monkeypatch.setattr(sys, "platform", "linux") + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setenv("XDG_CACHE_HOME", "relative-cache") + + assert _default_cache_dir() == tmp_path / ".cache" / "docx-editor" + + def test_workspace_dir_is_owner_only(self, temp_docx, tmp_path, monkeypatch): + """The workspace holds document plaintext — it must not be group/world readable.""" + monkeypatch.setenv("DOCX_EDITOR_WORKSPACE_DIR", str(tmp_path / "base")) + + workspace = Workspace(temp_docx) + assert stat.S_IMODE(workspace.workspace_path.stat().st_mode) == 0o700 + assert stat.S_IMODE(workspace.workspace_path.parent.stat().st_mode) == 0o700 + workspace.close() + + @pytest.mark.skipif(os.getuid() == 0, reason="root ignores directory permissions") + def test_unwritable_base_raises_workspace_error(self, temp_docx, tmp_path, monkeypatch): + """A filesystem failure surfaces as WorkspaceError, not a raw PermissionError.""" + readonly = tmp_path / "readonly" + readonly.mkdir(mode=0o500) + monkeypatch.setenv("DOCX_EDITOR_WORKSPACE_DIR", str(readonly / "nested")) + + with pytest.raises(WorkspaceError, match="DOCX_EDITOR_WORKSPACE_DIR"): + Workspace(temp_docx) + + def test_adopting_another_documents_workspace_is_refused(self, simple_docx, temp_dir, tmp_path, monkeypatch): + """A workspace whose meta names a different source must not be adopted. + + The workspace dir is keyed by a hash of the source path, so a mismatch + means a collision or a planted directory; adopting it would let save() + pack the other document's XML over this one's source. + """ + import shutil + + monkeypatch.setenv("DOCX_EDITOR_WORKSPACE_DIR", str(tmp_path)) + victim = temp_dir / "victim.docx" + shutil.copy(simple_docx, victim) + + workspace = Workspace(victim) + workspace_path = workspace.workspace_path + workspace.close(cleanup=False) + + # Rewrite meta.json so it claims to belong to a different document. + meta_path = workspace_path / Workspace.META_FILE + meta = json.loads(meta_path.read_text()) + meta["source_path"] = str(temp_dir / "other.docx") + meta_path.write_text(json.dumps(meta)) + + with pytest.raises(WorkspaceError, match="different document"): + Workspace(victim) + + Workspace.delete(victim) From bd0c9eb9f7179e8c9ac54e1ca628aec91862187e Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 13 Jul 2026 16:21:47 +0200 Subject: [PATCH 3/3] fix: correct three regressions in the workspace hardening Found by a second adversarial pass over the previous commit. The permissions test guarded itself with skipif(os.getuid() == 0). A skipif condition is evaluated at collection time on every platform, and os.getuid does not exist on Windows, so the whole test module would die on import there -- taking with it the two tests that exist specifically to cover the Windows cache dir. Guard on os.name first; `or` short-circuits before os.geteuid. The provenance check compared the raw source path while the workspace key is normcase-folded. On a case-insensitive filesystem two spellings map to one workspace directory, which the check would then reject as "a different document" -- and the message tells the user to pass force_recreate=True, which would rmtree the workspace. Fold both sides the same way. Stripping the override with str(override).strip() also stripped Path inputs, silently redirecting a path that legitimately ends in whitespace to a different directory. Strip str overrides (where stray whitespace is config noise) and take a Path verbatim. Also document the WorkspaceError exits that the hardening added to Document.open(), and drop the literal ~ from the workspace_path example -- the value is always expanded. --- docs/api.md | 3 ++- docx_editor/document.py | 3 +++ docx_editor/workspace.py | 19 ++++++++++++++----- tests/test_workspace.py | 8 +++++++- 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/docs/api.md b/docs/api.md index c5befac..d8649a0 100644 --- a/docs/api.md +++ b/docs/api.md @@ -26,6 +26,7 @@ Open a Word document for editing. **Raises:** - `WorkspaceSyncError`: If the source `.docx` was modified since the workspace was created. Pass `force_recreate=True` to discard the stale workspace and re-unpack from the current source. The workspace is never deleted silently. The error message includes the workspace path. +- `WorkspaceError`: If the workspace directory cannot be created (e.g. the base is not writable), the home directory backing the default cache cannot be determined, or an existing workspace was unpacked from a different document. The message names the override to set. **Example:** @@ -80,7 +81,7 @@ print(doc.source_path) # Path("/path/to/contract.docx") Get the path to this document's workspace folder. Since the workspace lives in the user cache by default, this is how you locate the unpacked XML — for example after `close(cleanup=False)`, or when a workspace was preserved because an exception was raised. ```python -print(doc.workspace_path) # Path("~/.cache/docx-editor/0bebafb463a87cfa") +print(doc.workspace_path) # Path("/home/you/.cache/docx-editor/0bebafb463a87cfa") ``` ### Track Changes Methods diff --git a/docx_editor/document.py b/docx_editor/document.py index 18b4657..e8ab983 100644 --- a/docx_editor/document.py +++ b/docx_editor/document.py @@ -113,6 +113,9 @@ def open( Document instance ready for editing Raises: + WorkspaceError: If the workspace cannot be created (unwritable base, + undeterminable home directory) or an existing workspace was + unpacked from a different document. WorkspaceSyncError: If the source document was modified since the workspace was created. The message includes the workspace path. Pass force_recreate=True to discard the stale workspace and diff --git a/docx_editor/workspace.py b/docx_editor/workspace.py index 6f60afb..23baaae 100644 --- a/docx_editor/workspace.py +++ b/docx_editor/workspace.py @@ -170,7 +170,10 @@ def _check_provenance(self, meta: dict[str, Any]) -> None: document's XML over this one's source file. """ recorded = meta.get("source_path") - if recorded != str(self.source_path): + # Compare with the same folding the workspace key uses (see + # _resolve_workspace_path), or two spellings that map to one workspace on + # a case-insensitive filesystem would be rejected as different documents. + if recorded is None or os.path.normcase(recorded) != os.path.normcase(str(self.source_path)): raise WorkspaceError( f"Workspace {self.workspace_path} belongs to a different document " f"({recorded!r}, expected {str(self.source_path)!r}). " @@ -203,10 +206,16 @@ def _resolve_workspace_path(cls, source_path: Path, workspace_dir: str | Path | """ base = None for override in (workspace_dir, os.environ.get("DOCX_EDITOR_WORKSPACE_DIR")): - text = str(override).strip() if override is not None else "" - if text: # an empty/whitespace override falls through to the next level - base = Path(os.path.expanduser(text)) - break + if override is None: + continue + # A str override (including the env var) is stripped — stray whitespace + # there is config noise, not part of a real path. A Path is used + # verbatim, so an exotic path really ending in a space still works. + text = override.strip() if isinstance(override, str) else os.fspath(override) + if not text: # empty/whitespace override falls through to the next level + continue + base = Path(os.path.expanduser(text)) + break if base is None: base = _default_cache_dir() diff --git a/tests/test_workspace.py b/tests/test_workspace.py index 39fc7b1..875a396 100644 --- a/tests/test_workspace.py +++ b/tests/test_workspace.py @@ -556,7 +556,13 @@ def test_workspace_dir_is_owner_only(self, temp_docx, tmp_path, monkeypatch): assert stat.S_IMODE(workspace.workspace_path.parent.stat().st_mode) == 0o700 workspace.close() - @pytest.mark.skipif(os.getuid() == 0, reason="root ignores directory permissions") + # NB: a skipif condition is evaluated at collection time on every platform, + # so it must not touch os.geteuid (absent on Windows) unless already guarded + # by the short-circuiting os.name check. + @pytest.mark.skipif( + os.name != "posix" or os.geteuid() == 0, + reason="POSIX only; root ignores directory permissions", + ) def test_unwritable_base_raises_workspace_error(self, temp_docx, tmp_path, monkeypatch): """A filesystem failure surfaces as WorkspaceError, not a raw PermissionError.""" readonly = tmp_path / "readonly"