feat(audit): opt-in loader size/path safety (#112 #113) - #116
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR adds opt-in filesystem safety validation to all loaders. A new ChangesFilesystem Safety Guards for Loaders
Sequence DiagramsequenceDiagram
participant Caller
participant Loader
participant SafetyModule
participant FileSystem
Caller->>Loader: load(file_path, max_bytes, base_dir)
Loader->>SafetyModule: resolve_within(file_path, base_dir)
SafetyModule->>SafetyModule: validate containment
SafetyModule-->>Loader: safe_path
Loader->>SafetyModule: check_size(safe_path, max_bytes)
SafetyModule->>FileSystem: getsize(safe_path)
FileSystem-->>SafetyModule: file size
SafetyModule-->>Loader: size valid
Loader->>FileSystem: open(safe_path)
FileSystem-->>Loader: file handle
Loader->>Loader: parse content
Loader-->>Caller: Blob
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ) | ||
|
|
||
|
|
||
| def resolve_within(path: str | Path, base_dir: str | Path | None = None) -> Path: |
There was a problem hiding this comment.
SUGGESTION: Document that base_dir should be a directory
The function assumes base_dir is a directory for the containment check to make sense. If a caller passes a file path as base_dir, the resolved base will be that file, and base not in resolved.parents will behave unexpectedly (a file has no parents in the Path.parents sense). Consider adding a note in the docstring: "base_dir should be a directory; behavior is undefined if a file path is given."
| means traversal (``../``), absolute escapes, and symlinks that point | ||
| out of the tree are all caught. | ||
| """ | ||
| resolved = Path(path).resolve() |
There was a problem hiding this comment.
SUGGESTION: Note TOCTOU race condition in docstring
There's a time-of-check-time-of-use window between resolve_within/check_size and the actual open() call in the loaders. An attacker could swap a symlink after the check but before the open. This is a known limitation of path-based checks. Consider adding a note: "Note: There is a TOCTOU race window between this check and the subsequent file open. For high-security contexts, open the file first and use os.fstat/os.readlink on the fd."
| base = Path(base_dir).resolve() | ||
| if resolved != base and base not in resolved.parents: | ||
| raise ValueError( | ||
| f"{path} resolves to {resolved}, which is outside the " |
There was a problem hiding this comment.
SUGGESTION: Error message says "base directory" but base_dir could be a file
The error message refers to "allowed base directory" but the parameter accepts any path. If base_dir is a file, this message is slightly misleading. Could say "allowed base path" for accuracy.
| base_dir: str | Path | None = None, | ||
| **kwargs, | ||
| ) -> ChatBlob: | ||
| path = Path(file_path) |
There was a problem hiding this comment.
SUGGESTION: path variable shadows safe_path purpose
path = Path(file_path) is created but only used for source_path=str(path) in the returned blob. The actual file operations use safe_path. This is intentional (preserving the user-provided path in the blob) but could be confusing. Consider renaming path to original_path or adding a comment: # Keep original path for blob.source_path.
| base_dir: str | Path | None = None, | ||
| **kwargs, | ||
| ) -> DocBlob: | ||
| path = Path(file_path) |
There was a problem hiding this comment.
SUGGESTION: path variable shadows safe_path purpose
path = Path(file_path) is created but only used for source_path=str(path) in the returned blob. The actual file operations use safe_path. This is intentional (preserving the user-provided path in the blob) but could be confusing. Consider renaming path to original_path or adding a comment: # Keep original path for blob.source_path.
| base_dir: str | Path | None = None, | ||
| **kwargs, | ||
| ) -> EmailBlob: | ||
| path = Path(file_path) |
There was a problem hiding this comment.
SUGGESTION: path variable shadows safe_path purpose
path = Path(file_path) is created but only used for source_path=str(path) in the returned blob. The actual file operations use safe_path. This is intentional (preserving the user-provided path in the blob) but could be confusing. Consider renaming path to original_path or adding a comment: # Keep original path for blob.source_path.
| base_dir: str | Path | None = None, | ||
| **kwargs, | ||
| ) -> TranscriptBlob: | ||
| path = Path(file_path) |
There was a problem hiding this comment.
SUGGESTION: path variable shadows safe_path purpose
path = Path(file_path) is created but only used for source_path=str(path) in the returned blob. The actual file operations use safe_path. This is intentional (preserving the user-provided path in the blob) but could be confusing. Consider renaming path to original_path or adding a comment: # Keep original path for blob.source_path.
| @@ -47,13 +48,34 @@ def can_handle(cls, extension: str = "", mime_type: str = "") -> bool: | |||
| return False | |||
|
|
|||
| @abstractmethod | |||
There was a problem hiding this comment.
WARNING: Interface change may break custom loaders
The load method signature now adds two keyword-only parameters (max_bytes, base_dir) before **kwargs. Existing custom loaders that override load(self, file_path, **kwargs) will still work because the new args go into **kwargs. However, loaders with explicit signatures like load(self, file_path, max_bytes=..., **kwargs) (without base_dir) will receive base_dir in **kwargs which is fine. But loaders that explicitly type **kwargs: Any or do kwargs.pop('max_bytes') may need updates. Consider adding a migration note in the docstring or changelog.
Code Review SummaryStatus: 11 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Reviewed by nemotron-3-ultra-550b-a55b-20260604:free · 714,876 tokens |
| def resolve_within(path: str | Path, base_dir: str | Path | None = None) -> Path: | ||
| """Resolve ``path`` and, when ``base_dir`` is set, confine it there. | ||
|
|
||
| ``base_dir`` is expected to be a directory that contains (directly or |
There was a problem hiding this comment.
SUGGESTION: Docstring for base_dir is confusing
It says "expected to be a directory" but then "passing a file as base_dir only ever matches that exact file." This is contradictory. Clarify whether base_dir should be a directory or can be a file, and what the behavior is in each case.
| """ | ||
| if max_bytes is None: | ||
| return | ||
| size = os.path.getsize(path) |
There was a problem hiding this comment.
WARNING: check_size doesn't handle missing files
os.path.getsize(path) raises FileNotFoundError if the path doesn't exist. This exception is not caught or documented. Callers may get an unexpected FileNotFoundError instead of a clear ValueError. Consider catching and re-raising as ValueError with a clearer message, or document the exception.
| base_dir: str | Path | None = None, | ||
| **kwargs, | ||
| ) -> ChatBlob: | ||
| path = Path(file_path) |
There was a problem hiding this comment.
SUGGESTION: Redundant path variable shadows safe_path purpose
path = Path(file_path) is created but only used for source_path and error messages. The actual file operations use safe_path. Consider using safe_path consistently and removing the redundant path variable.
| base_dir: str | Path | None = None, | ||
| **kwargs, | ||
| ) -> DocBlob: | ||
| path = Path(file_path) |
There was a problem hiding this comment.
SUGGESTION: Redundant path variable shadows safe_path purpose
path = Path(file_path) is created but only used for source_path. The actual file operations use safe_path. Consider using safe_path consistently and removing the redundant path variable.
| base_dir: str | Path | None = None, | ||
| **kwargs, | ||
| ) -> EmailBlob: | ||
| path = Path(file_path) |
There was a problem hiding this comment.
SUGGESTION: Redundant path variable shadows safe_path purpose
path = Path(file_path) is created but only used for source_path. The actual file operations use safe_path. Consider using safe_path consistently and removing the redundant path variable.
| base_dir: str | Path | None = None, | ||
| **kwargs, | ||
| ) -> TranscriptBlob: | ||
| path = Path(file_path) |
There was a problem hiding this comment.
SUGGESTION: Redundant path variable shadows safe_path purpose
path = Path(file_path) is created but only used for source_path and error messages. The actual file operations use safe_path. Consider using safe_path consistently and removing the redundant path variable.
| @@ -47,13 +48,34 @@ def can_handle(cls, extension: str = "", mime_type: str = "") -> bool: | |||
| return False | |||
|
|
|||
| @abstractmethod | |||
There was a problem hiding this comment.
WARNING: Interface change breaks custom loaders
The abstract load() method signature now requires max_bytes and base_dir keyword-only parameters. Any third-party or custom LoaderInterface subclasses that don't accept these will fail at runtime with TypeError. This is a breaking change. Consider providing a default implementation in the base class that calls the safety helpers, or at minimum document this as a breaking change in release notes.
1 participant
What
Adds two opt-in safety guards to the typed loaders under
taosmd/loaders/, closing audit items #112 (file-size limits) and #113 (path-traversal containment). Both default to behaviour that never breaks standalone direct use of any path.Why
The loaders (
doc_loader.py,chat_loader.py,email_loader.py,transcript_loader.py) did full-file reads (f.read()/json.load) with no size guard, and openedPath(file_path)directly with no containment or symlink check. A stray multi-gigabyte file could blow up memory, and a caller passing untrusted paths had no way to confine reads to a directory.How
New stdlib-only helper
taosmd/loaders/_safety.py:check_size(path, max_bytes=DEFAULT_MAX_BYTES)— raisesValueErrorwhen the file exceeds the cap. Default 100 MB (generous; no normal chat/transcript/email/doc file trips it).max_bytes=Noneis an explicit opt-out.resolve_within(path, base_dir=None)— withbase_dir=None(default) returns the resolved path with no restriction. Withbase_dirset, the resolved path must sit inside the resolved base or it raisesValueError. Resolving first catches../traversal, absolute escapes, and out-of-tree symlinks.Each loader's
load()now takes optional keyword-onlymax_bytesandbase_dir(threaded through theLoaderInterfaceABC) and callsresolve_within+check_sizebefore reading.source_pathon the returned blob is preserved as the original (un-resolved) path so existing provenance stays unchanged.Standalone-safe by design
loader.load(path)) has nobase_dir→ no path restriction, and a 100 MB cap no real file hits.os+pathlib. No new deps (matters for the Pi tier).Tests
tests/test_loaders.pygains a safety section: oversized file raises;base_dirblocks traversal / absolute / symlink escapes and allows normal/nested paths; nobase_dirallows any path; default call loads a small file unchanged.Summary by CodeRabbit
Release Notes
New Features
max_bytes=None.base_dirparameter to restrict file loading to a specified directory for added path safety.Tests