Add followlinks parameter to zip creation #77
Conversation
WalkthroughUpdated create_module_zip to call os.walk with followlinks=True, causing the zip creation process to traverse and include files/directories reachable through symbolic links. No other logic, signatures, or error handling were changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CLI as FTF CLI
participant Op as create_module_zip
participant FS as FileSystem (os.walk: followlinks=true)
participant Zip as ZipFile
Dev->>CLI: Run zip command
CLI->>Op: create_module_zip(path)
rect rgba(230,240,255,0.6)
note right of Op: Traverse directory tree
Op->>FS: os.walk(path, followlinks=true)
FS-->>Op: Yield dirs/files (incl. symlink targets)
end
alt Regular file/dir
Op->>Zip: add file to archive
else Symlink encountered
note over Op,FS: Follows link and traverses target
Op->>Zip: add resolved files
end
Op-->>CLI: Return created archive
CLI-->>Dev: Success / path to zip
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ftf_cli/operations.py (2)
46-51: Guard against symlink cycles and out-of-root traversal when enabling followlinks=TrueFollowing directory symlinks can:
- create infinite/very long traversals via cycles
- accidentally include content outside the module root (secrets, huge dirs)
Harden traversal by pruning dirs that (a) resolve outside the root or (b) have already been visited. Also defensively skip Terraform artifacts even if reached via symlinks.Apply this minimal, local diff:
- for root, dirs, files in os.walk(path, followlinks=True): - for file in files: - file_path = os.path.join(root, file) - arcname = os.path.relpath(file_path, path) - zipf.write(file_path, arcname) + base_real = os.path.realpath(path) + visited_dirs = set() + for root, dirs, files in os.walk(path, topdown=True, followlinks=True): + # Prune directories that would cause cycles or leave the module root + pruned = [] + for d in dirs: + if d == ".terraform": + continue # defend against symlinked terraform caches + dpath = os.path.join(root, d) + try: + real_d = os.path.realpath(dpath) + st = os.stat(real_d) + except OSError: + continue # skip dirs we can't stat + if not (real_d == base_real or real_d.startswith(base_real + os.sep)): + continue # don't traverse outside module root + key = (st.st_dev, st.st_ino) + if key in visited_dirs: + continue # avoid cycles via symlinks/junctions + visited_dirs.add(key) + pruned.append(d) + dirs[:] = pruned + + for fname in files: + if fname == ".terraform.lock.hcl": + continue + file_path = os.path.join(root, fname) + arcname = os.path.relpath(file_path, path) + zipf.write(file_path, arcname)
46-51: Ensurecreate_module_zipsafely handles symlinks and add a regression testThe manual test reveals that when
create_module_zipencounters a symlink cycle, it follows links indefinitely until the OS errors out with “Too many levels of symbolic links,” and it doesn’t explicitly guard against including content outside the specified root. We need to:
- Update the implementation in
ftf_cli/operations.pyto
- Track visited paths or inodes and skip revisiting them (avoiding infinite loops).
- Refuse to follow symlinks that escape the
rootdirectory.- Add a regression test (e.g. in
tests/test_operations.py) that
- Creates a symlink cycle under a temporary module root.
- Creates a symlink pointing outside the module root.
- Asserts that
create_module_zip(root)completes normally (no error or hang).- Verifies the zip archive includes expected files (e.g.
A/B/file.txt) and excludes external files (secret.txt).ftf_cli/operations.py:
--- a/ftf_cli/operations.py +++ b/ftf_cli/operations.py @@ -40,7 +40,25 @@ def create_module_zip(path: str) -> str: with zipfile.ZipFile(tmp_zip, "w", zipfile.ZIP_DEFLATED) as zipf: - for root, dirs, files in os.walk(path, followlinks=True): + seen = set() + for curr_root, dirs, files in os.walk(path, followlinks=True): + real_root = os.path.realpath(curr_root) + if real_root in seen: + # Skip cycles + continue + seen.add(real_root) + + # Filter out dirs that escape the module root + dirs[:] = [ + d for d in dirs + if os.path.realpath(os.path.join(curr_root, d)).startswith(os.path.realpath(path)) + ] + + for file in files: + file_path = os.path.join(curr_root, file) + real_file = os.path.realpath(file_path) + # Skip files outside the module root + if not real_file.startswith(os.path.realpath(path)): + continue + arcname = os.path.relpath(file_path, path) + zipf.write(file_path, arcname)tests/test_operations.py (new):
import os import tempfile import zipfile import pytest from ftf_cli.operations import create_module_zip def test_symlink_cycle_and_escape(tmp_path, monkeypatch): # Setup module root mod = tmp_path / "mod" (mod / "A" / "B").mkdir(parents=True) (mod / "A" / "B" / "file.txt").write_text("ok") # External directory and escape symlink outside = tmp_path / "outside" outside.mkdir() (outside / "secret.txt").write_text("secret") (mod / "A" / "link_out").symlink_to(outside) # Symlink cycle (mod / "A" / "B" / "loop").symlink_to(mod / "A") # Point create_module_zip at our temp repo root monkeypatch.setenv("PYTHONPATH", os.getcwd()) zip_path = create_module_zip(str(mod)) with zipfile.ZipFile(zip_path) as z: names = sorted(z.namelist()) assert "A/B/file.txt" in names assert not any("secret.txt" in n for n in names)This change ensures non-hanging behavior on cycles, enforces root boundaries, and adds a reproducible regression test.
🧹 Nitpick comments (2)
ftf_cli/operations.py (2)
46-46: Address Ruff B007: currentlydirsis unused in the loopIf you decide to keep the simple loop without pruning, rename
dirsto_dirsto satisfy Ruff. If you adopt the pruning fix above, this becomes moot becausedirsis used.- for root, dirs, files in os.walk(path, followlinks=True): + for root, _dirs, files in os.walk(path, followlinks=True):
46-51: Make followlinks behavior configurable to reduce blast radiusConsider making symlink traversal opt-in via a CLI flag (e.g.,
--follow-links/--no-follow-links, default off) or environment variable, then thread that boolean intocreate_module_zip(path, followlinks=False). This avoids surprising users with larger zips or leaking external content.Happy to draft the CLI/plumbing changes if you want this made configurable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ftf_cli/operations.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
ftf_cli/operations.py
46-46: Loop control variable dirs not used within loop body
Rename unused dirs to _dirs
(B007)
Summary
followlinks=Trueparameter toos.walk()in thecreate_module_zipfunctionTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit